Re: [PATCH 1/2] i2c: pxa: implement generic i2c bus recovery

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Wed, May 06, 2020 at 04:00:54PM +0200, Andrew Lunn wrote:
> On Wed, May 06, 2020 at 10:40:31AM +0100, Russell King wrote:
> > Implement generic GPIO-based I2C bus recovery for the PXA I2C driver.
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/i2c/busses/i2c-pxa.c | 176 +++++++++++++++++++++++++++++++----
> >  1 file changed, 159 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
> > index 0e194d6cd1b5..da15ab2a519e 100644
> > --- a/drivers/i2c/busses/i2c-pxa.c
> > +++ b/drivers/i2c/busses/i2c-pxa.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/delay.h>
> >  #include <linux/err.h>
> >  #include <linux/errno.h>
> > +#include <linux/gpio/consumer.h>
> >  #include <linux/i2c.h>
> >  #include <linux/init.h>
> >  #include <linux/interrupt.h>
> > @@ -28,6 +29,7 @@
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> > +#include <linux/pinctrl/consumer.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/platform_data/i2c-pxa.h>
> >  #include <linux/slab.h>
> > @@ -260,6 +262,11 @@ struct pxa_i2c {
> >  	bool			highmode_enter;
> >  	u32			fm_mask;
> >  	u32			hs_mask;
> > +
> > +	struct i2c_bus_recovery_info recovery;
> > +	struct pinctrl		*pinctrl;
> > +	struct pinctrl_state	*pinctrl_default;
> > +	struct pinctrl_state	*pinctrl_recovery;
> >  };
> >  
> >  #define _IBMR(i2c)	((i2c)->reg_ibmr)
> > @@ -559,13 +566,8 @@ static void i2c_pxa_set_slave(struct pxa_i2c *i2c, int errcode)
> >  #define i2c_pxa_set_slave(i2c, err)	do { } while (0)
> >  #endif
> >  
> > -static void i2c_pxa_reset(struct pxa_i2c *i2c)
> > +static void i2c_pxa_do_reset(struct pxa_i2c *i2c)
> >  {
> > -	pr_debug("Resetting I2C Controller Unit\n");
> > -
> > -	/* abort any transfer currently under way */
> > -	i2c_pxa_abort(i2c);
> > -
> >  	/* reset according to 9.8 */
> >  	writel(ICR_UR, _ICR(i2c));
> >  	writel(I2C_ISR_INIT, _ISR(i2c));
> > @@ -584,12 +586,25 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c)
> >  #endif
> >  
> >  	i2c_pxa_set_slave(i2c, 0);
> > +}
> >  
> > +static void i2c_pxa_enable(struct pxa_i2c *i2c)
> > +{
> >  	/* enable unit */
> >  	writel(readl(_ICR(i2c)) | ICR_IUE, _ICR(i2c));
> >  	udelay(100);
> >  }
> >  
> > +static void i2c_pxa_reset(struct pxa_i2c *i2c)
> > +{
> > +	pr_debug("Resetting I2C Controller Unit\n");
> 
> Hi Russell
> 
> I know you are just moving code around, but maybe pr_debug() could be
> cleaned up to dev_debug()? 

Yes, but I don't want to add irrelevant changes into this; changing
the way the driver prints stuff wouldn't be part of adding recovery
support, and would likely get review comments suggesting it should
be a separate patch.

> > +	/*
> > +	 * Claiming GPIOs can change the pinmux state, which confuses the
> > +	 * pinctrl since since pinctrl's idea of the current setting is
> 
> since since
> 
> I don't know too much about this hardware, but for what it is worth,
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up