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

[Andrew Lunn <andrew@lunn.ch>]

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

> +	/*
> +	 * 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>

    Andrew