[PATCH v4] mfd: dm355evm_msp: Refactoring for add_child()

[PATCH v4] mfd: dm355evm_msp: Refactoring for add_child()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>

Adjust jump targets according to the Linux coding style convention.
Another check for the variable "status" can be omitted then at the end.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
v4: Further feedback was integrated into this message.
v3: Deletion of another blank line
v2: Rebasing

 drivers/mfd/dm355evm_msp.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/mfd/dm355evm_msp.c b/drivers/mfd/dm355evm_msp.c
index 270e19c..86eca61 100644
--- a/drivers/mfd/dm355evm_msp.c
+++ b/drivers/mfd/dm355evm_msp.c
@@ -209,7 +209,7 @@ static struct device *add_child(struct i2c_client *client, const char *name,
 		status = platform_device_add_data(pdev, pdata, pdata_len);
 		if (status < 0) {
 			dev_dbg(&pdev->dev, "can't add platform_data\n");
-			goto err;
+			goto put_device;
 		}
 	}
 
@@ -222,19 +222,20 @@ static struct device *add_child(struct i2c_client *client, const char *name,
 		status = platform_device_add_resources(pdev, &r, 1);
 		if (status < 0) {
 			dev_dbg(&pdev->dev, "can't add irq\n");
-			goto err;
+			goto put_device;
 		}
 	}
 
 	status = platform_device_add(pdev);
+	if (status)
+		goto put_device;
 
-err:
-	if (status < 0) {
-		platform_device_put(pdev);
-		dev_err(&client->dev, "can't add %s dev\n", name);
-		return ERR_PTR(status);
-	}
 	return &pdev->dev;
+
+put_device:
+	platform_device_put(pdev);
+	dev_err(&client->dev, "failed to add device %s\n", name);
+	return ERR_PTR(status);
 }
 
 static int add_children(struct i2c_client *client)
-- 
2.9.0

[PATCH v4] mfd: dm355evm_msp: Refactoring for add_child()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>

Adjust jump targets according to the Linux coding style convention.
Another check for the variable "status" can be omitted then at the end.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
v4: Further feedback was integrated into this message.
v3: Deletion of another blank line
v2: Rebasing

 drivers/mfd/dm355evm_msp.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/mfd/dm355evm_msp.c b/drivers/mfd/dm355evm_msp.c
index 270e19c..86eca61 100644
--- a/drivers/mfd/dm355evm_msp.c
+++ b/drivers/mfd/dm355evm_msp.c
@@ -209,7 +209,7 @@ static struct device *add_child(struct i2c_client *client, const char *name,
 		status = platform_device_add_data(pdev, pdata, pdata_len);
 		if (status < 0) {
 			dev_dbg(&pdev->dev, "can't add platform_data\n");
-			goto err;
+			goto put_device;
 		}
 	}
 
@@ -222,19 +222,20 @@ static struct device *add_child(struct i2c_client *client, const char *name,
 		status = platform_device_add_resources(pdev, &r, 1);
 		if (status < 0) {
 			dev_dbg(&pdev->dev, "can't add irq\n");
-			goto err;
+			goto put_device;
 		}
 	}
 
 	status = platform_device_add(pdev);
+	if (status)
+		goto put_device;
 
-err:
-	if (status < 0) {
-		platform_device_put(pdev);
-		dev_err(&client->dev, "can't add %s dev\n", name);
-		return ERR_PTR(status);
-	}
 	return &pdev->dev;
+
+put_device:
+	platform_device_put(pdev);
+	dev_err(&client->dev, "failed to add device %s\n", name);
+	return ERR_PTR(status);
 }
 
 static int add_children(struct i2c_client *client)
-- 
2.9.0

Re: [PATCH v4] mfd: dm355evm_msp: Refactoring for add_child()

[Lee Jones]

On Fri, 01 Jul 2016, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>

Please use `git send-email` to submit your patches.

> Adjust jump targets according to the Linux coding style convention.
> Another check for the variable "status" can be omitted then at the end.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> v4: Further feedback was integrated into this message.

This is not a good change-log.  What actually changed?

