Re: [PATCH v2 1/1] i2c: designware: set pinctrl recovery information from device pinctrl

[Andy Shevchenko]

On Thu, Dec 15, 2022 at 03:06:13PM +0100, Linus Walleij wrote:
> On Thu, Dec 15, 2022 at 11:28 AM Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > On Thu, Dec 15, 2022 at 10:15:35AM +0200, Hawa, Hanna wrote:
> > > On 12/14/2022 6:09 PM, Andy Shevchenko wrote:

...

> > > > > +     if (dev->dev->pins && dev->dev->pins->p)
> > > > > +             rinfo->pinctrl = dev->dev->pins->p;
> > > > Hmm... I don't see how this field is being used.
> > > > Can you elaborate?
> > >
> > > This field is used in i2c_generic_scl_recovery(), if it's not NULL then the
> > > flow will set the state to GPIO before running the recovery mechanism.
> > >         if (bri->pinctrl)
> > >                 pinctrl_select_state(bri->pinctrl, bri->pins_gpio);
> >
> > OK, but why that function doesn't use the dev->pins->p if it's defined?
> > (As a fallback when rinfo->pinctrl is NULL.)
> 
> I don't understand the context of these things so can't say much
> about it.

Main question here is that, is it possible that GPIOs that back up I²C bus are
on the different pin control device that the bus pins themselves?

And while writing above I think it may be the case if we use additional
GPIO pins over the I²C bus for purpose of recovery. In such case the pin control
device can be different.

At the same time, the fallback option might still work, in case the pinctrl not
overridden and I²C bus has backed GPIO function (on SCL/SDA).

...

> > > I saw that that the change failed in complication for SPARC architecture, as
> > > the pins field is wraparound with CONFIG_PINCTRL in device struct. I though
> > > on two options to solve the compilation error, first by adding wraparound of
> > > CONFIG_PINCTRL when accessing the pins field. And the second option is to
> > > add get function in pinctrl/devinfo.h file, which return the pins field, or
> > > NULL in case the PINCTRL is not defined. Which option you think we can go
> > > with?
> >
> > Getter with a stub sounds better to me, so you won't access some device core
> > fields.
> >
> > Linus, what do you think about all these (including previous paragraph)?
> 
> A getter may be a good solution, it depends, it can also be pushed
> somewhere local in the designware i2c driver can it not?

Yeah, but my point in the above paragraph that it uses the generic recovery
mechanism which may (or may not?) utilise the same pin control as I²C bus
sitting on).

> I am thinking that the rest of the code that is using that field is
> certainly not going to work without pinctrl either.

-- 
With Best Regards,
Andy Shevchenko