LinuxLists.cc - [PATCH 00/14] Assorted small patches for regulators

[permalink] [raw]

Subject: [PATCH 01/14] Regulators: virtual - use sysfs attribute groups

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 <[email protected]>
---

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);

2010-02-24 07:38:12

[permalink] [raw]

Subject: [PATCH 05/14] Regulators: tps65023-regulator - mark probe method as __devinit

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 <[email protected]>
---

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;

2010-02-24 07:38:20

[permalink] [raw]

Subject: [PATCH 06/14] Regulators: tps6507x-regulator - mark probe method as __devinit

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 <[email protected]>
---

drivers/regulator/tps6507x-regulator.c | 34 ++++++++++++++------------------
1 files changed, 15 insertions(+), 19 deletions(-)

diff --git a/drivers/regulator/tps6507x-regulator.c b/drivers/regulator/tps6507x-regulator.c
index f8a6dfb..c2a9539 100644
--- a/drivers/regulator/tps6507x-regulator.c
+++ b/drivers/regulator/tps6507x-regulator.c
@@ -538,8 +538,8 @@ static struct regulator_ops tps6507x_ldo_ops = {
.list_voltage = tps6507x_ldo_list_voltage,
};

-static
-int tps_6507x_probe(struct i2c_client *client, const struct i2c_device_id *id)
+static int __devinit tps_6507x_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
{
static int desc_id;
const struct tps_info *info = (void *)id->driver_data;
@@ -547,6 +547,7 @@ int tps_6507x_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))
@@ -557,7 +558,6 @@ int tps_6507x_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;

@@ -586,18 +586,8 @@ int tps_6507x_probe(struct i2c_client *client, const struct i2c_device_id *id)
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 */
@@ -607,6 +597,13 @@ int tps_6507x_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;
}

