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@goldelico.com --- 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); }
Am 17.02.2020 um 14:38 schrieb H. Nikolaus Schaller hns@goldelico.com:
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@goldelico.com
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;
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 hns@goldelico.com:
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@goldelico.com
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;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 ladis@linux-mips.org:
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@goldelico.com:
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@goldelico.com
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;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 ladis@linux-mips.org:
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@goldelico.com:
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@goldelico.com
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;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 ladis@linux-mips.org:
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@linux-mips.org:
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@goldelico.com:
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@goldelico.com
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;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 ladis@linux-mips.org: 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 ladis@linux-mips.org:
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 ladis@linux-mips.org: 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
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 2/17/20 10:58 PM, H. Nikolaus Schaller wrote:
Am 17.02.2020 um 14:38 schrieb H. Nikolaus Schaller hns@goldelico.com:
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@goldelico.com
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;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.
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 hns@goldelico.com
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.
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 hns@goldelico.com
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 hns@goldelico.com
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
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 hns@goldelico.com
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 hns@goldelico.com
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
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 hns@goldelico.com
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=g...
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 hns@goldelico.com > --- > 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=g...
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."
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 hns@goldelico.com >> --- >> 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=g...
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
-
Chanwoo Choi -
H. Nikolaus Schaller -
Ladislav Michl