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@xxxxxxxxx> wrote:
> > On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@xxxxxxxxxx> 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
