[PATCH v3 22/23] i2c: designware_i2c: Separate out the speed calculation

[Simon Glass]

Hi Heinrich,

On Wed, 22 Apr 2020 at 00:43, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 1/23/20 7:48 PM, Simon Glass wrote:
> > We want to be able to calculate the speed separately from actually setting
> > the speed, so we can generate the required ACPI tables. Split out the
> > calculation into its own function.
> >
> > Drop the double underscore on __dw_i2c_set_bus_speed while we are here.
> > That is reserved for compiler internals.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
> > Changes in v3:
> > - Add new patch to separate out the speed calculation
> >
> > Changes in v2: None
> >
> >  drivers/i2c/designware_i2c.c | 78 +++++++++++++++++++++---------------
> >  drivers/i2c/designware_i2c.h |  3 ++
> >  2 files changed, 48 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/i2c/designware_i2c.c b/drivers/i2c/designware_i2c.c
> > index 6be98ee43b..39af25af9a 100644
> > --- a/drivers/i2c/designware_i2c.c
> > +++ b/drivers/i2c/designware_i2c.c
> > @@ -194,22 +194,12 @@ static int dw_i2c_calc_timing(struct dw_i2c *priv, enum i2c_speed_mode mode,
> >       return 0;
> >  }
> >
> > -/*
> > - * i2c_set_bus_speed - Set the i2c speed
> > - * @speed:   required i2c speed
> > - *
> > - * Set the i2c speed.
> > - */
> > -static unsigned int __dw_i2c_set_bus_speed(struct dw_i2c *priv,
> > -                                        struct i2c_regs *i2c_base,
> > -                                        unsigned int speed,
> > -                                        unsigned int bus_clk)
> > +static int calc_bus_speed(struct dw_i2c *priv, int speed, ulong bus_clk,
> > +                       struct dw_i2c_speed_config *config)
> >  {
> >       const struct dw_scl_sda_cfg *scl_sda_cfg = NULL;
> > -     struct dw_i2c_speed_config config;
> > +     struct i2c_regs *regs = priv->regs;
>
> Later in the code you have 'if (priv)'. Please, do not dereference priv
> before the check.
>
> Overall the code is somehow odd:
>
> _dw_i2c_set_bus_speed() is called in multiple places with priv == NULL
> and then calls calc_bus_speed(priv, ...).
>
> Then in calc_bus_speed() you have:
>
> comp_param1 = readl(&regs->comp_param1);
>
> where regs == NULL->regs.
>
> comp_param1 is used later on in the code to determine i2c_spd which is
> returned in config->speed_mode.

Only for non-DM though I think. Still this is horrible, will send a patch.

What board are you testing with?

>
> Could you, please, have a close look at the driver.
>
> Best regards
>
> Heinrich
>
Regards,
Simon