* [PATCH 1/3] regulator: s5m8767: Use of_get_child_count()
@ 2013-02-06 2:55 Axel Lin
2013-02-06 2:56 ` [PATCH 2/3] regulator: s5m8767: Fix using wrong dev argument at various places Axel Lin
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Axel Lin @ 2013-02-06 2:55 UTC (permalink / raw)
To: Mark Brown
Cc: Amit Daniel Kachhap, Sachin Kamat, Thomas Abraham, Sangbeom Kim,
LiamGirdwood, linux-kernel
Signed-off-by: Axel Lin <axel.lin@ingics.com>
---
drivers/regulator/s5m8767.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c
index 1250cef..194b5dd 100644
--- a/drivers/regulator/s5m8767.c
+++ b/drivers/regulator/s5m8767.c
@@ -536,9 +536,7 @@ static int s5m8767_pmic_dt_parse_pdata(struct sec_pmic_dev *iodev,
}
/* count the number of regulators to be supported in pmic */
- pdata->num_regulators = 0;
- for_each_child_of_node(regulators_np, reg_np)
- pdata->num_regulators++;
+ pdata->num_regulators = of_get_child_count(regulators_np);
rdata = devm_kzalloc(iodev->dev, sizeof(*rdata) *
pdata->num_regulators, GFP_KERNEL);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/3] regulator: s5m8767: Fix using wrong dev argument at various places 2013-02-06 2:55 [PATCH 1/3] regulator: s5m8767: Use of_get_child_count() Axel Lin @ 2013-02-06 2:56 ` Axel Lin 2013-02-08 11:20 ` Mark Brown 2013-02-06 2:58 ` [PATCH 3/3] regulator: s5m8767: Prevent possible NULL pointer dereference Axel Lin 2013-02-08 11:22 ` [PATCH 1/3] regulator: s5m8767: Use of_get_child_count() Mark Brown 2 siblings, 1 reply; 6+ messages in thread From: Axel Lin @ 2013-02-06 2:56 UTC (permalink / raw) To: Mark Brown Cc: Amit Daniel Kachhap, Sachin Kamat, Thomas Abraham, Sangbeom Kim, LiamGirdwood, linux-kernel Use &pdev->dev rather than iodev->dev for dev_err(), dev_warn() and dev_info(). Use &pdev->dev rather than iodev->dev for devm_kzalloc() and of_get_regulator_init_data(), this fixes memory leak. Signed-off-by: Axel Lin <axel.lin@ingics.com> --- drivers/regulator/s5m8767.c | 47 ++++++++++++++++++++++--------------------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c index 194b5dd..4cb65e3 100644 --- a/drivers/regulator/s5m8767.c +++ b/drivers/regulator/s5m8767.c @@ -479,7 +479,7 @@ static struct regulator_desc regulators[] = { }; #ifdef CONFIG_OF -static int s5m8767_pmic_dt_parse_dvs_gpio(struct sec_pmic_dev *iodev, +static int s5m8767_pmic_dt_parse_dvs_gpio(struct platform_device *pdev, struct sec_platform_data *pdata, struct device_node *pmic_np) { @@ -489,7 +489,7 @@ static int s5m8767_pmic_dt_parse_dvs_gpio(struct sec_pmic_dev *iodev, gpio = of_get_named_gpio(pmic_np, "s5m8767,pmic-buck-dvs-gpios", i); if (!gpio_is_valid(gpio)) { - dev_err(iodev->dev, "invalid gpio[%d]: %d\n", i, gpio); + dev_err(&pdev->dev, "invalid gpio[%d]: %d\n", i, gpio); return -EINVAL; } pdata->buck_gpios[i] = gpio; @@ -497,7 +497,7 @@ static int s5m8767_pmic_dt_parse_dvs_gpio(struct sec_pmic_dev *iodev, return 0; } -static int s5m8767_pmic_dt_parse_ds_gpio(struct sec_pmic_dev *iodev, +static int s5m8767_pmic_dt_parse_ds_gpio(struct platform_device *pdev, struct sec_platform_data *pdata, struct device_node *pmic_np) { @@ -507,7 +507,7 @@ static int s5m8767_pmic_dt_parse_ds_gpio(struct sec_pmic_dev *iodev, gpio = of_get_named_gpio(pmic_np, "s5m8767,pmic-buck-ds-gpios", i); if (!gpio_is_valid(gpio)) { - dev_err(iodev->dev, "invalid gpio[%d]: %d\n", i, gpio); + dev_err(&pdev->dev, "invalid gpio[%d]: %d\n", i, gpio); return -EINVAL; } pdata->buck_ds[i] = gpio; @@ -515,9 +515,10 @@ static int s5m8767_pmic_dt_parse_ds_gpio(struct sec_pmic_dev *iodev, return 0; } -static int s5m8767_pmic_dt_parse_pdata(struct sec_pmic_dev *iodev, +static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev, struct sec_platform_data *pdata) { + struct sec_pmic_dev *iodev = dev_get_drvdata(pdev->dev.parent); struct device_node *pmic_np, *regulators_np, *reg_np; struct sec_regulator_data *rdata; struct sec_opmode_data *rmode; @@ -525,31 +526,31 @@ static int s5m8767_pmic_dt_parse_pdata(struct sec_pmic_dev *iodev, pmic_np = iodev->dev->of_node; if (!pmic_np) { - dev_err(iodev->dev, "could not find pmic sub-node\n"); + dev_err(&pdev->dev, "could not find pmic sub-node\n"); return -ENODEV; } regulators_np = of_find_node_by_name(pmic_np, "regulators"); if (!regulators_np) { - dev_err(iodev->dev, "could not find regulators sub-node\n"); + dev_err(&pdev->dev, "could not find regulators sub-node\n"); return -EINVAL; } /* count the number of regulators to be supported in pmic */ pdata->num_regulators = of_get_child_count(regulators_np); - rdata = devm_kzalloc(iodev->dev, sizeof(*rdata) * + rdata = devm_kzalloc(&pdev->dev, sizeof(*rdata) * pdata->num_regulators, GFP_KERNEL); if (!rdata) { - dev_err(iodev->dev, + dev_err(&pdev->dev, "could not allocate memory for regulator data\n"); return -ENOMEM; } - rmode = devm_kzalloc(iodev->dev, sizeof(*rmode) * + rmode = devm_kzalloc(&pdev->dev, sizeof(*rmode) * pdata->num_regulators, GFP_KERNEL); if (!rdata) { - dev_err(iodev->dev, + dev_err(&pdev->dev, "could not allocate memory for regulator mode\n"); return -ENOMEM; } @@ -562,7 +563,7 @@ static int s5m8767_pmic_dt_parse_pdata(struct sec_pmic_dev *iodev, break; if (i == ARRAY_SIZE(regulators)) { - dev_warn(iodev->dev, + dev_warn(&pdev->dev, "don't know how to configure regulator %s\n", reg_np->name); continue; @@ -570,13 +571,13 @@ static int s5m8767_pmic_dt_parse_pdata(struct sec_pmic_dev *iodev, rdata->id = i; rdata->initdata = of_get_regulator_init_data( - iodev->dev, reg_np); + &pdev->dev, reg_np); rdata->reg_node = reg_np; rdata++; rmode->id = i; if (of_property_read_u32(reg_np, "op_mode", &rmode->mode)) { - dev_warn(iodev->dev, + dev_warn(&pdev->dev, "no op_mode property property at %s\n", reg_np->full_name); @@ -596,7 +597,7 @@ static int s5m8767_pmic_dt_parse_pdata(struct sec_pmic_dev *iodev, if (pdata->buck2_gpiodvs || pdata->buck3_gpiodvs || pdata->buck4_gpiodvs) { - ret = s5m8767_pmic_dt_parse_dvs_gpio(iodev, pdata, pmic_np); + ret = s5m8767_pmic_dt_parse_dvs_gpio(pdev, pdata, pmic_np); if (ret) return -EINVAL; @@ -607,42 +608,42 @@ static int s5m8767_pmic_dt_parse_pdata(struct sec_pmic_dev *iodev, } else { if (pdata->buck_default_idx >= 8) { pdata->buck_default_idx = 0; - dev_info(iodev->dev, + dev_info(&pdev->dev, "invalid value for default dvs index, use 0\n"); } } dvs_voltage_nr = 8; } - ret = s5m8767_pmic_dt_parse_ds_gpio(iodev, pdata, pmic_np); + ret = s5m8767_pmic_dt_parse_ds_gpio(pdev, pdata, pmic_np); if (ret) return -EINVAL; if (of_property_read_u32_array(pmic_np, "s5m8767,pmic-buck2-dvs-voltage", pdata->buck2_voltage, dvs_voltage_nr)) { - dev_err(iodev->dev, "buck2 voltages not specified\n"); + dev_err(&pdev->dev, "buck2 voltages not specified\n"); return -EINVAL; } if (of_property_read_u32_array(pmic_np, "s5m8767,pmic-buck3-dvs-voltage", pdata->buck3_voltage, dvs_voltage_nr)) { - dev_err(iodev->dev, "buck3 voltages not specified\n"); + dev_err(&pdev->dev, "buck3 voltages not specified\n"); return -EINVAL; } if (of_property_read_u32_array(pmic_np, "s5m8767,pmic-buck4-dvs-voltage", pdata->buck4_voltage, dvs_voltage_nr)) { - dev_err(iodev->dev, "buck4 voltages not specified\n"); + dev_err(&pdev->dev, "buck4 voltages not specified\n"); return -EINVAL; } return 0; } #else -static int s5m8767_pmic_dt_parse_pdata(struct sec_pmic_dev *iodev, +static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev, struct sec_platform_data *pdata) { return 0; @@ -659,13 +660,13 @@ static int s5m8767_pmic_probe(struct platform_device *pdev) int i, ret, size, buck_init; if (iodev->dev->of_node) { - ret = s5m8767_pmic_dt_parse_pdata(iodev, pdata); + ret = s5m8767_pmic_dt_parse_pdata(pdev, pdata); if (ret) return ret; } if (!pdata) { - dev_err(pdev->dev.parent, "Platform data not supplied\n"); + dev_err(&pdev->dev, "Platform data not supplied\n"); return -ENODEV; } -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] regulator: s5m8767: Fix using wrong dev argument at various places 2013-02-06 2:56 ` [PATCH 2/3] regulator: s5m8767: Fix using wrong dev argument at various places Axel Lin @ 2013-02-08 11:20 ` Mark Brown 2013-02-10 10:15 ` Axel Lin 0 siblings, 1 reply; 6+ messages in thread From: Mark Brown @ 2013-02-08 11:20 UTC (permalink / raw) To: Axel Lin Cc: Amit Daniel Kachhap, Sachin Kamat, Thomas Abraham, Sangbeom Kim, LiamGirdwood, linux-kernel [-- Attachment #1: Type: text/plain, Size: 611 bytes --] On Wed, Feb 06, 2013 at 10:56:52AM +0800, Axel Lin wrote: > Use &pdev->dev rather than iodev->dev for dev_err(), dev_warn() and dev_info(). It's not clear to me that this is actually an improvement, the pdev is mostly just an internal Linux implementation detail so it's common to use the physical device for MFD logging in order to help the user understand what's causing the log message. > Use &pdev->dev rather than iodev->dev for devm_kzalloc() and > of_get_regulator_init_data(), this fixes memory leak. This is a good fix though. This sort of thing is one reason for splitting out unrelated changes. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] regulator: s5m8767: Fix using wrong dev argument at various places 2013-02-08 11:20 ` Mark Brown @ 2013-02-10 10:15 ` Axel Lin 0 siblings, 0 replies; 6+ messages in thread From: Axel Lin @ 2013-02-10 10:15 UTC (permalink / raw) To: Mark Brown Cc: Amit Daniel Kachhap, Sachin Kamat, Thomas Abraham, Sangbeom Kim, LiamGirdwood, linux-kernel 2013/2/8, Mark Brown <broonie@opensource.wolfsonmicro.com>: > On Wed, Feb 06, 2013 at 10:56:52AM +0800, Axel Lin wrote: > >> Use &pdev->dev rather than iodev->dev for dev_err(), dev_warn() and >> dev_info(). > > It's not clear to me that this is actually an improvement, the pdev is > mostly just an internal Linux implementation detail so it's common to > use the physical device for MFD logging in order to help the user > understand what's causing the log message. My understanding is if people (at least it's me) found some error message in dmesg, they usually grep the driver that emit the error message. If we use the parent device here, people might grep wrong files. > >> Use &pdev->dev rather than iodev->dev for devm_kzalloc() and >> of_get_regulator_init_data(), this fixes memory leak. > > This is a good fix though. This sort of thing is one reason for > splitting out unrelated changes. Ok. I'll resend the patchs (for one topic a patch) a few days latter. (It's Chinese New Year vacation in Taiwan now.) ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 3/3] regulator: s5m8767: Prevent possible NULL pointer dereference 2013-02-06 2:55 [PATCH 1/3] regulator: s5m8767: Use of_get_child_count() Axel Lin 2013-02-06 2:56 ` [PATCH 2/3] regulator: s5m8767: Fix using wrong dev argument at various places Axel Lin @ 2013-02-06 2:58 ` Axel Lin 2013-02-08 11:22 ` [PATCH 1/3] regulator: s5m8767: Use of_get_child_count() Mark Brown 2 siblings, 0 replies; 6+ messages in thread From: Axel Lin @ 2013-02-06 2:58 UTC (permalink / raw) To: Mark Brown Cc: Amit Daniel Kachhap, Sachin Kamat, Thomas Abraham, Sangbeom Kim, LiamGirdwood, linux-kernel s5m8767_pmic_dt_parse_pdata dereferenes pdata, thus check pdata earlier to avoid NULL pointer dereference. Signed-off-by: Axel Lin <axel.lin@ingics.com> --- drivers/regulator/s5m8767.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c index 4cb65e3..0349358 100644 --- a/drivers/regulator/s5m8767.c +++ b/drivers/regulator/s5m8767.c @@ -659,17 +659,17 @@ static int s5m8767_pmic_probe(struct platform_device *pdev) struct s5m8767_info *s5m8767; int i, ret, size, buck_init; + if (!pdata) { + dev_err(&pdev->dev, "Platform data not supplied\n"); + return -ENODEV; + } + if (iodev->dev->of_node) { ret = s5m8767_pmic_dt_parse_pdata(pdev, pdata); if (ret) return ret; } - if (!pdata) { - dev_err(&pdev->dev, "Platform data not supplied\n"); - return -ENODEV; - } - if (pdata->buck2_gpiodvs) { if (pdata->buck3_gpiodvs || pdata->buck4_gpiodvs) { dev_err(&pdev->dev, "S5M8767 GPIO DVS NOT VALID\n"); -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] regulator: s5m8767: Use of_get_child_count() 2013-02-06 2:55 [PATCH 1/3] regulator: s5m8767: Use of_get_child_count() Axel Lin 2013-02-06 2:56 ` [PATCH 2/3] regulator: s5m8767: Fix using wrong dev argument at various places Axel Lin 2013-02-06 2:58 ` [PATCH 3/3] regulator: s5m8767: Prevent possible NULL pointer dereference Axel Lin @ 2013-02-08 11:22 ` Mark Brown 2 siblings, 0 replies; 6+ messages in thread From: Mark Brown @ 2013-02-08 11:22 UTC (permalink / raw) To: Axel Lin Cc: Amit Daniel Kachhap, Sachin Kamat, Thomas Abraham, Sangbeom Kim, LiamGirdwood, linux-kernel [-- Attachment #1: Type: text/plain, Size: 124 bytes --] On Wed, Feb 06, 2013 at 10:55:05AM +0800, Axel Lin wrote: > Signed-off-by: Axel Lin <axel.lin@ingics.com> Applied, thanks. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-02-10 10:15 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-02-06 2:55 [PATCH 1/3] regulator: s5m8767: Use of_get_child_count() Axel Lin 2013-02-06 2:56 ` [PATCH 2/3] regulator: s5m8767: Fix using wrong dev argument at various places Axel Lin 2013-02-08 11:20 ` Mark Brown 2013-02-10 10:15 ` Axel Lin 2013-02-06 2:58 ` [PATCH 3/3] regulator: s5m8767: Prevent possible NULL pointer dereference Axel Lin 2013-02-08 11:22 ` [PATCH 1/3] regulator: s5m8767: Use of_get_child_count() Mark Brown
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.