Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1456926 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4] mfd: dm355evm_msp: Refactoring for add_child() Lee Jones <lee.jones@linaro.org> - 2016-08-05 10:00 +0200
    Re: [PATCH v4] mfd: dm355evm_msp: Refactoring for add_child() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-08-08 13:40 +0200
      Re: [PATCH v4] mfd: dm355evm_msp: Refactoring for add_child() Lee Jones <lee.jones@linaro.org> - 2016-08-09 11:30 +0200
        Re: [PATCH v4] mfd: dm355evm_msp: Refactoring for add_child() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-08-09 12:00 +0200
          Re: [PATCH v4] mfd: dm355evm_msp: Refactoring for add_child() Lee Jones <lee.jones@linaro.org> - 2016-08-09 17:40 +0200

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

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

[toc] | [next] | [standalone]

#1457736

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

[toc] | [prev] | [next] | [standalone]

#1458542

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

[toc] | [prev] | [next] | [standalone]

#1458559

> 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

[toc] | [prev] | [next] | [standalone]

#1458857

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web