> v3: Deletion of another blank line
> v2: Rebasing
> 
>  drivers/mfd/dm355evm_msp.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mfd/dm355evm_msp.c b/drivers/mfd/dm355evm_msp.c
> index 270e19c..86eca61 100644
> --- a/drivers/mfd/dm355evm_msp.c
> +++ b/drivers/mfd/dm355evm_msp.c
> @@ -209,7 +209,7 @@ static struct device *add_child(struct i2c_client *client, const char *name,
>  		status = platform_device_add_data(pdev, pdata, pdata_len);
>  		if (status < 0) {
>  			dev_dbg(&pdev->dev, "can't add platform_data\n");

Please take the opportunity to convert these to dev_err()s.

> -			goto err;
> +			goto put_device;
>  		}
>  	}
>  
> @@ -222,19 +222,20 @@ static struct device *add_child(struct i2c_client *client, const char *name,
>  		status = platform_device_add_resources(pdev, &r, 1);
>  		if (status < 0) {
>  			dev_dbg(&pdev->dev, "can't add irq\n");
> -			goto err;
> +			goto put_device;
>  		}
>  	}
>  
>  	status = platform_device_add(pdev);
> +	if (status)
> +		goto put_device;
>  
> -err:
> -	if (status < 0) {
> -		platform_device_put(pdev);
> -		dev_err(&client->dev, "can't add %s dev\n", name);
> -		return ERR_PTR(status);
> -	}
>  	return &pdev->dev;
> +
> +put_device:
> +	platform_device_put(pdev);
> +	dev_err(&client->dev, "failed to add device %s\n", name);

... and remove this line.

> +	return ERR_PTR(status);
>  }
>  
>  static int add_children(struct i2c_client *client)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4] mfd: dm355evm_msp: Refactoring for add_child()

[Lee Jones]

On Fri, 01 Jul 2016, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>

Please use `git send-email` to submit your patches.

> Adjust jump targets according to the Linux coding style convention.
> Another check for the variable "status" can be omitted then at the end.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> v4: Further feedback was integrated into this message.

This is not a good change-log.  What actually changed?

> v3: Deletion of another blank line
> v2: Rebasing
> 
>  drivers/mfd/dm355evm_msp.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mfd/dm355evm_msp.c b/drivers/mfd/dm355evm_msp.c
> index 270e19c..86eca61 100644
> --- a/drivers/mfd/dm355evm_msp.c
> +++ b/drivers/mfd/dm355evm_msp.c
> @@ -209,7 +209,7 @@ static struct device *add_child(struct i2c_client *client, const char *name,
>  		status = platform_device_add_data(pdev, pdata, pdata_len);
>  		if (status < 0) {
>  			dev_dbg(&pdev->dev, "can't add platform_data\n");

Please take the opportunity to convert these to dev_err()s.

> -			goto err;
> +			goto put_device;
>  		}
>  	}
>  
> @@ -222,19 +222,20 @@ static struct device *add_child(struct i2c_client *client, const char *name,
>  		status = platform_device_add_resources(pdev, &r, 1);
>  		if (status < 0) {
>  			dev_dbg(&pdev->dev, "can't add irq\n");
> -			goto err;
> +			goto put_device;
>  		}
>  	}
>  
>  	status = platform_device_add(pdev);
> +	if (status)
> +		goto put_device;
>  
> -err:
> -	if (status < 0) {
> -		platform_device_put(pdev);
> -		dev_err(&client->dev, "can't add %s dev\n", name);
> -		return ERR_PTR(status);
> -	}
>  	return &pdev->dev;
> +
> +put_device:
> +	platform_device_put(pdev);
> +	dev_err(&client->dev, "failed to add device %s\n", name);

... and remove this line.

> +	return ERR_PTR(status);
>  }
>  
>  static int add_children(struct i2c_client *client)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] mfd: dm355evm_msp: Refactoring for add_child()

[SF Markus Elfring]

>> v4: Further feedback was integrated into this message.
> 
> This is not a good change-log.  What actually changed?

Which kind of information would you find more useful in this case?


>> @@ -222,19 +222,20 @@ static struct device *add_child(struct i2c_client *client, const char *name,
>>  		status = platform_device_add_resources(pdev, &r, 1);
>>  		if (status < 0) {
>>  			dev_dbg(&pdev->dev, "can't add irq\n");
>> -			goto err;
>> +			goto put_device;
>>  		}
>>  	}
>>  
>>  	status = platform_device_add(pdev);
>> +	if (status)
>> +		goto put_device;
>>  
>> -err:
>> -	if (status < 0) {
>> -		platform_device_put(pdev);
>> -		dev_err(&client->dev, "can't add %s dev\n", name);
>> -		return ERR_PTR(status);
>> -	}
>>  	return &pdev->dev;
>> +
>> +put_device:
>> +	platform_device_put(pdev);
>> +	dev_err(&client->dev, "failed to add device %s\n", name);
> 
> ... and remove this line.

