Re: [PATCH v5 02/11] platform: cznic: Add preliminary support for Turris Omnia MCU

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

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