[PATCH] i2c: spacemit: configure ILCR for accurate SCL frequency

[Andi Shyti]

Hi Troy,

> - #include <linux/clk.h>
> - #include <linux/i2c.h>
> - #include <linux/iopoll.h>
> - #include <linux/module.h>
> - #include <linux/of_address.h>
> - #include <linux/platform_device.h>
> +#include "linux/bits.h"

this should be <linux/bits.h>

> +#include <linux/clk.h>
> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
> +#include <linux/i2c.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>

Is the diff a bit noisy or am I failing to see some differences?

>  /* spacemit i2c registers */
>  #define SPACEMIT_ICR		 0x0		/* Control register */
>  #define SPACEMIT_ISR		 0x4		/* Status register */
>  #define SPACEMIT_IDBR		 0xc		/* Data buffer register */
> +#define SPACEMIT_ILCR		 0x10		/* Load Count Register */
>  #define SPACEMIT_IBMR		 0x1c		/* Bus monitor register */

...

> +static int spacemit_i2c_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> +				unsigned long parent_rate)

here the alignment is a bit off.

> +{
> +	struct spacemit_i2c_dev *i2c = container_of(hw, struct spacemit_i2c_dev, scl_clk_hw);
> +	u32 lv, lcr;
> +
> +	lv = DIV_ROUND_UP(parent_rate, rate);

do we want to sanity check the lv value here?

> +	lcr = readl(i2c->base + SPACEMIT_ILCR);
> +	if (i2c->mode == SPACEMIT_MODE_STANDARD) {
> +		lcr &= ~SPACEMIT_LCR_LV_STANDARD_MASK;
> +		lcr |= lv << SPACEMIT_LCR_LV_STANDARD_SHIFT;
> +	} else if (i2c->mode == SPACEMIT_MODE_FAST) {
> +		lcr &= ~SPACEMIT_LCR_LV_FAST_MASK;
> +		lcr |= lv << SPACEMIT_LCR_LV_FAST_SHIFT;
> +	}
> +	writel(lcr, i2c->base + SPACEMIT_ILCR);
> +
> +	return 0;
> +}
> +
> +static long spacemit_i2c_clk_round_rate(struct clk_hw *hw, unsigned long rate,
> +				unsigned long *parent_rate)

Alignment.

> +{
> +	u32 lv, freq;
> +
> +	lv = DIV_ROUND_UP(*parent_rate, rate);
> +	freq = DIV_ROUND_UP(*parent_rate, lv);
> +
> +	return freq;
> +}
> +
> +static unsigned long spacemit_i2c_clk_recalc_rate(struct clk_hw *hw,
> +						unsigned long parent_rate)

alignment.

> +{

...

> @@ -519,15 +636,13 @@ static int spacemit_i2c_probe(struct platform_device *pdev)
>  		dev_warn(dev, "failed to read clock-frequency property: %d\n", ret);
>  
>  	/* For now, this driver doesn't support high-speed. */
> -	if (!i2c->clock_freq || i2c->clock_freq > SPACEMIT_I2C_MAX_FAST_MODE_FREQ) {
> -		dev_warn(dev, "unsupported clock frequency %u; using %u\n",
> -			 i2c->clock_freq, SPACEMIT_I2C_MAX_FAST_MODE_FREQ);
> -		i2c->clock_freq = SPACEMIT_I2C_MAX_FAST_MODE_FREQ;
> -	} else if (i2c->clock_freq < SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ) {
> -		dev_warn(dev, "unsupported clock frequency %u; using %u\n",
> -			 i2c->clock_freq,  SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ);
> -		i2c->clock_freq = SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ;
> -	}
> +	if (i2c->clock_freq > SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ &&
> +	    i2c->clock_freq <= SPACEMIT_I2C_MAX_FAST_MODE_FREQ)
> +		i2c->mode = SPACEMIT_MODE_FAST;
> +	else if (i2c->clock_freq && i2c->clock_freq <= SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ)
> +		i2c->mode = SPACEMIT_MODE_STANDARD;
> +	else
> +		return -EINVAL;

Should we print an error here? And how likely is this case to
happen? Perhaps we can force the mode even if the frequency is
off and move forward instead of forcing the failure.

Andi

>  
>  	i2c->dev = &pdev->dev;