* [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices
@ 2013-12-16 11:47 Laszlo Papp
2013-12-16 13:46 ` Lee Jones
0 siblings, 1 reply; 16+ messages in thread
From: Laszlo Papp @ 2013-12-16 11:47 UTC (permalink / raw)
To: sameo, lee.jones; +Cc: linux-kernel, Laszlo Papp
Signed-off-by: Laszlo Papp <lpapp@kde.org>
---
drivers/mfd/max8997.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/mfd/max8997.c b/drivers/mfd/max8997.c
index 791aea3..c7cc235 100644
--- a/drivers/mfd/max8997.c
+++ b/drivers/mfd/max8997.c
@@ -227,19 +227,19 @@ static int max8997_i2c_probe(struct i2c_client *i2c,
pm_runtime_set_active(max8997->dev);
max8997_irq_init(max8997);
-
- mfd_add_devices(max8997->dev, -1, max8997_devs,
+ ret = mfd_add_devices(max8997->dev, -1, max8997_devs,
ARRAY_SIZE(max8997_devs),
NULL, 0, NULL);
+ if (ret < 0) {
+ dev_err(dev, "cannot add mfd cells\n");
+ goto err_mfd;
+ }
/*
* TODO: enable others (flash, muic, rtc, battery, ...) and
* check the return value
*/
- if (ret < 0)
- goto err_mfd;
-
/* MAX8997 has a power button input. */
device_init_wakeup(max8997->dev, pdata->wakeup);
--
1.8.5.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices
2013-12-16 11:47 [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices Laszlo Papp
@ 2013-12-16 13:46 ` Lee Jones
2013-12-16 13:53 ` Laszlo Papp
0 siblings, 1 reply; 16+ messages in thread
From: Lee Jones @ 2013-12-16 13:46 UTC (permalink / raw)
To: Laszlo Papp; +Cc: sameo, linux-kernel
The $SUBJECT line is wrong. To see how a subsystem usually formats
theirs you must do something like `git log --oneline -- <subsystem>`.
And duplicate the format.
Commit message?
> Signed-off-by: Laszlo Papp <lpapp@kde.org>
> ---
> drivers/mfd/max8997.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mfd/max8997.c b/drivers/mfd/max8997.c
> index 791aea3..c7cc235 100644
> --- a/drivers/mfd/max8997.c
> +++ b/drivers/mfd/max8997.c
> @@ -227,19 +227,19 @@ static int max8997_i2c_probe(struct i2c_client *i2c,
> pm_runtime_set_active(max8997->dev);
>
> max8997_irq_init(max8997);
> -
> - mfd_add_devices(max8997->dev, -1, max8997_devs,
> + ret = mfd_add_devices(max8997->dev, -1, max8997_devs,
> ARRAY_SIZE(max8997_devs),
> NULL, 0, NULL);
> + if (ret < 0) {
> + dev_err(dev, "cannot add mfd cells\n");
> + goto err_mfd;
> + }
Have you tested this patch on h/w? Did you even compile it?
> /*
> * TODO: enable others (flash, muic, rtc, battery, ...) and
> * check the return value
> */
>
> - if (ret < 0)
> - goto err_mfd;
> -
> /* MAX8997 has a power button input. */
> device_init_wakeup(max8997->dev, pdata->wakeup);
>
--
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] 16+ messages in thread
* Re: [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices
2013-12-16 13:46 ` Lee Jones
@ 2013-12-16 13:53 ` Laszlo Papp
2013-12-16 15:09 ` Lee Jones
0 siblings, 1 reply; 16+ messages in thread
From: Laszlo Papp @ 2013-12-16 13:53 UTC (permalink / raw)
To: Lee Jones; +Cc: sameo, linux-kernel
I think you commented on the wrong patch. There has been a newer submitted.
On Mon, Dec 16, 2013 at 1:46 PM, Lee Jones <lee.jones@linaro.org> wrote:
> The $SUBJECT line is wrong. To see how a subsystem usually formats
> theirs you must do something like `git log --oneline -- <subsystem>`.
> And duplicate the format.
>
> Commit message?
>
>> Signed-off-by: Laszlo Papp <lpapp@kde.org>
>> ---
>> drivers/mfd/max8997.c | 10 +++++-----
>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mfd/max8997.c b/drivers/mfd/max8997.c
>> index 791aea3..c7cc235 100644
>> --- a/drivers/mfd/max8997.c
>> +++ b/drivers/mfd/max8997.c
>> @@ -227,19 +227,19 @@ static int max8997_i2c_probe(struct i2c_client *i2c,
>> pm_runtime_set_active(max8997->dev);
>>
>> max8997_irq_init(max8997);
>> -
>> - mfd_add_devices(max8997->dev, -1, max8997_devs,
>> + ret = mfd_add_devices(max8997->dev, -1, max8997_devs,
>> ARRAY_SIZE(max8997_devs),
>> NULL, 0, NULL);
>> + if (ret < 0) {
>> + dev_err(dev, "cannot add mfd cells\n");
>> + goto err_mfd;
>> + }
>
> Have you tested this patch on h/w? Did you even compile it?
>
>> /*
>> * TODO: enable others (flash, muic, rtc, battery, ...) and
>> * check the return value
>> */
>>
>> - if (ret < 0)
>> - goto err_mfd;
>> -
>> /* MAX8997 has a power button input. */
>> device_init_wakeup(max8997->dev, pdata->wakeup);
>>
>
> --
> 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] 16+ messages in thread
* Re: [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices
2013-12-16 13:53 ` Laszlo Papp
@ 2013-12-16 15:09 ` Lee Jones
2013-12-16 15:51 ` Laszlo Papp
0 siblings, 1 reply; 16+ messages in thread
From: Lee Jones @ 2013-12-16 15:09 UTC (permalink / raw)
To: Laszlo Papp; +Cc: sameo, linux-kernel
> I think you commented on the wrong patch. There has been a newer submitted.
No top posting please.
> > The $SUBJECT line is wrong. To see how a subsystem usually formats
> > theirs you must do something like `git log --oneline -- <subsystem>`.
> > And duplicate the format.
> >
> > Commit message?
These comments are still relevant, please re-post your patch with the
points rectified.
> >> Signed-off-by: Laszlo Papp <lpapp@kde.org>
> >> ---
> >> drivers/mfd/max8997.c | 10 +++++-----
> >> 1 file changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/mfd/max8997.c b/drivers/mfd/max8997.c
> >> index 791aea3..c7cc235 100644
> >> --- a/drivers/mfd/max8997.c
> >> +++ b/drivers/mfd/max8997.c
> >> @@ -227,19 +227,19 @@ static int max8997_i2c_probe(struct i2c_client *i2c,
> >> pm_runtime_set_active(max8997->dev);
> >>
> >> max8997_irq_init(max8997);
> >> -
> >> - mfd_add_devices(max8997->dev, -1, max8997_devs,
> >> + ret = mfd_add_devices(max8997->dev, -1, max8997_devs,
> >> ARRAY_SIZE(max8997_devs),
> >> NULL, 0, NULL);
> >> + if (ret < 0) {
> >> + dev_err(dev, "cannot add mfd cells\n");
> >> + goto err_mfd;
> >> + }
> >
> > Have you tested this patch on h/w? Did you even compile it?
You must ensure to test your patches before sending to the MLs, it's
the very least we expect.
> >> /*
> >> * TODO: enable others (flash, muic, rtc, battery, ...) and
> >> * check the return value
> >> */
> >>
> >> - if (ret < 0)
> >> - goto err_mfd;
> >> -
> >> /* MAX8997 has a power button input. */
> >> device_init_wakeup(max8997->dev, pdata->wakeup);
> >>
> >
--
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] 16+ messages in thread
* Re: [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices
2013-12-16 15:09 ` Lee Jones
@ 2013-12-16 15:51 ` Laszlo Papp
2013-12-16 16:05 ` Richard Weinberger
2013-12-16 16:35 ` Lee Jones
0 siblings, 2 replies; 16+ messages in thread
From: Laszlo Papp @ 2013-12-16 15:51 UTC (permalink / raw)
To: Lee Jones; +Cc: sameo, linux-kernel
On Mon, Dec 16, 2013 at 3:09 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> I think you commented on the wrong patch. There has been a newer submitted.
>
> No top posting please.
Tell that to the client I need to use. IMO, making these inline posts
mandatorily when the reply is a single line makes not much sense.
Anyway, I will follow the inconvenient way.
>> > The $SUBJECT line is wrong. To see how a subsystem usually formats
>> > theirs you must do something like `git log --oneline -- <subsystem>`.
>> > And duplicate the format.
>> >
>> > Commit message?
>
> These comments are still relevant, please re-post your patch with the
> points rectified.
I really do not understand how they relevant. "Commit message?" ->
What about it? It has a pretty clear commit message. Are you now just
picking nits about "foo:" vs "(foo)" in the short line?
>> >> Signed-off-by: Laszlo Papp <lpapp@kde.org>
>> >> ---
>> >> drivers/mfd/max8997.c | 10 +++++-----
>> >> 1 file changed, 5 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/drivers/mfd/max8997.c b/drivers/mfd/max8997.c
>> >> index 791aea3..c7cc235 100644
>> >> --- a/drivers/mfd/max8997.c
>> >> +++ b/drivers/mfd/max8997.c
>> >> @@ -227,19 +227,19 @@ static int max8997_i2c_probe(struct i2c_client *i2c,
>> >> pm_runtime_set_active(max8997->dev);
>> >>
>> >> max8997_irq_init(max8997);
>> >> -
>> >> - mfd_add_devices(max8997->dev, -1, max8997_devs,
>> >> + ret = mfd_add_devices(max8997->dev, -1, max8997_devs,
>> >> ARRAY_SIZE(max8997_devs),
>> >> NULL, 0, NULL);
>> >> + if (ret < 0) {
>> >> + dev_err(dev, "cannot add mfd cells\n");
>> >> + goto err_mfd;
>> >> + }
>> >
>> > Have you tested this patch on h/w? Did you even compile it?
>
> You must ensure to test your patches before sending to the MLs, it's
> the very least we expect.
I am not sure what point you are trying to make. Feel free to reject
the patch for this error handling. Clearly, the patch has been updated
due to a previous mistake. I would not make a fuss about an issue
which had been fixed before getting any comment.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices
2013-12-16 15:51 ` Laszlo Papp
@ 2013-12-16 16:05 ` Richard Weinberger
2013-12-16 16:13 ` Laszlo Papp
2013-12-16 16:35 ` Lee Jones
1 sibling, 1 reply; 16+ messages in thread
From: Richard Weinberger @ 2013-12-16 16:05 UTC (permalink / raw)
To: Laszlo Papp; +Cc: Lee Jones, sameo, LKML
On Mon, Dec 16, 2013 at 4:51 PM, Laszlo Papp <lpapp@kde.org> wrote:
>>> > The $SUBJECT line is wrong. To see how a subsystem usually formats
>>> > theirs you must do something like `git log --oneline -- <subsystem>`.
>>> > And duplicate the format.
>>> >
>>> > Commit message?
>>
>> These comments are still relevant, please re-post your patch with the
>> points rectified.
>
> I really do not understand how they relevant. "Commit message?" ->
> What about it? It has a pretty clear commit message. Are you now just
> picking nits about "foo:" vs "(foo)" in the short line?
The commit message needs to have subject and a body.
A good message body explains what the current situation is, why it is
broken/has problems
and how this patch fixes that.
--
Thanks,
//richard
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices
2013-12-16 16:05 ` Richard Weinberger
@ 2013-12-16 16:13 ` Laszlo Papp
2013-12-16 16:20 ` Richard Weinberger
0 siblings, 1 reply; 16+ messages in thread
From: Laszlo Papp @ 2013-12-16 16:13 UTC (permalink / raw)
To: Richard Weinberger; +Cc: Lee Jones, sameo, LKML
On Mon, Dec 16, 2013 at 4:05 PM, Richard Weinberger
<richard.weinberger@gmail.com> wrote:
> On Mon, Dec 16, 2013 at 4:51 PM, Laszlo Papp <lpapp@kde.org> wrote:
>>>> > The $SUBJECT line is wrong. To see how a subsystem usually formats
>>>> > theirs you must do something like `git log --oneline -- <subsystem>`.
>>>> > And duplicate the format.
>>>> >
>>>> > Commit message?
>>>
>>> These comments are still relevant, please re-post your patch with the
>>> points rectified.
>>
>> I really do not understand how they relevant. "Commit message?" ->
>> What about it? It has a pretty clear commit message. Are you now just
>> picking nits about "foo:" vs "(foo)" in the short line?
>
> The commit message needs to have subject and a body.
Why? What advantage does that have in very simple cases like this?
> A good message body explains what the current situation is, why it is
> broken/has problems
> and how this patch fixes that.
The current commit message explains that in my opinion. Please bring
up real issues with it.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices
2013-12-16 16:13 ` Laszlo Papp
@ 2013-12-16 16:20 ` Richard Weinberger
0 siblings, 0 replies; 16+ messages in thread
From: Richard Weinberger @ 2013-12-16 16:20 UTC (permalink / raw)
To: Laszlo Papp; +Cc: Lee Jones, sameo, LKML
On Mon, Dec 16, 2013 at 5:13 PM, Laszlo Papp <lpapp@kde.org> wrote:
> On Mon, Dec 16, 2013 at 4:05 PM, Richard Weinberger
> <richard.weinberger@gmail.com> wrote:
>> On Mon, Dec 16, 2013 at 4:51 PM, Laszlo Papp <lpapp@kde.org> wrote:
>>>>> > The $SUBJECT line is wrong. To see how a subsystem usually formats
>>>>> > theirs you must do something like `git log --oneline -- <subsystem>`.
>>>>> > And duplicate the format.
>>>>> >
>>>>> > Commit message?
>>>>
>>>> These comments are still relevant, please re-post your patch with the
>>>> points rectified.
>>>
>>> I really do not understand how they relevant. "Commit message?" ->
>>> What about it? It has a pretty clear commit message. Are you now just
>>> picking nits about "foo:" vs "(foo)" in the short line?
>>
>> The commit message needs to have subject and a body.
>
> Why? What advantage does that have in very simple cases like this?
>
>> A good message body explains what the current situation is, why it is
>> broken/has problems
>> and how this patch fixes that.
>
> The current commit message explains that in my opinion. Please bring
> up real issues with it.
Now as I read your arguments I remember your name from IRC.
Time to stop the discussion.
--
Thanks,
//richard
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices
2013-12-16 15:51 ` Laszlo Papp
2013-12-16 16:05 ` Richard Weinberger
@ 2013-12-16 16:35 ` Lee Jones
2013-12-16 16:47 ` Laszlo Papp
1 sibling, 1 reply; 16+ messages in thread
From: Lee Jones @ 2013-12-16 16:35 UTC (permalink / raw)
To: Laszlo Papp; +Cc: sameo, linux-kernel
This is not a good introduction to the Kernel Community.
Please adapt your attitude or people will stop helping you.
> >> I think you commented on the wrong patch. There has been a newer submitted.
> >
> > No top posting please.
>
> Tell that to the client I need to use. IMO, making these inline posts
> mandatorily when the reply is a single line makes not much sense.
> Anyway, I will follow the inconvenient way.
If you are not replying to a particular comment, then there is no need
to quote it.
Please read and inwardly digest:
Documentation/email-clients.txt
> >> > The $SUBJECT line is wrong. To see how a subsystem usually formats
> >> > theirs you must do something like `git log --oneline -- <subsystem>`.
> >> > And duplicate the format.
> >> >
> >> > Commit message?
> >
> > These comments are still relevant, please re-post your patch with the
> > points rectified.
>
> I really do not understand how they relevant. "Commit message?" ->
> What about it?
The issue is that there isn't one.
> It has a pretty clear commit message.
If you are referencing my comments about the $SUBJECT line, then I
have to disagree with you there. It's actually pretty vague, does not
describe either the issue or what steps you've taken to rectify it.
> Are you now just
> picking nits about "foo:" vs "(foo)" in the short line?
That is also an issue. Did you issue the command I sent you:
`git log --oneline -- drivers/mfd`
Issue it now and see if _anyone_ has _ever_ used your formatting.
> >> >> + if (ret < 0) {
> >> >> + dev_err(dev, "cannot add mfd cells\n");
> >> >> + goto err_mfd;
> >> >> + }
> >> >
> >> > Have you tested this patch on h/w? Did you even compile it?
> >
> > You must ensure to test your patches before sending to the MLs, it's
> > the very least we expect.
>
> I am not sure what point you are trying to make. Feel free to reject
> the patch for this error handling.
I'm not rejecting it because of the error handling, I'm rejecting it
because it hasn't been tested and it doesn't even compile.
> Clearly, the patch has been updated
> due to a previous mistake. I would not make a fuss about an issue
> which had been fixed before getting any comment.
How was this 'clear'? Our inboxes are date/time sequential.
This patch was read _before_ the one you posted _subsequently_.
--
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] 16+ messages in thread
* Re: [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices
2013-12-16 16:35 ` Lee Jones
@ 2013-12-16 16:47 ` Laszlo Papp
2013-12-16 17:02 ` Lee Jones
0 siblings, 1 reply; 16+ messages in thread
From: Laszlo Papp @ 2013-12-16 16:47 UTC (permalink / raw)
To: Lee Jones; +Cc: sameo, LKML
On Mon, Dec 16, 2013 at 4:35 PM, Lee Jones <lee.jones@linaro.org> wrote:
> This is not a good introduction to the Kernel Community.
>
> Please adapt your attitude or people will stop helping you.
(I would personally like to stay civil here...)
>> >> I think you commented on the wrong patch. There has been a newer submitted.
>> >
>> > No top posting please.
>>
>> Tell that to the client I need to use. IMO, making these inline posts
>> mandatorily when the reply is a single line makes not much sense.
>> Anyway, I will follow the inconvenient way.
>
> If you are not replying to a particular comment, then there is no need
> to quote it.
I did not actually quote anything above my reply.
>
> Please read and inwardly digest:
> Documentation/email-clients.txt
I have read that, however I still have certain restrictions here which
are over the kernel community rules. That should not block a useful
contribution in my opinion.
>> >> > The $SUBJECT line is wrong. To see how a subsystem usually formats
>> >> > theirs you must do something like `git log --oneline -- <subsystem>`.
>> >> > And duplicate the format.
>> >> >
>> >> > Commit message?
>> >
>> > These comments are still relevant, please re-post your patch with the
>> > points rectified.
>>
>> I really do not understand how they relevant. "Commit message?" ->
>> What about it?
>
> The issue is that there isn't one.
I do not follow. Here is the commit message: "mfd: (max8997) Handle
the potential error for mfd_add_devices". What is missing? It now
handles an error for adding mfd devices which was not handled before.
It mentions for which chip. What more needs to be written? I am
currently lost.
>> It has a pretty clear commit message.
>
> If you are referencing my comments about the $SUBJECT line, then I
> have to disagree with you there. It's actually pretty vague, does not
> describe either the issue or what steps you've taken to rectify it.
>
>> Are you now just
>> picking nits about "foo:" vs "(foo)" in the short line?
>
> That is also an issue. Did you issue the command I sent you:
>
> `git log --oneline -- drivers/mfd`
>
> Issue it now and see if _anyone_ has _ever_ used your formatting.
Right, so nitpicking about a minor nuance over a somewhat important
error handling. Is that blocking the error handling change or you can
fix that up yourself? I currently do not have time, nor environment
for satisfy this request. I can probably do it the upcoming days.
>> >> >> + if (ret < 0) {
>> >> >> + dev_err(dev, "cannot add mfd cells\n");
>> >> >> + goto err_mfd;
>> >> >> + }
>> >> >
>> >> > Have you tested this patch on h/w? Did you even compile it?
>> >
>> > You must ensure to test your patches before sending to the MLs, it's
>> > the very least we expect.
>>
>> I am not sure what point you are trying to make. Feel free to reject
>> the patch for this error handling.
>
> I'm not rejecting it because of the error handling, I'm rejecting it
> because it hasn't been tested and it doesn't even compile.
It *has* been tested, and it does compile here. I think you just got
stuck with the old patch rather than taking any look at new version.
May I ask you to do please so? That has been fixed in the new
submission before your email.
>> Clearly, the patch has been updated
>> due to a previous mistake. I would not make a fuss about an issue
>> which had been fixed before getting any comment.
>
> How was this 'clear'? Our inboxes are date/time sequential.
>
> This patch was read _before_ the one you posted _subsequently_.
My personal suggestion is to read the latest as well before replying
to a potentially obsolete email.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices
2013-12-16 16:47 ` Laszlo Papp
@ 2013-12-16 17:02 ` Lee Jones
2013-12-16 17:18 ` Laszlo Papp
0 siblings, 1 reply; 16+ messages in thread
From: Lee Jones @ 2013-12-16 17:02 UTC (permalink / raw)
To: Laszlo Papp; +Cc: sameo, LKML
> >> > No top posting please.
> >>
> >> Tell that to the client I need to use. IMO, making these inline posts
> >> mandatorily when the reply is a single line makes not much sense.
> >> Anyway, I will follow the inconvenient way.
> >
> > If you are not replying to a particular comment, then there is no need
> > to quote it.
>
> I did not actually quote anything above my reply.
No, you quoted the entire message _below_ your reply, which is worse.
> > Please read and inwardly digest:
> > Documentation/email-clients.txt
>
> I have read that, however I still have certain restrictions here which
> are over the kernel community rules. That should not block a useful
> contribution in my opinion.
Your email client does not prevent you from replying inline, which
you've proven by this email. Please abide by the rules if you're going
to contribute.
> >> >> > The $SUBJECT line is wrong. To see how a subsystem usually formats
> >> >> > theirs you must do something like `git log --oneline -- <subsystem>`.
> >> >> > And duplicate the format.
> >> >> >
> >> >> > Commit message?
> >> >
> >> > These comments are still relevant, please re-post your patch with the
> >> > points rectified.
> >>
> >> I really do not understand how they relevant. "Commit message?" ->
> >> What about it?
> >
> > The issue is that there isn't one.
>
> I do not follow. Here is the commit message: "mfd: (max8997) Handle
> the potential error for mfd_add_devices". What is missing? It now
> handles an error for adding mfd devices which was not handled before.
> It mentions for which chip. What more needs to be written? I am
> currently lost.
Please read:
Documentation/SubmittingPatches
Specifically No2.
> >> It has a pretty clear commit message.
> >
> > If you are referencing my comments about the $SUBJECT line, then I
> > have to disagree with you there. It's actually pretty vague, does not
> > describe either the issue or what steps you've taken to rectify it.
> >
> >> Are you now just
> >> picking nits about "foo:" vs "(foo)" in the short line?
> >
> > That is also an issue. Did you issue the command I sent you:
> >
> > `git log --oneline -- drivers/mfd`
> >
> > Issue it now and see if _anyone_ has _ever_ used your formatting.
>
> Right, so nitpicking about a minor nuance over a somewhat important
> error handling. Is that blocking the error handling change or you can
> fix that up yourself? I currently do not have time, nor environment
> for satisfy this request. I can probably do it the upcoming days.
It's not my responsibility to fixup your patches for you. It's your
job to ensure they are correct on submission. I am happy to review
them for you and provide you with my comments, which I have done.
Either fix them up and re-submit or don't. It's no skin off my nose.
> >> >> >> + if (ret < 0) {
> >> >> >> + dev_err(dev, "cannot add mfd cells\n");
> >> >> >> + goto err_mfd;
> >> >> >> + }
> >> >> >
> >> >> > Have you tested this patch on h/w? Did you even compile it?
> >> >
> >> > You must ensure to test your patches before sending to the MLs, it's
> >> > the very least we expect.
> >>
> >> I am not sure what point you are trying to make. Feel free to reject
> >> the patch for this error handling.
> >
> > I'm not rejecting it because of the error handling, I'm rejecting it
> > because it hasn't been tested and it doesn't even compile.
>
> It *has* been tested, and it does compile here. I think you just got
> stuck with the old patch rather than taking any look at new version.
> May I ask you to do please so? That has been fixed in the new
> submission before your email.
I have seen the new patch where you fixed it. My comments are solely in
reference to this patch though. Testing patches _after_ you've sent
them to the MLs is not acceptable.
--
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] 16+ messages in thread
* Re: [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices
2013-12-16 17:02 ` Lee Jones
@ 2013-12-16 17:18 ` Laszlo Papp
2013-12-16 17:43 ` Lee Jones
0 siblings, 1 reply; 16+ messages in thread
From: Laszlo Papp @ 2013-12-16 17:18 UTC (permalink / raw)
To: Lee Jones; +Cc: sameo, LKML
On Mon, Dec 16, 2013 at 5:02 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> >> > No top posting please.
>> >>
>> >> Tell that to the client I need to use. IMO, making these inline posts
>> >> mandatorily when the reply is a single line makes not much sense.
>> >> Anyway, I will follow the inconvenient way.
>> >
>> > If you are not replying to a particular comment, then there is no need
>> > to quote it.
>>
>> I did not actually quote anything above my reply.
>
> No, you quoted the entire message _below_ your reply, which is worse.
Why is that bad? Cannot you just reply to the "top-post" sentence with
dropping all the quotes below if that is what you wish?
>> > Please read and inwardly digest:
>> > Documentation/email-clients.txt
>>
>> I have read that, however I still have certain restrictions here which
>> are over the kernel community rules. That should not block a useful
>> contribution in my opinion.
>
> Your email client does not prevent you from replying inline, which
> you've proven by this email. Please abide by the rules if you're going
> to contribute.
Yes, I can spend more effort with it inconveniently, but currently I
do not see why that is for good. Yes, I will follow, as I already
wrote, but I still do not see why that is better for anyone. :)
>> >> >> > The $SUBJECT line is wrong. To see how a subsystem usually formats
>> >> >> > theirs you must do something like `git log --oneline -- <subsystem>`.
>> >> >> > And duplicate the format.
>> >> >> >
>> >> >> > Commit message?
>> >> >
>> >> > These comments are still relevant, please re-post your patch with the
>> >> > points rectified.
>> >>
>> >> I really do not understand how they relevant. "Commit message?" ->
>> >> What about it?
>> >
>> > The issue is that there isn't one.
>>
>> I do not follow. Here is the commit message: "mfd: (max8997) Handle
>> the potential error for mfd_add_devices". What is missing? It now
>> handles an error for adding mfd devices which was not handled before.
>> It mentions for which chip. What more needs to be written? I am
>> currently lost.
>
> Please read:
> Documentation/SubmittingPatches
>
> Specifically No2.
I had read that, but as written, I am not sure what more you want to
add. Should I replicate the title in the body, pretty much? Please be
specific, and write what you would like to see in the body. I will
copy/paste it. Currently, I am not sure.
>> >> It has a pretty clear commit message.
>> >
>> > If you are referencing my comments about the $SUBJECT line, then I
>> > have to disagree with you there. It's actually pretty vague, does not
>> > describe either the issue or what steps you've taken to rectify it.
>> >
>> >> Are you now just
>> >> picking nits about "foo:" vs "(foo)" in the short line?
>> >
>> > That is also an issue. Did you issue the command I sent you:
>> >
>> > `git log --oneline -- drivers/mfd`
>> >
>> > Issue it now and see if _anyone_ has _ever_ used your formatting.
>>
>> Right, so nitpicking about a minor nuance over a somewhat important
>> error handling. Is that blocking the error handling change or you can
>> fix that up yourself? I currently do not have time, nor environment
>> for satisfy this request. I can probably do it the upcoming days.
>
> It's not my responsibility to fixup your patches for you. It's your
> job to ensure they are correct on submission. I am happy to review
> them for you and provide you with my comments, which I have done.
>
> Either fix them up and re-submit or don't. It's no skin off my nose.
Well, maintainers do it from time to time they apply changes when it
only needs a minor nitpick modification like this in the commit
message, but it is no problem. I am fine the linux kernel not having
this error check, at least for now. :-)
>> >> >> >> + if (ret < 0) {
>> >> >> >> + dev_err(dev, "cannot add mfd cells\n");
>> >> >> >> + goto err_mfd;
>> >> >> >> + }
>> >> >> >
>> >> >> > Have you tested this patch on h/w? Did you even compile it?
>> >> >
>> >> > You must ensure to test your patches before sending to the MLs, it's
>> >> > the very least we expect.
>> >>
>> >> I am not sure what point you are trying to make. Feel free to reject
>> >> the patch for this error handling.
>> >
>> > I'm not rejecting it because of the error handling, I'm rejecting it
>> > because it hasn't been tested and it doesn't even compile.
>>
>> It *has* been tested, and it does compile here. I think you just got
>> stuck with the old patch rather than taking any look at new version.
>> May I ask you to do please so? That has been fixed in the new
>> submission before your email.
>
> I have seen the new patch where you fixed it. My comments are solely in
> reference to this patch though. Testing patches _after_ you've sent
> them to the MLs is not acceptable.
I am afraid, you are incorrect here. I did test it before. It was a
patch sending workflow issue, but even then: why cannot we move on? It
had been fixed pretty much within a few seconds, and then I was
resending it.
> The $SUBJECT line does not conform to what's expected of MFD commits.
> The $SUBJECT line is vague and you are missing a commit body.
As written, I am lost what I would need to add to the commit message.
Please advise, and I will copy and paste that before resending.
> Why have you removed this line?
That is just noise. It is better not to remove it, thanks.
> We're adding devices here, not cells.
> "failed to add devices"
That description is not chosen by me. Actually, that is coming from
the other mfd drivers, particularly from the other existing MAXIM mfd
driver. I would not personally break the consistency there.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices
2013-12-16 17:18 ` Laszlo Papp
@ 2013-12-16 17:43 ` Lee Jones
0 siblings, 0 replies; 16+ messages in thread
From: Lee Jones @ 2013-12-16 17:43 UTC (permalink / raw)
To: Laszlo Papp; +Cc: sameo, LKML
> Why is that bad? Cannot you just reply to the "top-post" sentence with
> dropping all the quotes below if that is what you wish?
Yes, only quote what you're replying to and quote _below_.
> >> > Please read and inwardly digest:
> >> > Documentation/email-clients.txt
> >>
> >> I have read that, however I still have certain restrictions here which
> >> are over the kernel community rules. That should not block a useful
> >> contribution in my opinion.
> >
> > Your email client does not prevent you from replying inline, which
> > you've proven by this email. Please abide by the rules if you're going
> > to contribute.
>
> Yes, I can spend more effort with it inconveniently,
Great, thanks.
<snip>
> > Please read:
> > Documentation/SubmittingPatches
> >
> > Specifically No2.
>
> I had read that, but as written, I am not sure what more you want to
> add. Should I replicate the title in the body, pretty much? Please be
> specific, and write what you would like to see in the body. I will
> copy/paste it. Currently, I am not sure.
A good commit for your patch might look like this:
mfd: max8997: Enforce mfd_add_devices() return value check
The original author provided a random return value check which is
redundant and seemingly floating. This patch not only relocates
the check so it is more clearly associated with the invokation of
mfd_add_devices(), but provides a store for the error value. We
also print a meaningful message on error before returning.
> > It's not my responsibility to fixup your patches for you. It's your
> > job to ensure they are correct on submission. I am happy to review
> > them for you and provide you with my comments, which I have done.
> >
> > Either fix them up and re-submit or don't. It's no skin off my nose.
>
> Well, maintainers do it from time to time they apply changes when it
> only needs a minor nitpick modification like this in the commit
> message, but it is no problem. I am fine the linux kernel not having
> this error check, at least for now. :-)
It's better that you do it yourself. If I fixed up everything that was
sent to me incorrect a) I'd have no time left and b) they'd never learn.
This is about teaching a man to fish.
> > The $SUBJECT line does not conform to what's expected of MFD commits.
> > The $SUBJECT line is vague and you are missing a commit body.
>
> As written, I am lost what I would need to add to the commit message.
> Please advise, and I will copy and paste that before resending.
>
> > Why have you removed this line?
>
> That is just noise. It is better not to remove it, thanks.
>
> > We're adding devices here, not cells.
> > "failed to add devices"
>
> That description is not chosen by me. Actually, that is coming from
> the other mfd drivers, particularly from the other existing MAXIM mfd
> driver. I would not personally break the consistency there.
Please reply to the patch in question, or else things could get
confusing.
--
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] 16+ messages in thread
* Re: [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices
2013-12-16 12:01 Laszlo Papp
@ 2013-12-16 17:07 ` Lee Jones
0 siblings, 0 replies; 16+ messages in thread
From: Lee Jones @ 2013-12-16 17:07 UTC (permalink / raw)
To: Laszlo Papp; +Cc: sameo, linux-kernel
The $SUBJECT line does not conform to what's expected of MFD commits.
The $SUBJECT line is vague and you are missing a commit body.
Please read:
Documentation/SubmittingPatches
Specifically No2.
> Signed-off-by: Laszlo Papp <lpapp@kde.org>
> ---
> drivers/mfd/max8997.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mfd/max8997.c b/drivers/mfd/max8997.c
> index 791aea3..ace1a2b 100644
> --- a/drivers/mfd/max8997.c
> +++ b/drivers/mfd/max8997.c
> @@ -227,19 +227,19 @@ static int max8997_i2c_probe(struct i2c_client *i2c,
> pm_runtime_set_active(max8997->dev);
>
> max8997_irq_init(max8997);
> -
Why have you removed this line?
> - mfd_add_devices(max8997->dev, -1, max8997_devs,
> + ret = mfd_add_devices(max8997->dev, -1, max8997_devs,
> ARRAY_SIZE(max8997_devs),
> NULL, 0, NULL);
> + if (ret < 0) {
> + dev_err(max8997->dev, "cannot add mfd cells\n");
We're adding devices here, not cells.
"failed to add devices"
> + goto err_mfd;
> + }
>
> /*
> * TODO: enable others (flash, muic, rtc, battery, ...) and
> * check the return value
> */
>
> - if (ret < 0)
> - goto err_mfd;
> -
> /* MAX8997 has a power button input. */
> device_init_wakeup(max8997->dev, pdata->wakeup);
>
--
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] 16+ messages in thread
* [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices
@ 2013-12-16 12:01 Laszlo Papp
2013-12-16 17:07 ` Lee Jones
0 siblings, 1 reply; 16+ messages in thread
From: Laszlo Papp @ 2013-12-16 12:01 UTC (permalink / raw)
To: sameo, lee.jones; +Cc: linux-kernel, Laszlo Papp
Signed-off-by: Laszlo Papp <lpapp@kde.org>
---
drivers/mfd/max8997.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/mfd/max8997.c b/drivers/mfd/max8997.c
index 791aea3..ace1a2b 100644
--- a/drivers/mfd/max8997.c
+++ b/drivers/mfd/max8997.c
@@ -227,19 +227,19 @@ static int max8997_i2c_probe(struct i2c_client *i2c,
pm_runtime_set_active(max8997->dev);
max8997_irq_init(max8997);
-
- mfd_add_devices(max8997->dev, -1, max8997_devs,
+ ret = mfd_add_devices(max8997->dev, -1, max8997_devs,
ARRAY_SIZE(max8997_devs),
NULL, 0, NULL);
+ if (ret < 0) {
+ dev_err(max8997->dev, "cannot add mfd cells\n");
+ goto err_mfd;
+ }
/*
* TODO: enable others (flash, muic, rtc, battery, ...) and
* check the return value
*/
- if (ret < 0)
- goto err_mfd;
-
/* MAX8997 has a power button input. */
device_init_wakeup(max8997->dev, pdata->wakeup);
--
1.8.5.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices
@ 2013-12-16 12:01 Laszlo Papp
0 siblings, 0 replies; 16+ messages in thread
From: Laszlo Papp @ 2013-12-16 12:01 UTC (permalink / raw)
To: sameo, lee.jones; +Cc: linux-kernel, Laszlo Papp
Signed-off-by: Laszlo Papp <lpapp@kde.org>
---
drivers/mfd/max8997.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/mfd/max8997.c b/drivers/mfd/max8997.c
index 791aea3..c7cc235 100644
--- a/drivers/mfd/max8997.c
+++ b/drivers/mfd/max8997.c
@@ -227,19 +227,19 @@ static int max8997_i2c_probe(struct i2c_client *i2c,
pm_runtime_set_active(max8997->dev);
max8997_irq_init(max8997);
-
- mfd_add_devices(max8997->dev, -1, max8997_devs,
+ ret = mfd_add_devices(max8997->dev, -1, max8997_devs,
ARRAY_SIZE(max8997_devs),
NULL, 0, NULL);
+ if (ret < 0) {
+ dev_err(dev, "cannot add mfd cells\n");
+ goto err_mfd;
+ }
/*
* TODO: enable others (flash, muic, rtc, battery, ...) and
* check the return value
*/
- if (ret < 0)
- goto err_mfd;
-
/* MAX8997 has a power button input. */
device_init_wakeup(max8997->dev, pdata->wakeup);
--
1.8.5.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
end of thread, other threads:[~2013-12-16 17:43 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-12-16 11:47 [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices Laszlo Papp
2013-12-16 13:46 ` Lee Jones
2013-12-16 13:53 ` Laszlo Papp
2013-12-16 15:09 ` Lee Jones
2013-12-16 15:51 ` Laszlo Papp
2013-12-16 16:05 ` Richard Weinberger
2013-12-16 16:13 ` Laszlo Papp
2013-12-16 16:20 ` Richard Weinberger
2013-12-16 16:35 ` Lee Jones
2013-12-16 16:47 ` Laszlo Papp
2013-12-16 17:02 ` Lee Jones
2013-12-16 17:18 ` Laszlo Papp
2013-12-16 17:43 ` Lee Jones
2013-12-16 12:01 Laszlo Papp
2013-12-16 12:01 Laszlo Papp
2013-12-16 17:07 ` Lee Jones
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.