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:
> >
> > 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@xxxxxxxxxx>
> > +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.
