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@xxxxxxxxx>
> > > 
> > > 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@xxxxxxxxx>
> > > Signed-off-by: Yann Sionneau <ysionneau@xxxxxxxxx>
> > 
> > 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