[5/8] regulator: pwm-regulator: Don't assign structure attributes right away

Message ID 1433529771-24496-6-git-send-email-lee.jones@linaro.org
State New
Headers show

Commit Message

Lee Jones June 5, 2015, 6:42 p.m.
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(-)

Comments

Lee Jones June 9, 2015, 7:03 a.m. | #1
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;
Lee Jones June 9, 2015, 11:07 a.m. | #2
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?

Patch

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