[4/8] i2c: sprd: Add IIC controller driver to support dynamic switching of 400K/1M/3.4M frequency

When IIC-slaves supporting different frequencies use the same IIC
controller, the IIC controller usually only operates at lower frequencies.
In order to improve the performance of IIC-slaves transmission supporting
faster frequencies, we dynamically configure the IIC operating frequency
based on the value of the input parameter msg ->flag.

Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
---
 drivers/i2c/busses/i2c-sprd.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

On Thu, 17 Aug 2023 at 17:46, Huangzheng Lai <Huangzheng.Lai@unisoc.com> wrote:
>
> When IIC-slaves supporting different frequencies use the same IIC

%s/I2C/IIC

> controller, the IIC controller usually only operates at lower frequencies.
> In order to improve the performance of IIC-slaves transmission supporting
> faster frequencies, we dynamically configure the IIC operating frequency
> based on the value of the input parameter msg ->flag.
>
> Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
> ---
>  drivers/i2c/busses/i2c-sprd.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> index 549b60dd3273..02c11a9ff2da 100644
> --- a/drivers/i2c/busses/i2c-sprd.c
> +++ b/drivers/i2c/busses/i2c-sprd.c
> @@ -75,7 +75,14 @@
>  #define SPRD_I2C_PM_TIMEOUT    1000
>  /* timeout (ms) for transfer message */
>  #define I2C_XFER_TIMEOUT       1000
> -
> +/* dynamic modify clk_freq flag  */
> +#define        I2C_3M4_FLAG            0x0100

#define <space> I2C_3M4_FLAG <tab> 0x0100

> +#define        I2C_1M_FLAG             0x0080
> +#define        I2C_400K_FLAG           0x0040
> +
> +#define        I2C_FREQ_400K           400000
> +#define        I2C_FREQ_1M             1000000
> +#define        I2C_FREQ_3_4M           3400000

ditto

>  /* SPRD i2c data structure */
>  struct sprd_i2c {
>         struct i2c_adapter adap;
> @@ -94,6 +101,8 @@ struct sprd_i2c {
>         int err;
>  };
>
> +static void sprd_i2c_set_clk(struct sprd_i2c *i2c_dev, u32 freq);

I would suggest moving this whole function above its caller.

> +
>  static void sprd_i2c_set_count(struct sprd_i2c *i2c_dev, u32 count)
>  {
>         writel(count, i2c_dev->base + I2C_COUNT);
> @@ -269,6 +278,12 @@ static int sprd_i2c_handle_msg(struct i2c_adapter *i2c_adap,
>                 sprd_i2c_send_stop(i2c_dev, !!is_last_msg);
>         }
>
> +       if (msg->flags & I2C_400K_FLAG)

Where does this flag set? I'm not sure we can use msg->flags like
this. I don't know i2c very well.

Thanks,
Chunyan

> +               sprd_i2c_set_clk(i2c_dev, I2C_FREQ_400K);
> +       else if (msg->flags & I2C_1M_FLAG)
> +               sprd_i2c_set_clk(i2c_dev, I2C_FREQ_1M);
> +       else if (msg->flags & I2C_3M4_FLAG)
> +               sprd_i2c_set_clk(i2c_dev, I2C_FREQ_3_4M);
>         /*
>          * We should enable rx fifo full interrupt to get data when receiving
>          * full data.
> --
> 2.17.1
>

Hi Chunyan,

[...]

> > When IIC-slaves supporting different frequencies use the same IIC
> 
> %s/I2C/IIC

[...]

> >  #define SPRD_I2C_PM_TIMEOUT    1000
> >  /* timeout (ms) for transfer message */
> >  #define I2C_XFER_TIMEOUT       1000
> > -
> > +/* dynamic modify clk_freq flag  */
> > +#define        I2C_3M4_FLAG            0x0100
> 
> #define <space> I2C_3M4_FLAG <tab> 0x0100
> 
> > +#define        I2C_1M_FLAG             0x0080
> > +#define        I2C_400K_FLAG           0x0040
> > +
> > +#define        I2C_FREQ_400K           400000
> > +#define        I2C_FREQ_1M             1000000
> > +#define        I2C_FREQ_3_4M           3400000
> 
> ditto

why should he use IIC instead of I2C. The file's defines start
with I2C, for consistency he should use the same prefix.

Andi

Hi Andi,

On Sun, 3 Sept 2023 at 05:08, Andi Shyti <andi.shyti@kernel.org> wrote:
>
> Hi Chunyan,
>
> [...]
>
> > > When IIC-slaves supporting different frequencies use the same IIC
> >
> > %s/I2C/IIC

I just noticed that this was the reverse, I meant "%s/IIC/I2C" :)
That's saying we should use I2C in the whole driver.

> [...]
>
> > >  #define SPRD_I2C_PM_TIMEOUT    1000
> > >  /* timeout (ms) for transfer message */
> > >  #define I2C_XFER_TIMEOUT       1000
> > > -
> > > +/* dynamic modify clk_freq flag  */
> > > +#define        I2C_3M4_FLAG            0x0100
> >
> > #define <space> I2C_3M4_FLAG <tab> 0x0100
> >
> > > +#define        I2C_1M_FLAG             0x0080
> > > +#define        I2C_400K_FLAG           0x0040
> > > +
> > > +#define        I2C_FREQ_400K           400000
> > > +#define        I2C_FREQ_1M             1000000
> > > +#define        I2C_FREQ_3_4M           3400000
> >
> > ditto

I meant "#define <space> I2C_FREQ_3_4M <tab> 3400000"

>
> why should he use IIC instead of I2C. The file's defines start
> with I2C, for consistency he should use the same prefix.
>

Yes, I agree.

Thanks,
Chunyan

Hi Chunyan,

> > > >  #define SPRD_I2C_PM_TIMEOUT    1000
> > > >  /* timeout (ms) for transfer message */
> > > >  #define I2C_XFER_TIMEOUT       1000
> > > > -
> > > > +/* dynamic modify clk_freq flag  */
> > > > +#define        I2C_3M4_FLAG            0x0100
> > >
> > > #define <space> I2C_3M4_FLAG <tab> 0x0100
> > >
> > > > +#define        I2C_1M_FLAG             0x0080
> > > > +#define        I2C_400K_FLAG           0x0040
> > > > +
> > > > +#define        I2C_FREQ_400K           400000
> > > > +#define        I2C_FREQ_1M             1000000
> > > > +#define        I2C_FREQ_3_4M           3400000
> > >
> > > ditto
> 
> I meant "#define <space> I2C_FREQ_3_4M <tab> 3400000"

right! Agree with your comment!

> >
> > why should he use IIC instead of I2C. The file's defines start
> > with I2C, for consistency he should use the same prefix.
> >
> 
> Yes, I agree.

Thanks,
Andi