Re: [PATCH v2 1/5] i2c: mv64xxx: Add reset deassert call

[Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

Hi Maxime,

On 13/01/2014 11:34, Maxime Ripard wrote:
> The Allwinner A31 SoC using that IP has a reset controller maintaining
> it reset unless told otherwise.
> 
> Add some optional reset support to the driver.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  .../devicetree/bindings/i2c/i2c-mv64xxx.txt         |  1 +
>  drivers/i2c/busses/Kconfig                          |  1 +
>  drivers/i2c/busses/i2c-mv64xxx.c                    | 21 +++++++++++++++++++--
>  3 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> index 82e8f6f..603003a 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> @@ -12,6 +12,7 @@ Optional properties :
>  
>   - clock-frequency : Desired I2C bus clock frequency in Hz. If not set the
>  default frequency is 100kHz
> + - resets          : phandle to the parent reset controller
>  
>  Examples:
>  
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 3b26129..69aa599 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -528,6 +528,7 @@ config I2C_MPC
>  config I2C_MV64XXX
>  	tristate "Marvell mv64xxx I2C Controller"
>  	depends on (MV64X60 || PLAT_ORION || ARCH_SUNXI)
> +	select RESET_CONTROLLER

This one could maybe just depend on ARCH_SUNXI with something like:

select RESET_CONTROLLER if ARCH_SUNXI

>  	help
>  	  If you say yes to this option, support will be included for the
>  	  built-in I2C interface on the Marvell 64xxx line of host bridges.
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> index 8be7e42..0f6dde5 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -17,6 +17,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/mv643xx_i2c.h>
>  #include <linux/platform_device.h>
> +#include <linux/reset.h>
>  #include <linux/io.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> @@ -149,6 +150,7 @@ struct mv64xxx_i2c_data {
>  	bool			offload_enabled;
>  /* 5us delay in order to avoid repeated start timing violation */
>  	bool			errata_delay;
> +	struct reset_control	*rstc;
>  };
>  
>  static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
> @@ -763,6 +765,16 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
>  	}
>  	drv_data->irq = irq_of_parse_and_map(np, 0);
>  
> +	drv_data->rstc = devm_reset_control_get(dev, NULL);

Hum ok, you need RESET_CONTROLLER in all case to use it here.
As most of the architecture also use RESET_CONTROLLER it is not
a big deal to unable it then.

> +	if (IS_ERR(drv_data->rstc)) {
> +		if (PTR_ERR(drv_data->rstc) == -EPROBE_DEFER) {
> +			rc = -EPROBE_DEFER;
> +			goto out;
> +		}
> +	} else {
> +		reset_control_deassert(drv_data->rstc);
> +	}
> +
>  	/* Its not yet defined how timeouts will be specified in device tree.
>  	 * So hard code the value to 1 second.
>  	 */
> @@ -845,7 +857,7 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>  	}
>  	if (drv_data->irq < 0) {
>  		rc = -ENXIO;
> -		goto exit_clk;
> +		goto exit_reset;
>  	}
>  
>  	drv_data->adapter.dev.parent = &pd->dev;
> @@ -865,7 +877,7 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>  		dev_err(&drv_data->adapter.dev,
>  			"mv64xxx: Can't register intr handler irq%d: %d\n",
>  			drv_data->irq, rc);
> -		goto exit_clk;
> +		goto exit_reset;
>  	} else if ((rc = i2c_add_numbered_adapter(&drv_data->adapter)) != 0) {
>  		dev_err(&drv_data->adapter.dev,
>  			"mv64xxx: Can't add i2c adapter, rc: %d\n", -rc);
> @@ -876,6 +888,9 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>  
>  exit_free_irq:
>  	free_irq(drv_data->irq, drv_data);
> +exit_reset:
> +	if (pd->dev.of_node && !IS_ERR(drv_data->rstc))
> +		reset_control_assert(drv_data->rstc);
>  exit_clk:
>  #if defined(CONFIG_HAVE_CLK)
>  	/* Not all platforms have a clk */
> @@ -894,6 +909,8 @@ mv64xxx_i2c_remove(struct platform_device *dev)
>  
>  	i2c_del_adapter(&drv_data->adapter);
>  	free_irq(drv_data->irq, drv_data);
> +	if (dev->dev.of_node && !IS_ERR(drv_data->rstc))
> +		reset_control_assert(drv_data->rstc);
>  #if defined(CONFIG_HAVE_CLK)
>  	/* Not all platforms have a clk */
>  	if (!IS_ERR(drv_data->clk)) {
> 

Everything else looks sensible, I also tested in on an Armada 370.
You can add my

Reviewed-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Tested-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com