Re: [PATCH] i2c: imx: add shutdown interface

[Oleksij Rempel]

Hi,

thank you for your patch. It is good, but some changes are needed.

This driver has multiple attempts to reset the bus controller to
initial state:
- on probe. In this case controller is reset with  same sequence by using
  I2CR_IEN bit, but no zeroing of IMX_I2C_IADR is made.
- on module remove. All registers are set to zero. Which is not Vybrid
  compatible. In this cases, bus controller on Vybrid is not under
  reset.
- on shutdown (a new one), this is like probe, but with zeroing of
  IMX_I2C_IADR.

Why do we need to zero IMX_I2C_IADR if we do a controller reset? Do
reset is not enough to clear this register? Do we need to set I2CR_IEN
back on remove and shutdown? The documentation says:
|I2C enable. Also controls the software reset of the entire I2C.
|Resetting the bit generates an internal reset to the block.

I would assume, we should disable controller on remove and shutdown,
and enable it only for probe case.

Please create a new function and use in all 3 cases: probe, remove and
shutdown. This function should disable the bus controller and clear
registers (if really needed). Enable should be done only on probe.

Should we disable clock on shutdown as well?

Can you please describe, why i2c shutdown sequence is needed on this
chip. What problem is fixed with this patch?


On Mon, Mar 30, 2020 at 08:15:46PM +0800, Biwen Li wrote:
> From: Biwen Li <biwen.li@xxxxxxx>
> 
> Add shutdown interface
> 
> Signed-off-by: Biwen Li <biwen.li@xxxxxxx>
> ---
>  drivers/i2c/busses/i2c-imx.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 0ab5381aa012..07da42cb0be4 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -1281,6 +1281,16 @@ static int i2c_imx_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static void i2c_imx_shutdown(struct platform_device *pdev)
> +{
> +	struct imx_i2c_struct *i2c_imx = platform_get_drvdata(pdev);
> +
> +	imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IADR);
> +	imx_i2c_write_reg(i2c_imx->hwdata->i2cr_ien_opcode ^ I2CR_IEN,
> +			i2c_imx, IMX_I2C_I2CR);
> +	imx_i2c_write_reg(i2c_imx->hwdata->i2sr_clr_opcode, i2c_imx, IMX_I2C_I2SR);
> +}
> +
>  static int __maybe_unused i2c_imx_runtime_suspend(struct device *dev)
>  {
>  	struct imx_i2c_struct *i2c_imx = dev_get_drvdata(dev);
> @@ -1310,6 +1320,7 @@ static const struct dev_pm_ops i2c_imx_pm_ops = {
>  static struct platform_driver i2c_imx_driver = {
>  	.probe = i2c_imx_probe,
>  	.remove = i2c_imx_remove,
> +	.shutdown = i2c_imx_shutdown,
>  	.driver = {
>  		.name = DRIVER_NAME,
>  		.pm = &i2c_imx_pm_ops,
> -- 
> 2.17.1


Regrads,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Attachment: signature.asc
Description: PGP signature