Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 15 Dec 2022 17:22:09 +0200
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     Linus Walleij <linus.walleij@linaro.org>
Cc:     "Hawa, Hanna" <hhhawa@amazon.com>, Wolfram Sang <wsa@kernel.org>,
        jarkko.nikula@linux.intel.com, mika.westerberg@linux.intel.com,
        jsd@semihalf.com, linux-i2c@vger.kernel.org,
        linux-kernel@vger.kernel.org, dwmw@amazon.co.uk, benh@amazon.com,
        ronenk@amazon.com, talel@amazon.com, jonnyc@amazon.com,
        hanochu@amazon.com, farbere@amazon.com, itamark@amazon.com
Subject: Re: [PATCH v2 1/1] i2c: designware: set pinctrl recovery information
 from device pinctrl
Message-ID: <Y5s7ofYGaec8+zIA@smile.fi.intel.com>
References: <20221214142725.23881-1-hhhawa@amazon.com>
 <Y5n1U1lYbcbJ5U1k@smile.fi.intel.com>
 <efa9171f-98ac-f518-e59e-f6c4d7d3d4e6@amazon.com>
 <Y5r2pZhe17dVBMme@smile.fi.intel.com>
 <CACRpkdbawj+Wg2D75MdZavcfyjRa2JRAvqROnNLVH7oMcZTdMg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CACRpkdbawj+Wg2D75MdZavcfyjRa2JRAvqROnNLVH7oMcZTdMg@mail.gmail.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

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@linux.intel.com> 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