Do you really want that this error message should be deleted?

How does this response fit to your request to introduce such a message
for the function "add_numbered_child" (on 2016-06-08)?
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1162299.html
https://lkml.org/lkml/2016/6/8/467

Regards,
Markus

Re: [PATCH v4] mfd: dm355evm_msp: Refactoring for add_child()

[SF Markus Elfring]

>> v4: Further feedback was integrated into this message.
> 
> This is not a good change-log.  What actually changed?

Which kind of information would you find more useful in this case?


>> @@ -222,19 +222,20 @@ static struct device *add_child(struct i2c_client *client, const char *name,
>>  		status = platform_device_add_resources(pdev, &r, 1);
>>  		if (status < 0) {
>>  			dev_dbg(&pdev->dev, "can't add irq\n");
>> -			goto err;
>> +			goto put_device;
>>  		}
>>  	}
>>  
>>  	status = platform_device_add(pdev);
>> +	if (status)
>> +		goto put_device;
>>  
>> -err:
>> -	if (status < 0) {
>> -		platform_device_put(pdev);
>> -		dev_err(&client->dev, "can't add %s dev\n", name);
>> -		return ERR_PTR(status);
>> -	}
>>  	return &pdev->dev;
>> +
>> +put_device:
>> +	platform_device_put(pdev);
>> +	dev_err(&client->dev, "failed to add device %s\n", name);
> 
> ... and remove this line.

Do you really want that this error message should be deleted?

How does this response fit to your request to introduce such a message
for the function "add_numbered_child" (on 2016-06-08)?
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1162299.html
https://lkml.org/lkml/2016/6/8/467

Regards,
Markus

Re: [PATCH v4] mfd: dm355evm_msp: Refactoring for add_child()

[Lee Jones]

On Mon, 08 Aug 2016, SF Markus Elfring wrote:

> >> v4: Further feedback was integrated into this message.
> > 
> > This is not a good change-log.  What actually changed?
> 
> Which kind of information would you find more useful in this case?

That's for you to tell me surely?

If I wanted to know what changed, I would normally look at the patch
change-log.  But the change-log in this patch says "I did some
stuff".  What stuff did you change?  Which review comments did you
tend to?
> 
> >> @@ -222,19 +222,20 @@ static struct device *add_child(struct i2c_client *client, const char *name,
> >>  		status = platform_device_add_resources(pdev, &r, 1);
> >>  		if (status < 0) {
> >>  			dev_dbg(&pdev->dev, "can't add irq\n");
> >> -			goto err;
> >> +			goto put_device;
> >>  		}
> >>  	}
> >>  
> >>  	status = platform_device_add(pdev);
> >> +	if (status)
> >> +		goto put_device;
> >>  
> >> -err:
> >> -	if (status < 0) {
> >> -		platform_device_put(pdev);
> >> -		dev_err(&client->dev, "can't add %s dev\n", name);
> >> -		return ERR_PTR(status);
> >> -	}
> >>  	return &pdev->dev;
> >> +
> >> +put_device:
> >> +	platform_device_put(pdev);
> >> +	dev_err(&client->dev, "failed to add device %s\n", name);
> > 
> > ... and remove this line.
> 
> -Do you really want that this error message should be deleted?
> 
> How does this response fit to your request to introduce such a message
> for the function "add_numbered_child" (on 2016-06-08)?
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1162299.html
> https://lkml.org/lkml/2016/6/8/467

You've lost the context.  The "..." is meant to intimate that it
follows on from a previous comment.  In this case:

>  > status = platform_device_add_data(pdev, pdata, pdata_len);
>  > if (status < 0) {
>  > 	dev_dbg(&pdev->dev, "can't add platform_data\n");
>
>  Please take the opportunity to convert these to dev_err()s.

So, convert the specific dev_dbg() calls to dev_err() and remove the
contentless one at the bottom.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4] mfd: dm355evm_msp: Refactoring for add_child()

[Lee Jones]

On Mon, 08 Aug 2016, SF Markus Elfring wrote:

