Re: [PATCH v1 11/15] aspeed: Add I2C Driver

[Maxim Sloyko <maxims@google.com>]

[-- Attachment #1: Type: text/plain, Size: 1686 bytes --]

On Tue, Apr 18, 2017 at 5:12 PM, Simon Glass <sjg@chromium.org> wrote:

> On 17 April 2017 at 13:00, Maxim Sloyko <maxims@google.com> wrote:
> > Add Device Model based I2C driver for ast2500/ast2400 SoCs.
> > The driver is very limited, it only supports master mode and
> > synchronous byte-by-byte reads/writes, no DMA or Pool Buffers.
> >
> > Signed-off-by: Maxim Sloyko <maxims@google.com>
> >
> > ---
> >
> > Changes in v1:
> > - Style fixes
> >
> >
> > ---
> >  drivers/i2c/Kconfig   |   9 ++
> >  drivers/i2c/Makefile  |   1 +
> >  drivers/i2c/ast_i2c.c | 357 ++++++++++++++++++++++++++++++
> ++++++++++++++++++++
> >  drivers/i2c/ast_i2c.h | 132 +++++++++++++++++++
> >  4 files changed, 499 insertions(+)
> >  create mode 100644 drivers/i2c/ast_i2c.c
> >  create mode 100644 drivers/i2c/ast_i2c.h
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> nit below
>
> [..]
> > +static int ast_i2c_ofdata_to_platdata(struct udevice *dev)
> > +{
> > +       struct ast_i2c_priv *priv = dev_get_priv(dev);
> > +       int ret;
> > +
> > +       priv->regs = dev_get_addr_ptr(dev);
> > +       if (IS_ERR(priv->regs))
> > +               return PTR_ERR(priv->regs);
>
> Should be
>
>  if (!priv->regs)
>
> I think
>

Looks like dev_get_addr_ptr returns FDT_ADDR_T_NONE (cast to void*) in case
of error. FDT_ADDR_T_NONE is -1, so simple !priv->regs check would be
incorrect, as far as I understand.


>
> > +
> > +       ret = clk_get_by_index(dev, 0, &priv->clk);
> > +       if (ret < 0) {
> > +               debug("%s: Can't get clock for %s: %d\n", __func__,
> dev->name,
> > +                     ret);
> > +               return ret;
>
> Regards,
> Simon
>



-- 
*M*axim *S*loyko

[-- Attachment #2: Type: text/html, Size: 2834 bytes --]