If the gpios are probed after this driver (e.g. if they
come from an i2c expander) there is no need to print an
error message.
Signed-off-by: H. Nikolaus Schaller <[email protected]>
---
drivers/extcon/extcon-palmas.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
index edc5016f46f1..cea58d0cb457 100644
--- a/drivers/extcon/extcon-palmas.c
+++ b/drivers/extcon/extcon-palmas.c
@@ -205,14 +205,18 @@ static int palmas_usb_probe(struct platform_device *pdev)
palmas_usb->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id",
GPIOD_IN);
- if (IS_ERR(palmas_usb->id_gpiod)) {
+ if (PTR_ERR(palmas_usb->id_gpiod) == -EPROBE_DEFER) {
+ return -EPROBE_DEFER;
+ } else if (IS_ERR(palmas_usb->id_gpiod)) {
dev_err(&pdev->dev, "failed to get id gpio\n");
return PTR_ERR(palmas_usb->id_gpiod);
}
palmas_usb->vbus_gpiod = devm_gpiod_get_optional(&pdev->dev, "vbus",
GPIOD_IN);
- if (IS_ERR(palmas_usb->vbus_gpiod)) {
+ if (PTR_ERR(palmas_usb->vbus_gpiod) == -EPROBE_DEFER) {
+ return -EPROBE_DEFER;
+ } else if (IS_ERR(palmas_usb->vbus_gpiod)) {
dev_err(&pdev->dev, "failed to get vbus gpio\n");
return PTR_ERR(palmas_usb->vbus_gpiod);
}
--
2.23.0
> Am 17.02.2020 um 14:38 schrieb H. Nikolaus Schaller <[email protected]>:
>
> If the gpios are probed after this driver (e.g. if they
> come from an i2c expander) there is no need to print an
> error message.
>
> Signed-off-by: H. Nikolaus Schaller <[email protected]>
> ---
> drivers/extcon/extcon-palmas.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
> index edc5016f46f1..cea58d0cb457 100644
> --- a/drivers/extcon/extcon-palmas.c
> +++ b/drivers/extcon/extcon-palmas.c
> @@ -205,14 +205,18 @@ static int palmas_usb_probe(struct platform_device *pdev)
>
> palmas_usb->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id",
> GPIOD_IN);
> - if (IS_ERR(palmas_usb->id_gpiod)) {
> + if (PTR_ERR(palmas_usb->id_gpiod) == -EPROBE_DEFER) {
> + return -EPROBE_DEFER;
> + } else if (IS_ERR(palmas_usb->id_gpiod)) {
Hm.
While looking again at that: why do we need the "{" and "} else "?
It should be sufficient to have
> palmas_usb->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id",
> GPIOD_IN);
> + if (PTR_ERR(palmas_usb->id_gpiod) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> if (IS_ERR(palmas_usb->id_gpiod)) {
What do you think is better coding style here?
BR,
Nikolaus Schaller
On Mon, Feb 17, 2020 at 02:58:14PM +0100, H. Nikolaus Schaller wrote:
>
> > Am 17.02.2020 um 14:38 schrieb H. Nikolaus Schaller <[email protected]>:
> >
> > If the gpios are probed after this driver (e.g. if they
> > come from an i2c expander) there is no need to print an
> > error message.
> >
> > Signed-off-by: H. Nikolaus Schaller <[email protected]>
> > ---
> > drivers/extcon/extcon-palmas.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
> > index edc5016f46f1..cea58d0cb457 100644
> > --- a/drivers/extcon/extcon-palmas.c
> > +++ b/drivers/extcon/extcon-palmas.c
> > @@ -205,14 +205,18 @@ static int palmas_usb_probe(struct platform_device *pdev)
> >
> > palmas_usb->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id",
> > GPIOD_IN);
> > - if (IS_ERR(palmas_usb->id_gpiod)) {
> > + if (PTR_ERR(palmas_usb->id_gpiod) == -EPROBE_DEFER) {
> > + return -EPROBE_DEFER;
> > + } else if (IS_ERR(palmas_usb->id_gpiod)) {
>
> Hm.
>
> While looking again at that: why do we need the "{" and "} else "?
>
> It should be sufficient to have
>
> > palmas_usb->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id",
> > GPIOD_IN);
> > + if (PTR_ERR(palmas_usb->id_gpiod) == -EPROBE_DEFER)
> > + return -EPROBE_DEFER;
> > if (IS_ERR(palmas_usb->id_gpiod)) {
>
> What do you think is better coding style here?
How about something like this? (just an idea with some work left for you ;-))
--- a/drivers/extcon/extcon-palmas.c
+++ b/drivers/extcon/extcon-palmas.c
@@ -206,8 +206,10 @@ static int palmas_usb_probe(struct platform_device *pdev)
palmas_usb->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id",
GPIOD_IN);
if (IS_ERR(palmas_usb->id_gpiod)) {
- dev_err(&pdev->dev, "failed to get id gpio\n");
- return PTR_ERR(palmas_usb->id_gpiod);
+ status = PTR_ERR(palmas_usb->id_gpiod);
+ if (status != -EPROBE_DEFER)
+ dev_err(&pdev->dev, "failed to get id gpio: %d\n", status);
+ return status;
}
palmas_usb->vbus_gpiod = devm_gpiod_get_optional(&pdev->dev, "vbus",
Hi,
> Am 17.02.2020 um 19:29 schrieb Ladislav Michl <[email protected]>:
>
> On Mon, Feb 17, 2020 at 02:58:14PM +0100, H. Nikolaus Schaller wrote:
>>
>>> Am 17.02.2020 um 14:38 schrieb H. Nikolaus Schaller <[email protected]>:
>>>
>>> If the gpios are probed after this driver (e.g. if they
>>> come from an i2c expander) there is no need to print an
>>> error message.
>>>
>>> Signed-off-by: H. Nikolaus Schaller <[email protected]>
>>> ---
>>> drivers/extcon/extcon-palmas.c | 8 ++++++--
>>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
>>> index edc5016f46f1..cea58d0cb457 100644
>>> --- a/drivers/extcon/extcon-palmas.c
>>> +++ b/drivers/extcon/extcon-palmas.c
>>> @@ -205,14 +205,18 @@ static int palmas_usb_probe(struct platform_device *pdev)
>>>
>>> palmas_usb->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id",
>>> GPIOD_IN);
>>> - if (IS_ERR(palmas_usb->id_gpiod)) {
>>> + if (PTR_ERR(palmas_usb->id_gpiod) == -EPROBE_DEFER) {
>>> + return -EPROBE_DEFER;
>>> + } else if (IS_ERR(palmas_usb->id_gpiod)) {
>>
>> Hm.
>>
>> While looking again at that: why do we need the "{" and "} else "?
>>
>> It should be sufficient to have
>>
>>> palmas_usb->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id",
>>> GPIOD_IN);
>>> + if (PTR_ERR(palmas_usb->id_gpiod) == -EPROBE_DEFER)
>>> + return -EPROBE_DEFER;
>>> if (IS_ERR(palmas_usb->id_gpiod)) {
>>
>> What do you think is better coding style here?
>
> How about something like this? (just an idea with some work left for you ;-))
>
> --- a/drivers/extcon/extcon-palmas.c
> +++ b/drivers/extcon/extcon-palmas.c
> @@ -206,8 +206,10 @@ static int palmas_usb_probe(struct platform_device *pdev)
> palmas_usb->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id",
> GPIOD_IN);
> if (IS_ERR(palmas_usb->id_gpiod)) {
> - dev_err(&pdev->dev, "failed to get id gpio\n");
> - return PTR_ERR(palmas_usb->id_gpiod);
> + status = PTR_ERR(palmas_usb->id_gpiod);
> + if (status != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "failed to get id gpio: %d\n", status);
> + return status;
> }
Well, what would be the improvement?
It needs an additional variable and makes the change more complex.
The main suggestion by Chanwoo Choi was to move the check for EPROBE_DEFER
outside of the IS_ERR() because checking this first and then for EPROBE_DEFER
is not necessary.
If acceptable I'd prefer my last proposal. It just adds 2 LOC before
and without touching the existing if (IS_ERR(...)).
If the compiler is clever it can cache palmas_usb->id_gpiod in a register
which serves the same purpose as the status variable.
>
> palmas_usb->vbus_gpiod = devm_gpiod_get_optional(&pdev->dev, "vbus",
BR and thanks,
Nikolaus
On Mon, Feb 17, 2020 at 07:38:16PM +0100, H. Nikolaus Schaller wrote:
> Hi,
>
> > Am 17.02.2020 um 19:29 schrieb Ladislav Michl <[email protected]>:
> >
> > On Mon, Feb 17, 2020 at 02:58:14PM +0100, H. Nikolaus Schaller wrote:
> >>
> >>> Am 17.02.2020 um 14:38 schrieb H. Nikolaus Schaller <[email protected]>:
> >>>
> >>> If the gpios are probed after this driver (e.g. if they
> >>> come from an i2c expander) there is no need to print an
> >>> error message.
> >>>
> >>> Signed-off-by: H. Nikolaus Schaller <[email protected]>
> >>> ---
> >>> drivers/extcon/extcon-palmas.c | 8 ++++++--
> >>> 1 file changed, 6 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
> >>> index edc5016f46f1..cea58d0cb457 100644
> >>> --- a/drivers/extcon/extcon-palmas.c
> >>> +++ b/drivers/extcon/extcon-palmas.c
> >>> @@ -205,14 +205,18 @@ static int palmas_usb_probe(struct platform_device *pdev)
> >>>
> >>> palmas_usb->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id",
> >>> GPIOD_IN);
> >>> - if (IS_ERR(palmas_usb->id_gpiod)) {
> >>> + if (PTR_ERR(palmas_usb->id_gpiod) == -EPROBE_DEFER) {
> >>> + return -EPROBE_DEFER;
> >>> + } else if (IS_ERR(palmas_usb->id_gpiod)) {
> >>
> >> Hm.
> >>
> >> While looking again at that: why do we need the "{" and "} else "?
> >>
> >> It should be sufficient to have
> >>
> >>> palmas_usb->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id",
> >>> GPIOD_IN);
> >>> + if (PTR_ERR(palmas_usb->id_gpiod) == -EPROBE_DEFER)
> >>> + return -EPROBE_DEFER;
> >>> if (IS_ERR(palmas_usb->id_gpiod)) {
> >>
> >> What do you think is better coding style here?
> >
> > How about something like this? (just an idea with some work left for you ;-))
> >
> > --- a/drivers/extcon/extcon-palmas.c
> > +++ b/drivers/extcon/extcon-palmas.c
> > @@ -206,8 +206,10 @@ static int palmas_usb_probe(struct platform_device *pdev)
> > palmas_usb->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id",
> > GPIOD_IN);
> > if (IS_ERR(palmas_usb->id_gpiod)) {
> > - dev_err(&pdev->dev, "failed to get id gpio\n");
> > - return PTR_ERR(palmas_usb->id_gpiod);
> > + status = PTR_ERR(palmas_usb->id_gpiod);
> > + if (status != -EPROBE_DEFER)
> > + dev_err(&pdev->dev, "failed to get id gpio: %d\n", status);
> > + return status;
> > }
>
> Well, what would be the improvement?
Linux kernel prints so many lines on bootup and only few of them are
valuable. Lets improve it by printing error value to give a clue
why it failed.
> It needs an additional variable and makes the change more complex.
That additional variable is already there...
> The main suggestion by Chanwoo Choi was to move the check for EPROBE_DEFER
> outside of the IS_ERR() because checking this first and then for EPROBE_DEFER
> is not necessary.
True, but there are two checks either way and this is slow path.
> If acceptable I'd prefer my last proposal. It just adds 2 LOC before
> and without touching the existing if (IS_ERR(...)).
I have no strong opinion. I was just waiting for project to compile
so, consider my reply as product of boredom :)
(However, I do not like "let's touch only minimal number of lines"
argument. End result should still matter more)
> If the compiler is clever it can cache palmas_usb->id_gpiod in a register
> which serves the same purpose as the status variable.
I'm not trying to outsmart compiler, but note status variable is needed
three times.
> >
> > palmas_usb->vbus_gpiod = devm_gpiod_get_optional(&pdev->dev, "vbus",
>
> BR and thanks,
> Nikolaus
Hi Ladis,
> Am 17.02.2020 um 20:07 schrieb Ladislav Michl <[email protected]>:
>
> On Mon, Feb 17, 2020 at 07:38:16PM +0100, H. Nikolaus Schaller wrote:
>> Hi,
>>
>>> Am 17.02.2020 um 19:29 schrieb Ladislav Michl <[email protected]>:
>>>
>>> On Mon, Feb 17, 2020 at 02:58:14PM +0100, H. Nikolaus Schaller wrote:
>>>>
>>>>> Am 17.02.2020 um 14:38 schrieb H. Nikolaus Schaller <[email protected]>:
>>>>>
>>>>> If the gpios are probed after this driver (e.g. if they
>>>>> come from an i2c expander) there is no need to print an
>>>>> error message.
>>>>>
>>>>> Signed-off-by: H. Nikolaus Schaller <[email protected]>
>>>>> ---
>>>>> drivers/extcon/extcon-palmas.c | 8 ++++++--
>>>>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
>>>>> index edc5016f46f1..cea58d0cb457 100644
>>>>> --- a/drivers/extcon/extcon-palmas.c
>>>>> +++ b/drivers/extcon/extcon-palmas.c
>>>>> @@ -205,14 +205,18 @@ static int palmas_usb_probe(struct platform_device *pdev)
>>>>>
>>>>> palmas_usb->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id",
>>>>> GPIOD_IN);
>>>>> - if (IS_ERR(palmas_usb->id_gpiod)) {
>>>>> + if (PTR_ERR(palmas_usb->id_gpiod) == -EPROBE_DEFER) {
>>>>> + return -EPROBE_DEFER;
>>>>> + } else if (IS_ERR(palmas_usb->id_gpiod)) {
>>>>
>>>> Hm.
>>>>
>>>> While looking again at that: why do we need the "{" and "} else "?
>>>>
>>>> It should be sufficient to have
>>>>
>>>>> palmas_usb->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id",
>>>>> GPIOD_IN);
>>>>> + if (PTR_ERR(palmas_usb->id_gpiod) == -EPROBE_DEFER)
>>>>> + return -EPROBE_DEFER;
>>>>> if (IS_ERR(palmas_usb->id_gpiod)) {
>>>>
>>>> What do you think is better coding style here?
>>>
>>> How about something like this? (just an idea with some work left for you ;-))
>>>
>>> --- a/drivers/extcon/extcon-palmas.c
>>> +++ b/drivers/extcon/extcon-palmas.c
>>> @@ -206,8 +206,10 @@ static int palmas_usb_probe(struct platform_device *pdev)
>>> palmas_usb->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id",
>>> GPIOD_IN);
>>> if (IS_ERR(palmas_usb->id_gpiod)) {
>>> - dev_err(&pdev->dev, "failed to get id gpio\n");
>>> - return PTR_ERR(palmas_usb->id_gpiod);
>>> + status = PTR_ERR(palmas_usb->id_gpiod);
>>> + if (status != -EPROBE_DEFER)
>>> + dev_err(&pdev->dev, "failed to get id gpio: %d\n", status);
>>> + return status;
>>> }
>>
>> Well, what would be the improvement?
>
> Linux kernel prints so many lines on bootup and only few of them are
> valuable. Lets improve it by printing error value to give a clue
> why it failed.
Hm. The upstream code does already print the error. This feature is not removed.
But it is also printing an error in the EPROBE_DEFER case as well, where it is
not needed or not an error.
And that is the whole purpose of this patch to get rid of it in the EPROBE_DEFER
case.
My question meant: what your proposal does improve over previous versions of
this patch. My first one was:
https://lkml.org/lkml/2020/2/17/220
which is very similar except not using an explicit temporary variable (which I
think is something every modern compiler will introduce). So the assembler
code is likely the same.
>
>> It needs an additional variable and makes the change more complex.
>
> That additional variable is already there...
Or a register assigned by the compiler.
>
>> The main suggestion by Chanwoo Choi was to move the check for EPROBE_DEFER
>> outside of the IS_ERR() because checking this first and then for EPROBE_DEFER
>> is not necessary.
>
> True, but there are two checks either way and this is slow path.
Well, it depends on likelihood of each code path... That is quite
difficult to assess.
>
>> If acceptable I'd prefer my last proposal. It just adds 2 LOC before
>> and without touching the existing if (IS_ERR(...)).
>
> I have no strong opinion. I was just waiting for project to compile
> so, consider my reply as product of boredom :)
Yes, compile times have increased significantly over the years :)
> (However, I do not like "let's touch only minimal number of lines"
> argument. End result should still matter more)
I would have been happy with my first proposal, but review suggested
to change it.
There is also some discussion for using IS_ERR() and PTR_ERR() == -Esomething
first or second: https://lore.kernel.org/patchwork/patch/999602/
Well, about the end-result: this code path is run only once during
probe time. And that makes a difference in the sub-µs range. So I don't
mind about the likelyhood. More concern is to have the code correct
and not introduce regressions.
And there the lines of code rule comes in: the less I change the less
I can break.
(Yes my compiler is also busy making me wait for result... so that
I can formulate such fundamental statements :).
>
>> If the compiler is clever it can cache palmas_usb->id_gpiod in a register
>> which serves the same purpose as the status variable.
>
> I'm not trying to outsmart compiler, but note status variable is needed
> three times.
BR and thanks,
Nikolaus
Hi Nikolaus,
On Mon, Feb 17, 2020 at 08:33:52PM +0100, H. Nikolaus Schaller wrote:
> Hi Ladis,
>
> > Am 17.02.2020 um 20:07 schrieb Ladislav Michl <[email protected]>:
> > Linux kernel prints so many lines on bootup and only few of them are
> > valuable. Lets improve it by printing error value to give a clue
> > why it failed.
>
> Hm. The upstream code does already print the error. This feature is not removed.
> But it is also printing an error in the EPROBE_DEFER case as well, where it is
> not needed or not an error.
It seems you missed I'm printing also error _value_. The rest of discussion
would need disassembly and I'll do it once I'll be waiting for yet another
long run recompile.
ladis
Hi Ladis,
> Am 17.02.2020 um 21:19 schrieb Ladislav Michl <[email protected]>:
>
> Hi Nikolaus,
>
> On Mon, Feb 17, 2020 at 08:33:52PM +0100, H. Nikolaus Schaller wrote:
>> Hi Ladis,
>>
>>> Am 17.02.2020 um 20:07 schrieb Ladislav Michl <[email protected]>:
>>> Linux kernel prints so many lines on bootup and only few of them are
>>> valuable. Lets improve it by printing error value to give a clue
>>> why it failed.
>>
>> Hm. The upstream code does already print the error. This feature is not removed.
>> But it is also printing an error in the EPROBE_DEFER case as well, where it is
>> not needed or not an error.
>
> It seems you missed I'm printing also error _value_.
Indeed.
Well, usually there is no error so adding the error number is not of much
use (except for debugging)... The most likely one seems to be -EPROBE_DEFER.
On the omap5 boards where I have seen the error message being printed they were
EPROBE_DEFER.
So your proposal changes the subject (which has a very small focus).
> The rest of discussion
> would need disassembly and I'll do it once I'll be waiting for yet another
> long run recompile.
:)
BR and thanks,
Nikolaus
On 2/17/20 10:38 PM, H. Nikolaus Schaller wrote:
> If the gpios are probed after this driver (e.g. if they
> come from an i2c expander) there is no need to print an
> error message.
>
> Signed-off-by: H. Nikolaus Schaller <[email protected]>
> ---
> drivers/extcon/extcon-palmas.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
> index edc5016f46f1..cea58d0cb457 100644
> --- a/drivers/extcon/extcon-palmas.c
> +++ b/drivers/extcon/extcon-palmas.c
> @@ -205,14 +205,18 @@ static int palmas_usb_probe(struct platform_device *pdev)
>
> palmas_usb->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id",
> GPIOD_IN);
> - if (IS_ERR(palmas_usb->id_gpiod)) {
> + if (PTR_ERR(palmas_usb->id_gpiod) == -EPROBE_DEFER) {
> + return -EPROBE_DEFER;
> + } else if (IS_ERR(palmas_usb->id_gpiod)) {
> dev_err(&pdev->dev, "failed to get id gpio\n");
> return PTR_ERR(palmas_usb->id_gpiod);
> }
>
> palmas_usb->vbus_gpiod = devm_gpiod_get_optional(&pdev->dev, "vbus",
> GPIOD_IN);
> - if (IS_ERR(palmas_usb->vbus_gpiod)) {
> + if (PTR_ERR(palmas_usb->vbus_gpiod) == -EPROBE_DEFER) {
> + return -EPROBE_DEFER;
> + } else if (IS_ERR(palmas_usb->vbus_gpiod)) {
> dev_err(&pdev->dev, "failed to get vbus gpio\n");
> return PTR_ERR(palmas_usb->vbus_gpiod);
> }
>
I think that it is enough to handle the -EPROBE_DEFER.
Also, I prefer to use single if/else statement
instead of the nested if/else statement.
Applied it.
--
Best Regards,
Chanwoo Choi
Samsung Electronics
On Tue, Feb 18, 2020 at 12:28:25PM +0900, Chanwoo Choi wrote:
> On 2/17/20 10:38 PM, H. Nikolaus Schaller wrote:
> > If the gpios are probed after this driver (e.g. if they
> > come from an i2c expander) there is no need to print an
> > error message.
> >
> > Signed-off-by: H. Nikolaus Schaller <[email protected]>
> > ---
> > drivers/extcon/extcon-palmas.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
> > index edc5016f46f1..cea58d0cb457 100644
> > --- a/drivers/extcon/extcon-palmas.c
> > +++ b/drivers/extcon/extcon-palmas.c
> > @@ -205,14 +205,18 @@ static int palmas_usb_probe(struct platform_device *pdev)
> >
> > palmas_usb->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id",
> > GPIOD_IN);
> > - if (IS_ERR(palmas_usb->id_gpiod)) {
> > + if (PTR_ERR(palmas_usb->id_gpiod) == -EPROBE_DEFER) {
> > + return -EPROBE_DEFER;
> > + } else if (IS_ERR(palmas_usb->id_gpiod)) {
> > dev_err(&pdev->dev, "failed to get id gpio\n");
> > return PTR_ERR(palmas_usb->id_gpiod);
> > }
> >
> > palmas_usb->vbus_gpiod = devm_gpiod_get_optional(&pdev->dev, "vbus",
> > GPIOD_IN);
> > - if (IS_ERR(palmas_usb->vbus_gpiod)) {
> > + if (PTR_ERR(palmas_usb->vbus_gpiod) == -EPROBE_DEFER) {
> > + return -EPROBE_DEFER;
> > + } else if (IS_ERR(palmas_usb->vbus_gpiod)) {
> > dev_err(&pdev->dev, "failed to get vbus gpio\n");
> > return PTR_ERR(palmas_usb->vbus_gpiod);
> > }
> >
>
> I think that it is enough to handle the -EPROBE_DEFER.
> Also, I prefer to use single if/else statement
> instead of the nested if/else statement.
>
> Applied it.
Uh... As it is? Then it is matter of time it triggers someones cocci
script pointing to else after return. Could you at least fix this?
Thanks,
ladis
On 2/18/20 7:21 PM, Ladislav Michl wrote:
> On Tue, Feb 18, 2020 at 12:28:25PM +0900, Chanwoo Choi wrote:
>> On 2/17/20 10:38 PM, H. Nikolaus Schaller wrote:
>>> If the gpios are probed after this driver (e.g. if they
>>> come from an i2c expander) there is no need to print an
>>> error message.
>>>
>>> Signed-off-by: H. Nikolaus Schaller <[email protected]>
>>> ---
>>> drivers/extcon/extcon-palmas.c | 8 ++++++--
>>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
>>> index edc5016f46f1..cea58d0cb457 100644
>>> --- a/drivers/extcon/extcon-palmas.c
>>> +++ b/drivers/extcon/extcon-palmas.c
>>> @@ -205,14 +205,18 @@ static int palmas_usb_probe(struct platform_device *pdev)
>>>
>>> palmas_usb->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id",
>>> GPIOD_IN);
>>> - if (IS_ERR(palmas_usb->id_gpiod)) {
>>> + if (PTR_ERR(palmas_usb->id_gpiod) == -EPROBE_DEFER) {
>>> + return -EPROBE_DEFER;
>>> + } else if (IS_ERR(palmas_usb->id_gpiod)) {
>>> dev_err(&pdev->dev, "failed to get id gpio\n");
>>> return PTR_ERR(palmas_usb->id_gpiod);
>>> }
>>>
>>> palmas_usb->vbus_gpiod = devm_gpiod_get_optional(&pdev->dev, "vbus",
>>> GPIOD_IN);
>>> - if (IS_ERR(palmas_usb->vbus_gpiod)) {
>>> + if (PTR_ERR(palmas_usb->vbus_gpiod) == -EPROBE_DEFER) {
>>> + return -EPROBE_DEFER;
>>> + } else if (IS_ERR(palmas_usb->vbus_gpiod)) {
>>> dev_err(&pdev->dev, "failed to get vbus gpio\n");
>>> return PTR_ERR(palmas_usb->vbus_gpiod);
>>> }
>>>
>>
>> I think that it is enough to handle the -EPROBE_DEFER.
>> Also, I prefer to use single if/else statement
>> instead of the nested if/else statement.
>>
>> Applied it.
>
> Uh... As it is? Then it is matter of time it triggers someones cocci
> script pointing to else after return. Could you at least fix this?
Sorry. I don't understand. Do you mean that this patch has the
some issue of cocci script?
I think that it fixes the probe sequence issue
between extcon-palmas and gpio driver. It is not related to
any result from cocci script. If the extcon-palmas.c has
the issue by cocci or checkpatch, anyone can send the other patch
for fixup.
I think that it is enough to fix the issue which is only
related to the probe sequence between gpio and extcon-palmas.c
>
> Thanks,
> ladis
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
On Tue, Feb 18, 2020 at 07:35:47PM +0900, Chanwoo Choi wrote:
> On 2/18/20 7:21 PM, Ladislav Michl wrote:
> > On Tue, Feb 18, 2020 at 12:28:25PM +0900, Chanwoo Choi wrote:
> >> On 2/17/20 10:38 PM, H. Nikolaus Schaller wrote:
> >>> If the gpios are probed after this driver (e.g. if they
> >>> come from an i2c expander) there is no need to print an
> >>> error message.
> >>>
> >>> Signed-off-by: H. Nikolaus Schaller <[email protected]>
> >>> ---
> >>> drivers/extcon/extcon-palmas.c | 8 ++++++--
> >>> 1 file changed, 6 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
> >>> index edc5016f46f1..cea58d0cb457 100644
> >>> --- a/drivers/extcon/extcon-palmas.c
> >>> +++ b/drivers/extcon/extcon-palmas.c
> >>> @@ -205,14 +205,18 @@ static int palmas_usb_probe(struct platform_device *pdev)
> >>>
> >>> palmas_usb->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id",
> >>> GPIOD_IN);
> >>> - if (IS_ERR(palmas_usb->id_gpiod)) {
> >>> + if (PTR_ERR(palmas_usb->id_gpiod) == -EPROBE_DEFER) {
> >>> + return -EPROBE_DEFER;
Here we returned...
> >>> + } else if (IS_ERR(palmas_usb->id_gpiod)) {
How could this else get triggered?
> >>> dev_err(&pdev->dev, "failed to get id gpio\n");
> >>> return PTR_ERR(palmas_usb->id_gpiod);
> >>> }
> >>>
> >>> palmas_usb->vbus_gpiod = devm_gpiod_get_optional(&pdev->dev, "vbus",
> >>> GPIOD_IN);
> >>> - if (IS_ERR(palmas_usb->vbus_gpiod)) {
> >>> + if (PTR_ERR(palmas_usb->vbus_gpiod) == -EPROBE_DEFER) {
> >>> + return -EPROBE_DEFER;
> >>> + } else if (IS_ERR(palmas_usb->vbus_gpiod)) {
> >>> dev_err(&pdev->dev, "failed to get vbus gpio\n");
> >>> return PTR_ERR(palmas_usb->vbus_gpiod);
> >>> }
> >>>
> >>
> >> I think that it is enough to handle the -EPROBE_DEFER.
> >> Also, I prefer to use single if/else statement
> >> instead of the nested if/else statement.
> >>
> >> Applied it.
> >
> > Uh... As it is? Then it is matter of time it triggers someones cocci
> > script pointing to else after return. Could you at least fix this?
>
> Sorry. I don't understand. Do you mean that this patch has the
> some issue of cocci script?
Yes.
> I think that it fixes the probe sequence issue
> between extcon-palmas and gpio driver. It is not related to
> any result from cocci script. If the extcon-palmas.c has
> the issue by cocci or checkpatch, anyone can send the other patch
> for fixup.
Do you mean to send fixup to what you just applied? What happened
to review process? Nikolaus himself told you patch could be better
and we were just waiting which solution you choose to send final patch.
> I think that it is enough to fix the issue which is only
> related to the probe sequence between gpio and extcon-palmas.c
Agree, but look again at the patch.
ladis
On 2/18/20 7:48 PM, Ladislav Michl wrote:
> On Tue, Feb 18, 2020 at 07:35:47PM +0900, Chanwoo Choi wrote:
>> On 2/18/20 7:21 PM, Ladislav Michl wrote:
>>> On Tue, Feb 18, 2020 at 12:28:25PM +0900, Chanwoo Choi wrote:
>>>> On 2/17/20 10:38 PM, H. Nikolaus Schaller wrote:
>>>>> If the gpios are probed after this driver (e.g. if they
>>>>> come from an i2c expander) there is no need to print an
>>>>> error message.
>>>>>
>>>>> Signed-off-by: H. Nikolaus Schaller <[email protected]>
>>>>> ---
>>>>> drivers/extcon/extcon-palmas.c | 8 ++++++--
>>>>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
>>>>> index edc5016f46f1..cea58d0cb457 100644
>>>>> --- a/drivers/extcon/extcon-palmas.c
>>>>> +++ b/drivers/extcon/extcon-palmas.c
>>>>> @@ -205,14 +205,18 @@ static int palmas_usb_probe(struct platform_device *pdev)
>>>>>
>>>>> palmas_usb->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id",
>>>>> GPIOD_IN);
>>>>> - if (IS_ERR(palmas_usb->id_gpiod)) {
>>>>> + if (PTR_ERR(palmas_usb->id_gpiod) == -EPROBE_DEFER) {
>>>>> + return -EPROBE_DEFER;
>
> Here we returned...
hmm. you better to suggest the result of cocci script
to understand why it is matter.
>
>>>>> + } else if (IS_ERR(palmas_usb->id_gpiod)) {
>
> How could this else get triggered?
I don't understand your intention.
If devm_gpiod_get_optional return the error except of -EPROBE_DEFER,
it is triggered. Is it wrong?
>
>>>>> dev_err(&pdev->dev, "failed to get id gpio\n");
>>>>> return PTR_ERR(palmas_usb->id_gpiod);
>>>>> }
>>>>>
>>>>> palmas_usb->vbus_gpiod = devm_gpiod_get_optional(&pdev->dev, "vbus",
>>>>> GPIOD_IN);
>>>>> - if (IS_ERR(palmas_usb->vbus_gpiod)) {
>>>>> + if (PTR_ERR(palmas_usb->vbus_gpiod) == -EPROBE_DEFER) {
>>>>> + return -EPROBE_DEFER;
>>>>> + } else if (IS_ERR(palmas_usb->vbus_gpiod)) {
>>>>> dev_err(&pdev->dev, "failed to get vbus gpio\n");
>>>>> return PTR_ERR(palmas_usb->vbus_gpiod);
>>>>> }
>>>>>
>>>>
>>>> I think that it is enough to handle the -EPROBE_DEFER.
>>>> Also, I prefer to use single if/else statement
>>>> instead of the nested if/else statement.
>>>>
>>>> Applied it.
>>>
>>> Uh... As it is? Then it is matter of time it triggers someones cocci
>>> script pointing to else after return. Could you at least fix this?
>>
>> Sorry. I don't understand. Do you mean that this patch has the
>> some issue of cocci script?
>
> Yes.
As I said, you better to suggest the result of cocci script.
>
>> I think that it fixes the probe sequence issue
>> between extcon-palmas and gpio driver. It is not related to
>> any result from cocci script. If the extcon-palmas.c has
>> the issue by cocci or checkpatch, anyone can send the other patch
>> for fixup.
>
> Do you mean to send fixup to what you just applied? What happened
> to review process? Nikolaus himself told you patch could be better
> and we were just waiting which solution you choose to send final patch.
I has not thought that Nikolaus will send next patch
when I read this thread.
>
>> I think that it is enough to fix the issue which is only
>> related to the probe sequence between gpio and extcon-palmas.c
>
> Agree, but look again at the patch.
>
> ladis
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
On 2/17/20 10:58 PM, H. Nikolaus Schaller wrote:
>
>> Am 17.02.2020 um 14:38 schrieb H. Nikolaus Schaller <[email protected]>:
>>
>> If the gpios are probed after this driver (e.g. if they
>> come from an i2c expander) there is no need to print an
>> error message.
>>
>> Signed-off-by: H. Nikolaus Schaller <[email protected]>
>> ---
>> drivers/extcon/extcon-palmas.c | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
>> index edc5016f46f1..cea58d0cb457 100644
>> --- a/drivers/extcon/extcon-palmas.c
>> +++ b/drivers/extcon/extcon-palmas.c
>> @@ -205,14 +205,18 @@ static int palmas_usb_probe(struct platform_device *pdev)
>>
>> palmas_usb->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id",
>> GPIOD_IN);
>> - if (IS_ERR(palmas_usb->id_gpiod)) {
>> + if (PTR_ERR(palmas_usb->id_gpiod) == -EPROBE_DEFER) {
>> + return -EPROBE_DEFER;
>> + } else if (IS_ERR(palmas_usb->id_gpiod)) {
>
> Hm.
>
> While looking again at that: why do we need the "{" and "} else "?
>
> It should be sufficient to have
>
>> palmas_usb->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id",
>> GPIOD_IN);
>> + if (PTR_ERR(palmas_usb->id_gpiod) == -EPROBE_DEFER)
>> + return -EPROBE_DEFER;
>> if (IS_ERR(palmas_usb->id_gpiod)) {
>
> What do you think is better coding style here?
I think that my suggestion is better because 'if' and 'else if'
check the error state of same 'palmas_usb->id_gpiod' variable.
If 'if' and 'else if' checks the different variable,
your suggestion is better.
--
Best Regards,
Chanwoo Choi
Samsung Electronics
Hi Nikolaus,
On Mon, Feb 17, 2020 at 09:25:32PM +0100, H. Nikolaus Schaller wrote:
> > The rest of discussion
> > would need disassembly and I'll do it once I'll be waiting for yet another
> > long run recompile.
>
> :)
Just FYI, original compiled with gcc-8.2.1:
text data bss dec hex filename
4358 104 0 4462 116e drivers/extcon/extcon-palmas.o
Your patch:
text data bss dec hex filename
4382 104 0 4486 1186 drivers/extcon/extcon-palmas.o
My patch with printing error value:
text data bss dec hex filename
4406 104 0 4510 119e drivers/extcon/extcon-palmas.o
So even without disassembling numbers speak for themselves :)
l.
On Tue, Feb 18, 2020 at 08:09:16PM +0900, Chanwoo Choi wrote:
> On 2/18/20 7:48 PM, Ladislav Michl wrote:
> > On Tue, Feb 18, 2020 at 07:35:47PM +0900, Chanwoo Choi wrote:
> >> On 2/18/20 7:21 PM, Ladislav Michl wrote:
> >>> On Tue, Feb 18, 2020 at 12:28:25PM +0900, Chanwoo Choi wrote:
> >>>> On 2/17/20 10:38 PM, H. Nikolaus Schaller wrote:
> >>>>> If the gpios are probed after this driver (e.g. if they
> >>>>> come from an i2c expander) there is no need to print an
> >>>>> error message.
> >>>>>
> >>>>> Signed-off-by: H. Nikolaus Schaller <[email protected]>
> >>>>> ---
> >>>>> drivers/extcon/extcon-palmas.c | 8 ++++++--
> >>>>> 1 file changed, 6 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
> >>>>> index edc5016f46f1..cea58d0cb457 100644
> >>>>> --- a/drivers/extcon/extcon-palmas.c
> >>>>> +++ b/drivers/extcon/extcon-palmas.c
> >>>>> @@ -205,14 +205,18 @@ static int palmas_usb_probe(struct platform_device *pdev)
> >>>>>
> >>>>> palmas_usb->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id",
> >>>>> GPIOD_IN);
> >>>>> - if (IS_ERR(palmas_usb->id_gpiod)) {
> >>>>> + if (PTR_ERR(palmas_usb->id_gpiod) == -EPROBE_DEFER) {
> >>>>> + return -EPROBE_DEFER;
> >
> > Here we returned...
>
> hmm. you better to suggest the result of cocci script
> to understand why it is matter.
You can browse similar fixes online :)
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=grep&q=else+after+return
Best regards,
ladis
On 2/21/20 4:47 PM, Ladislav Michl wrote:
> On Tue, Feb 18, 2020 at 08:09:16PM +0900, Chanwoo Choi wrote:
>> On 2/18/20 7:48 PM, Ladislav Michl wrote:
>>> On Tue, Feb 18, 2020 at 07:35:47PM +0900, Chanwoo Choi wrote:
>>>> On 2/18/20 7:21 PM, Ladislav Michl wrote:
>>>>> On Tue, Feb 18, 2020 at 12:28:25PM +0900, Chanwoo Choi wrote:
>>>>>> On 2/17/20 10:38 PM, H. Nikolaus Schaller wrote:
>>>>>>> If the gpios are probed after this driver (e.g. if they
>>>>>>> come from an i2c expander) there is no need to print an
>>>>>>> error message.
>>>>>>>
>>>>>>> Signed-off-by: H. Nikolaus Schaller <[email protected]>
>>>>>>> ---
>>>>>>> drivers/extcon/extcon-palmas.c | 8 ++++++--
>>>>>>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
>>>>>>> index edc5016f46f1..cea58d0cb457 100644
>>>>>>> --- a/drivers/extcon/extcon-palmas.c
>>>>>>> +++ b/drivers/extcon/extcon-palmas.c
>>>>>>> @@ -205,14 +205,18 @@ static int palmas_usb_probe(struct platform_device *pdev)
>>>>>>>
>>>>>>> palmas_usb->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id",
>>>>>>> GPIOD_IN);
>>>>>>> - if (IS_ERR(palmas_usb->id_gpiod)) {
>>>>>>> + if (PTR_ERR(palmas_usb->id_gpiod) == -EPROBE_DEFER) {
>>>>>>> + return -EPROBE_DEFER;
>>>
>>> Here we returned...
>>
>> hmm. you better to suggest the result of cocci script
>> to understand why it is matter.
>
> You can browse similar fixes online :)
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=grep&q=else+after+return
>
As you commented, please share the result
of cocci or checkpatch warning. It is simple to finish
this discussion.
As I commented on other reply,
"I think that my suggestion is better because 'if' and 'else if'
check the error state of same 'palmas_usb->id_gpiod' variable.
If 'if' and 'else if' checks the different variable,
your suggestion is better."
--
Best Regards,
Chanwoo Choi
Samsung Electronics
On Mon, Feb 24, 2020 at 11:12:08AM +0900, Chanwoo Choi wrote:
> On 2/21/20 4:47 PM, Ladislav Michl wrote:
> > On Tue, Feb 18, 2020 at 08:09:16PM +0900, Chanwoo Choi wrote:
> >> On 2/18/20 7:48 PM, Ladislav Michl wrote:
> >>> On Tue, Feb 18, 2020 at 07:35:47PM +0900, Chanwoo Choi wrote:
> >>>> On 2/18/20 7:21 PM, Ladislav Michl wrote:
> >>>>> On Tue, Feb 18, 2020 at 12:28:25PM +0900, Chanwoo Choi wrote:
> >>>>>> On 2/17/20 10:38 PM, H. Nikolaus Schaller wrote:
> >>>>>>> If the gpios are probed after this driver (e.g. if they
> >>>>>>> come from an i2c expander) there is no need to print an
> >>>>>>> error message.
> >>>>>>>
> >>>>>>> Signed-off-by: H. Nikolaus Schaller <[email protected]>
> >>>>>>> ---
> >>>>>>> drivers/extcon/extcon-palmas.c | 8 ++++++--
> >>>>>>> 1 file changed, 6 insertions(+), 2 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
> >>>>>>> index edc5016f46f1..cea58d0cb457 100644
> >>>>>>> --- a/drivers/extcon/extcon-palmas.c
> >>>>>>> +++ b/drivers/extcon/extcon-palmas.c
> >>>>>>> @@ -205,14 +205,18 @@ static int palmas_usb_probe(struct platform_device *pdev)
> >>>>>>>
> >>>>>>> palmas_usb->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id",
> >>>>>>> GPIOD_IN);
> >>>>>>> - if (IS_ERR(palmas_usb->id_gpiod)) {
> >>>>>>> + if (PTR_ERR(palmas_usb->id_gpiod) == -EPROBE_DEFER) {
> >>>>>>> + return -EPROBE_DEFER;
> >>>
> >>> Here we returned...
> >>
> >> hmm. you better to suggest the result of cocci script
> >> to understand why it is matter.
> >
> > You can browse similar fixes online :)
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=grep&q=else+after+return
> >
>
> As you commented, please share the result
> of cocci or checkpatch warning. It is simple to finish
> this discussion.
What is happening here? Do we really need tools to see the obvious?
See for example commit 09971adc33b ("staging: iio: addac: Remove unnecessary
else after return"). Running script mentioned in above commit with
"[PATCH v3] extcon: palmas: hide error messages if gpio returns"
applied gives:
~/src/linux$ spatch -sp_file s.cocci -in_place drivers/extcon/extcon-palmas.c
init_defs_builtins: /usr/lib/coccinelle/standard.h
HANDLING: drivers/extcon/extcon-palmas.c
diff =
--- drivers/extcon/extcon-palmas.c
+++ /tmp/cocci-output-67907-55371b-extcon-palmas.c
@@ -207,7 +207,7 @@ static int palmas_usb_probe(struct platf
GPIOD_IN);
if (PTR_ERR(palmas_usb->id_gpiod) == -EPROBE_DEFER) {
return -EPROBE_DEFER;
- } else if (IS_ERR(palmas_usb->id_gpiod)) {
+ } if (IS_ERR(palmas_usb->id_gpiod)) {
dev_err(&pdev->dev, "failed to get id gpio\n");
return PTR_ERR(palmas_usb->id_gpiod);
}
@@ -216,7 +216,7 @@ static int palmas_usb_probe(struct platf
GPIOD_IN);
if (PTR_ERR(palmas_usb->vbus_gpiod) == -EPROBE_DEFER) {
return -EPROBE_DEFER;
- } else if (IS_ERR(palmas_usb->vbus_gpiod)) {
+ } if (IS_ERR(palmas_usb->vbus_gpiod)) {
dev_err(&pdev->dev, "failed to get vbus gpio\n");
return PTR_ERR(palmas_usb->vbus_gpiod);
}
That's why I wrote previously: "Then it is matter of time it triggers
someones cocci script pointing to else after return."
Linux git history proves there are people running such a scripts
and results of such a scripts gets applied.
I do not care too much, you are the one adding more work for
yourself ;-)
ladis