[PATCH v2] i2c: designware: add support for pinctrl for recovery

[PATCH v2] i2c: designware: add support for pinctrl for recovery

[Yann Sionneau]

From: Yann Sionneau <ysionneau@kalray.eu>

Currently if the SoC needs pinctrl to switch the SCL and SDA
from the I2C function to GPIO function, the recovery won't work.

scl-gpio = <>;
sda-gpio = <>;

Are not enough for some SoCs to have a working recovery.
Some need:

scl-gpio = <>;
sda-gpio = <>;
pinctrl-names = "default", "recovery";
pinctrl-0 = <&i2c_pins_hw>;
pinctrl-1 = <&i2c_pins_gpio>;

The driver was not filling rinfo->pinctrl with the device node
pinctrl data which is needed by generic recovery code.

Tested-by: Yann Sionneau <ysionneau@kalray.eu>
Signed-off-by: Yann Sionneau <ysionneau@kalray.eu>
---
V1 -> V2:
* remove the unnecessary 'if (!rinfo->pinctrl)' test
* test if return is -EPROBE_DEFER, in that case, return it.
* Reword the commit message according to review

 drivers/i2c/busses/i2c-designware-master.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index 3bfd7a2232db..b45accbff915 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -17,6 +17,7 @@
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/module.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/reset.h>
@@ -905,6 +906,15 @@ static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev)
 		return PTR_ERR(gpio);
 	rinfo->sda_gpiod = gpio;
 
+	rinfo->pinctrl = devm_pinctrl_get(dev->dev);
+	if (IS_ERR(rinfo->pinctrl)) {
+		if (PTR_ERR(rinfo->pinctrl) == -EPROBE_DEFER)
+			return PTR_ERR(rinfo->pinctrl);
+
+		rinfo->pinctrl = NULL;
+		dev_info(dev->dev, "can't get pinctrl, bus recovery might not work\n");
+	}
+
 	rinfo->recover_bus = i2c_generic_scl_recovery;
 	rinfo->prepare_recovery = i2c_dw_prepare_recovery;
 	rinfo->unprepare_recovery = i2c_dw_unprepare_recovery;

base-commit: 2ccdd1b13c591d306f0401d98dedc4bdcd02b421
-- 
2.34.1

Re: [PATCH v2] i2c: designware: add support for pinctrl for recovery

[Jarkko Nikula]

Hi

On 8/16/23 12:50, Yann Sionneau wrote:
> From: Yann Sionneau <ysionneau@kalray.eu>
> 
> Currently if the SoC needs pinctrl to switch the SCL and SDA
> from the I2C function to GPIO function, the recovery won't work.
> 
> scl-gpio = <>;
> sda-gpio = <>;
> 
> Are not enough for some SoCs to have a working recovery.
> Some need:
> 
> scl-gpio = <>;
> sda-gpio = <>;
> pinctrl-names = "default", "recovery";
> pinctrl-0 = <&i2c_pins_hw>;
> pinctrl-1 = <&i2c_pins_gpio>;
> 
> The driver was not filling rinfo->pinctrl with the device node
> pinctrl data which is needed by generic recovery code.
> 
> Tested-by: Yann Sionneau <ysionneau@kalray.eu>
> Signed-off-by: Yann Sionneau <ysionneau@kalray.eu>

Tested-by from author is needless. Expectation is that author has tested 
the patch while not always true :-)

