From: "Marek Behún" <kabel@kernel.org>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: "Gregory CLEMENT" <gregory.clement@bootlin.com>,
"Arnd Bergmann" <arnd@arndb.de>,
soc@kernel.org, arm@kernel.org,
"Andy Shevchenko" <andy@kernel.org>,
"Hans de Goede" <hdegoede@redhat.com>,
"Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>,
"Linus Walleij" <linus.walleij@linaro.org>,
"Bartosz Golaszewski" <brgl@bgdev.pl>,
linux-gpio@vger.kernel.org,
"Alessandro Zummo" <a.zummo@towertech.it>,
"Alexandre Belloni" <alexandre.belloni@bootlin.com>,
linux-rtc@vger.kernel.org,
"Wim Van Sebroeck" <wim@linux-watchdog.org>,
"Guenter Roeck" <linux@roeck-us.net>,
linux-watchdog@vger.kernel.org
Subject: Re: [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU
Date: Tue, 30 Apr 2024 16:05:07 +0200 [thread overview]
Message-ID: <20240430160507.45f1f098@dellmb> (raw)
In-Reply-To: <CAHp75VcgfvyZ9rcNev9CpQEN3CkUVozEkv+ycaQggPbE4tx+1Q@mail.gmail.com>
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.
next prev parent reply other threads:[~2024-04-30 14:05 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-30 11:51 [PATCH v8 0/9] Turris Omnia MCU driver Marek Behún
2024-04-30 11:51 ` [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU Marek Behún
2024-04-30 12:53 ` Andy Shevchenko
2024-04-30 14:05 ` Marek Behún [this message]
2024-04-30 15:10 ` Dan Carpenter
2024-04-30 15:17 ` Andy Shevchenko
2024-05-02 18:40 ` Marek Behún
2024-05-02 18:47 ` Andy Shevchenko
2024-05-02 19:17 ` Marek Behún
2024-05-03 3:59 ` Andy Shevchenko
2024-05-03 6:51 ` Marek Behún
2024-04-30 15:31 ` Ilpo Järvinen
2024-05-02 19:19 ` Marek Behún
2024-04-30 11:51 ` [PATCH v8 5/9] platform: cznic: turris-omnia-mcu: Add support for MCU watchdog Marek Behún
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240430160507.45f1f098@dellmb \
--to=kabel@kernel.org \
--cc=a.zummo@towertech.it \
--cc=alexandre.belloni@bootlin.com \
--cc=andy.shevchenko@gmail.com \
--cc=andy@kernel.org \
--cc=arm@kernel.org \
--cc=arnd@arndb.de \
--cc=brgl@bgdev.pl \
--cc=gregory.clement@bootlin.com \
--cc=hdegoede@redhat.com \
--cc=ilpo.jarvinen@linux.intel.com \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-rtc@vger.kernel.org \
--cc=linux-watchdog@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=soc@kernel.org \
--cc=wim@linux-watchdog.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).