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