* [PATCH v4] mfd: dm355evm_msp: Refactoring for add_child()
@ 2016-07-01 17:29 SF Markus Elfring
2016-08-05 7:55 ` Lee Jones
0 siblings, 1 reply; 6+ messages in thread
From: SF Markus Elfring @ 2016-07-01 17:29 UTC (permalink / raw)
To: Lee Jones, kernel-janitors; +Cc: Linux Kernel Mailing List, Julia Lawall
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v4] mfd: dm355evm_msp: Refactoring for add_child()
2016-07-01 17:29 [PATCH v4] mfd: dm355evm_msp: Refactoring for add_child() SF Markus Elfring
@ 2016-08-05 7:55 ` Lee Jones
2016-08-08 11:36 ` SF Markus Elfring
0 siblings, 1 reply; 6+ messages in thread
From: Lee Jones @ 2016-08-05 7:55 UTC (permalink / raw)
To: SF Markus Elfring
Cc: kernel-janitors, Linux Kernel Mailing List, Julia Lawall
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v4] mfd: dm355evm_msp: Refactoring for add_child()
2016-08-05 7:55 ` Lee Jones
@ 2016-08-08 11:36 ` SF Markus Elfring
2016-08-09 9:30 ` Lee Jones
0 siblings, 1 reply; 6+ messages in thread
From: SF Markus Elfring @ 2016-08-08 11:36 UTC (permalink / raw)
To: Lee Jones; +Cc: LKML, kernel-janitors, Julia Lawall
>> 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v4] mfd: dm355evm_msp: Refactoring for add_child()
2016-08-08 11:36 ` SF Markus Elfring
@ 2016-08-09 9:30 ` Lee Jones
2016-08-09 9:56 ` SF Markus Elfring
0 siblings, 1 reply; 6+ messages in thread
From: Lee Jones @ 2016-08-09 9:30 UTC (permalink / raw)
To: SF Markus Elfring; +Cc: LKML, kernel-janitors, Julia Lawall
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v4] mfd: dm355evm_msp: Refactoring for add_child()
2016-08-09 9:30 ` Lee Jones
@ 2016-08-09 9:56 ` SF Markus Elfring
2016-08-09 15:36 ` Lee Jones
0 siblings, 1 reply; 6+ messages in thread
From: SF Markus Elfring @ 2016-08-09 9:56 UTC (permalink / raw)
To: Lee Jones; +Cc: LKML, kernel-janitors, Julia Lawall
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v4] mfd: dm355evm_msp: Refactoring for add_child()
2016-08-09 9:56 ` SF Markus Elfring
@ 2016-08-09 15:36 ` Lee Jones
0 siblings, 0 replies; 6+ messages in thread
From: Lee Jones @ 2016-08-09 15:36 UTC (permalink / raw)
To: SF Markus Elfring; +Cc: LKML, kernel-janitors, Julia Lawall
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-08-09 15:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-07-01 17:29 [PATCH v4] mfd: dm355evm_msp: Refactoring for add_child() SF Markus Elfring
2016-08-05 7:55 ` Lee Jones
2016-08-08 11:36 ` SF Markus Elfring
2016-08-09 9:30 ` Lee Jones
2016-08-09 9:56 ` SF Markus Elfring
2016-08-09 15:36 ` Lee Jones
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).