Re: [PATCH 1/3] i2c: davinci: Add helper functions

[Troy Kisky <troy.kisky-Q5RJGjKts06CY9SHAMCTRUEOCMrvLtNR@public.gmane.org>]

Chaithrika U S wrote:
> Add i2c reset control and clock divider calculation functions
> which will be useful for power management features.
> 
> Signed-off-by: Chaithrika U S <chaithrika-l0cyMroinI0@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-davinci.c |   56 +++++++++++++++++++++++++-------------
>  1 files changed, 37 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> index c89687a..5f3838c 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -133,12 +133,21 @@ static inline u16 davinci_i2c_read_reg(struct davinci_i2c_dev *i2c_dev, int reg)
>  	return __raw_readw(i2c_dev->base + reg);
>  }
>  
> -/*
> - * This functions configures I2C and brings I2C out of reset.
> - * This function is called during I2C init function. This function
> - * also gets called if I2C encounters any errors.
> - */
> -static int i2c_davinci_init(struct davinci_i2c_dev *dev)
> +static inline void davinci_i2c_reset_ctrl(struct davinci_i2c_dev *i2c_dev,
> +								int val)
> +{
> +	u16 w;
> +
> +	w = davinci_i2c_read_reg(i2c_dev, DAVINCI_I2C_MDR_REG);
> +	if (!val)	/* put I2C into reset */
> +		MOD_REG_BIT(w, DAVINCI_I2C_MDR_IRS, 0);
> +	else		/* take I2C out of reset */
> +		MOD_REG_BIT(w, DAVINCI_I2C_MDR_IRS, 1);
> +


I dislike the MOD_REG_BIT macro. Maybe if you wrote
MOD_REG_BIT(w, DAVINCI_I2C_MDR_IRS, val);

it would be slightly better, but I still wouldn't like it.
Please use w &= ~DAVINCI_I2C_MDR_IRS and
 w |= DAVINCI_I2C_MDR_IRS directly.


I know the code you're replacing uses this obfuscating macro
but this is a good time to remove it.


> +	davinci_i2c_write_reg(i2c_dev, DAVINCI_I2C_MDR_REG, w);
> +}
> +
> +static void i2c_davinci_calc_clk_dividers(struct davinci_i2c_dev *dev)
>  {
>  	struct davinci_i2c_platform_data *pdata = dev->dev->platform_data;
>  	u16 psc;
> @@ -147,15 +156,6 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
>  	u32 clkh;
>  	u32 clkl;
>  	u32 input_clock = clk_get_rate(dev->clk);
> -	u16 w;
> -
> -	if (!pdata)
> -		pdata = &davinci_i2c_platform_data_default;
> -
> -	/* put I2C into reset */
> -	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> -	MOD_REG_BIT(w, DAVINCI_I2C_MDR_IRS, 0);
> -	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
>  
>  	/* NOTE: I2C Clock divider programming info
>  	 * As per I2C specs the following formulas provide prescaler
> @@ -187,12 +187,32 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_CLKH_REG, clkh);
>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_CLKL_REG, clkl);
>  
> +	dev_dbg(dev->dev, "input_clock = %d, CLK = %d\n", input_clock, clk);
> +}
> +
> +/*
> + * This functions configures I2C and brings I2C out of reset.
> + * This function is called during I2C init function. This function
> + * also gets called if I2C encounters any errors.
> + */
> +static int i2c_davinci_init(struct davinci_i2c_dev *dev)
> +{
> +	struct davinci_i2c_platform_data *pdata = dev->dev->platform_data;
> +
> +	if (!pdata)
> +		pdata = &davinci_i2c_platform_data_default;
> +
> +	/* put I2C into reset */
> +	davinci_i2c_reset_ctrl(dev, 0);
> +
> +	/* compute clock dividers */
> +	i2c_davinci_calc_clk_dividers(dev);
> +
>  	/* Respond at reserved "SMBus Host" slave address" (and zero);
>  	 * we seem to have no option to not respond...
>  	 */
>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_OAR_REG, 0x08);
>  
> -	dev_dbg(dev->dev, "input_clock = %d, CLK = %d\n", input_clock, clk);
>  	dev_dbg(dev->dev, "PSC  = %d\n",
>  		davinci_i2c_read_reg(dev, DAVINCI_I2C_PSC_REG));
>  	dev_dbg(dev->dev, "CLKL = %d\n",
> @@ -203,9 +223,7 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
>  		pdata->bus_freq, pdata->bus_delay);
>  
>  	/* Take the I2C module out of reset: */
> -	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> -	MOD_REG_BIT(w, DAVINCI_I2C_MDR_IRS, 1);
> -	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
> +	davinci_i2c_reset_ctrl(dev, 1);
>  
>  	/* Enable interrupts */
>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, I2C_DAVINCI_INTR_ALL);