> >> v4: Further feedback was integrated into this message.
> > 
> > This is not a good change-log.  What actually changed?
> 
> Which kind of information would you find more useful in this case?

That's for you to tell me surely?

If I wanted to know what changed, I would normally look at the patch
change-log.  But the change-log in this patch says "I did some
stuff".  What stuff did you change?  Which review comments did you
tend to?
> 
> >> @@ -222,19 +222,20 @@ static struct device *add_child(struct i2c_client *client, const char *name,
> >>  		status = platform_device_add_resources(pdev, &r, 1);
> >>  		if (status < 0) {
> >>  			dev_dbg(&pdev->dev, "can't add irq\n");
> >> -			goto err;
> >> +			goto put_device;
> >>  		}
> >>  	}
> >>  
> >>  	status = platform_device_add(pdev);
> >> +	if (status)
> >> +		goto put_device;
> >>  
> >> -err:
> >> -	if (status < 0) {
> >> -		platform_device_put(pdev);
> >> -		dev_err(&client->dev, "can't add %s dev\n", name);
> >> -		return ERR_PTR(status);
> >> -	}
> >>  	return &pdev->dev;
> >> +
> >> +put_device:
> >> +	platform_device_put(pdev);
> >> +	dev_err(&client->dev, "failed to add device %s\n", name);
> > 
> > ... and remove this line.
> 
> -Do you really want that this error message should be deleted?
> 
> How does this response fit to your request to introduce such a message
> for the function "add_numbered_child" (on 2016-06-08)?
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1162299.html
> https://lkml.org/lkml/2016/6/8/467

You've lost the context.  The "..." is meant to intimate that it
follows on from a previous comment.  In this case:

>  > status = platform_device_add_data(pdev, pdata, pdata_len);
>  > if (status < 0) {
>  > 	dev_dbg(&pdev->dev, "can't add platform_data\n");
>
>  Please take the opportunity to convert these to dev_err()s.

So, convert the specific dev_dbg() calls to dev_err() and remove the
contentless one at the bottom.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] mfd: dm355evm_msp: Refactoring for add_child()

[SF Markus Elfring]

> But the change-log in this patch says "I did some stuff".
> What stuff did you change?  Which review comments did you
> tend to?

I imagine that I could increase the description granularity
to a detail level which you might also not like.


>>>> +put_device:
>>>> +	platform_device_put(pdev);
>>>> +	dev_err(&client->dev, "failed to add device %s\n", name);
>>>
>>> ... and remove this line.
>>
>> Do you really want that this error message should be deleted?
>>
>> How does this response fit to your request to introduce such a message
>> for the function "add_numbered_child" (on 2016-06-08)?
>> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1162299.html
>> https://lkml.org/lkml/2016/6/8/467
> 
> You've lost the context.

I interpreted the suggested message adjustments as separate changes.
So I wondered about a different handling for the Linux modules
"dm355evm_msp" and "twl-core".


