* Re: [PATCH v2 2/7] platform: cznic: Add preliminary support for Turris Omnia MCU
[not found] ` <20230919103815.16818-3-kabel@kernel.org>
@ 2023-09-19 12:29 ` Andy Shevchenko
[not found] ` <20230919171638.19bc1619@dellmb>
0 siblings, 1 reply; 4+ messages in thread
From: Andy Shevchenko @ 2023-09-19 12:29 UTC (permalink / raw)
To: Marek Behún
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Linus Walleij,
Bartosz Golaszewski, linux-gpio, Alessandro Zummo,
Alexandre Belloni, linux-rtc, Wim Van Sebroeck, Guenter Roeck,
linux-watchdog
On Tue, Sep 19, 2023 at 12:38:10PM +0200, Marek Behún wrote:
> Add a new platform driver that exposes the features provided by the
> microcontroller on the Turris Omnia board.
> This commit adds the basic skeleton for the driver.
Read "Submitting Patches" documentation (find "This patch" in it) and amend
your commit message accordingly.
...
> +Date: August 2023
> +KernelVersion: 6.6
Wrong and outdated. Use phb-crystall-ball to find the closest possible values
for both parameters here.
Ditto for all others with the same issue.
Note, it likely might be part of v6.7, not earlier.
...
> @@ -11,3 +11,4 @@ obj-$(CONFIG_OLPC_EC) += olpc/
> obj-$(CONFIG_GOLDFISH) += goldfish/
> obj-$(CONFIG_CHROME_PLATFORMS) += chrome/
> obj-$(CONFIG_SURFACE_PLATFORMS) += surface/
> +obj-$(CONFIG_CZNIC_PLATFORMS) += cznic/
Why not ordered (to some extent) here (as you did in the other file)?
...
> +turris-omnia-mcu-objs := turris-omnia-mcu-base.o
objs is for user space tools. Kernel code should use m,y.
...
> +#include <linux/hex.h>
> +#include <linux/module.h>
Missing types.h, sysfs.h, mod_devicetable.h, i2c.h, ...
...
> +static int omnia_get_version_hash(struct omnia_mcu *mcu, bool bootloader,
> + u8 *version)
> +{
> + u8 reply[20];
> + int ret;
> +
> + ret = omnia_cmd_read(mcu->client, bootloader ? CMD_GET_FW_VERSION_BOOT :
> + CMD_GET_FW_VERSION_APP,
> + reply, sizeof(reply));
> + if (ret < 0)
Can it return positive value? What would be the meaning of it?
> + return ret;
> + version[40] = '\0';
How do you know the version has enough space?
> + bin2hex(version, reply, 20);
> +
> + return 0;
> +}
> +
> +static ssize_t fw_version_hash_show(struct device *dev, char *buf,
> + bool bootloader)
> +{
> + struct omnia_mcu *mcu = i2c_get_clientdata(to_i2c_client(dev));
> + u8 version[41];
> + int err;
In two near functions you already inconsistent in the naming of the return code
variable. Be consistent across all your code, i.e. choose one name and use it
everywhere.
> + err = omnia_get_version_hash(mcu, bootloader, version);
> + if (err)
> + return err;
> +
> + return sysfs_emit(buf, "%s\n", version);
> +}
...
> + return sysfs_emit(buf, "%#x\n", mcu->features);
"0" will be printed differently, is this on purpose?
...
> + int ret = omnia_cmd_read_u8(to_i2c_client(dev), CMD_GET_RESET);
> +
> + if (ret < 0)
> + return ret;
Better from maintenance perspective to have
int ret;
ret = omnia_cmd_read_u8(to_i2c_client(dev), CMD_GET_RESET);
if (ret < 0)
return ret;
...
> +static struct attribute *omnia_mcu_attrs[] = {
> + &dev_attr_fw_version_hash_application.attr,
> + &dev_attr_fw_version_hash_bootloader.attr,
> + &dev_attr_fw_features.attr,
> + &dev_attr_mcu_type.attr,
> + &dev_attr_reset_selector.attr,
> + NULL,
No comma for the terminator line. It will make your code robust at compile time
against some misplaced entries.
> +};
> +
Unneeded blank line.
> +ATTRIBUTE_GROUPS(omnia_mcu);
...
> + u8 version[41];
This magic sizes should go away. Use predefined constant, or allocate on the
heap, depending on the case(s) you have.
...
> + int status;
My gosh, it's a _third_ name for the same!
> + status = omnia_cmd_read_u16(mcu->client, CMD_GET_STATUS_WORD);
> + if (status < 0)
> + return status;
...
> + for (int i = 0; i < ARRAY_SIZE(features); ++i) {
Why signed?
Why pre-increment?
> + if (!(mcu->features & features[i].mask)) {
Wouldn't be better
if (mcu->features & features[i].mask)
continue;
?
> + omnia_info_missing_feature(dev, features[i].name);
> + suggest_fw_upgrade = true;
> + }
> + }
...
> + dev_info(dev,
> + "Consider upgrading MCU firmware with the omnia-mcutool utility.\n");
You have so-o many dev_info() calls, are you sure you not abusing use of that?
...
> + if (ret) {
> + dev_err(dev, "Cannot determine MCU supported features: %d\n",
> + ret);
> + return ret;
return dev_err_probe(...);
> + }
...
> + if (!client->irq) {
> + dev_err(dev, "IRQ resource not found\n");
> + return -ENODATA;
> + }
Why you need to allocate memory, go through the initial checks, etc and then
fail? What's wrong with checking this first?
Why -ENODATA?!
...
> +static const struct of_device_id of_omnia_mcu_match[] = {
> + { .compatible = "cznic,turris-omnia-mcu", },
Inner comma is not needed.
> + {},
No comma for the terminator entry.
> +};
...
> +static const struct i2c_device_id omnia_mcu_id[] = {
> + { "turris-omnia-mcu", 0 },
', 0' part is redundant.
> + { }
> +};
...
> +static struct i2c_driver omnia_mcu_driver = {
> + .probe = omnia_mcu_probe,
> + .id_table = omnia_mcu_id,
> + .driver = {
> + .name = "turris-omnia-mcu",
> + .of_match_table = of_omnia_mcu_match,
> + .dev_groups = omnia_mcu_groups,
> + },
> +};
> +
Redundant blank line.
> +module_i2c_driver(omnia_mcu_driver);
...
> +#ifndef __DRIVERS_PLATFORM_CZNIC_TURRIS_OMNIA_MCU_H
> +#define __DRIVERS_PLATFORM_CZNIC_TURRIS_OMNIA_MCU_H
WE_LIKE_VERY_LONG_AND_OVERFLOWED_DEFINITIONS_H!
...
> +#include <asm/byteorder.h>
This usually goes after linux/*.h.
> +#include <linux/i2c.h>
Missing types.h, errno.h, ...
+ blank line?
> +#include <linux/turris-omnia-mcu-interface.h>
...
> + ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> + if (likely(ret == ARRAY_SIZE(msgs)))
Why likely()? Please, justify.
> + return len;
> + else if (ret < 0)
> + return ret;
> + else
> + return -EIO;
In both cases the 'else' is redundant. Moreover, the usual pattern is to check
for the errors first.
> +}
...
> +#ifndef __INCLUDE_LINUX_TURRIS_OMNIA_MCU_INTERFACE_H
> +#define __INCLUDE_LINUX_TURRIS_OMNIA_MCU_INTERFACE_H
My gosh!
...
> +#include <linux/bits.h>
> +enum commands_e {
Are you sure the name is unique enough / properly namespaced?
Same Q to all enums.
...
> + /*CTL_RESERVED = BIT(2),*/
Missing spaces?
Also, needs a comment why this is commented out.
...
> + CTL_BOOTLOADER = BIT(7)
Add trailing comma.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 5/7] platform: cznic: turris-omnia-mcu: Add support for MCU watchdog
[not found] ` <20230919103815.16818-6-kabel@kernel.org>
@ 2023-09-19 13:50 ` Guenter Roeck
0 siblings, 0 replies; 4+ messages in thread
From: Guenter Roeck @ 2023-09-19 13:50 UTC (permalink / raw)
To: Marek Behún, Gregory CLEMENT, Arnd Bergmann, soc, arm,
Wim Van Sebroeck, linux-watchdog
On 9/19/23 03:38, Marek Behún wrote:
> Add support for the watchdog mechanism provided by the MCU.
>
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
> drivers/platform/cznic/Kconfig | 2 +
> drivers/platform/cznic/Makefile | 1 +
> .../platform/cznic/turris-omnia-mcu-base.c | 4 +
> .../cznic/turris-omnia-mcu-watchdog.c | 126 ++++++++++++++++++
> drivers/platform/cznic/turris-omnia-mcu.h | 24 ++++
> 5 files changed, 157 insertions(+)
> create mode 100644 drivers/platform/cznic/turris-omnia-mcu-watchdog.c
>
> diff --git a/drivers/platform/cznic/Kconfig b/drivers/platform/cznic/Kconfig
> index 0a752aa654fa..e2649cdecc38 100644
> --- a/drivers/platform/cznic/Kconfig
> +++ b/drivers/platform/cznic/Kconfig
> @@ -20,6 +20,7 @@ config TURRIS_OMNIA_MCU
> select GPIOLIB
> select GPIOLIB_IRQCHIP
> select RTC_CLASS
> + select WATCHDOG_CORE
> help
> Say Y here to add support for the features implemented by the
> microcontroller on the CZ.NIC's Turris Omnia SOHO router.
> @@ -27,6 +28,7 @@ config TURRIS_OMNIA_MCU
> - board poweroff into true low power mode (with voltage regulators
> disabled) and the ability to configure wake up from this mode (via
> rtcwake)
> + - MCU watchdog
> - GPIO pins
> - to get front button press events (the front button can be
> configured either to generate press events to the CPU or to change
> diff --git a/drivers/platform/cznic/Makefile b/drivers/platform/cznic/Makefile
> index 6f1470d1f673..a43997a12d74 100644
> --- a/drivers/platform/cznic/Makefile
> +++ b/drivers/platform/cznic/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_TURRIS_OMNIA_MCU) += turris-omnia-mcu.o
> turris-omnia-mcu-objs := turris-omnia-mcu-base.o
> turris-omnia-mcu-objs += turris-omnia-mcu-gpio.o
> turris-omnia-mcu-objs += turris-omnia-mcu-sys-off-wakeup.o
> +turris-omnia-mcu-objs += turris-omnia-mcu-watchdog.o
> diff --git a/drivers/platform/cznic/turris-omnia-mcu-base.c b/drivers/platform/cznic/turris-omnia-mcu-base.c
> index b507f81ebe0a..708265203cc1 100644
> --- a/drivers/platform/cznic/turris-omnia-mcu-base.c
> +++ b/drivers/platform/cznic/turris-omnia-mcu-base.c
> @@ -230,6 +230,10 @@ static int omnia_mcu_probe(struct i2c_client *client)
> if (ret)
> return ret;
>
> + ret = omnia_mcu_register_watchdog(mcu);
> + if (ret)
> + return ret;
> +
> return 0;
> }
>
> diff --git a/drivers/platform/cznic/turris-omnia-mcu-watchdog.c b/drivers/platform/cznic/turris-omnia-mcu-watchdog.c
> new file mode 100644
> index 000000000000..2238b2167c45
> --- /dev/null
> +++ b/drivers/platform/cznic/turris-omnia-mcu-watchdog.c
> @@ -0,0 +1,126 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CZ.NIC's Turris Omnia MCU watchdog driver
> + *
> + * 2023 by Marek Behún <kabel@kernel.org>
> + */
> +
> +#include "turris-omnia-mcu.h"
> +
> +#define WATCHDOG_TIMEOUT 120
> +
> +static unsigned int timeout;
> +module_param(timeout, int, 0);
> +MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds");
> +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, bool, 0);
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +static int omnia_wdt_start(struct watchdog_device *wdt)
> +{
> + struct omnia_mcu *mcu = watchdog_get_drvdata(wdt);
> +
> + /*
> + * The MCU also implements LED controller, and the LED driver may in
> + * some cases require more I2C bandwidth. In order to prevent stealing
> + * that bandwidth from the LED driver, configure minimum time between
> + * pings to 10% of the configured timeout (for 120 second timeout we
> + * will be pinging at most every 12 seconds).
> + */
> + mcu->wdt.min_hw_heartbeat_ms = 100 * mcu->wdt.timeout;
This is not what min_hw_heartbeat_ms is supposed to be used for, and it is
an abuse of the API which is not guaranteed to work. The value is exopected
to be set at registration time, and it is not supposed to change dynamically.
It is also competely unnecessary since no sane watchdog daemon will ping
at such a high rate. Please drop.
Guenter
> +
> + return omnia_cmd_write_u8(mcu->client, CMD_SET_WATCHDOG_STATE, 1);
> +}
> +
> +static int omnia_wdt_stop(struct watchdog_device *wdt)
> +{
> + struct omnia_mcu *mcu = watchdog_get_drvdata(wdt);
> +
> + return omnia_cmd_write_u8(mcu->client, CMD_SET_WATCHDOG_STATE, 0);
> +}
> +
> +static int omnia_wdt_ping(struct watchdog_device *wdt)
> +{
> + struct omnia_mcu *mcu = watchdog_get_drvdata(wdt);
> +
> + return omnia_cmd_write_u8(mcu->client, CMD_SET_WATCHDOG_STATE, 1);
> +}
> +
> +static int omnia_wdt_set_timeout(struct watchdog_device *wdt,
> + unsigned int timeout)
> +{
> + struct omnia_mcu *mcu = watchdog_get_drvdata(wdt);
> +
> + return omnia_cmd_write_u16(mcu->client, CMD_SET_WDT_TIMEOUT,
> + timeout * 10);
> +}
> +
> +static unsigned int omnia_wdt_get_timeleft(struct watchdog_device *wdt)
> +{
> + struct omnia_mcu *mcu = watchdog_get_drvdata(wdt);
> + int ret;
> +
> + ret = omnia_cmd_read_u16(mcu->client, CMD_GET_WDT_TIMELEFT);
> + if (ret < 0) {
> + dev_err(&mcu->client->dev, "Cannot get watchdog timeleft: %d\n",
> + ret);
> + return 0;
> + }
> +
> + return ret / 10;
> +}
> +
> +static const struct watchdog_info omnia_wdt_info = {
> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> + .identity = "Turris Omnia MCU Watchdog",
> +};
> +
> +static const struct watchdog_ops omnia_wdt_ops = {
> + .owner = THIS_MODULE,
> + .start = omnia_wdt_start,
> + .stop = omnia_wdt_stop,
> + .ping = omnia_wdt_ping,
> + .set_timeout = omnia_wdt_set_timeout,
> + .get_timeleft = omnia_wdt_get_timeleft,
> +};
> +
> +int omnia_mcu_register_watchdog(struct omnia_mcu *mcu)
> +{
> + struct device *dev = &mcu->client->dev;
> + int ret;
> +
> + if (!(mcu->features & FEAT_WDT_PING))
> + return 0;
> +
> + mcu->wdt.info = &omnia_wdt_info;
> + mcu->wdt.ops = &omnia_wdt_ops;
> + mcu->wdt.parent = dev;
> + mcu->wdt.min_timeout = 1;
> + mcu->wdt.max_timeout = 6553; /* 65535 deciseconds */
> +
> + mcu->wdt.timeout = WATCHDOG_TIMEOUT;
> + watchdog_init_timeout(&mcu->wdt, timeout, dev);
> +
> + watchdog_set_drvdata(&mcu->wdt, mcu);
> +
> + omnia_wdt_set_timeout(&mcu->wdt, mcu->wdt.timeout);
> +
> + ret = omnia_cmd_read_u8(mcu->client, CMD_GET_WATCHDOG_STATE);
> + if (ret < 0) {
> + dev_err(dev, "Cannot get MCU watchdog state: %d\n", ret);
> + return ret;
> + }
> +
> + if (ret)
> + set_bit(WDOG_HW_RUNNING, &mcu->wdt.status);
> +
> + watchdog_set_nowayout(&mcu->wdt, nowayout);
> + watchdog_stop_on_reboot(&mcu->wdt);
> + ret = devm_watchdog_register_device(dev, &mcu->wdt);
> + if (ret)
> + dev_err(dev, "Cannot register MCU watchdog: %d\n", ret);
> +
> + return ret;
> +}
> diff --git a/drivers/platform/cznic/turris-omnia-mcu.h b/drivers/platform/cznic/turris-omnia-mcu.h
> index c40ac07be5f5..89fe39f4255f 100644
> --- a/drivers/platform/cznic/turris-omnia-mcu.h
> +++ b/drivers/platform/cznic/turris-omnia-mcu.h
> @@ -16,6 +16,7 @@
> #include <linux/mutex.h>
> #include <linux/rtc.h>
> #include <linux/turris-omnia-mcu-interface.h>
> +#include <linux/watchdog.h>
> #include <linux/workqueue.h>
>
> struct omnia_mcu {
> @@ -36,6 +37,9 @@ struct omnia_mcu {
> struct rtc_device *rtcdev;
> u32 rtc_alarm;
> bool front_button_poweron;
> +
> + /* MCU watchdog */
> + struct watchdog_device wdt;
> };
>
> static inline int omnia_cmd_write(const struct i2c_client *client, void *cmd,
> @@ -46,6 +50,25 @@ static inline int omnia_cmd_write(const struct i2c_client *client, void *cmd,
> return ret < 0 ? ret : 0;
> }
>
> +static inline int omnia_cmd_write_u8(const struct i2c_client *client, u8 cmd,
> + u8 val)
> +{
> + u8 buf[2] = { cmd, val };
> +
> + return omnia_cmd_write(client, buf, sizeof(buf));
> +}
> +
> +static inline int omnia_cmd_write_u16(const struct i2c_client *client, u8 cmd,
> + u16 val)
> +{
> + u8 buf[3];
> +
> + buf[0] = cmd;
> + put_unaligned_le16(val, &buf[1]);
> +
> + return omnia_cmd_write(client, buf, sizeof(buf));
> +}
> +
> static inline int omnia_cmd_write_u32(const struct i2c_client *client, u8 cmd,
> u32 val)
> {
> @@ -136,5 +159,6 @@ static inline int omnia_cmd_read_u8(const struct i2c_client *client, u8 cmd)
> int omnia_mcu_register_gpiochip(struct omnia_mcu *mcu);
> void omnia_mcu_unregister_gpiochip(struct omnia_mcu *mcu);
> int omnia_mcu_register_sys_off_and_wakeup(struct omnia_mcu *mcu);
> +int omnia_mcu_register_watchdog(struct omnia_mcu *mcu);
>
> #endif /* __DRIVERS_PLATFORM_CZNIC_TURRIS_OMNIA_MCU_H */
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 2/7] platform: cznic: Add preliminary support for Turris Omnia MCU
[not found] ` <20230919171638.19bc1619@dellmb>
@ 2023-09-19 18:27 ` Andy Shevchenko
[not found] ` <20230920161953.6d952392@dellmb>
0 siblings, 1 reply; 4+ messages in thread
From: Andy Shevchenko @ 2023-09-19 18:27 UTC (permalink / raw)
To: Marek Behún
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Linus Walleij,
Bartosz Golaszewski, linux-gpio, Alessandro Zummo,
Alexandre Belloni, linux-rtc, Wim Van Sebroeck, Guenter Roeck,
linux-watchdog
On Tue, Sep 19, 2023 at 05:16:38PM +0200, Marek Behún wrote:
> On Tue, 19 Sep 2023 15:29:08 +0300
> Andy Shevchenko <andy@kernel.org> wrote:
> > On Tue, Sep 19, 2023 at 12:38:10PM +0200, Marek Behún wrote:
...
> > > obj-$(CONFIG_GOLDFISH) += goldfish/
> > > obj-$(CONFIG_CHROME_PLATFORMS) += chrome/
> > > obj-$(CONFIG_SURFACE_PLATFORMS) += surface/
> > > +obj-$(CONFIG_CZNIC_PLATFORMS) += cznic/
> >
> > Why not ordered (to some extent) here (as you did in the other file)?
>
> Where should I put it? The other entries are not ordered, see
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/Makefile?h=v6.6-rc2
With the visible context above, at least after chrome already much better, no?
...
> > > +turris-omnia-mcu-objs := turris-omnia-mcu-base.o
> >
> > objs is for user space tools. Kernel code should use m,y.
>
> I don't understand. There are plenty driver using -objs. The driver
> consists of multiple C source files. I thought this should be done as
> obj-$(CONFIG_XXX) += xxx.o
> xxx-objs := xxx-a.o xxx-b.o xxx-c.o
>
> If you choose CONFIG_XXX = y, then the driver will be compiled into
> kernel, if you choose CONFIG_XXX = m, then the driver will be a module
> xxx.ko.
Hmm... Maybe I mixed something... Let me check.
For now than it's okay.
...
> > > +#include <linux/hex.h>
> > > +#include <linux/module.h>
> >
> > Missing types.h, sysfs.h, mod_devicetable.h, i2c.h, ...
>
> i2c.h comes from turris-omnia-mcu.h. The others I will fix. Does the
> the .c file also include all those headers if it includes a local
> header already?
For generic ones, yes.
...
> > > + if (ret < 0)
> >
> > Can it return positive value? What would be the meaning of it?
>
> Originally I had it return 0 on success, but recently I've had a
> discussion about this same thing with LED subsystem maintainer about
> some LED patches, Lee Jones, who requested the _read function to return
> number of bytes read on success. See at
> https://lore.kernel.org/linux-leds/20230821122644.GJ1380343@google.com/
> where he says:
> Read and write APIs, even abstracted ones such as these generally
> return the number of bytes successfully read and written respectively.
> If you are going to normalise, then please do so against this
> standard.
> I do not agree with him, since this is a local command accessor which
> only succeeds if all the bytes are read/written as requested. But I
> adjusted my code as per his request since he is the maintainer.
This is strange. For example, regmap APIs never returns amount of data written
or read. I think it's solely depends on the API. It might be useful for i²c
APIs, in case you can do something about it. but if you have wrappers on top
of that already (meaning not using directly the i2c_*() calls, I dunno the
positive return is anyhow useful.
> I will change the code back so that it returns 0 on success instead of
> the number of bytes written.
> > > + return ret;
...
> > > + version[40] = '\0';
> >
> > How do you know the version has enough space?
>
> The function is only use within the file, and both users know how much
> space it needs. But ok, I shall enforce it with the static keyword as
> static int
> omnia_get_version_hash(...,
> u8 version[static 2*OMNIA_FW_VERSION_LEN + 1]);
> Would that be okay?
Perhaps, it's a thing in a category "TIL", and it seems we have so few callers
of that.
...
> > > + int err;
> >
> > In two near functions you already inconsistent in the naming of the
> > return code variable. Be consistent across all your code, i.e. choose
> > one name and use it everywhere.
>
> I use
> ret - when the return value can also be positive and mean something
> err - when the return value is either 0 (no error) or negative errno
> Is this inconsistent?
TBH sounds good, but I close to never had heard about such a distinction in
a Linux kernel source file(s).
...
> > > + int status;
> >
> > My gosh, it's a _third_ name for the same!
>
> The variable holds value of the status register, which is needed at
> another point in this function. I thought naming it just "ret" would
> cause confusions... Do you really propose to rename it such?
It's not bad per se, just put yourself on the place of somebody who would like
to understand the semantics of all these variables. Maybe you can add a comment
on top explaining that (in short)?
...
> > > + for (int i = 0; i < ARRAY_SIZE(features); ++i) {
> >
> > Why signed?
>
> Because it is shorter and makes no difference and there are tons of
> for (int i = 0; i < ARRAY_SIZE(x); i++)
> in the kernel already. But if you really want, I can change it. Please
> let me know.
It just makes harder to get the code, because of possible unneeded questions
like "if it's a signed type, maybe it's on purpose?"
> > Why pre-increment?
>
> Again, it makes no difference, is widely used in the kernel and I
> personally prefer it. But I will change it if you want.
Pre-increment usually also a sign of something special about the iterator
variable. In non-special cases we use post-increment. And if you count
the use of each you will see the difference.
...
> > > + dev_info(dev,
> > > + "Consider upgrading MCU firmware with the
> > > omnia-mcutool utility.\n");
> >
> > You have so-o many dev_info() calls, are you sure you not abusing use
> > of that?
>
> I want the users to upgrade the MCU firmware. These infos are only
> printed during probe time, and won't be printed at all if the firmware
> is upgraded. Is this a problem?
Depends how really useful they are. If you think it's way to go, go for it.
...
> > > + if (likely(ret == ARRAY_SIZE(msgs)))
> >
> > Why likely()? Please, justify.
>
> Becuase it is unlikely the I2C transaction will fail. In most cases, it
> does not.
Yes, but why likely() is needed? So, i.o.w. what's the benefit in _this_ case?
> > > + return len;
...
> > > +enum commands_e {
> >
> > Are you sure the name is unique enough / properly namespaced?
> > Same Q to all enums.
>
> I can change it. I wanted it to be compatible with the header in the
> firmware repository.
Are these being generated, or also statically written in that repo?
If the latter, I think better to use more unique naming schema in
the kernel.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 2/7] platform: cznic: Add preliminary support for Turris Omnia MCU
[not found] ` <20230920161953.6d952392@dellmb>
@ 2023-09-20 14:47 ` Andy Shevchenko
0 siblings, 0 replies; 4+ messages in thread
From: Andy Shevchenko @ 2023-09-20 14:47 UTC (permalink / raw)
To: Marek Behún
Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Linus Walleij,
Bartosz Golaszewski, linux-gpio, Alessandro Zummo,
Alexandre Belloni, linux-rtc, Wim Van Sebroeck, Guenter Roeck,
linux-watchdog
On Wed, Sep 20, 2023 at 04:19:53PM +0200, Marek Behún wrote:
> On Tue, 19 Sep 2023 21:27:04 +0300
> Andy Shevchenko <andy@kernel.org> wrote:
> > On Tue, Sep 19, 2023 at 05:16:38PM +0200, Marek Behún wrote:
> > > On Tue, 19 Sep 2023 15:29:08 +0300
> > > Andy Shevchenko <andy@kernel.org> wrote:
> > > > On Tue, Sep 19, 2023 at 12:38:10PM +0200, Marek Behún wrote:
...
> > > > > + if (likely(ret == ARRAY_SIZE(msgs)))
> > > >
> > > > Why likely()? Please, justify.
> > >
> > > Becuase it is unlikely the I2C transaction will fail. In most cases, it
> > > does not.
> >
> > Yes, but why likely() is needed? So, i.o.w. what's the benefit in _this_ case?
>
> Compiler optimization (one branch avoided). But I guess this isn't a
> hot path, since I2C is insanely slow anyway. OK, I shall remove the
> likely() usage.
Have you seen the difference in the generated code, btw?
I don't think it will get you one independently on the hot/slow
path.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-09-20 14:48 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20230919103815.16818-1-kabel@kernel.org>
[not found] ` <20230919103815.16818-3-kabel@kernel.org>
2023-09-19 12:29 ` [PATCH v2 2/7] platform: cznic: Add preliminary support for Turris Omnia MCU Andy Shevchenko
[not found] ` <20230919171638.19bc1619@dellmb>
2023-09-19 18:27 ` Andy Shevchenko
[not found] ` <20230920161953.6d952392@dellmb>
2023-09-20 14:47 ` Andy Shevchenko
[not found] ` <20230919103815.16818-6-kabel@kernel.org>
2023-09-19 13:50 ` [PATCH v2 5/7] platform: cznic: turris-omnia-mcu: Add support for MCU watchdog Guenter Roeck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox