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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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

...

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

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

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

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

> +}

...

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

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

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

...

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

...

> +#include <linux/i2c.h>

No users of this, you may replace with

struct i2c_client;

Am I right?

...

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

Git

...

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

LED-controller

...

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

> +};

--
With Best Regards,
Andy Shevchenko





[Index of Archives]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux