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
