Hi Ladis,
> Am 17.02.2020 um 20:07 schrieb Ladislav Michl <ladis@xxxxxxxxxxxxxx>:
>
> 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 <ladis@xxxxxxxxxxxxxx>:
>>>
>>> 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 <hns@xxxxxxxxxxxxx>:
>>>>>
>>>>> 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 <hns@xxxxxxxxxxxxx>
>>>>> ---
>>>>> 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
