Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

[PATCH 07/14] Regulators: lp3971 - fail if platform data was not supplied

2 views
Skip to first unread message

Dmitry Torokhov

unread,
Feb 24, 2010, 2:40:01 AM2/24/10
to
There is no point in completing probe if platform data is missing so
let's abort loading early.

Also, use kcalloc when allocating several instances of the same data
structure and mark setup_regulators() as __devinit since it is only
called from lp3971_i2c_probe() which is __devinit.

Signed-off-by: Dmitry Torokhov <dt...@mail.ru>
---

drivers/regulator/lp3971.c | 58 ++++++++++++++++++++++----------------------
1 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/drivers/regulator/lp3971.c b/drivers/regulator/lp3971.c
index 3ea639f..f5532ed 100644
--- a/drivers/regulator/lp3971.c
+++ b/drivers/regulator/lp3971.c
@@ -431,20 +431,20 @@ static int lp3971_set_bits(struct lp3971 *lp3971, u8 reg, u16 mask, u16 val)
return ret;
}

-static int setup_regulators(struct lp3971 *lp3971,
- struct lp3971_platform_data *pdata)
+static int __devinit setup_regulators(struct lp3971 *lp3971,
+ struct lp3971_platform_data *pdata)
{
int i, err;
- int num_regulators = pdata->num_regulators;
- lp3971->num_regulators = num_regulators;
- lp3971->rdev = kzalloc(sizeof(struct regulator_dev *) * num_regulators,
- GFP_KERNEL);
+
+ lp3971->num_regulators = pdata->num_regulators;
+ lp3971->rdev = kcalloc(pdata->num_regulators,
+ sizeof(struct regulator_dev *), GFP_KERNEL);

/* Instantiate the regulators */
- for (i = 0; i < num_regulators; i++) {
- int id = pdata->regulators[i].id;
- lp3971->rdev[i] = regulator_register(&regulators[id],
- lp3971->dev, pdata->regulators[i].initdata, lp3971);
+ for (i = 0; i < pdata->num_regulators; i++) {
+ struct lp3971_regulator_subdev *reg = &pdata->regulators[i];
+ lp3971->rdev[i] = regulator_register(&regulators[reg->id],
+ lp3971->dev, reg->initdata, lp3971);

if (IS_ERR(lp3971->rdev[i])) {
err = PTR_ERR(lp3971->rdev[i]);
@@ -455,10 +455,10 @@ static int setup_regulators(struct lp3971 *lp3971,
}

return 0;
+
error:
- for (i = 0; i < num_regulators; i++)
- if (lp3971->rdev[i])
- regulator_unregister(lp3971->rdev[i]);
+ while (--i >= 0)
+ regulator_unregister(lp3971->rdev[i]);
kfree(lp3971->rdev);
lp3971->rdev = NULL;
return err;
@@ -472,15 +472,17 @@ static int __devinit lp3971_i2c_probe(struct i2c_client *i2c,
int ret;
u16 val;

- lp3971 = kzalloc(sizeof(struct lp3971), GFP_KERNEL);
- if (lp3971 == NULL) {
- ret = -ENOMEM;
- goto err;
+ if (!pdata) {
+ dev_dbg(&i2c->dev, "No platform init data supplied\n");
+ return -ENODEV;
}

+ lp3971 = kzalloc(sizeof(struct lp3971), GFP_KERNEL);
+ if (lp3971 == NULL)
+ return -ENOMEM;
+
lp3971->i2c = i2c;
lp3971->dev = &i2c->dev;
- i2c_set_clientdata(i2c, lp3971);

mutex_init(&lp3971->io_lock);

@@ -493,19 +495,15 @@ static int __devinit lp3971_i2c_probe(struct i2c_client *i2c,
goto err_detect;
}

- if (pdata) {
- ret = setup_regulators(lp3971, pdata);
- if (ret < 0)
- goto err_detect;
- } else
- dev_warn(lp3971->dev, "No platform init data supplied\n");
+ ret = setup_regulators(lp3971, pdata);
+ if (ret < 0)
+ goto err_detect;

+ i2c_set_clientdata(i2c, lp3971);
return 0;

err_detect:
- i2c_set_clientdata(i2c, NULL);
kfree(lp3971);
-err:
return ret;
}

@@ -513,11 +511,13 @@ static int __devexit lp3971_i2c_remove(struct i2c_client *i2c)
{
struct lp3971 *lp3971 = i2c_get_clientdata(i2c);
int i;
+
+ i2c_set_clientdata(i2c, NULL);
+
for (i = 0; i < lp3971->num_regulators; i++)
- if (lp3971->rdev[i])
- regulator_unregister(lp3971->rdev[i]);
+ regulator_unregister(lp3971->rdev[i]);
+
kfree(lp3971->rdev);
- i2c_set_clientdata(i2c, NULL);
kfree(lp3971);

return 0;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Dmitry Torokhov

unread,
Feb 24, 2010, 2:40:02 AM2/24/10
to
It is a good tone to reset driver data after unbinding the device.
Also set up drivers owner.

Signed-off-by: Dmitry Torokhov <dt...@mail.ru>
---

drivers/regulator/wm831x-dcdc.c | 12 ++++++++++++
drivers/regulator/wm831x-isink.c | 3 +++
drivers/regulator/wm831x-ldo.c | 5 +++++
3 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/drivers/regulator/wm831x-dcdc.c b/drivers/regulator/wm831x-dcdc.c
index 0a65775..6e18e56 100644
--- a/drivers/regulator/wm831x-dcdc.c
+++ b/drivers/regulator/wm831x-dcdc.c
@@ -600,6 +600,8 @@ static __devexit int wm831x_buckv_remove(struct platform_device *pdev)
struct wm831x_dcdc *dcdc = platform_get_drvdata(pdev);
struct wm831x *wm831x = dcdc->wm831x;

+ platform_set_drvdata(pdev, NULL);
+
wm831x_free_irq(wm831x, platform_get_irq_byname(pdev, "HC"), dcdc);
wm831x_free_irq(wm831x, platform_get_irq_byname(pdev, "UV"), dcdc);
regulator_unregister(dcdc->regulator);
@@ -615,6 +617,7 @@ static struct platform_driver wm831x_buckv_driver = {
.remove = __devexit_p(wm831x_buckv_remove),
.driver = {
.name = "wm831x-buckv",
+ .owner = THIS_MODULE,
},
};

@@ -769,6 +772,8 @@ static __devexit int wm831x_buckp_remove(struct platform_device *pdev)
struct wm831x_dcdc *dcdc = platform_get_drvdata(pdev);
struct wm831x *wm831x = dcdc->wm831x;

+ platform_set_drvdata(pdev, NULL);
+
wm831x_free_irq(wm831x, platform_get_irq_byname(pdev, "UV"), dcdc);
regulator_unregister(dcdc->regulator);
kfree(dcdc);
@@ -781,6 +786,7 @@ static struct platform_driver wm831x_buckp_driver = {
.remove = __devexit_p(wm831x_buckp_remove),
.driver = {
.name = "wm831x-buckp",
+ .owner = THIS_MODULE,
},
};

@@ -895,6 +901,8 @@ static __devexit int wm831x_boostp_remove(struct platform_device *pdev)
struct wm831x_dcdc *dcdc = platform_get_drvdata(pdev);
struct wm831x *wm831x = dcdc->wm831x;

+ platform_set_drvdata(pdev, NULL);
+
wm831x_free_irq(wm831x, platform_get_irq_byname(pdev, "UV"), dcdc);
regulator_unregister(dcdc->regulator);
kfree(dcdc);
@@ -907,6 +915,7 @@ static struct platform_driver wm831x_boostp_driver = {
.remove = __devexit_p(wm831x_boostp_remove),
.driver = {
.name = "wm831x-boostp",
+ .owner = THIS_MODULE,
},
};

@@ -979,6 +988,8 @@ static __devexit int wm831x_epe_remove(struct platform_device *pdev)
{
struct wm831x_dcdc *dcdc = platform_get_drvdata(pdev);

+ platform_set_drvdata(pdev, NULL);
+
regulator_unregister(dcdc->regulator);
kfree(dcdc);

@@ -990,6 +1001,7 @@ static struct platform_driver wm831x_epe_driver = {
.remove = __devexit_p(wm831x_epe_remove),
.driver = {
.name = "wm831x-epe",
+ .owner = THIS_MODULE,
},
};

diff --git a/drivers/regulator/wm831x-isink.c b/drivers/regulator/wm831x-isink.c
index 4885700..ca0f6b6 100644
--- a/drivers/regulator/wm831x-isink.c
+++ b/drivers/regulator/wm831x-isink.c
@@ -222,6 +222,8 @@ static __devexit int wm831x_isink_remove(struct platform_device *pdev)
struct wm831x_isink *isink = platform_get_drvdata(pdev);
struct wm831x *wm831x = isink->wm831x;

+ platform_set_drvdata(pdev, NULL);
+
wm831x_free_irq(wm831x, platform_get_irq(pdev, 0), isink);

regulator_unregister(isink->regulator);
@@ -235,6 +237,7 @@ static struct platform_driver wm831x_isink_driver = {
.remove = __devexit_p(wm831x_isink_remove),
.driver = {
.name = "wm831x-isink",
+ .owner = THIS_MODULE,
},
};

diff --git a/drivers/regulator/wm831x-ldo.c b/drivers/regulator/wm831x-ldo.c
index 61e02ac..d2406c1 100644
--- a/drivers/regulator/wm831x-ldo.c
+++ b/drivers/regulator/wm831x-ldo.c
@@ -371,6 +371,8 @@ static __devexit int wm831x_gp_ldo_remove(struct platform_device *pdev)
struct wm831x_ldo *ldo = platform_get_drvdata(pdev);
struct wm831x *wm831x = ldo->wm831x;

+ platform_set_drvdata(pdev, NULL);
+
wm831x_free_irq(wm831x, platform_get_irq_byname(pdev, "UV"), ldo);
regulator_unregister(ldo->regulator);
kfree(ldo);
@@ -383,6 +385,7 @@ static struct platform_driver wm831x_gp_ldo_driver = {
.remove = __devexit_p(wm831x_gp_ldo_remove),
.driver = {
.name = "wm831x-ldo",
+ .owner = THIS_MODULE,
},
};

@@ -640,6 +643,7 @@ static struct platform_driver wm831x_aldo_driver = {
.remove = __devexit_p(wm831x_aldo_remove),
.driver = {
.name = "wm831x-aldo",
+ .owner = THIS_MODULE,
},
};

@@ -811,6 +815,7 @@ static struct platform_driver wm831x_alive_ldo_driver = {
.remove = __devexit_p(wm831x_alive_ldo_remove),
.driver = {
.name = "wm831x-alive-ldo",
+ .owner = THIS_MODULE,
},

Dmitry Torokhov

unread,
Feb 24, 2010, 2:40:02 AM2/24/10
to
Signed-off-by: Dmitry Torokhov <dt...@mail.ru>
---

drivers/regulator/max8660.c | 11 ++++++-----
1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/regulator/max8660.c b/drivers/regulator/max8660.c
index acc2fb7..f12f1bb 100644
--- a/drivers/regulator/max8660.c
+++ b/drivers/regulator/max8660.c
@@ -345,8 +345,8 @@ static struct regulator_desc max8660_reg[] = {
},
};

-static int max8660_probe(struct i2c_client *client,
- const struct i2c_device_id *i2c_id)
+static int __devinit max8660_probe(struct i2c_client *client,
+ const struct i2c_device_id *i2c_id)
{
struct regulator_dev **rdev;
struct max8660_platform_data *pdata = client->dev.platform_data;
@@ -354,7 +354,7 @@ static int max8660_probe(struct i2c_client *client,
int boot_on, i, id, ret = -EINVAL;

if (pdata->num_subdevs > MAX8660_V_END) {
- dev_err(&client->dev, "Too much regulators found!\n");
+ dev_err(&client->dev, "Too many regulators found!\n");
goto out;
}

@@ -462,7 +462,7 @@ out:
return ret;
}

-static int max8660_remove(struct i2c_client *client)
+static int __devexit max8660_remove(struct i2c_client *client)
{
struct regulator_dev **rdev = i2c_get_clientdata(client);
int i;
@@ -485,9 +485,10 @@ MODULE_DEVICE_TABLE(i2c, max8660_id);

static struct i2c_driver max8660_driver = {
.probe = max8660_probe,
- .remove = max8660_remove,
+ .remove = __devexit_p(max8660_remove),
.driver = {
.name = "max8660",


+ .owner = THIS_MODULE,
},

.id_table = max8660_id,

Dmitry Torokhov

unread,
Feb 24, 2010, 2:40:02 AM2/24/10
to
Signed-off-by: Dmitry Torokhov <dt...@mail.ru>
---

drivers/regulator/max1586.c | 9 +++++----
1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/max1586.c b/drivers/regulator/max1586.c
index 2c082d3..a49fc95 100644
--- a/drivers/regulator/max1586.c
+++ b/drivers/regulator/max1586.c
@@ -179,8 +179,8 @@ static struct regulator_desc max1586_reg[] = {
},
};

-static int max1586_pmic_probe(struct i2c_client *client,


- const struct i2c_device_id *i2c_id)

+static int __devinit max1586_pmic_probe(struct i2c_client *client,


+ const struct i2c_device_id *i2c_id)
{
struct regulator_dev **rdev;

struct max1586_platform_data *pdata = client->dev.platform_data;
@@ -235,7 +235,7 @@ out:
return ret;
}

-static int max1586_pmic_remove(struct i2c_client *client)
+static int __devexit max1586_pmic_remove(struct i2c_client *client)


{
struct regulator_dev **rdev = i2c_get_clientdata(client);
int i;

@@ -257,9 +257,10 @@ MODULE_DEVICE_TABLE(i2c, max1586_id);

static struct i2c_driver max1586_pmic_driver = {
.probe = max1586_pmic_probe,
- .remove = max1586_pmic_remove,
+ .remove = __devexit_p(max1586_pmic_remove),
.driver = {
.name = "max1586",


+ .owner = THIS_MODULE,
},

.id_table = max1586_id,

Dmitry Torokhov

unread,
Feb 24, 2010, 2:40:02 AM2/24/10
to
It is a good tone to reset driver data after unbinding the device.

Signed-off-by: Dmitry Torokhov <dt...@mail.ru>
---

drivers/regulator/pcap-regulator.c | 8 +++++---
1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/pcap-regulator.c b/drivers/regulator/pcap-regulator.c
index 33d7d89..29d0566 100644
--- a/drivers/regulator/pcap-regulator.c
+++ b/drivers/regulator/pcap-regulator.c
@@ -288,16 +288,18 @@ static int __devexit pcap_regulator_remove(struct platform_device *pdev)
struct regulator_dev *rdev = platform_get_drvdata(pdev);

regulator_unregister(rdev);
+ platform_set_drvdata(pdev, NULL);

return 0;
}

static struct platform_driver pcap_regulator_driver = {
.driver = {
- .name = "pcap-regulator",
+ .name = "pcap-regulator",


+ .owner = THIS_MODULE,
},

- .probe = pcap_regulator_probe,
- .remove = __devexit_p(pcap_regulator_remove),
+ .probe = pcap_regulator_probe,
+ .remove = __devexit_p(pcap_regulator_remove),
};

static int __init pcap_regulator_init(void)

Dmitry Torokhov

unread,
Feb 24, 2010, 2:40:01 AM2/24/10
to
It is a good tone to reset driver data after unbinding the device.

Signed-off-by: Dmitry Torokhov <dt...@mail.ru>
---

drivers/regulator/wm8994-regulator.c | 11 +++++++----
1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/wm8994-regulator.c b/drivers/regulator/wm8994-regulator.c
index d09e018..95454a4 100644
--- a/drivers/regulator/wm8994-regulator.c
+++ b/drivers/regulator/wm8994-regulator.c
@@ -26,7 +26,7 @@

struct wm8994_ldo {
int enable;
- int is_enabled;
+ bool is_enabled;
struct regulator_dev *regulator;
struct wm8994 *wm8994;
};
@@ -43,7 +43,7 @@ static int wm8994_ldo_enable(struct regulator_dev *rdev)
return 0;

gpio_set_value(ldo->enable, 1);
- ldo->is_enabled = 1;
+ ldo->is_enabled = true;

return 0;
}
@@ -57,7 +57,7 @@ static int wm8994_ldo_disable(struct regulator_dev *rdev)
return -EINVAL;

gpio_set_value(ldo->enable, 0);
- ldo->is_enabled = 0;
+ ldo->is_enabled = false;

return 0;
}
@@ -218,7 +218,7 @@ static __devinit int wm8994_ldo_probe(struct platform_device *pdev)

ldo->wm8994 = wm8994;

- ldo->is_enabled = 1;
+ ldo->is_enabled = true;

if (pdata->ldo[id].enable && gpio_is_valid(pdata->ldo[id].enable)) {
ldo->enable = pdata->ldo[id].enable;
@@ -263,6 +263,8 @@ static __devexit int wm8994_ldo_remove(struct platform_device *pdev)
{
struct wm8994_ldo *ldo = platform_get_drvdata(pdev);

+ platform_set_drvdata(pdev, NULL);
+
regulator_unregister(ldo->regulator);
if (gpio_is_valid(ldo->enable))
gpio_free(ldo->enable);
@@ -276,6 +278,7 @@ static struct platform_driver wm8994_ldo_driver = {
.remove = __devexit_p(wm8994_ldo_remove),
.driver = {
.name = "wm8994-ldo",


+ .owner = THIS_MODULE,
},

};

Dmitry Torokhov

unread,
Feb 24, 2010, 2:40:02 AM2/24/10
to
Also move error handling in probe() out of line and do not bother
to reset fields in structures that are about to be freed.

Signed-off-by: Dmitry Torokhov <dt...@mail.ru>
---

drivers/regulator/tps65023-regulator.c | 35 +++++++++++++++-----------------
1 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/drivers/regulator/tps65023-regulator.c b/drivers/regulator/tps65023-regulator.c
index 07fda0a..1f18354 100644
--- a/drivers/regulator/tps65023-regulator.c
+++ b/drivers/regulator/tps65023-regulator.c
@@ -457,8 +457,8 @@ static struct regulator_ops tps65023_ldo_ops = {
.list_voltage = tps65023_ldo_list_voltage,
};

-static
-int tps_65023_probe(struct i2c_client *client, const struct i2c_device_id *id)
+static int __devinit tps_65023_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
{
static int desc_id;
const struct tps_info *info = (void *)id->driver_data;
@@ -466,6 +466,7 @@ int tps_65023_probe(struct i2c_client *client, const struct i2c_device_id *id)
struct regulator_dev *rdev;
struct tps_pmic *tps;
int i;
+ int error;

if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
return -EIO;
@@ -475,7 +476,6 @@ int tps_65023_probe(struct i2c_client *client, const struct i2c_device_id *id)
* coming from the board-evm file.
*/
init_data = client->dev.platform_data;
-
if (!init_data)
return -EIO;

@@ -502,21 +502,12 @@ int tps_65023_probe(struct i2c_client *client, const struct i2c_device_id *id)

/* Register the regulators */
rdev = regulator_register(&tps->desc[i], &client->dev,
- init_data, tps);
+ init_data, tps);
if (IS_ERR(rdev)) {
dev_err(&client->dev, "failed to register %s\n",
id->name);
-
- /* Unregister */
- while (i)
- regulator_unregister(tps->rdev[--i]);
-
- tps->client = NULL;
-
- /* clear the client data in i2c */
- i2c_set_clientdata(client, NULL);
- kfree(tps);
- return PTR_ERR(rdev);
+ error = PTR_ERR(rdev);
+ goto fail;
}

/* Save regulator for cleanup */
@@ -526,6 +517,13 @@ int tps_65023_probe(struct i2c_client *client, const struct i2c_device_id *id)
i2c_set_clientdata(client, tps);

return 0;
+
+ fail:


+ while (--i >= 0)

+ regulator_unregister(tps->rdev[i]);
+
+ kfree(tps);
+ return error;
}

/**
@@ -539,13 +537,12 @@ static int __devexit tps_65023_remove(struct i2c_client *client)
struct tps_pmic *tps = i2c_get_clientdata(client);
int i;

+ /* clear the client data in i2c */
+ i2c_set_clientdata(client, NULL);
+
for (i = 0; i < TPS65023_NUM_REGULATOR; i++)
regulator_unregister(tps->rdev[i]);

- tps->client = NULL;
-
- /* clear the client data in i2c */
- i2c_set_clientdata(client, NULL);
kfree(tps);

return 0;

Dmitry Torokhov

unread,
Feb 24, 2010, 2:40:02 AM2/24/10
to
Instead of open-coding sysfs attribute group use canned solution.
Also add __devinit/__devexit markups for probe and remove methods
and use 'bool' where it makes sense.

Signed-off-by: Dmitry Torokhov <dt...@mail.ru>
---

drivers/regulator/virtual.c | 64 ++++++++++++++++++++++---------------------
1 files changed, 33 insertions(+), 31 deletions(-)

diff --git a/drivers/regulator/virtual.c b/drivers/regulator/virtual.c
index addc032..d96ceca 100644
--- a/drivers/regulator/virtual.c
+++ b/drivers/regulator/virtual.c
@@ -19,7 +19,7 @@
struct virtual_consumer_data {
struct mutex lock;
struct regulator *regulator;
- int enabled;
+ bool enabled;
int min_uV;
int max_uV;
int min_uA;
@@ -49,7 +49,7 @@ static void update_voltage_constraints(struct device *dev,
dev_dbg(dev, "Enabling regulator\n");
ret = regulator_enable(data->regulator);
if (ret == 0)
- data->enabled = 1;
+ data->enabled = true;
else
dev_err(dev, "regulator_enable() failed: %d\n",
ret);
@@ -59,7 +59,7 @@ static void update_voltage_constraints(struct device *dev,
dev_dbg(dev, "Disabling regulator\n");
ret = regulator_disable(data->regulator);
if (ret == 0)
- data->enabled = 0;
+ data->enabled = false;
else
dev_err(dev, "regulator_disable() failed: %d\n",
ret);
@@ -89,7 +89,7 @@ static void update_current_limit_constraints(struct device *dev,
dev_dbg(dev, "Enabling regulator\n");
ret = regulator_enable(data->regulator);
if (ret == 0)
- data->enabled = 1;
+ data->enabled = true;
else
dev_err(dev, "regulator_enable() failed: %d\n",
ret);
@@ -99,7 +99,7 @@ static void update_current_limit_constraints(struct device *dev,
dev_dbg(dev, "Disabling regulator\n");
ret = regulator_disable(data->regulator);
if (ret == 0)
- data->enabled = 0;
+ data->enabled = false;
else
dev_err(dev, "regulator_disable() failed: %d\n",
ret);
@@ -270,24 +270,28 @@ static DEVICE_ATTR(min_microamps, 0666, show_min_uA, set_min_uA);
static DEVICE_ATTR(max_microamps, 0666, show_max_uA, set_max_uA);
static DEVICE_ATTR(mode, 0666, show_mode, set_mode);

-static struct device_attribute *attributes[] = {
- &dev_attr_min_microvolts,
- &dev_attr_max_microvolts,
- &dev_attr_min_microamps,
- &dev_attr_max_microamps,
- &dev_attr_mode,
+static struct attribute *regulator_virtual_attributes[] = {
+ &dev_attr_min_microvolts.attr,
+ &dev_attr_max_microvolts.attr,
+ &dev_attr_min_microamps.attr,
+ &dev_attr_max_microamps.attr,
+ &dev_attr_mode.attr,
+ NULL
};

-static int regulator_virtual_consumer_probe(struct platform_device *pdev)
+static const struct attribute_group regulator_virtual_attr_group = {
+ .attrs = regulator_virtual_attributes,
+};
+
+static int __devinit regulator_virtual_probe(struct platform_device *pdev)
{
char *reg_id = pdev->dev.platform_data;
struct virtual_consumer_data *drvdata;
- int ret, i;
+ int ret;

drvdata = kzalloc(sizeof(struct virtual_consumer_data), GFP_KERNEL);
- if (drvdata == NULL) {
+ if (drvdata == NULL)
return -ENOMEM;
- }

mutex_init(&drvdata->lock);

@@ -299,13 +303,12 @@ static int regulator_virtual_consumer_probe(struct platform_device *pdev)
goto err;
}

- for (i = 0; i < ARRAY_SIZE(attributes); i++) {
- ret = device_create_file(&pdev->dev, attributes[i]);
- if (ret != 0) {
- dev_err(&pdev->dev, "Failed to create attr %d: %d\n",
- i, ret);
- goto err_regulator;
- }
+ ret = sysfs_create_group(&pdev->dev.kobj,
+ &regulator_virtual_attr_group);
+ if (ret != 0) {
+ dev_err(&pdev->dev,
+ "Failed to create attribute group: %d\n", ret);
+ goto err_regulator;
}

drvdata->mode = regulator_get_mode(drvdata->regulator);
@@ -317,37 +320,36 @@ static int regulator_virtual_consumer_probe(struct platform_device *pdev)
err_regulator:
regulator_put(drvdata->regulator);
err:
- for (i = 0; i < ARRAY_SIZE(attributes); i++)
- device_remove_file(&pdev->dev, attributes[i]);
kfree(drvdata);
return ret;
}

-static int regulator_virtual_consumer_remove(struct platform_device *pdev)
+static int __devexit regulator_virtual_remove(struct platform_device *pdev)
{
struct virtual_consumer_data *drvdata = platform_get_drvdata(pdev);
- int i;

- for (i = 0; i < ARRAY_SIZE(attributes); i++)
- device_remove_file(&pdev->dev, attributes[i]);
+ sysfs_remove_group(&pdev->dev.kobj, &regulator_virtual_attr_group);
+
if (drvdata->enabled)
regulator_disable(drvdata->regulator);
regulator_put(drvdata->regulator);

kfree(drvdata);

+ platform_set_drvdata(pdev, NULL);
+
return 0;
}

static struct platform_driver regulator_virtual_consumer_driver = {
- .probe = regulator_virtual_consumer_probe,
- .remove = regulator_virtual_consumer_remove,
+ .probe = regulator_virtual_probe,
+ .remove = __devexit_p(regulator_virtual_remove),
.driver = {
.name = "reg-virt-consumer",


+ .owner = THIS_MODULE,
},
};

-

static int __init regulator_virtual_consumer_init(void)
{
return platform_driver_register(&regulator_virtual_consumer_driver);

Mark Brown

unread,
Feb 24, 2010, 5:10:02 AM2/24/10
to
On Tue, Feb 23, 2010 at 11:38:23PM -0800, Dmitry Torokhov wrote:
> Signed-off-by: Dmitry Torokhov <dt...@mail.ru>

Acked-by: Mark Brown <bro...@opensource.wolfsonmicro.com>

> .driver = {
> .name = "max1586",
> + .owner = THIS_MODULE,
> },

Note that a large number of your patches introduce this change and none
of them note it in the changelog...

Mark Brown

unread,
Feb 24, 2010, 5:10:03 AM2/24/10
to
On Tue, Feb 23, 2010 at 11:38:28PM -0800, Dmitry Torokhov wrote:
> Signed-off-by: Dmitry Torokhov <dt...@mail.ru>

Acked-by: Mark Brown <bro...@opensource.wolfsonmicro.com>

Mark Brown

unread,
Feb 24, 2010, 5:30:02 AM2/24/10
to
On Tue, Feb 23, 2010 at 11:38:44PM -0800, Dmitry Torokhov wrote:
> It is a good tone to reset driver data after unbinding the device.

You mean "good style" here. Anyway,

Acked-by: Mark Brown <bro...@opensource.wolfsonmicro.com>

BTW, this patch is an example of the sort of stuff I'm talking about
with adding additional changes that aren't documented in the
changelog. As well as nulling out the driver data you're also...

> struct wm8994_ldo {
> int enable;
> - int is_enabled;
> + bool is_enabled;

...doing a conversion to bool here and...

> @@ -276,6 +278,7 @@ static struct platform_driver wm8994_ldo_driver = {
> .remove = __devexit_p(wm8994_ldo_remove),
> .driver = {
> .name = "wm8994-ldo",
> + .owner = THIS_MODULE,
> },

...adding this. The change you mention in the changelog is a single
line edit but with these extra changes the overall diffstat becomes:

> drivers/regulator/wm8994-regulator.c | 11 +++++++----
> 1 files changed, 7 insertions(+), 4 deletions(-)

Dmitry Torokhov

unread,
Feb 24, 2010, 5:30:02 AM2/24/10
to
Hi Mark,

On Wednesday 24 February 2010 02:06:55 am Mark Brown wrote:
> On Tue, Feb 23, 2010 at 11:38:23PM -0800, Dmitry Torokhov wrote:
> > Signed-off-by: Dmitry Torokhov <dt...@mail.ru>
>
> Acked-by: Mark Brown <bro...@opensource.wolfsonmicro.com>
>
> > .driver = {
> > .name = "max1586",
> > + .owner = THIS_MODULE,
> > },
>
> Note that a large number of your patches introduce this change and none
> of them note it in the changelog...
>

Thanks for reviewing the lot.

Yes, I quite often list only most important changes in the log, and just
throw stuff like this in because I prefer to work on driver by driver
basis and splitting everything up produce twice as many patches. I will
try to be more detailed in my future submissions.

--
Dmitry

Mark Brown

unread,
Feb 24, 2010, 5:40:01 AM2/24/10
to
On Tue, Feb 23, 2010 at 11:38:33PM -0800, Dmitry Torokhov wrote:
> It is a good tone to reset driver data after unbinding the device.
>
> Signed-off-by: Dmitry Torokhov <dt...@mail.ru>

Acked-by: Mark Brown <bro...@opensource.wolfsonmicro.com>

Mark Brown

unread,
Feb 24, 2010, 5:40:01 AM2/24/10
to
On Tue, Feb 23, 2010 at 11:38:39PM -0800, Dmitry Torokhov wrote:
> It is a good tone to reset driver data after unbinding the device.
> Also set up drivers owner.
>
> Signed-off-by: Dmitry Torokhov <dt...@mail.ru>

Acked-by: Mark Brown <bro...@opensource.wolfsonmicro.com>

Mark Brown

unread,
Feb 24, 2010, 6:00:02 AM2/24/10
to
On Tue, Feb 23, 2010 at 11:37:44PM -0800, Dmitry Torokhov wrote:
> Instead of open-coding sysfs attribute group use canned solution.
> Also add __devinit/__devexit markups for probe and remove methods
> and use 'bool' where it makes sense.

Acked-by: Mark Brown <bro...@opensource.wolfsonmicro.com>

Mark Brown

unread,
Feb 24, 2010, 6:40:02 AM2/24/10
to
On Tue, Feb 23, 2010 at 11:38:06PM -0800, Dmitry Torokhov wrote:
> Also move error handling in probe() out of line and do not bother
> to reset fields in structures that are about to be freed.
>
> Signed-off-by: Dmitry Torokhov <dt...@mail.ru>

Acked-by: Mark Brown <bro...@opensource.wolfsonmicro.com>

Mark Brown

unread,
Feb 24, 2010, 7:20:02 AM2/24/10
to
On Tue, Feb 23, 2010 at 11:38:17PM -0800, Dmitry Torokhov wrote:
> There is no point in completing probe if platform data is missing so
> let's abort loading early.
>
> Also, use kcalloc when allocating several instances of the same data
> structure and mark setup_regulators() as __devinit since it is only
> called from lp3971_i2c_probe() which is __devinit.
>
> Signed-off-by: Dmitry Torokhov <dt...@mail.ru>

Acked-by: Mark Brown <bro...@opensource.wolfsonmicro.com>

but this *really* should have been split into multiple patches, there's
a large number of different changes mixed in here, with random coding
style tweaks overlapping with substantial changes to the handling of
probe.

Liam Girdwood

unread,
Feb 25, 2010, 5:40:02 AM2/25/10
to
On Tue, 2010-02-23 at 23:38 -0800, Dmitry Torokhov wrote:
> It is a good tone to reset driver data after unbinding the device.
>
> Signed-off-by: Dmitry Torokhov <dt...@mail.ru>
> ---
>

Applied.

Thanks

Liam

--
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

Liam Girdwood

unread,
Feb 25, 2010, 5:40:02 AM2/25/10
to
On Tue, 2010-02-23 at 23:38 -0800, Dmitry Torokhov wrote:
> It is a good tone to reset driver data after unbinding the device.
> Also set up drivers owner.
>
> Signed-off-by: Dmitry Torokhov <dt...@mail.ru>
> ---

Applied.

Thanks

Liam

--
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

--

Liam Girdwood

unread,
Feb 25, 2010, 5:40:03 AM2/25/10
to
On Tue, 2010-02-23 at 23:38 -0800, Dmitry Torokhov wrote:

Applied.

Thanks

Liam

--
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

--

Liam Girdwood

unread,
Feb 25, 2010, 5:40:03 AM2/25/10
to
On Tue, 2010-02-23 at 23:38 -0800, Dmitry Torokhov wrote:
> Also move error handling in probe() out of line and do not bother
> to reset fields in structures that are about to be freed.
>
> Signed-off-by: Dmitry Torokhov <dt...@mail.ru>
> ---

Applied.

Thanks

Liam

--
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

--

Liam Girdwood

unread,
Feb 25, 2010, 5:40:03 AM2/25/10
to
On Tue, 2010-02-23 at 23:37 -0800, Dmitry Torokhov wrote:
> Instead of open-coding sysfs attribute group use canned solution.
> Also add __devinit/__devexit markups for probe and remove methods
> and use 'bool' where it makes sense.
>
> Signed-off-by: Dmitry Torokhov <dt...@mail.ru>
> ---

Applied.

Thanks

Liam

--
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

--

Liam Girdwood

unread,
Feb 25, 2010, 5:40:02 AM2/25/10
to
On Tue, 2010-02-23 at 23:38 -0800, Dmitry Torokhov wrote:
> There is no point in completing probe if platform data is missing so
> let's abort loading early.
>
> Also, use kcalloc when allocating several instances of the same data
> structure and mark setup_regulators() as __devinit since it is only
> called from lp3971_i2c_probe() which is __devinit.
>
> Signed-off-by: Dmitry Torokhov <dt...@mail.ru>

Applied.

Thanks

Liam

--
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

--

Liam Girdwood

unread,
Feb 25, 2010, 5:40:03 AM2/25/10
to
On Tue, 2010-02-23 at 23:38 -0800, Dmitry Torokhov wrote:

Applied.

Thanks

Liam

--
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

--

Liam Girdwood

unread,
Feb 25, 2010, 5:40:04 AM2/25/10
to
On Tue, 2010-02-23 at 23:38 -0800, Dmitry Torokhov wrote:
> It is a good tone to reset driver data after unbinding the device.
>
> Signed-off-by: Dmitry Torokhov <dt...@mail.ru>
> ---
>
> drivers/regulator/pcap-regulator.c | 8 +++++---
> 1 files changed, 5 insertions(+), 3 deletions(-)
>

Applied.

Thanks

Liam

--
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

--

0 new messages