[PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices

[PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices

[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

Re: [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices

[Lee Jones]

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

[PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices

[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

[PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices

[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

Re: [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices

[Lee Jones]

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

Re: [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices

[Laszlo Papp]

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

Re: [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices

[Lee Jones]

> 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

Re: [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices

[Laszlo Papp]

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.

Re: [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices

[Richard Weinberger]

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

Re: [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices

[Laszlo Papp]

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.

Re: [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices

[Richard Weinberger]

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

Re: [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices

[Lee Jones]

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

Re: [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices

[Laszlo Papp]

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.

Re: [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices

[Lee Jones]

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

Re: [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices

[Laszlo Papp]

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.

Re: [PATCH] mfd: (max8997) Handle the potential error for mfd_add_devices

[Lee Jones]

> 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