Hi Wolfgang,
On Tue, 19 May 2020 at 05:58, Wolfgang Wallner
<wolfgang.wall...@br-automation.com> wrote:
>
> Hi Simon,
>
> -----"Simon Glass" <s...@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 <s...@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
