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

["Marek Behún" <kabel@kernel.org>]

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:
> >
> > Add the basic skeleton for a new platform driver for the microcontroller
> > found on the Turris Omnia board.  
> 
> ...
> 
> > +What:          /sys/bus/i2c/devices/<mcu_device>/serial_number
> > +Date:          July 2024
> > +KernelVersion: 6.10
> > +Contact:       Marek Behún <kabel@kernel.org>
> > +Description:   (RO) Contains the 64 bit long board serial number in hexadecimal  
> 
> 64 bit long --> 64-bit

ok

> 
> > +               format.
> > +
> > +               Only available if board information is burned in the MCU (older
> > +               revisions have board information burned in the ATSHA204-A chip).
> > +
> > +               Format: %016X.  
> 
> It's strange to use capitalized hexadecimal here and not in other
> files, but maybe it's something special about "serial number"? Dunno.

Yes, the serial number is printed with captial hex letters.

> > +menuconfig CZNIC_PLATFORMS
> > +       bool "Platform support for CZ.NIC's Turris hardware"  
> 
> > +       depends on MACH_ARMADA_38X || COMPILE_TEST  
> 
> This...
> 
> > +       help
> > +         Say Y here to be able to choose driver support for CZ.NIC's Turris
> > +         devices. This option alone does not add any kernel code.
> > +
> > +if CZNIC_PLATFORMS
> > +
> > +config TURRIS_OMNIA_MCU
> > +       tristate "Turris Omnia MCU driver"  
> 
> > +       depends on MACH_ARMADA_38X || COMPILE_TEST  
> 
> ...or this dependency is redundant. I think one would expect that
> these platforms will not be always based on the same platform, hence I
> would drop the former and leave the latter. But you should know better
> than me.

ok

> 
> > +       depends on I2C
> > +       help
> > +         Say Y here to add support for the features implemented by the
> > +         microcontroller on the CZ.NIC's Turris Omnia SOHO router.
> > +         To compile this driver as a module, choose M here; the module will be
> > +         called turris-omnia-mcu.  
> 
> ...
> 
> > +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.

> 
> > +{
> > +       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?

> 
> > +       return 0;
> > +}  
> 
> ...
> 
> > +static umode_t omnia_mcu_base_attrs_visible(struct kobject *kobj,
> > +                                           struct attribute *a, int n)
> > +{
> > +       struct device *dev = kobj_to_dev(kobj);
> > +       struct omnia_mcu *mcu = dev_get_drvdata(dev);  
> 
> > +       umode_t mode = a->mode;  
> 
> Do you need this?
> 
> > +       if ((a == &dev_attr_serial_number.attr ||
> > +            a == &dev_attr_first_mac_address.attr ||
> > +            a == &dev_attr_board_revision.attr) &&
> > +           !(mcu->features & FEAT_BOARD_INFO))  
> 
> > +               mode = 0;  
> 
> return 0; ?
> 
> > +       return mode;  
> 
> return a->mode; ?

ok

> 
> > +}  
> 
> ...
> 
> > +static void omnia_mcu_print_version_hash(struct omnia_mcu *mcu, bool bootloader)
> > +{
> > +       const char *type = bootloader ? "bootloader" : "application";
> > +       struct device *dev = &mcu->client->dev;
> > +       u8 version[OMNIA_FW_VERSION_HEX_LEN];
> > +       int err;
> > +
> > +       err = omnia_get_version_hash(mcu, bootloader, version);
> > +       if (err) {
> > +               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.

> 
> > +               return;
> > +       }
> > +
> > +       dev_info(dev, "MCU %s firmware version hash: %s\n", type, version);
> > +}  
> 
> ...
> 
> > +static const char *omnia_status_to_mcu_type(uint16_t status)  
> 
> Why out of a sudden uint16_t instead of u16?

This was a mistake, thanks.

> > +{
> > +       switch (status & STS_MCU_TYPE_MASK) {
> > +       case STS_MCU_TYPE_STM32:
> > +               return "STM32";
> > +       case STS_MCU_TYPE_GD32:
> > +               return "GD32";
> > +       case STS_MCU_TYPE_MKL:
> > +               return "MKL";
> > +       default:
> > +               return "unknown";
> > +       }
> > +}  
> 
> ...
> 
> > +       static const struct {
> > +               uint16_t mask;  
> 
> Ditto.
> 
> > +               const char *name;
> > +       } features[] = {
> > +               { FEAT_EXT_CMDS,           "extended control and status" },
> > +               { FEAT_WDT_PING,           "watchdog pinging" },
> > +               { FEAT_LED_STATE_EXT_MASK, "peripheral LED pins reading" },
> > +               { FEAT_NEW_INT_API,        "new interrupt API" },
> > +               { FEAT_POWEROFF_WAKEUP,    "poweroff and wakeup" },
> > +               { FEAT_TRNG,               "true random number generator" },
> > +       };  
> 
> ...
> 
> > +               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.

> ...
> 
> > +       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.

> ...
> 
> > +#include <linux/i2c.h>  
> 
> No users of this, you may replace with
> 
> struct i2c_client;
> 
> Am I right?

OK.

> 
> ...
> 
> > +       CMD_GET_FW_VERSION_BOOT         = 0x0E, /* 20B git hash
> > number */  
> 
> Git

OK.

> ...
> 
> > +       /* available only at address 0x2b (led-controller) */  
> 
> LED-controller

OK

> 
> ...
> 
> > +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. In fact I need
to drop the last comma from omnia_ext_sts_dword_e and omnia_int_e.

Thanks, Andy.