Received-SPF: pass (google.com: domain of linux-kernel+bounces-144311-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20221228164813.67964-1-hhhawa@amazon.com> <20221228164813.67964-3-hhhawa@amazon.com>
 <416340b6-33a9-4b9e-bdc5-c5a9cffb3055@gmail.com> <ac51854b-09a6-4b79-b409-b950929655cb@moroto.mountain>
In-Reply-To: <ac51854b-09a6-4b79-b409-b950929655cb@moroto.mountain>
From: Robert Marko <robimarko@gmail.com>
Date: Sun, 14 Apr 2024 19:47:50 +0200
Message-ID: <CAOX2RU6YOBBgEuwdp8P0GTJ5vB0M5Cbqf5SnVJ9Jbou9w5405g@mail.gmail.com>
Subject: Re: [PATCH v5 2/2] i2c: Set i2c pinctrl recovery info from it's
 device pinctrl
To: Dan Carpenter <dan.carpenter@linaro.org>
Cc: Hanna Hawa <hhhawa@amazon.com>, andriy.shevchenko@linux.intel.com, wsa@kernel.org, 
	linus.walleij@linaro.org, linux-i2c@vger.kernel.org, 
	linux-kernel@vger.kernel.org, linux-gpio@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
Content-Type: text/plain; charset="UTF-8"

On Sun, 14 Apr 2024 at 12:34, Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Thu, Apr 11, 2024 at 07:08:56PM +0200, Robert Marko wrote:
> >
> > On 28. 12. 2022. 17:48, Hanna Hawa wrote:
> > > Currently the i2c subsystem rely on the controller device tree to
> > > initialize the pinctrl recovery information, part of the drivers does
> > > not set this field (rinfo->pinctrl), for example i2c DesignWare driver.
> > >
> > > The pins information is saved part of the device structure before probe
> > > and it's done on pinctrl_bind_pins().
> > >
> > > Make the i2c init recovery to get the device pins if it's not
> > > initialized by the driver from the device pins.
> > >
> > > Signed-off-by: Hanna Hawa <hhhawa@amazon.com>
> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > ---
> > >   drivers/i2c/i2c-core-base.c | 5 ++++-
> > >   1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> > > index 7539b0740351..fb5644457452 100644
> > > --- a/drivers/i2c/i2c-core-base.c
> > > +++ b/drivers/i2c/i2c-core-base.c
> > > @@ -34,6 +34,7 @@
> > >   #include <linux/of.h>
> > >   #include <linux/of_irq.h>
> > >   #include <linux/pinctrl/consumer.h>
> > > +#include <linux/pinctrl/devinfo.h>
> > >   #include <linux/pm_domain.h>
> > >   #include <linux/pm_runtime.h>
> > >   #include <linux/pm_wakeirq.h>
> > > @@ -282,7 +283,9 @@ static void i2c_gpio_init_pinctrl_recovery(struct i2c_adapter *adap)
> > >   {
> > >     struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> > >     struct device *dev = &adap->dev;
> > > -   struct pinctrl *p = bri->pinctrl;
> > > +   struct pinctrl *p = bri->pinctrl ?: dev_pinctrl(dev->parent);
> > > +
> > > +   bri->pinctrl = p;
> >
> > Hi Hanna,
> > I know this has already been merged, but setting bri->pinctrl breaks PXA
> > recovery.
>
> This is patch is a year and half old so it's a bit late to just revert
> it...

Hi there,
I know it's old but I just tried it on 6.6 in OpenWrt.

>
> What does "breaks" mean in this context?  Is there a NULL dereference?
> Do you have a stack trace?  It's really hard to get inspired to look at
> the code when the bug report is so vague...

I admit that I did not explain this properly, but if bri->pinctrl is set then
PXA I2C is completely broken as in it doesn't work at all, there are no errors
other than trying to probe for I2C devices will time out.
We had the same symptoms when PXA was converted to generic I2C recovery and that
had to be reverted.

I think its probably some pinctrl issue but nobody has been able to
track it down.

Regards,
Robert

>
> regards,
> dan carpenter