/**
@@ -620,13 +617,12 @@ static int __devexit tps_6507x_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 < TPS6507X_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;

2010-02-24 07:38:24

[permalink] [raw]

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

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 <[email protected]>
---

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;

2010-02-24 07:38:29

[permalink] [raw]

Subject: [PATCH 08/14] Regulators: max1586 - annotate probe and remove methods

Signed-off-by: Dmitry Torokhov <[email protected]>
---

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,
};

2010-02-24 07:38:39

[permalink] [raw]

Subject: [PATCH 10/14] Regulators: pcap-regulator - clean up driver data after removal

It is a good tone to reset driver data after unbinding the device.

Signed-off-by: Dmitry Torokhov <[email protected]>
---

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)

2010-02-24 07:38:44

[permalink] [raw]

Subject: [PATCH 11/14] Regulators: wm831x-xxx - clean up driver data after removal

It is a good tone to reset driver data after unbinding the device.
Also set up drivers owner.

Signed-off-by: Dmitry Torokhov <[email protected]>
---

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,
},
};

2010-02-24 07:38:50

[permalink] [raw]

Subject: [PATCH 12/14] Regulators: wm8994 - clean up driver data after removal

It is a good tone to reset driver data after unbinding the device.

Signed-off-by: Dmitry Torokhov <[email protected]>
---

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,
},
};

2010-02-24 07:39:00

[permalink] [raw]

Subject: [PATCH 14/14] Regulators: max8925-regulator - clean up driver data after removal

It is a good tone to reset driver data after unbinding the device.
Also change find_regulator_info() fro inline to __devinit - let compiler
figure out if it wants it to be inlined or not.

Signed-off-by: Dmitry Torokhov <[email protected]>
---

drivers/regulator/max8925-regulator.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/max8925-regulator.c b/drivers/regulator/max8925-regulator.c
index 67873f0..b6218f1 100644
--- a/drivers/regulator/max8925-regulator.c
+++ b/drivers/regulator/max8925-regulator.c
@@ -230,7 +230,7 @@ static struct max8925_regulator_info max8925_regulator_info[] = {
MAX8925_LDO(20, 750, 3900, 50),
};

-static inline struct max8925_regulator_info *find_regulator_info(int id)
+static struct max8925_regulator_info * __devinit find_regulator_info(int id)
{
struct max8925_regulator_info *ri;
int i;
@@ -247,7 +247,7 @@ static int __devinit max8925_regulator_probe(struct platform_device *pdev)
{
struct max8925_chip *chip = dev_get_drvdata(pdev->dev.parent);
struct max8925_platform_data *pdata = chip->dev->platform_data;
- struct max8925_regulator_info *ri = NULL;
+ struct max8925_regulator_info *ri;
struct regulator_dev *rdev;

ri = find_regulator_info(pdev->id);
@@ -274,7 +274,9 @@ static int __devexit max8925_regulator_remove(struct platform_device *pdev)
{
struct regulator_dev *rdev = platform_get_drvdata(pdev);

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

2010-02-24 07:38:37

[permalink] [raw]

Subject: [PATCH 09/14] Regulators: max8660 - annotate probe and remove methods

Signed-off-by: Dmitry Torokhov <[email protected]>
---

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,
};

2010-02-24 07:38:10

[permalink] [raw]

Subject: [PATCH 04/14] Regulators: twl-regulator - mark probe function as __devinit

Signed-off-by: Dmitry Torokhov <[email protected]>
---

drivers/regulator/twl-regulator.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
index 5f394bb..9729d76 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -531,7 +531,7 @@ static struct twlreg_info twl_regs[] = {
TWL6030_FIXED_LDO(VUSB, 0x70, 3300, 18, 0, 0x21)
};

-static int twlreg_probe(struct platform_device *pdev)
+static int __devinit twlreg_probe(struct platform_device *pdev)
{
int i;
struct twlreg_info *info;

2010-02-24 07:44:22

[permalink] [raw]

Subject: [PATCH 13/14] Regulators: wm8400 - cleanup platform driver data handling

Driver data set by platform_set_drvdata() is for private use of
the driver currently bound to teh device and not for use by parent,
subsystem and anyone else.

Also have wm8400_register_regulator() accept 'sturct wm8400 *'
instead of generic device structure.

Signed-off-by: Dmitry Torokhov <[email protected]>
---

drivers/regulator/wm8400-regulator.c | 13 +++++++------
include/linux/mfd/wm8400.h | 4 +++-
2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/regulator/wm8400-regulator.c b/drivers/regulator/wm8400-regulator.c
index d9a2c98..71b89e8 100644
--- a/drivers/regulator/wm8400-regulator.c
+++ b/drivers/regulator/wm8400-regulator.c
@@ -317,14 +317,17 @@ static struct regulator_desc regulators[] = {

static int __devinit wm8400_regulator_probe(struct platform_device *pdev)
{
+ struct wm8400 *wm8400 = container_of(pdev, struct wm8400, regulators[pdev->id]);
struct regulator_dev *rdev;

rdev = regulator_register(&regulators[pdev->id], &pdev->dev,
- pdev->dev.platform_data, dev_get_drvdata(&pdev->dev));
+ pdev->dev.platform_data, wm8400);

if (IS_ERR(rdev))
return PTR_ERR(rdev);

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

@@ -332,6 +335,7 @@ static int __devexit wm8400_regulator_remove(struct platform_device *pdev)
{
struct regulator_dev *rdev = platform_get_drvdata(pdev);

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

return 0;
@@ -356,11 +360,9 @@ static struct platform_driver wm8400_regulator_driver = {
* @param reg The regulator to control.
* @param initdata Regulator initdata for the regulator.
*/
-int wm8400_register_regulator(struct device *dev, int reg,
+int wm8400_register_regulator(struct wm8400 *wm8400, int reg,
struct regulator_init_data *initdata)
{
- struct wm8400 *wm8400 = dev_get_drvdata(dev);
-
if (wm8400->regulators[reg].name)
return -EBUSY;

@@ -368,9 +370,8 @@ int wm8400_register_regulator(struct device *dev, int reg,

wm8400->regulators[reg].name = "wm8400-regulator";
wm8400->regulators[reg].id = reg;
- wm8400->regulators[reg].dev.parent = dev;
+ wm8400->regulators[reg].dev.parent = wm8400->dev;
wm8400->regulators[reg].dev.platform_data = initdata;
- dev_set_drvdata(&wm8400->regulators[reg].dev, wm8400);

return platform_device_register(&wm8400->regulators[reg]);
}
diff --git a/include/linux/mfd/wm8400.h b/include/linux/mfd/wm8400.h
index b46b566..f9e49cc 100644
--- a/include/linux/mfd/wm8400.h
+++ b/include/linux/mfd/wm8400.h
@@ -34,7 +34,9 @@ struct wm8400_platform_data {
int (*platform_init)(struct device *dev);
};

-int wm8400_register_regulator(struct device *dev, int reg,
+struct wm8400;
+
+int wm8400_register_regulator(struct wm8400 *wm8400, int reg,
struct regulator_init_data *initdata);

#endif

2010-02-24 07:45:35

[permalink] [raw]

Subject: [PATCH 03/14] Regulators: fixed - annotate probe and remove methods

Add __devinit/__devexit markings to probe and remove methids of the
driver, change types of variables containing boolean data to boolean,
set up driver's owner field so we have proper sysfs link between
driver and the module.

Signed-off-by: Dmitry Torokhov <[email protected]>
---

drivers/regulator/fixed.c | 19 ++++++++++---------
1 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index a3d3bfc..d11f762 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -32,8 +32,8 @@ struct fixed_voltage_data {
int microvolts;
int gpio;
unsigned startup_delay;
- unsigned enable_high:1;
- unsigned is_enabled:1;
+ bool enable_high;
+ bool is_enabled;
};

static int fixed_voltage_is_enabled(struct regulator_dev *dev)
@@ -49,7 +49,7 @@ static int fixed_voltage_enable(struct regulator_dev *dev)

if (gpio_is_valid(data->gpio)) {
gpio_set_value_cansleep(data->gpio, data->enable_high);
- data->is_enabled = 1;
+ data->is_enabled = true;
}

return 0;
@@ -61,7 +61,7 @@ static int fixed_voltage_disable(struct regulator_dev *dev)

if (gpio_is_valid(data->gpio)) {
gpio_set_value_cansleep(data->gpio, !data->enable_high);
- data->is_enabled = 0;
+ data->is_enabled = false;
}

return 0;
@@ -101,7 +101,7 @@ static struct regulator_ops fixed_voltage_ops = {
.list_voltage = fixed_voltage_list_voltage,
};

-static int regulator_fixed_voltage_probe(struct platform_device *pdev)
+static int __devinit reg_fixed_voltage_probe(struct platform_device *pdev)
{
struct fixed_voltage_config *config = pdev->dev.platform_data;
struct fixed_voltage_data *drvdata;
@@ -174,7 +174,7 @@ static int regulator_fixed_voltage_probe(struct platform_device *pdev)
/* Regulator without GPIO control is considered
* always enabled
*/
- drvdata->is_enabled = 1;
+ drvdata->is_enabled = true;
}

drvdata->dev = regulator_register(&drvdata->desc, &pdev->dev,
@@ -202,7 +202,7 @@ err:
return ret;
}

-static int regulator_fixed_voltage_remove(struct platform_device *pdev)
+static int __devexit reg_fixed_voltage_remove(struct platform_device *pdev)
{
struct fixed_voltage_data *drvdata = platform_get_drvdata(pdev);

@@ -216,10 +216,11 @@ static int regulator_fixed_voltage_remove(struct platform_device *pdev)
}

static struct platform_driver regulator_fixed_voltage_driver = {
- .probe = regulator_fixed_voltage_probe,
- .remove = regulator_fixed_voltage_remove,
+ .probe = reg_fixed_voltage_probe,
+ .remove = __devexit_p(reg_fixed_voltage_remove),
.driver = {
.name = "reg-fixed-voltage",
+ .owner = THIS_MODULE,
},
};

2010-02-24 07:46:07

[permalink] [raw]

Subject: [PATCH 02/14] Regulators: ab3100 - fix probe and remove annotations

Probe and remove methods should not be marked as __init/__exit but
rather __devinit/__devexit so that the needed sections stay in memory
in presence of CONFIG_HOTPLUG. This is needed even on non hotpluggable
buses.

Signed-off-by: Dmitry Torokhov <[email protected]>
---

drivers/regulator/ab3100.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/ab3100.c b/drivers/regulator/ab3100.c
index b349db4..7de9509 100644
--- a/drivers/regulator/ab3100.c
+++ b/drivers/regulator/ab3100.c
@@ -561,7 +561,7 @@ ab3100_regulator_desc[AB3100_NUM_REGULATORS] = {
* for all the different regulators.
*/

-static int __init ab3100_regulators_probe(struct platform_device *pdev)
+static int __devinit ab3100_regulators_probe(struct platform_device *pdev)
{
struct ab3100_platform_data *plfdata = pdev->dev.platform_data;
struct ab3100 *ab3100 = platform_get_drvdata(pdev);
@@ -641,7 +641,7 @@ static int __init ab3100_regulators_probe(struct platform_device *pdev)
return 0;
}

-static int __exit ab3100_regulators_remove(struct platform_device *pdev)
+static int __devexit ab3100_regulators_remove(struct platform_device *pdev)
{
int i;

@@ -659,7 +659,7 @@ static struct platform_driver ab3100_regulators_driver = {
.owner = THIS_MODULE,
},
.probe = ab3100_regulators_probe,
- .remove = __exit_p(ab3100_regulators_remove),
+ .remove = __devexit_p(ab3100_regulators_remove),
};

static __init int ab3100_regulators_init(void)

2010-02-24 10:03:18

[permalink] [raw]

Subject: Re: [PATCH 00/14] Assorted small patches for regulators

On Tue, Feb 23, 2010 at 11:37:39PM -0800, Dmitry Torokhov wrote:

> I happend to peek into drivers/regulator and saw a bunch of small issues, so
> here goes. The patches are against linux-next, compile-tested only since I
> don't have the hardware,

I'm working through these now, but a few general issues:

- It'd be very much easier to review patches that do one thing at once,
especially when the patches do things more invasive than just adding
annotations or changing types. The random cleanup stuff makes the
invasive changes much harder to see and review.
- Frequently your patches include additional changes above those that
you list in the changelog which again increases the effort require to
reviwe - things like random whitespace changes and the addition of
module annotations seem particularly prone to this.
- Please always use a subject line for your patches which fits the
style of the subsystem. You're using "Regulators:" as a prefix when
pretty much everything else uses "regulator:".

2010-02-24 10:03:58

[permalink] [raw]

Subject: Re: [PATCH 02/14] Regulators: ab3100 - fix probe and remove annotations

On Tue, Feb 23, 2010 at 11:37:50PM -0800, Dmitry Torokhov wrote:
> Probe and remove methods should not be marked as __init/__exit but
> rather __devinit/__devexit so that the needed sections stay in memory
> in presence of CONFIG_HOTPLUG. This is needed even on non hotpluggable
> buses.
>
> Signed-off-by: Dmitry Torokhov <[email protected]>

Acked-by: Mark Brown <[email protected]>

2010-02-24 10:05:39

[permalink] [raw]

Subject: Re: [PATCH 04/14] Regulators: twl-regulator - mark probe function as __devinit

On Tue, Feb 23, 2010 at 11:38:01PM -0800, Dmitry Torokhov wrote:
> Signed-off-by: Dmitry Torokhov <[email protected]>

Acked-by: Mark Brown <[email protected]>

2010-02-24 10:06:58

[permalink] [raw]

Subject: Re: [PATCH 08/14] Regulators: max1586 - annotate probe and remove methods

On Tue, Feb 23, 2010 at 11:38:23PM -0800, Dmitry Torokhov wrote:
> Signed-off-by: Dmitry Torokhov <[email protected]>

Acked-by: Mark Brown <[email protected]>

> .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...

2010-02-24 10:07:18

[permalink] [raw]

Subject: Re: [PATCH 09/14] Regulators: max8660 - annotate probe and remove methods

On Tue, Feb 23, 2010 at 11:38:28PM -0800, Dmitry Torokhov wrote:
> Signed-off-by: Dmitry Torokhov <[email protected]>

Acked-by: Mark Brown <[email protected]>

2010-02-24 10:20:48

[permalink] [raw]

Subject: Re: [PATCH 12/14] Regulators: wm8994 - clean up driver data after removal

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 <[email protected]>

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(-)

2010-02-24 10:24:21

[permalink] [raw]

Subject: Re: [PATCH 08/14] Regulators: max1586 - annotate probe and remove methods

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 <[email protected]>
>
> Acked-by: Mark Brown <[email protected]>
>
> > .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

2010-02-24 10:25:51

[permalink] [raw]

Subject: Re: [PATCH 13/14] Regulators: wm8400 - cleanup platform driver data handling

On Tue, Feb 23, 2010 at 11:38:50PM -0800, Dmitry Torokhov wrote:
> Driver data set by platform_set_drvdata() is for private use of
> the driver currently bound to teh device and not for use by parent,
> subsystem and anyone else.
>
> Also have wm8400_register_regulator() accept 'sturct wm8400 *'
> instead of generic device structure.

Nack due to this change - this change would make it impossible for
callers to actually call the function. Note that nothing including only
wm8400.h even has a struct declaration, much less defniition, for struct
wm8400.

2010-02-24 10:28:24

[permalink] [raw]

Subject: Re: [PATCH 14/14] Regulators: max8925-regulator - clean up driver data after removal

On Tue, Feb 23, 2010 at 11:38:55PM -0800, Dmitry Torokhov wrote:
> It is a good tone to reset driver data after unbinding the device.
> Also change find_regulator_info() fro inline to __devinit - let compiler
> figure out if it wants it to be inlined or not.
>
> Signed-off-by: Dmitry Torokhov <[email protected]>

Acked-by: Mark Brown <[email protected]>

2010-02-24 10:36:27

[permalink] [raw]

Subject: Re: [PATCH 10/14] Regulators: pcap-regulator - clean up driver data after removal

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 <[email protected]>

Acked-by: Mark Brown <[email protected]>

2010-02-24 10:37:02

[permalink] [raw]

Subject: Re: [PATCH 11/14] Regulators: wm831x-xxx - clean up driver data after removal

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 <[email protected]>

Acked-by: Mark Brown <[email protected]>

2010-02-24 10:43:44

[permalink] [raw]

Subject: Re: [PATCH 03/14] Regulators: fixed - annotate probe and remove methods

On Tue, Feb 23, 2010 at 11:37:55PM -0800, Dmitry Torokhov wrote:
> Add __devinit/__devexit markings to probe and remove methids of the
> driver, change types of variables containing boolean data to boolean,
> set up driver's owner field so we have proper sysfs link between
> driver and the module.

> Signed-off-by: Dmitry Torokhov <[email protected]>

Acked-by: Mark Brown <[email protected]>

2010-02-24 10:50:18

[permalink] [raw]

Subject: Re: [PATCH 01/14] Regulators: virtual - use sysfs attribute groups

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 <[email protected]>

2010-02-24 11:31:52

[permalink] [raw]

Subject: Re: [PATCH 05/14] Regulators: tps65023-regulator - mark probe method as __devinit

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 <[email protected]>

Acked-by: Mark Brown <[email protected]>

2010-02-24 11:32:38

[permalink] [raw]

Subject: Re: [PATCH 06/14] Regulators: tps6507x-regulator - mark probe method as __devinit

On Tue, Feb 23, 2010 at 11:38:12PM -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 <[email protected]>

Acked-by: Mark Brown <[email protected]>

2010-02-24 12:11:36

[permalink] [raw]

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

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 <[email protected]>

Acked-by: Mark Brown <[email protected]>

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.

2010-02-24 19:02:44

[permalink] [raw]

Subject: Re: [PATCH 13/14] Regulators: wm8400 - cleanup platform driver data handling

On Wed, Feb 24, 2010 at 10:25:49AM +0000, Mark Brown wrote:
> On Tue, Feb 23, 2010 at 11:38:50PM -0800, Dmitry Torokhov wrote:
> > Driver data set by platform_set_drvdata() is for private use of
> > the driver currently bound to teh device and not for use by parent,
> > subsystem and anyone else.
> >
> > Also have wm8400_register_regulator() accept 'sturct wm8400 *'
> > instead of generic device structure.
>
> Nack due to this change - this change would make it impossible for
> callers to actually call the function. Note that nothing including only
> wm8400.h even has a struct declaration, much less defniition, for struct
> wm8400.

If you notice I added forward declaration of "struct wm8400" to wm8400.h
thus users can pass around pointer to the structure.

I really think we should refrain from passing naked 'struct device *'
pointers as much as possible since it is error prone. Otherwise
wm8400_register_regulator has no way of ensuting that passed 'dev' is
indeed wm8400.

--
Dmitry

2010-02-24 19:14:09

[permalink] [raw]

Subject: Re: [PATCH 13/14] Regulators: wm8400 - cleanup platform driver data handling

On Wed, Feb 24, 2010 at 11:02:34AM -0800, Dmitry Torokhov wrote:
> On Wed, Feb 24, 2010 at 10:25:49AM +0000, Mark Brown wrote:

> > Nack due to this change - this change would make it impossible for
> > callers to actually call the function. Note that nothing including only
> > wm8400.h even has a struct declaration, much less defniition, for struct
> > wm8400.

> If you notice I added forward declaration of "struct wm8400" to wm8400.h
> thus users can pass around pointer to the structure.

This doesn't help unless you also provide a way for users to obtain a
struct wm8400.

> I really think we should refrain from passing naked 'struct device *'
> pointers as much as possible since it is error prone. Otherwise
> wm8400_register_regulator has no way of ensuting that passed 'dev' is
> indeed wm8400.

Right, there's no type safety here (and this whole method of registering
regulators is fairly deprecated anyway in favour of just straight
platform data) but really it doesn't buy us much - the users get exactly
the struct device they need to use passed in to them, there's really
very little chance for them to get confused about what they're talking
about.

2010-02-24 19:21:40

[permalink] [raw]

Subject: Re: [PATCH 13/14] Regulators: wm8400 - cleanup platform driver data handling

On Wed, Feb 24, 2010 at 07:14:03PM +0000, Mark Brown wrote:
> On Wed, Feb 24, 2010 at 11:02:34AM -0800, Dmitry Torokhov wrote:
> > On Wed, Feb 24, 2010 at 10:25:49AM +0000, Mark Brown wrote:
>
> > > Nack due to this change - this change would make it impossible for
> > > callers to actually call the function. Note that nothing including only
> > > wm8400.h even has a struct declaration, much less defniition, for struct
> > > wm8400.
>
> > If you notice I added forward declaration of "struct wm8400" to wm8400.h
> > thus users can pass around pointer to the structure.
>
> This doesn't help unless you also provide a way for users to obtain a
> struct wm8400.

Why would they need it? Only code that creates instances of wm8400 needs
to know the definition of the sturcture, the rest can simply pass the
pointer around.

I guess there is disconnect between us and I do not see any users of
wm8400_register_regulator() in linux-next... Is there another tree I
could peek at?

>
> > I really think we should refrain from passing naked 'struct device *'
> > pointers as much as possible since it is error prone. Otherwise
> > wm8400_register_regulator has no way of ensuting that passed 'dev' is
> > indeed wm8400.
>
> Right, there's no type safety here (and this whole method of registering
> regulators is fairly deprecated anyway in favour of just straight
> platform data) but really it doesn't buy us much - the users get exactly
> the struct device they need to use passed in to them, there's really
> very little chance for them to get confused about what they're talking
> about.

--
Dmitry

2010-02-24 20:41:00

[permalink] [raw]

Subject: Re: [PATCH 13/14] Regulators: wm8400 - cleanup platform driver data handling

On Wed, Feb 24, 2010 at 11:21:26AM -0800, Dmitry Torokhov wrote:
> On Wed, Feb 24, 2010 at 07:14:03PM +0000, Mark Brown wrote:

> > This doesn't help unless you also provide a way for users to obtain a
> > struct wm8400.

> Why would they need it? Only code that creates instances of wm8400 needs
> to know the definition of the sturcture, the rest can simply pass the
> pointer around.

> I guess there is disconnect between us and I do not see any users of
> wm8400_register_regulator() in linux-next... Is there another tree I
> could peek at?

There are no users in mainline. This would be called by board specific
code from the init callback of the wm8400 - you'd need to pass that
callback the struct wm8400.

In any case, this is clearly an unrelated change to whatever else you
were doing to the driver so should be split off into a separate patch,
but if this is being changed at all then it'd be much more sensible to
change it to use a more modern pattern which completely removes the
wm8400_register_regulator() function and just uses platform data.

2010-02-25 09:55:43

[permalink] [raw]

Subject: Re: [PATCH 13/14] Regulators: wm8400 - cleanup platform driver data handling

On Wed, Feb 24, 2010 at 08:40:56PM +0000, Mark Brown wrote:
> On Wed, Feb 24, 2010 at 11:21:26AM -0800, Dmitry Torokhov wrote:
> > On Wed, Feb 24, 2010 at 07:14:03PM +0000, Mark Brown wrote:
>
> > > This doesn't help unless you also provide a way for users to obtain a
> > > struct wm8400.
>
> > Why would they need it? Only code that creates instances of wm8400 needs
> > to know the definition of the sturcture, the rest can simply pass the
> > pointer around.
>
> > I guess there is disconnect between us and I do not see any users of
> > wm8400_register_regulator() in linux-next... Is there another tree I
> > could peek at?
>
> There are no users in mainline. This would be called by board specific
> code from the init callback of the wm8400 - you'd need to pass that
> callback the struct wm8400.
>
> In any case, this is clearly an unrelated change to whatever else you
> were doing to the driver so should be split off into a separate patch,
> but if this is being changed at all then it'd be much more sensible to
> change it to use a more modern pattern which completely removes the
> wm8400_register_regulator() function and just uses platform data.

Fair enough, I removed the offending part, updated patch below.

--
Dmitry

regulator: wm8400 - cleanup platform driver data handling

Driver data set by platform_set_drvdata() is for private use of
the driver currently bound to teh device and not for use by parent,
subsystem and anyone else.

Signed-off-by: Dmitry Torokhov <[email protected]>
---

drivers/regulator/wm8400-regulator.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/wm8400-regulator.c b/drivers/regulator/wm8400-regulator.c
index d9a2c98..924c7eb 100644
--- a/drivers/regulator/wm8400-regulator.c
+++ b/drivers/regulator/wm8400-regulator.c
@@ -317,14 +317,17 @@ static struct regulator_desc regulators[] = {

static int __devinit wm8400_regulator_probe(struct platform_device *pdev)
{
+ struct wm8400 *wm8400 = container_of(pdev, struct wm8400, regulators[pdev->id]);
struct regulator_dev *rdev;

rdev = regulator_register(&regulators[pdev->id], &pdev->dev,
- pdev->dev.platform_data, dev_get_drvdata(&pdev->dev));
+ pdev->dev.platform_data, wm8400);

if (IS_ERR(rdev))
return PTR_ERR(rdev);

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

@@ -332,6 +335,7 @@ static int __devexit wm8400_regulator_remove(struct platform_device *pdev)
{
struct regulator_dev *rdev = platform_get_drvdata(pdev);

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

return 0;
@@ -370,7 +374,6 @@ int wm8400_register_regulator(struct device *dev, int reg,
wm8400->regulators[reg].id = reg;
wm8400->regulators[reg].dev.parent = dev;
wm8400->regulators[reg].dev.platform_data = initdata;
- dev_set_drvdata(&wm8400->regulators[reg].dev, wm8400);

return platform_device_register(&wm8400->regulators[reg]);
}

2010-02-25 10:36:00

[permalink] [raw]

Subject: Re: [PATCH 01/14] Regulators: virtual - use sysfs attribute groups

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 <[email protected]>
> ---

Applied.

Thanks

Liam

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

2010-02-25 10:36:10

[permalink] [raw]

Subject: Re: [PATCH 02/14] Regulators: ab3100 - fix probe and remove annotations

On Tue, 2010-02-23 at 23:37 -0800, Dmitry Torokhov wrote:
> Probe and remove methods should not be marked as __init/__exit but
> rather __devinit/__devexit so that the needed sections stay in memory
> in presence of CONFIG_HOTPLUG. This is needed even on non hotpluggable
> buses.
>
> Signed-off-by: Dmitry Torokhov <[email protected]>

Applied.

Thanks

Liam

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

2010-02-25 10:36:26

[permalink] [raw]

Subject: Re: [PATCH 03/14] Regulators: fixed - annotate probe and remove methods

On Tue, 2010-02-23 at 23:37 -0800, Dmitry Torokhov wrote:
> Add __devinit/__devexit markings to probe and remove methids of the
> driver, change types of variables containing boolean data to boolean,
> set up driver's owner field so we have proper sysfs link between
> driver and the module.
>
> Signed-off-by: Dmitry Torokhov <[email protected]>

Applied.

Thanks

Liam

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

2010-02-25 10:36:40

[permalink] [raw]

Subject: Re: [PATCH 04/14] Regulators: twl-regulator - mark probe function as __devinit

On Tue, 2010-02-23 at 23:38 -0800, Dmitry Torokhov wrote:
> Signed-off-by: Dmitry Torokhov <[email protected]>
> ---
>
> drivers/regulator/twl-regulator.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
> index 5f394bb..9729d76 100644
> --- a/drivers/regulator/twl-regulator.c
> +++ b/drivers/regulator/twl-regulator.c
> @@ -531,7 +531,7 @@ static struct twlreg_info twl_regs[] = {
> TWL6030_FIXED_LDO(VUSB, 0x70, 3300, 18, 0, 0x21)
> };
>
> -static int twlreg_probe(struct platform_device *pdev)
> +static int __devinit twlreg_probe(struct platform_device *pdev)
> {
> int i;
> struct twlreg_info *info;
>

Applied.

Thanks

Liam

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

2010-02-25 10:37:11

[permalink] [raw]

Subject: Re: [PATCH 05/14] Regulators: tps65023-regulator - mark probe method as __devinit

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 <[email protected]>
> ---

Applied.

Thanks

Liam

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

2010-02-25 10:37:22

[permalink] [raw]

Subject: Re: [PATCH 06/14] Regulators: tps6507x-regulator - mark probe method as __devinit

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 <[email protected]>
> ---
>

Applied.

Thanks

Liam

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

2010-02-25 10:37:34

[permalink] [raw]

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

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 <[email protected]>

Applied.

Thanks

Liam

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

2010-02-25 10:37:45

[permalink] [raw]

Subject: Re: [PATCH 08/14] Regulators: max1586 - annotate probe and remove methods

On Tue, 2010-02-23 at 23:38 -0800, Dmitry Torokhov wrote:
> Signed-off-by: Dmitry Torokhov <[email protected]>
> ---
>
> 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,
> };
>

Applied.

Thanks

Liam

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

2010-02-25 10:38:07

[permalink] [raw]

Subject: Re: [PATCH 09/14] Regulators: max8660 - annotate probe and remove methods

On Tue, 2010-02-23 at 23:38 -0800, Dmitry Torokhov wrote:
> Signed-off-by: Dmitry Torokhov <[email protected]>
> ---
>
> 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,
> };
>

Applied.

Thanks

Liam

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

2010-02-25 10:38:15

[permalink] [raw]

Subject: Re: [PATCH 10/14] Regulators: pcap-regulator - clean up driver data after removal

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 <[email protected]>
> ---
>
> 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

2010-02-25 10:38:31

[permalink] [raw]

Subject: Re: [PATCH 11/14] Regulators: wm831x-xxx - clean up driver data after removal

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 <[email protected]>
> ---

Applied.

Thanks

Liam

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

2010-02-25 10:39:13

[permalink] [raw]

Subject: Re: [PATCH 12/14] Regulators: wm8994 - clean up driver data after removal

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 <[email protected]>
> ---
>

Applied.

Thanks

Liam

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

2010-02-25 10:39:22

[permalink] [raw]

Subject: Re: [PATCH 14/14] Regulators: max8925-regulator - clean up driver data after removal

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 change find_regulator_info() fro inline to __devinit - let compiler
> figure out if it wants it to be inlined or not.
>
> Signed-off-by: Dmitry Torokhov <[email protected]>

Applied.

Thanks

Liam

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

2010-02-25 10:43:30

[permalink] [raw]

Subject: Re: [PATCH 13/14] Regulators: wm8400 - cleanup platform driver data handling

On Thu, Feb 25, 2010 at 01:55:37AM -0800, Dmitry Torokhov wrote:

> Fair enough, I removed the offending part, updated patch below.

Acked-by: Mark Brown <[email protected]>

> --
> Dmitry
>
> regulator: wm8400 - cleanup platform driver data handling

but note that including the patch after your sig separator means that a
lot of MUAs will hide the patch from users which makes it a bit hard to
read.

2010-02-25 11:02:46