[PATCH v2 11/35] acpi: Support generation of I2C descriptor

[Simon Glass]

Hi Wolfgang,

On Tue, 19 May 2020 at 05:58, Wolfgang Wallner
<wolfgang.wallner at br-automation.com> wrote:
>
> Hi Simon,
>
> -----"Simon Glass" <sjg at chromium.org> schrieb: -----
> > Betreff: [PATCH v2 11/35] acpi: Support generation of I2C descriptor
> >
> > Add a function to write a GPIO descriptor to the generated ACPI code.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
> > Changes in v2:
> > - Fix memset of I2C descriptor
> >
> > Changes in v1: None
> >
> >  drivers/i2c/sandbox_i2c.c  |  11 ++++
> >  drivers/rtc/sandbox_rtc.c  |  13 +++++
> >  include/acpi/acpi_device.h |  36 +++++++++++++
> >  lib/acpi/acpi_device.c     | 103 +++++++++++++++++++++++++++++++++++++
> >  test/dm/acpigen.c          |  32 ++++++++++++
> >  5 files changed, 195 insertions(+)
> >

[..]

> > +/**
> > + * acpi_device_set_i2c() - Set up an ACPI I2C struct from a device
> > + *
> > + * @dev: I2C device to convert
> > + * @i2c: Place to put the new structure
> > + * @scope: Scope of the I2C device (this is the controller path)
>
> From the declaration I would assume that "scope" is internally copied, but in
> the code it is only referenced.
> I would propose to add something like the following to its description:
> "The value of scope is not copied, but only referenced. This implies the
> caller has to ensure it stays valid for the lifetime of i2c."

OK done

>
> > + * @return 0 (always)
>
> dev_get_parent_platdata() could return NULL. Should we check this and return
> and error?
>
> > + */
> > +static int acpi_device_set_i2c(const struct udevice *dev, struct acpi_i2c *i2c,
> > +                            const char *scope)
> > +{
> > +     struct dm_i2c_chip *chip = dev_get_parent_platdata(dev);
> > +     struct udevice *bus = dev_get_parent(dev);
> > +
> > +     memset(i2c, '\0', sizeof(*i2c));
>
> Nit: memset(i2c, 0, sizeof(*i2c));
>
> This is only a style question. But it seems 0 is used for memset in existing
> U-Boot code much more often then '\0' (~120 grep results vs ~1100 grep results).

Yes I do feel in the minority. I used to write 0 but was corrected by
a CS professor years ago and it has stuck with me. It makes it clear
that it is the character that is repeated, not integer, which is
presumably larger than a byte.

>
> > +     i2c->address = chip->chip_addr;
> > +     i2c->mode_10bit = 0;
> > +
> > +     /*
> > +      * i2c_bus->speed_hz is set if this device is probed, but if not we
> > +      * must use the device tree
> > +      */
> > +     i2c->speed = dev_read_u32_default(bus, "clock-frequency", 100000);
>
> Nit: I2C_SPEED_STANDARD_RATE instead of 100000?

Done

Regards,
Simon