[v5,2/2] i2c: Set i2c pinctrl recovery info from it's device pinctrl

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(-)

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.

Regards,
Robert

>   
>   	/*
>   	 * we can't change states without pinctrl, so remove the states if

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...

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...

regards,
dan carpenter

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

On Sun, Apr 14, 2024 at 07:47:50PM +0200, Robert Marko wrote:
> > > > @@ -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.

If you wanted you could try the following patch with the change to
i2c_gpio_init_pinctrl_recovery() and without it.  (It won't fix anything
it only prints information to dmesg).

regards,
dan carpenter

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 888ca636f3f3..f9477089b980 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -34,6 +34,7 @@
 #include <linux/platform_data/i2c-pxa.h>
 #include <linux/property.h>
 #include <linux/slab.h>
+#include "../../pinctrl/core.h"
 
 /* I2C register field definitions */
 #define IBMR_SDAS	(1 << 0)
@@ -1345,6 +1346,12 @@ static int i2c_pxa_init_recovery(struct pxa_i2c *i2c)
 		return 0;
 
 	i2c->pinctrl = devm_pinctrl_get(dev);
+	if (IS_ERR(i2c->pinctrl))
+		dev_info(dev, "i2c->pinctrl: %pe\n", i2c->pinctrl);
+	else
+		dev_info(dev, "i2c->pinctrl: %s %s\n",
+			 dev_driver_string(i2c->pinctrl->dev),
+			 dev_name(i2c->pinctrl->dev));
 	if (PTR_ERR(i2c->pinctrl) == -ENODEV)
 		i2c->pinctrl = NULL;
 	if (IS_ERR(i2c->pinctrl))