Re: [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Tue, Apr 30, 2024 at 04:05:07PM +0200, Marek Behún wrote:
> On Tue, 30 Apr 2024 15:53:51 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote:

...

> > > +static int omnia_get_version_hash(struct omnia_mcu *mcu, bool bootloader,
> > > +                                 u8 version[static OMNIA_FW_VERSION_HEX_LEN])
> > 
> > Interesting format of the last parameter. Does it make any difference
> > to the compiler if you use u8 *version?
> 
> The compiler will warn if an array with not enough space is passed as
> argument.

Really?

> > > +{
> > > +       u8 reply[OMNIA_FW_VERSION_LEN];
> > > +       int err;
> > > +
> > > +       err = omnia_cmd_read(mcu->client,
> > > +                            bootloader ? CMD_GET_FW_VERSION_BOOT
> > > +                                       : CMD_GET_FW_VERSION_APP,
> > > +                            reply, sizeof(reply));
> > > +       if (err)
> > > +               return err;  
> > 
> > > +       version[OMNIA_FW_VERSION_HEX_LEN - 1] = '\0';
> > > +       bin2hex(version, reply, OMNIA_FW_VERSION_LEN);  
> > 
> > Hmm... I would rather use returned value
> > 
> > char *p;
> > 
> > p = bin2hex(...);
> > *p = '\0';
> > 
> > return 0;
> 
> OK. I guess
> 
>   *bin2hex(version, reply, OMNIA_FW_VERSION_LEN) = '\0';
> 
> would be too crazy, right?

Yes, it's not a Python :-)

> > > +       return 0;
> > > +}  

...

> > > +               dev_err(dev, "Cannot read MCU %s firmware version: %d\n", type,
> > > +                       err);  
> > 
> > One  line?
> 
> I'd like to keep this driver to 80 columns.

Then better to have a logical split then?

			dev_err(dev, "Cannot read MCU %s firmware version: %d\n",
				type, err);

...

> > > +               omnia_info_missing_feature(dev, "feature reading");  
> > 
> > Tautology. Read the final message. I believe you wanted to pass just
> > "reading" here.
> 
> No, I actually wanted it to print
>   Your board's MCU firmware does not support the feature reading
>   feature.
> as in the feature "feature reading" is not supported.
> I guess I could change it to
>   Your board's MCU firmware does not support the feature reading.
> but that would complicate the code: either I would need to add
> " feature" suffix to all the features[].name, or duplicate the
> info string from the omnia_info_missing_feature() function.

From point of a mere user (as I am towards this driver) I consider
the tautology confusing.

	...the 'reading' feature

may be a good compromise.

...

> > > +       memcpy(mcu->board_first_mac, &reply[9], ETH_ALEN);  
> > 
> > There is an API ether_copy_addr() or so, don't remember by heart.
> > You also need an include for ETH_ALEN definition.
> 
> Doc for ether_addr_copy says:
>   Please note: dst & src must both be aligned to u16.
> since the code does:
>   u16 *a = (u16 *)dst;
>   const u16 *b = (const u16 *)src;
> 
>   a[0] = b[0];
>   a[1] = b[1];
>   a[2] = b[2]
> 
> Since I am copying from &reply[9], which is not u16-aligned, this won't
> work.

It would work on architectures that support misaligned accesses, but in general
you are right. Perhaps a comment on top to avoid "cleanup" patches like this?

...

> > > +enum omnia_ctl_byte_e {
> > > +       CTL_LIGHT_RST           = BIT(0),
> > > +       CTL_HARD_RST            = BIT(1),
> > > +       /* BIT(2) is currently reserved */
> > > +       CTL_USB30_PWRON         = BIT(3),
> > > +       CTL_USB31_PWRON         = BIT(4),
> > > +       CTL_ENABLE_4V5          = BIT(5),
> > > +       CTL_BUTTON_MODE         = BIT(6),
> > > +       CTL_BOOTLOADER          = BIT(7)  
> > 
> > Keep trailing comma as it might be extended (theoretically). And you
> > do the similar in other enums anyway.
> 
> ctl_byt is 8-bit, so this enum really can't be extended.

I understand that (even before writing the previous reply).

> In fact I need
> to drop the last comma from omnia_ext_sts_dword_e and omnia_int_e.

Who prevents from having in a new firmware let's say

 BIT(31) | BIT(1)

as a new value?

From Linux perspective these are not terminating lines.

-- 
With Best Regards,
Andy Shevchenko