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

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

[Hanna Hawa]

This series include two changes:
1. Adding new API to get the device pinctrl information when it's
initialized before device probed.
2. Make the i2c init recovery to get the device pins if it's not
initialized by the driver from the device pins.

Change Log v4->v5:
- Fix include misplaced
- Update variable set order to reduce one line
- Add Reviewed-by: Andy Shevchenko

Change Log v3->v4:
- Split the change into 2 commits
- Add cover letter and remove change log from commit message
- Fix compilation failure when CONFIG_PINCTRL is not defined
Reported-by: kernel test robot <lkp@intel.com>

Change Log v2->v3:
- Add API to get the device pinctrl
- Make the i2c init recovery to get the device pins

Change Log v1->v2:
- set the rinfo->pinctrl to dev->pins->p instead calling
  devm_pinctrl_get()

Hanna Hawa (2):
  pinctrl: Add an API to get the pinctrl pins if initialized
  i2c: Set i2c pinctrl recovery info from it's device pinctrl

 drivers/i2c/i2c-core-base.c     |  5 ++++-
 include/linux/pinctrl/devinfo.h | 15 +++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

-- 
2.38.1

[PATCH v5 1/2] pinctrl: Add an API to get the pinctrl pins if initialized

[Hanna Hawa]

Add an API to get the pinctrl pins if it was initialized before driver
probed. This API will be used in I2C core to get the device pinctrl
information for recovery state change.

Signed-off-by: Hanna Hawa <hhhawa@amazon.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/pinctrl/devinfo.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/include/linux/pinctrl/devinfo.h b/include/linux/pinctrl/devinfo.h
index a48ff69acddd..670588bfc7ab 100644
--- a/include/linux/pinctrl/devinfo.h
+++ b/include/linux/pinctrl/devinfo.h
@@ -16,6 +16,8 @@
 
 #ifdef CONFIG_PINCTRL
 
+#include <linux/device.h>
+
 /* The device core acts as a consumer toward pinctrl */
 #include <linux/pinctrl/consumer.h>
 
@@ -40,6 +42,14 @@ struct dev_pin_info {
 extern int pinctrl_bind_pins(struct device *dev);
 extern int pinctrl_init_done(struct device *dev);
 
+static inline struct pinctrl *dev_pinctrl(struct device *dev)
+{
+	if (!dev->pins)
+		return NULL;
+
+	return dev->pins->p;
+}
+
 #else
 
 struct device;
@@ -56,5 +66,10 @@ static inline int pinctrl_init_done(struct device *dev)
 	return 0;
 }
 
+static inline struct pinctrl *dev_pinctrl(struct device *dev)
+{
+	return NULL;
+}
+
 #endif /* CONFIG_PINCTRL */
 #endif /* PINCTRL_DEVINFO_H */
-- 
2.38.1

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

[Hanna Hawa]

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;
 
 	/*
 	 * we can't change states without pinctrl, so remove the states if
-- 
2.38.1

Re: [PATCH v5 1/2] pinctrl: Add an API to get the pinctrl pins if initialized

[Linus Walleij]

On Wed, Dec 28, 2022 at 5:48 PM Hanna Hawa <hhhawa@amazon.com> wrote:

> Add an API to get the pinctrl pins if it was initialized before driver
> probed. This API will be used in I2C core to get the device pinctrl
> information for recovery state change.
>
> Signed-off-by: Hanna Hawa <hhhawa@amazon.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Okay then:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v5 1/2] pinctrl: Add an API to get the pinctrl pins if initialized

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 403 bytes --]

On Wed, Dec 28, 2022 at 04:48:12PM +0000, Hanna Hawa wrote:
> Add an API to get the pinctrl pins if it was initialized before driver
> probed. This API will be used in I2C core to get the device pinctrl
> information for recovery state change.
> 
> Signed-off-by: Hanna Hawa <hhhawa@amazon.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 673 bytes --]

On Wed, Dec 28, 2022 at 04:48:13PM +0000, 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>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Robert Marko]

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

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

[Dan Carpenter]

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

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

[Robert Marko]

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

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

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