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