> The "..." is meant to intimate that it
> follows on from a previous comment.  In this case:
> 
>>  > status = platform_device_add_data(pdev, pdata, pdata_len);
>>  > if (status < 0) {
>>  > 	dev_dbg(&pdev->dev, "can't add platform_data\n");
>>
>>  Please take the opportunity to convert these to dev_err()s.
> 
> So, convert the specific dev_dbg() calls to dev_err() and remove the
> contentless one at the bottom.

It seems then that you would like to get rid of an error message
at the end while increasing the importance of a related information.

Regards,
Markus

Re: [PATCH v4] mfd: dm355evm_msp: Refactoring for add_child()

[SF Markus Elfring]

> But the change-log in this patch says "I did some stuff".
> What stuff did you change?  Which review comments did you
> tend to?

I imagine that I could increase the description granularity
to a detail level which you might also not like.


>>>> +put_device:
>>>> +	platform_device_put(pdev);
>>>> +	dev_err(&client->dev, "failed to add device %s\n", name);
>>>
>>> ... and remove this line.
>>
>> Do you really want that this error message should be deleted?
>>
>> How does this response fit to your request to introduce such a message
>> for the function "add_numbered_child" (on 2016-06-08)?
>> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1162299.html
>> https://lkml.org/lkml/2016/6/8/467
> 
> You've lost the context.

I interpreted the suggested message adjustments as separate changes.
So I wondered about a different handling for the Linux modules
"dm355evm_msp" and "twl-core".


> The "..." is meant to intimate that it
> follows on from a previous comment.  In this case:
> 
>>  > status = platform_device_add_data(pdev, pdata, pdata_len);
>>  > if (status < 0) {
>>  > 	dev_dbg(&pdev->dev, "can't add platform_data\n");
>>
>>  Please take the opportunity to convert these to dev_err()s.
> 
> So, convert the specific dev_dbg() calls to dev_err() and remove the
> contentless one at the bottom.

It seems then that you would like to get rid of an error message
at the end while increasing the importance of a related information.

Regards,
Markus

Re: [PATCH v4] mfd: dm355evm_msp: Refactoring for add_child()

[Lee Jones]

On Tue, 09 Aug 2016, SF Markus Elfring wrote:

> > But the change-log in this patch says "I did some stuff".
> > What stuff did you change?  Which review comments did you
> > tend to?
> 
> I imagine that I could increase the description granularity
> to a detail level which you might also not like.

Right.  A certain level of common sense needs to be exercised.

> >>>> +put_device:
> >>>> +	platform_device_put(pdev);
> >>>> +	dev_err(&client->dev, "failed to add device %s\n", name);
> >>>
> >>> ... and remove this line.
> >>
> >> Do you really want that this error message should be deleted?
> >>
> >> How does this response fit to your request to introduce such a message
> >> for the function "add_numbered_child" (on 2016-06-08)?
> >> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1162299.html
> >> https://lkml.org/lkml/2016/6/8/467
> > 
> > You've lost the context.
> 
> I interpreted the suggested message adjustments as separate changes.
> So I wondered about a different handling for the Linux modules
> "dm355evm_msp" and "twl-core".

In what way?  The coding standards should be the same.

> > The "..." is meant to intimate that it
> > follows on from a previous comment.  In this case:
> > 
> >>  > status = platform_device_add_data(pdev, pdata, pdata_len);
> >>  > if (status < 0) {
> >>  > 	dev_dbg(&pdev->dev, "can't add platform_data\n");
> >>
> >>  Please take the opportunity to convert these to dev_err()s.
> > 
> > So, convert the specific dev_dbg() calls to dev_err() and remove the
> > contentless one at the bottom.
> 
> It seems then that you would like to get rid of an error message
> at the end while increasing the importance of a related information.

Yes.  Remove the pointless error message at the bottom and provide an
informative one, describing why things went wrong.  Remember; common
sense often prevails.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4] mfd: dm355evm_msp: Refactoring for add_child()

[Lee Jones]

On Tue, 09 Aug 2016, SF Markus Elfring wrote:

> > But the change-log in this patch says "I did some stuff".
> > What stuff did you change?  Which review comments did you
> > tend to?
> 
> I imagine that I could increase the description granularity
> to a detail level which you might also not like.

Right.  A certain level of common sense needs to be exercised.

> >>>> +put_device:
> >>>> +	platform_device_put(pdev);
> >>>> +	dev_err(&client->dev, "failed to add device %s\n", name);
> >>>
> >>> ... and remove this line.
> >>
> >> Do you really want that this error message should be deleted?
> >>
> >> How does this response fit to your request to introduce such a message
> >> for the function "add_numbered_child" (on 2016-06-08)?
> >> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1162299.html
> >> https://lkml.org/lkml/2016/6/8/467
> > 
> > You've lost the context.
> 
> I interpreted the suggested message adjustments as separate changes.
> So I wondered about a different handling for the Linux modules
> "dm355evm_msp" and "twl-core".

In what way?  The coding standards should be the same.

> > The "..." is meant to intimate that it
> > follows on from a previous comment.  In this case:
> > 
> >>  > status = platform_device_add_data(pdev, pdata, pdata_len);
> >>  > if (status < 0) {
> >>  > 	dev_dbg(&pdev->dev, "can't add platform_data\n");
> >>
> >>  Please take the opportunity to convert these to dev_err()s.
> > 
> > So, convert the specific dev_dbg() calls to dev_err() and remove the
> > contentless one at the bottom.
> 
> It seems then that you would like to get rid of an error message
> at the end while increasing the importance of a related information.

Yes.  Remove the pointless error message at the bottom and provide an
informative one, describing why things went wrong.  Remember; common
sense often prevails.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html