Re: [PATCH v2 2/4] hw/i2c: Fix checkpatch line over 80 chars warnings

[Philippe Mathieu-Daudé]

On 17/4/24 08:24, Cédric Le Goater wrote:
> Hello,
>
> On 4/16/24 20:47, Philippe Mathieu-Daudé wrote:
> > We are going to modify these lines, fix their style
> > in order to avoid checkpatch.pl warnings:
> >
> >    WARNING: line over 80 characters
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> >   include/hw/i2c/i2c.h            |  11 ++-
> >   include/hw/nvram/eeprom_at24c.h |   6 +-
> >   hw/arm/aspeed.c                 | 140 +++++++++++++++++++-------------
> >   hw/nvram/eeprom_at24c.c         |   6 +-
> >   4 files changed, 98 insertions(+), 65 deletions(-)


> > -    i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 3), 
> > "dps310", 0x76);
> > -    i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 3), 
> > "max31785", 0x52);
> > -    i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 4), 
> > "tmp423", 0x4c);
> > -    i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 5), 
> > "tmp423", 0x4c);
> > +    i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 3),
> > +                            "dps310", 0x76);
> > +    i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 3),
> > +                            "max31785", 0x52);
> > +    i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 4),
> > +                            "tmp423", 0x4c);
> > +    i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 5),
> > +                            "tmp423", 0x4c);
> >       /* The Witherspoon expects a TMP275 but a TMP105 is compatible */
> > -    i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 9), 
> > TYPE_TMP105,
> > -                     0x4a);
> > +    i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 9),
> > +                            TYPE_TMP105, 0x4a);
> >       /* The witherspoon board expects Epson RX8900 I2C RTC but a 
> > ds1338 is
> >        * good enough */
> > -    i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 11), 
> > "ds1338", 0x32);
> > +    i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 11),
> > +                            "ds1338", 0x32);
>
> If the definitions were on a single line, they would be more
> readable IMHO. So I would do the opposit change ...
>
> An alternate solution could be to define an array of devices
> at the machine class level, something like
>    struct i2c_device [
>            const char *type;
>            uint8_t bus;
>            uint8_t addr;
>    } devices[] = { ... };

I agree this would be better, but this should be done separately
of this series. For now I propose not modifying hw/arm/aspeed.c
in this patch, and ignoring the checkpatch errors in the next
patch. What do you think?