> @@ -905,6 +906,15 @@ static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev)
>   		return PTR_ERR(gpio);
>   	rinfo->sda_gpiod = gpio;
>   
> +	rinfo->pinctrl = devm_pinctrl_get(dev->dev);
> +	if (IS_ERR(rinfo->pinctrl)) {
> +		if (PTR_ERR(rinfo->pinctrl) == -EPROBE_DEFER)
> +			return PTR_ERR(rinfo->pinctrl);
> +
> +		rinfo->pinctrl = NULL;
> +		dev_info(dev->dev, "can't get pinctrl, bus recovery might not work\n");

I think dev_dbg() suits better here or is it needed at all? End user may 
not be able to do anything when sees this in dmesg. I.e. more like 
development time dev_dbg() information.

Does i2c-core-base.c: i2c_gpio_init_pinctrl_recovery() already do 
dev_info() print when pinctrl & GPIO are set properly making above also 
kind of needless?

Jarkko

Re: [PATCH v2] i2c: designware: add support for pinctrl for recovery

[Yann Sionneau]

Hi

Le 17/08/2023 à 10:07, Jarkko Nikula a écrit :
> Hi
>
> On 8/16/23 12:50, Yann Sionneau wrote:
>> From: Yann Sionneau <ysionneau@kalray.eu>
>>
>> Currently if the SoC needs pinctrl to switch the SCL and SDA
>> from the I2C function to GPIO function, the recovery won't work.
>>
>> scl-gpio = <>;
>> sda-gpio = <>;
>>
>> Are not enough for some SoCs to have a working recovery.
>> Some need:
>>
>> scl-gpio = <>;
>> sda-gpio = <>;
>> pinctrl-names = "default", "recovery";
>> pinctrl-0 = <&i2c_pins_hw>;
>> pinctrl-1 = <&i2c_pins_gpio>;
>>
>> The driver was not filling rinfo->pinctrl with the device node
>> pinctrl data which is needed by generic recovery code.
>>
>> Tested-by: Yann Sionneau <ysionneau@kalray.eu>
>> Signed-off-by: Yann Sionneau <ysionneau@kalray.eu>
>
> Tested-by from author is needless. Expectation is that author has 
> tested the patch while not always true :-)
Ok, I just wanted to emphasize the fact that I have the device and I 
tested the change with the device. Ack!
>
>> @@ -905,6 +906,15 @@ static int i2c_dw_init_recovery_info(struct 
>> dw_i2c_dev *dev)
>>           return PTR_ERR(gpio);
>>       rinfo->sda_gpiod = gpio;
>>   +    rinfo->pinctrl = devm_pinctrl_get(dev->dev);
>> +    if (IS_ERR(rinfo->pinctrl)) {
>> +        if (PTR_ERR(rinfo->pinctrl) == -EPROBE_DEFER)
>> +            return PTR_ERR(rinfo->pinctrl);
>> +
>> +        rinfo->pinctrl = NULL;
>> +        dev_info(dev->dev, "can't get pinctrl, bus recovery might 
>> not work\n");
>
> I think dev_dbg() suits better here or is it needed at all? End user 
> may not be able to do anything when sees this in dmesg. I.e. more like 
> development time dev_dbg() information.
I agree dev_dbg() is a better idea.
>
> Does i2c-core-base.c: i2c_gpio_init_pinctrl_recovery() already do 
> dev_info() print when pinctrl & GPIO are set properly making above 
> also kind of needless?

Thanks for the review. In fact I had to use gdb to understand why the 
recovery was not working. Because as you said, it only prints something 
to say "everything looks ok!".

I kind of prefer when it prints when something goes wrong.

But I let you decide what you think is the best.

-- 

Yann

Re: [PATCH v2] i2c: designware: add support for pinctrl for recovery

[Jarkko Nikula]

On 8/17/23 17:27, Yann Sionneau wrote:
> Hi
> 
> Le 17/08/2023 à 10:07, Jarkko Nikula a écrit :
>> Hi
>>
>> On 8/16/23 12:50, Yann Sionneau wrote:
>>> From: Yann Sionneau <ysionneau@kalray.eu>
>>>
>>> Currently if the SoC needs pinctrl to switch the SCL and SDA
>>> from the I2C function to GPIO function, the recovery won't work.
>>>
>>> scl-gpio = <>;
>>> sda-gpio = <>;
>>>
>>> Are not enough for some SoCs to have a working recovery.
>>> Some need:
>>>
>>> scl-gpio = <>;
>>> sda-gpio = <>;
>>> pinctrl-names = "default", "recovery";
>>> pinctrl-0 = <&i2c_pins_hw>;
>>> pinctrl-1 = <&i2c_pins_gpio>;
>>>
>>> The driver was not filling rinfo->pinctrl with the device node
>>> pinctrl data which is needed by generic recovery code.
>>>
>>> Tested-by: Yann Sionneau <ysionneau@kalray.eu>
>>> Signed-off-by: Yann Sionneau <ysionneau@kalray.eu>
>>
>> Tested-by from author is needless. Expectation is that author has 
>> tested the patch while not always true :-)
> Ok, I just wanted to emphasize the fact that I have the device and I 
> tested the change with the device. Ack!
>>
>>> @@ -905,6 +906,15 @@ static int i2c_dw_init_recovery_info(struct 
>>> dw_i2c_dev *dev)
>>>           return PTR_ERR(gpio);
>>>       rinfo->sda_gpiod = gpio;
>>>   +    rinfo->pinctrl = devm_pinctrl_get(dev->dev);
>>> +    if (IS_ERR(rinfo->pinctrl)) {
>>> +        if (PTR_ERR(rinfo->pinctrl) == -EPROBE_DEFER)
>>> +            return PTR_ERR(rinfo->pinctrl);
>>> +
>>> +        rinfo->pinctrl = NULL;
>>> +        dev_info(dev->dev, "can't get pinctrl, bus recovery might 
>>> not work\n");
>>
>> I think dev_dbg() suits better here or is it needed at all? End user 
>> may not be able to do anything when sees this in dmesg. I.e. more like 
>> development time dev_dbg() information.
> I agree dev_dbg() is a better idea.
>>
>> Does i2c-core-base.c: i2c_gpio_init_pinctrl_recovery() already do 
>> dev_info() print when pinctrl & GPIO are set properly making above 
>> also kind of needless?
> 
> Thanks for the review. In fact I had to use gdb to understand why the 
> recovery was not working. Because as you said, it only prints something 
> to say "everything looks ok!".
> 
> I kind of prefer when it prints when something goes wrong.
> 
> But I let you decide what you think is the best.
> 
Fair enough, dev_dbg() is justified when it makes developer's life 
easier :-)

Re: [PATCH v2] i2c: designware: add support for pinctrl for recovery

[Andi Shyti]

Hi,

On Thu, Aug 17, 2023 at 04:27:26PM +0200, Yann Sionneau wrote:
> Hi
> 
> Le 17/08/2023 à 10:07, Jarkko Nikula a écrit :
> > Hi
> > 
> > On 8/16/23 12:50, Yann Sionneau wrote:
> > > From: Yann Sionneau <ysionneau@kalray.eu>
> > > 
> > > Currently if the SoC needs pinctrl to switch the SCL and SDA
> > > from the I2C function to GPIO function, the recovery won't work.
> > > 
> > > scl-gpio = <>;
> > > sda-gpio = <>;
> > > 
> > > Are not enough for some SoCs to have a working recovery.
> > > Some need:
> > > 
> > > scl-gpio = <>;
> > > sda-gpio = <>;
> > > pinctrl-names = "default", "recovery";
> > > pinctrl-0 = <&i2c_pins_hw>;
> > > pinctrl-1 = <&i2c_pins_gpio>;
> > > 
> > > The driver was not filling rinfo->pinctrl with the device node
> > > pinctrl data which is needed by generic recovery code.
> > > 
> > > Tested-by: Yann Sionneau <ysionneau@kalray.eu>
> > > Signed-off-by: Yann Sionneau <ysionneau@kalray.eu>
> > 
> > Tested-by from author is needless. Expectation is that author has tested
> > the patch while not always true :-)
> Ok, I just wanted to emphasize the fact that I have the device and I tested
> the change with the device. Ack!
> > 
> > > @@ -905,6 +906,15 @@ static int i2c_dw_init_recovery_info(struct
> > > dw_i2c_dev *dev)
> > >           return PTR_ERR(gpio);
> > >       rinfo->sda_gpiod = gpio;
> > >   +    rinfo->pinctrl = devm_pinctrl_get(dev->dev);
> > > +    if (IS_ERR(rinfo->pinctrl)) {
> > > +        if (PTR_ERR(rinfo->pinctrl) == -EPROBE_DEFER)
> > > +            return PTR_ERR(rinfo->pinctrl);
> > > +
> > > +        rinfo->pinctrl = NULL;
> > > +        dev_info(dev->dev, "can't get pinctrl, bus recovery might
> > > not work\n");
> > 
> > I think dev_dbg() suits better here or is it needed at all? End user may
> > not be able to do anything when sees this in dmesg. I.e. more like
> > development time dev_dbg() information.
> I agree dev_dbg() is a better idea.
> > 
> > Does i2c-core-base.c: i2c_gpio_init_pinctrl_recovery() already do
> > dev_info() print when pinctrl & GPIO are set properly making above also
> > kind of needless?
> 
> Thanks for the review. In fact I had to use gdb to understand why the
> recovery was not working. Because as you said, it only prints something to
> say "everything looks ok!".
> 
> I kind of prefer when it prints when something goes wrong.
> But I let you decide what you think is the best.

You need to differentiate here between an error and not an error.
If the return value is an ENOMEM, then this is an error. Although
I think you should not return, but the message needs to be an
dev_err().

On the other hand, if the return value is a '0', then I think
dev_info() is correct.

Either remove the logging or make it correct.

One more note, the sentence "can't get pinctrl,... " sounds like
an error. If the pinctrl is not connected on your system, maybe
it's because your system is not designed to have recovery. Please
write a message that doesn't sound like an error (or suppress the
logging).

Thanks,
Andi

Re: [PATCH v2] i2c: designware: add support for pinctrl for recovery

[Yann]

Hi, 

August 20, 2023 1:02 PM, "Andi Shyti" <andi.shyti@kernel.org> wrote:

> Hi,
> 
> On Thu, Aug 17, 2023 at 04:27:26PM +0200, Yann Sionneau wrote:
> 
>> Hi
>> 
>> Le 17/08/2023 à 10:07, Jarkko Nikula a écrit :
>> Hi
>> 
>> On 8/16/23 12:50, Yann Sionneau wrote:
> 
> From: Yann Sionneau <ysionneau@kalray.eu>
> 
> Currently if the SoC needs pinctrl to switch the SCL and SDA
> from the I2C function to GPIO function, the recovery won't work.
> 
> scl-gpio = <>;
> sda-gpio = <>;
> 
> Are not enough for some SoCs to have a working recovery.
> Some need:
> 
> scl-gpio = <>;
> sda-gpio = <>;
> pinctrl-names = "default", "recovery";
> pinctrl-0 = <&i2c_pins_hw>;
> pinctrl-1 = <&i2c_pins_gpio>;
> 
> The driver was not filling rinfo->pinctrl with the device node
> pinctrl data which is needed by generic recovery code.
> 
> Tested-by: Yann Sionneau <ysionneau@kalray.eu>
> Signed-off-by: Yann Sionneau <ysionneau@kalray.eu>
>> Tested-by from author is needless. Expectation is that author has tested
>> the patch while not always true :-)
>> Ok, I just wanted to emphasize the fact that I have the device and I tested
>> the change with the device. Ack!
> 
> @@ -905,6 +906,15 @@ static int i2c_dw_init_recovery_info(struct
> dw_i2c_dev *dev)
> return PTR_ERR(gpio);
> rinfo->sda_gpiod = gpio;
> + rinfo->pinctrl = devm_pinctrl_get(dev->dev);
> + if (IS_ERR(rinfo->pinctrl)) {
> + if (PTR_ERR(rinfo->pinctrl) == -EPROBE_DEFER)
> + return PTR_ERR(rinfo->pinctrl);
> +
> + rinfo->pinctrl = NULL;
> + dev_info(dev->dev, "can't get pinctrl, bus recovery might
> not work\n");
>> I think dev_dbg() suits better here or is it needed at all? End user may
>> not be able to do anything when sees this in dmesg. I.e. more like
>> development time dev_dbg() information.
>> I agree dev_dbg() is a better idea.
>> 
>> Does i2c-core-base.c: i2c_gpio_init_pinctrl_recovery() already do
>> dev_info() print when pinctrl & GPIO are set properly making above also
>> kind of needless?
>> 
>> Thanks for the review. In fact I had to use gdb to understand why the
>> recovery was not working. Because as you said, it only prints something to
>> say "everything looks ok!".
>> 
>> I kind of prefer when it prints when something goes wrong.
>> But I let you decide what you think is the best.
> 
> You need to differentiate here between an error and not an error.
> If the return value is an ENOMEM, then this is an error. Although
> I think you should not return, but the message needs to be an
> dev_err().

Ack.
> On the other hand, if the return value is a '0', then I think
> dev_info() is correct.

Ack.

> Either remove the logging or make it correct.

I'll print a dev_err in case of error (except for -EPROBE_DEFER) and a dev_info in case of NULL.

> 
> One more note, the sentence "can't get pinctrl,... " sounds like
> an error. If the pinctrl is not connected on your system, maybe
> it's because your system is not designed to have recovery. Please
> write a message that doesn't sound like an error (or suppress the
> logging).

I insist that I would have liked to see a message in case of pinctrl setup failure whatever the reason, even if it's because CONFIG_PINCTRL is not set. That would be a "bug" for the developer, in case he/she wants to have recovery working.
Sometimes developer just forget to enable the correct config or to put the correct stuff in DTB and printing debug messages really helps to understand what's going on.

Regards, 

-- 
Yann

Re: [PATCH v2] i2c: designware: add support for pinctrl for recovery

[Andi Shyti]

Hi Yann,

> >> Tested-by from author is needless. Expectation is that author has tested
> >> the patch while not always true :-)
> >> Ok, I just wanted to emphasize the fact that I have the device and I tested
> >> the change with the device. Ack!
> > 
> > @@ -905,6 +906,15 @@ static int i2c_dw_init_recovery_info(struct
> > dw_i2c_dev *dev)
> > return PTR_ERR(gpio);
> > rinfo->sda_gpiod = gpio;
> > + rinfo->pinctrl = devm_pinctrl_get(dev->dev);
> > + if (IS_ERR(rinfo->pinctrl)) {
> > + if (PTR_ERR(rinfo->pinctrl) == -EPROBE_DEFER)
> > + return PTR_ERR(rinfo->pinctrl);
> > +
> > + rinfo->pinctrl = NULL;
> > + dev_info(dev->dev, "can't get pinctrl, bus recovery might
> > not work\n");
> >> I think dev_dbg() suits better here or is it needed at all? End user may
> >> not be able to do anything when sees this in dmesg. I.e. more like
> >> development time dev_dbg() information.
> >> I agree dev_dbg() is a better idea.
> >> 
> >> Does i2c-core-base.c: i2c_gpio_init_pinctrl_recovery() already do
> >> dev_info() print when pinctrl & GPIO are set properly making above also
> >> kind of needless?
> >> 
> >> Thanks for the review. In fact I had to use gdb to understand why the
> >> recovery was not working. Because as you said, it only prints something to
> >> say "everything looks ok!".
> >> 
> >> I kind of prefer when it prints when something goes wrong.
> >> But I let you decide what you think is the best.
> > 
> > You need to differentiate here between an error and not an error.
> > If the return value is an ENOMEM, then this is an error. Although
> > I think you should not return, but the message needs to be an
> > dev_err().
> 
> Ack.
> > On the other hand, if the return value is a '0', then I think
> > dev_info() is correct.
> 
> Ack.
> 
> > Either remove the logging or make it correct.
> 
> I'll print a dev_err in case of error (except for -EPROBE_DEFER) and a dev_info in case of NULL.

Maybe dev_warn() is the best if there is a failure but you keep
going anyway.

> > One more note, the sentence "can't get pinctrl,... " sounds like
> > an error. If the pinctrl is not connected on your system, maybe
> > it's because your system is not designed to have recovery. Please
> > write a message that doesn't sound like an error (or suppress the
> > logging).
> 
> I insist that I would have liked to see a message in case of pinctrl setup failure whatever the reason, even if it's because CONFIG_PINCTRL is not set. That would be a "bug" for the developer, in case he/she wants to have recovery working.
> Sometimes developer just forget to enable the correct config or to put the correct stuff in DTB and printing debug messages really helps to understand what's going on.

Please, read more carefully. I am not suggesting to remove the
message; I'm suggesting to reword it so that it doesn't sound as
an error if it's not an error. But this is a note, i.e. a minor
review.

Andi