On Sun, Mar 24, 2024 at 5:04 PM Marek Behún <kabel@xxxxxxxxxx> wrote:
>
> Hi Andy,
>
> thank you very much for the review. I have some notes and some
> questions, see below.
Btw, I'll look into other patches next week.
> On Sun, 24 Mar 2024 13:01:55 +0200
> Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> > Sat, Mar 23, 2024 at 05:43:50PM +0100, Marek Behún kirjoitti:
...
> > > + err = omnia_cmd_read(mcu->client, bootloader ? CMD_GET_FW_VERSION_BOOT :
> > > + CMD_GET_FW_VERSION_APP,
> > > + reply, sizeof(reply));
> >
> > Wouldn't be better to have a logical split?
> >
> > err = omnia_cmd_read(mcu->client,
> > bootloader ? CMD_GET_FW_VERSION_BOOT : CMD_GET_FW_VERSION_APP,
> > reply, sizeof(reply));
>
> Changed for v6 to
>
> > err = omnia_cmd_read(mcu->client,
> > bootloader ? CMD_GET_FW_VERSION_BOOT
> > : CMD_GET_FW_VERSION_APP,
> > reply, sizeof(reply));
>
> There are still some people wanting only 80 columns, and the whole
> driver is written that way.
Hmm... Is it still a hard limit for drivers/platform/cznic for the _new_ code?
> > ?
...
> > > +static ssize_t fw_features_show(struct device *dev, struct device_attribute *a,
> > > + char *buf)
> >
> > One line?
>
> 80 columns...
Ditto.
...
> > > +static const struct attribute_group *omnia_mcu_groups[] = {
> > > + &omnia_mcu_base_group,
> > > + NULL
> > > +};
> >
> > __ATTRIBUTE_GROUPS()
>
> The next patches add more groups into this array, after the whole
> series it looks like this:
>
> static const struct attribute_group *omnia_mcu_groups[] = {
> &omnia_mcu_base_group,
> &omnia_mcu_gpio_group,
> &omnia_mcu_poweroff_group,
> NULL
> };
>
> There is no macro for that.
Good point.
> Should I still use __ATTRIBUTE_GROUPS() in
> the first patch and than change it in the next one?
...
> > > +static inline int omnia_cmd_read(const struct i2c_client *client, u8 cmd,
> > > + void *reply, unsigned int len)
> > > +{
> >
> > Why is this in the header?
>
> I considered it a helper function that should be defined in the header
> file, like the rest of the cmd helpers in this file. If you disagree, I
> will put it into the -base.c file.
I don't see the technical justification to hold it in the *.h rather
than *.c. To me this one is big enough in C and likely in assembly to
be copied to each user. Besides that aspect, it slows down the build a
lot (mostly due to i2c.h inclusion which otherwise is not needed).
> > > + struct i2c_msg msgs[2];
> > > + int ret;
> > > +
> > > + msgs[0].addr = client->addr;
> > > + msgs[0].flags = 0;
> > > + msgs[0].len = 1;
> > > + msgs[0].buf = &cmd;
> > > + msgs[1].addr = client->addr;
> > > + msgs[1].flags = I2C_M_RD;
> > > + msgs[1].len = len;
> > > + msgs[1].buf = reply;
> > > +
> > > + ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> > > + if (ret < 0)
> > > + return ret;
> > > + if (ret != ARRAY_SIZE(msgs))
> > > + return -EIO;
> > > +
> > > + return 0;
> > > +}
--
With Best Regards,
Andy Shevchenko
