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