| Message ID | 20240323164359.21642-1-kabel@kernel.org |
|---|---|
| Headers | show |
| Series | Turris Omnia MCU driver | expand |
On 3/23/24 09:43, Marek Behún wrote: > A few drivers register a devm action to remove a debugfs directory, > implementing a one-liner function that calls debufs_remove_recursive(). > Help drivers avoid this repeated implementations by adding managed > version of debugfs directory create function. > > Use the new function devm_debugfs_create_dir() in the following > drivers: > drivers/crypto/caam/ctrl.c > drivers/gpu/drm/bridge/ti-sn65dsi86.c > drivers/hwmon/hp-wmi-sensors.c > drivers/hwmon/mr75203.c > drivers/hwmon/pmbus/pmbus_core.c > Please split this up into multiple patches, at least separating out the hwmon patches. Guenter
On Sat, 23 Mar 2024 22:10:40 +0100 Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > > - return devm_add_action_or_reset(dev, gpio_mockup_debugfs_cleanup, chip); > > + return devm_add_action_or_reset(dev, devm_debugfs_dir_recursive_drop, > > + chip->dbg_dir); > > This look strange. Shouldn't the debugfs_create_dir() call in > gpio_mockup_debugfs_setup() be changed, instead? > > (I've not look in the previous version if something was said about it, > so apologies if already discussed) Yeah, this specific user needs a more complicated refactoring. I was reluctant to do more complicated refactors in the patch that also introduces these helpers. But Guenter asked me to split the patch anyway... > > static int pvt_ts_dbgfs_create(struct pvt_device *pvt, struct device *dev) > > { > > - pvt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL); > > + pvt->dbgfs_dir = devm_debugfs_create_dir(dev, dev_name(dev), NULL); > > + if (IS_ERR(pvt->dbgfs_dir)) > > + return PTR_ERR(pvt->dbgfs_dir); > > Not sure if the test and error handling should be added here. > *If I'm correct*, functions related to debugfs already handle this case > and just do nothing. And failure in debugfs related code is not > considered as something that need to be reported and abort a probe function. > > Maybe the same other (already existing) tests in this patch should be > removed as well, in a separated patch. Functions related to debugfs maybe do, but devm_ resource management functions may fail to allocate release structure, and those errors need to be handled, AFAIK. Marek
Hi Andy, thank you very much for the review. I have some notes and some questions, see below. On Sun, 24 Mar 2024 13:01:55 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > Sat, Mar 23, 2024 at 05:43:50PM +0100, Marek Behún kirjoitti: > > Add the basic skeleton for a new platform driver for the microcontroller > > found on the Turris Omnia board. > > ... > > > +++ b/drivers/platform/cznic/Makefile > > @@ -0,0 +1,4 @@ > > +# SPDX-License-Identifier: GPL-2.0-only > > + > > +obj-$(CONFIG_TURRIS_OMNIA_MCU) += turris-omnia-mcu.o > > +turris-omnia-mcu-objs := turris-omnia-mcu-base.o > > 'objs' is for user space. You need to use 'y'. Same applies to the entire > series. Fixed for v6. > > + array_size.h > + bits.h Fixed for v6. Is there some tool for this? > + string.h Fixed for v6. > > > +#include <linux/sysfs.h> > > ... > > > + err = omnia_cmd_read(mcu->client, bootloader ? CMD_GET_FW_VERSION_BOOT : > > + CMD_GET_FW_VERSION_APP, > > + reply, sizeof(reply)); > > Wouldn't be better to have a logical split? > > err = omnia_cmd_read(mcu->client, > bootloader ? CMD_GET_FW_VERSION_BOOT : CMD_GET_FW_VERSION_APP, > reply, sizeof(reply)); Changed for v6 to > err = omnia_cmd_read(mcu->client, > bootloader ? CMD_GET_FW_VERSION_BOOT > : CMD_GET_FW_VERSION_APP, > reply, sizeof(reply)); There are still some people wanting only 80 columns, and the whole driver is written that way. > > ? > > ... > > > + struct omnia_mcu *mcu = i2c_get_clientdata(to_i2c_client(dev)); > > What's wrong with dev_get_drvdata()? Fixed for v6. > ... > > > +static ssize_t fw_features_show(struct device *dev, struct device_attribute *a, > > + char *buf) > > One line? 80 columns... ... > > +static const struct attribute_group omnia_mcu_base_group = { > > + .attrs = omnia_mcu_base_attrs, > > + .is_visible = omnia_mcu_base_attrs_visible, > > +}; > > + > > +static const struct attribute_group *omnia_mcu_groups[] = { > > + &omnia_mcu_base_group, > > + NULL > > +}; > > __ATTRIBUTE_GROUPS() The next patches add more groups into this array, after the whole series it looks like this: static const struct attribute_group *omnia_mcu_groups[] = { &omnia_mcu_base_group, &omnia_mcu_gpio_group, &omnia_mcu_poweroff_group, NULL }; There is no macro for that. Should I still use __ATTRIBUTE_GROUPS() in the first patch and than change it in the next one? > > ... > > > +static struct i2c_driver omnia_mcu_driver = { > > + .probe = omnia_mcu_probe, > > + .driver = { > > + .name = "turris-omnia-mcu", > > + .of_match_table = of_omnia_mcu_match, > > + .dev_groups = omnia_mcu_groups, > > + }, > > +}; > > > + > > Redundant blank line. > Fixed for v6. > > +module_i2c_driver(omnia_mcu_driver); > > ... > > > +#ifndef __TURRIS_OMNIA_MCU_H > > +#define __TURRIS_OMNIA_MCU_H > > + array_size.h Fixed for v6. > > > +#include <linux/i2c.h> > > +#include <linux/if_ether.h> > > +#include <linux/types.h> > > +#include <asm/byteorder.h> > > ... > > > +static inline int omnia_cmd_read(const struct i2c_client *client, u8 cmd, > > + void *reply, unsigned int len) > > +{ > > Why is this in the header? I considered it a helper function that should be defined in the header file, like the rest of the cmd helpers in this file. If you disagree, I will put it into the -base.c file. > > > + struct i2c_msg msgs[2]; > > + int ret; > > + > > + msgs[0].addr = client->addr; > > + msgs[0].flags = 0; > > + msgs[0].len = 1; > > + msgs[0].buf = &cmd; > > + msgs[1].addr = client->addr; > > + msgs[1].flags = I2C_M_RD; > > + msgs[1].len = len; > > + msgs[1].buf = reply; > > + > > + ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); > > + if (ret < 0) > > + return ret; > > + if (ret != ARRAY_SIZE(msgs)) > > + return -EIO; > > + > > + return 0; > > +} > > ... > > > +#ifndef __TURRIS_OMNIA_MCU_INTERFACE_H > > +#define __TURRIS_OMNIA_MCU_INTERFACE_H > > + > > +#include <linux/bits.h> > > + bitfield.h Fixed for v6. > > > +#endif /* __TURRIS_OMNIA_MCU_INTERFACE_H */ >
On Sun, 24 Mar 2024 10:21:28 +0100 Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > Le 23/03/2024 à 22:25, Marek Behún a écrit : > > On Sat, 23 Mar 2024 22:10:40 +0100 > > Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > > > > ... > > >>> static int pvt_ts_dbgfs_create(struct pvt_device *pvt, struct device *dev) > >>> { > >>> - pvt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL); > >>> + pvt->dbgfs_dir = devm_debugfs_create_dir(dev, dev_name(dev), NULL); > >>> + if (IS_ERR(pvt->dbgfs_dir)) > >>> + return PTR_ERR(pvt->dbgfs_dir); > >> > >> Not sure if the test and error handling should be added here. > >> *If I'm correct*, functions related to debugfs already handle this case > >> and just do nothing. And failure in debugfs related code is not > >> considered as something that need to be reported and abort a probe function. > >> > >> Maybe the same other (already existing) tests in this patch should be > >> removed as well, in a separated patch. > > > > Functions related to debugfs maybe do, but devm_ resource management > > functions may fail to allocate release structure, and those errors need > > to be handled, AFAIK. > > I would say no. > If this memory allocation fails, then debugfs_create_dir() will not be > called, but that's not a really big deal if the driver itself can still > run normally without it. debugfs_create_dir() will always be called. Resource allocation is done afterwards, and if it fails, then the created dir will be removed. But now I don't know what to do, because yes, it seems that the debugfs errors are being ignored at many places... > > Up to you to leave it as-is or remove what I think is a useless error > handling. > At least, maybe it could be said in the commit log, so that maintainers > can comment on it, if they don't spot the error handling you introduce. > > CJ > > > > > Marek > > >
On Sun, Mar 24, 2024 at 5:04 PM Marek Behún <kabel@kernel.org> wrote: > > Hi Andy, > > thank you very much for the review. I have some notes and some > questions, see below. Btw, I'll look into other patches next week. > On Sun, 24 Mar 2024 13:01:55 +0200 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > Sat, Mar 23, 2024 at 05:43:50PM +0100, Marek Behún kirjoitti: ... > > > + err = omnia_cmd_read(mcu->client, bootloader ? CMD_GET_FW_VERSION_BOOT : > > > + CMD_GET_FW_VERSION_APP, > > > + reply, sizeof(reply)); > > > > Wouldn't be better to have a logical split? > > > > err = omnia_cmd_read(mcu->client, > > bootloader ? CMD_GET_FW_VERSION_BOOT : CMD_GET_FW_VERSION_APP, > > reply, sizeof(reply)); > > Changed for v6 to > > > err = omnia_cmd_read(mcu->client, > > bootloader ? CMD_GET_FW_VERSION_BOOT > > : CMD_GET_FW_VERSION_APP, > > reply, sizeof(reply)); > > There are still some people wanting only 80 columns, and the whole > driver is written that way. Hmm... Is it still a hard limit for drivers/platform/cznic for the _new_ code? > > ? ... > > > +static ssize_t fw_features_show(struct device *dev, struct device_attribute *a, > > > + char *buf) > > > > One line? > > 80 columns... Ditto. ... > > > +static const struct attribute_group *omnia_mcu_groups[] = { > > > + &omnia_mcu_base_group, > > > + NULL > > > +}; > > > > __ATTRIBUTE_GROUPS() > > The next patches add more groups into this array, after the whole > series it looks like this: > > static const struct attribute_group *omnia_mcu_groups[] = { > &omnia_mcu_base_group, > &omnia_mcu_gpio_group, > &omnia_mcu_poweroff_group, > NULL > }; > > There is no macro for that. Good point. > Should I still use __ATTRIBUTE_GROUPS() in > the first patch and than change it in the next one? ... > > > +static inline int omnia_cmd_read(const struct i2c_client *client, u8 cmd, > > > + void *reply, unsigned int len) > > > +{ > > > > Why is this in the header? > > I considered it a helper function that should be defined in the header > file, like the rest of the cmd helpers in this file. If you disagree, I > will put it into the -base.c file. I don't see the technical justification to hold it in the *.h rather than *.c. To me this one is big enough in C and likely in assembly to be copied to each user. Besides that aspect, it slows down the build a lot (mostly due to i2c.h inclusion which otherwise is not needed). > > > + struct i2c_msg msgs[2]; > > > + int ret; > > > + > > > + msgs[0].addr = client->addr; > > > + msgs[0].flags = 0; > > > + msgs[0].len = 1; > > > + msgs[0].buf = &cmd; > > > + msgs[1].addr = client->addr; > > > + msgs[1].flags = I2C_M_RD; > > > + msgs[1].len = len; > > > + msgs[1].buf = reply; > > > + > > > + ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); > > > + if (ret < 0) > > > + return ret; > > > + if (ret != ARRAY_SIZE(msgs)) > > > + return -EIO; > > > + > > > + return 0; > > > +}
On Sun, 24 Mar 2024 17:30:39 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Sun, Mar 24, 2024 at 5:04 PM Marek Behún <kabel@kernel.org> wrote: > > > > Hi Andy, > > > > thank you very much for the review. I have some notes and some > > questions, see below. > > Btw, I'll look into other patches next week. Thx. ... > > > > There are still some people wanting only 80 columns, and the whole > > driver is written that way. > > Hmm... Is it still a hard limit for drivers/platform/cznic for the _new_ code? I don't think so, but I personally would also prefer leaving this at 80 columns. Is this a problem? > > I considered it a helper function that should be defined in the header > > file, like the rest of the cmd helpers in this file. If you disagree, I > > will put it into the -base.c file. > > I don't see the technical justification to hold it in the *.h rather > than *.c. To me this one is big enough in C and likely in assembly to > be copied to each user. Besides that aspect, it slows down the build a > lot (mostly due to i2c.h inclusion which otherwise is not needed). OK, I moved it into -base.c. Marek
On Sat, Mar 23, 2024 at 10:10:40PM +0100, Christophe JAILLET wrote: > > static int pvt_ts_dbgfs_create(struct pvt_device *pvt, struct device *dev) > > { > > - pvt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL); > > + pvt->dbgfs_dir = devm_debugfs_create_dir(dev, dev_name(dev), NULL); > > + if (IS_ERR(pvt->dbgfs_dir)) > > + return PTR_ERR(pvt->dbgfs_dir); > > Not sure if the test and error handling should be added here. Yep. debugfs_create_dir() is not supposed to be checked here. It breaks the driver if CONFIG_DEBUGFS is disabled. I have written a blog about this: https://staticthinking.wordpress.com/2023/07/24/debugfs-functions-are-not-supposed-to-be-checked/ regards, dan carpenter
On Sun, Mar 24, 2024 at 05:30:39PM +0200, Andy Shevchenko wrote: > On Sun, Mar 24, 2024 at 5:04 PM Marek Behún <kabel@kernel.org> wrote: > > > > Hi Andy, > > > > thank you very much for the review. I have some notes and some > > questions, see below. > > Btw, I'll look into other patches next week. Hello Andy, did you have a chance to look at the other patches? Marek
Hello Andy, Linus, Arnd, Gregory, and others, I am sending v5 of the series adding Turris Omnia MCU driver. See the cover letters for v1, v2, v3 and v4: https://patchwork.kernel.org/project/linux-soc/cover/20230823161012.6986-1-kabel@kernel.org/ https://patchwork.kernel.org/project/linux-soc/cover/20230919103815.16818-1-kabel@kernel.org/ https://patchwork.kernel.org/project/linux-soc/cover/20231023143130.11602-1-kabel@kernel.org/ https://patchwork.kernel.org/project/linux-soc/cover/20231026161803.16750-1-kabel@kernel.org/ Changes since v4: - added new patches 06/11 devm-helpers: Add resource managed version of irq_create_mapping() 07/11 platform: cznic: turris-omnia-mcu: Add support for MCU provided TRNG 08/11 devm-helpers: Add resource managed version of debugfs directory create function 09/11 platform: cznic: turris-omnia-mcu: Add support for digital message signing via debugfs - for changes specific to patches which were also sent in previous versions see the notes in those patches Marek Behún (11): dt-bindings: arm: add cznic,turris-omnia-mcu binding platform: cznic: Add preliminary support for Turris Omnia MCU platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs platform: cznic: turris-omnia-mcu: Add support for poweroff and wakeup platform: cznic: turris-omnia-mcu: Add support for MCU watchdog devm-helpers: Add resource managed version of irq_create_mapping() platform: cznic: turris-omnia-mcu: Add support for MCU provided TRNG devm-helpers: Add resource managed version of debugfs directory create function platform: cznic: turris-omnia-mcu: Add support for digital message signing via debugfs ARM: dts: turris-omnia: Add MCU system-controller node ARM: dts: turris-omnia: Add GPIO key node for front button .../ABI/testing/debugfs-turris-omnia-mcu | 13 + .../sysfs-bus-i2c-devices-turris-omnia-mcu | 126 ++ .../bindings/arm/cznic,turris-omnia-mcu.yaml | 86 ++ MAINTAINERS | 5 + .../dts/marvell/armada-385-turris-omnia.dts | 35 +- drivers/crypto/caam/ctrl.c | 16 +- drivers/crypto/caam/jr.c | 8 +- drivers/gpio/gpio-mockup.c | 11 +- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 13 +- drivers/hwmon/hp-wmi-sensors.c | 15 +- drivers/hwmon/mr75203.c | 15 +- drivers/hwmon/pmbus/pmbus_core.c | 16 +- drivers/platform/Kconfig | 2 + drivers/platform/Makefile | 1 + drivers/platform/cznic/Kconfig | 51 + drivers/platform/cznic/Makefile | 10 + .../platform/cznic/turris-omnia-mcu-base.c | 406 +++++++ .../platform/cznic/turris-omnia-mcu-debugfs.c | 216 ++++ .../platform/cznic/turris-omnia-mcu-gpio.c | 1056 +++++++++++++++++ .../cznic/turris-omnia-mcu-sys-off-wakeup.c | 258 ++++ .../platform/cznic/turris-omnia-mcu-trng.c | 89 ++ .../cznic/turris-omnia-mcu-watchdog.c | 122 ++ drivers/platform/cznic/turris-omnia-mcu.h | 214 ++++ include/linux/devm-helpers.h | 94 ++ include/linux/turris-omnia-mcu-interface.h | 238 ++++ 25 files changed, 3044 insertions(+), 72 deletions(-) create mode 100644 Documentation/ABI/testing/debugfs-turris-omnia-mcu create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu create mode 100644 Documentation/devicetree/bindings/arm/cznic,turris-omnia-mcu.yaml create mode 100644 drivers/platform/cznic/Kconfig create mode 100644 drivers/platform/cznic/Makefile create mode 100644 drivers/platform/cznic/turris-omnia-mcu-base.c create mode 100644 drivers/platform/cznic/turris-omnia-mcu-debugfs.c create mode 100644 drivers/platform/cznic/turris-omnia-mcu-gpio.c create mode 100644 drivers/platform/cznic/turris-omnia-mcu-sys-off-wakeup.c create mode 100644 drivers/platform/cznic/turris-omnia-mcu-trng.c create mode 100644 drivers/platform/cznic/turris-omnia-mcu-watchdog.c create mode 100644 drivers/platform/cznic/turris-omnia-mcu.h create mode 100644 include/linux/turris-omnia-mcu-interface.h