| Message ID | 1433529771-24496-6-git-send-email-lee.jones@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Mon, 08 Jun 2015, Mark Brown wrote: > On Fri, Jun 05, 2015 at 07:42:48PM +0100, Lee Jones wrote: > > Perhaps this is just personal preference, but ... > > Possibly... > > > Prevents this kind of nonsense: > > > this->is->just.silly = fetch_silly_value(&pointer); > > if (!this->is->just.silly) { > > printk("Silly value failed: %d\n", this->is->just.silly); > > return this->is->just.silly; > > } > > But we don't have any of that code? Well, one if statement where we > check config.init_data but that's it. A temporary does help with things > like the above but we're not doing that in this driver are we? Admittedly this is an extreme example, but I do consider: init_data = of_get_regulator_init_data(<blah>); if (init_data) return -ENOMEM; ... neater than: config.init_data = of_get_regulator_init_data(<blah>); if (!config.init_data) return -ENOMEM;
On Tue, 09 Jun 2015, Mark Brown wrote: > On Tue, Jun 09, 2015 at 08:03:47AM +0100, Lee Jones wrote: > > > Admittedly this is an extreme example, but I do consider: > > > init_data = of_get_regulator_init_data(<blah>); > > if (init_data) > > return -ENOMEM; > > > ... neater than: > > > config.init_data = of_get_regulator_init_data(<blah>); > > if (!config.init_data) > > return -ENOMEM; > > Oh, I see. I pretty much see things the other way for things where the > temporary has no other users. I don't feel passionate enough about it to contest. Skip this patch then. Are you okay to continue the review?
diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c index ae32086..779ecf9 100644 --- a/drivers/regulator/pwm-regulator.c +++ b/drivers/regulator/pwm-regulator.c @@ -96,6 +96,7 @@ static struct regulator_desc pwm_regulator_desc = { static int pwm_regulator_probe(struct platform_device *pdev) { + const struct regulator_init_data *init_data; struct pwm_regulator_data *drvdata; struct property *prop; struct regulator_dev *regulator; @@ -142,14 +143,15 @@ static int pwm_regulator_probe(struct platform_device *pdev) return ret; } - config.init_data = of_get_regulator_init_data(&pdev->dev, np, - &pwm_regulator_desc); - if (!config.init_data) + init_data = of_get_regulator_init_data(&pdev->dev, np, + &pwm_regulator_desc); + if (init_data) return -ENOMEM; config.of_node = np; config.dev = &pdev->dev; config.driver_data = drvdata; + config.init_data = init_data; drvdata->pwm = devm_pwm_get(&pdev->dev, NULL); if (IS_ERR(drvdata->pwm)) {
Perhaps this is just personal preference, but ... This patch introduces a new local variable to receive and test regulator initialisation data. It simplifies and cleans up the code making it that little bit easier to read and maintain. The local value is assigned to the structure attribute when all the others are. This is the way we usually do things. Prevents this kind of nonsense: this->is->just.silly = fetch_silly_value(&pointer); if (!this->is->just.silly) { printk("Silly value failed: %d\n", this->is->just.silly); return this->is->just.silly; } Signed-off-by: Lee Jones <lee.jones@linaro.org> --- drivers/regulator/pwm-regulator.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)