| Message ID | 20230919103815.16818-3-kabel@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Turris Omnia MCU driver | expand |
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.
Hello Andy, thank you for your review. I have some questions regarding your comments. 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: > > 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. OK. > > +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. OK > > 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 > > +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. > > +#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? > > +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 : > > +n average development ti > > CMD_GET_FW_VERSION_APP, > > + reply, sizeof(reply)); > > > + 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. 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? > > + 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. 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? > > + 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? No, I wanted it to print 0x0, didn't know '#' would print just 0. Thanks. > > + 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; OK > > +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. OK > > +}; > > > + > > Unneeded blank line. OK > > +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. OK > > + 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? > > + 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? 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. > 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. > > > + if (!(mcu->features & features[i].mask)) { > > Wouldn't be better > > if (mcu->features & features[i].mask) > continue; > > ? OK > > + 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? 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? > > ... > > > + if (ret) { > > + dev_err(dev, "Cannot determine MCU supported > > features: %d\n", > > + ret); > > + return ret; > > return dev_err_probe(...); Thx. Didn't know this one. > > + } > > ... > > > + 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?! OK > > +static const struct of_device_id of_omnia_mcu_match[] = { > > + { .compatible = "cznic,turris-omnia-mcu", }, > > Inner comma is not needed. OK > > + {}, > > No comma for the terminator entry. OK > > +}; > > ... > > > +static const struct i2c_device_id omnia_mcu_id[] = { > > + { "turris-omnia-mcu", 0 }, > > ', 0' part is redundant. OK > > + { } > > +}; > > ... > > > +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. OK > > +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! Sigh... OK, I will change it :) > > +#include <asm/byteorder.h> > > This usually goes after linux/*.h. OK > > > +#include <linux/i2c.h> > > Missing types.h, errno.h, ... > > + blank line? Will change. > > +#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. Becuase it is unlikely the I2C transaction will fail. In most cases, it does not. > > + 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. I can change it. I wanted it to be compatible with the header in the firmware repository. > > ... > > > + /*CTL_RESERVED = BIT(2),*/ > > Missing spaces? > Also, needs a comment why this is commented out. OK > > + CTL_BOOTLOADER = BIT(7) > > Add trailing comma. OK Marek
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.
Hello Andy, 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: > > ... > > > > > 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? OK ... > > > 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. OK > > > > + 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 shall change it back then. ... > > > > + 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)? OK > > > > + 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?" OK > > > 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. OK ... > > > > + 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. > ... > > > > > +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. OK, I shall add some prefix. Marek
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.
diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu b/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu new file mode 100644 index 000000000000..69a4db7e20c0 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu @@ -0,0 +1,45 @@ +What: /sys/bus/i2c/devices/<mcu_device>/fw_features +Date: August 2023 +KernelVersion: 6.6 +Contact: Marek Behún <kabel@kernel.org> +Description: (RO) Newer versions of the microcontroller firmware report the + features they support. These can be read from this file. If the + MCU firmware is too old, this file reads 0x0. + + Format: 0x%x. + +What: /sys/bus/i2c/devices/<mcu_device>/fw_version_hash_application +Date: August 2023 +KernelVersion: 6.6 +Contact: Marek Behún <kabel@kernel.org> +Description: (RO) Contains the version hash (commit hash) of the application + part of the microcontroller firmware. + + Format: %s. + +What: /sys/bus/i2c/devices/<mcu_device>/fw_version_hash_bootloader +Date: August 2023 +KernelVersion: 6.6 +Contact: Marek Behún <kabel@kernel.org> +Description: (RO) Contains the version hash (commit hash) of the bootloader + part of the microcontroller firmware. + + Format: %s. + +What: /sys/bus/i2c/devices/<mcu_device>/mcu_type +Date: August 2023 +KernelVersion: 6.6 +Contact: Marek Behún <kabel@kernel.org> +Description: (RO) Contains the microcontroller type (STM32, GD32, MKL). + + Format: %s. + +What: /sys/bus/i2c/devices/<mcu_device>/reset_selector +Date: August 2023 +KernelVersion: 6.6 +Contact: Marek Behún <kabel@kernel.org> +Description: (RO) Contains the selected factory reset level, determined by + how long the rear reset button was held by the user during board + reset. + + Format: %i. diff --git a/MAINTAINERS b/MAINTAINERS index 0a4d7d6ed63a..1396743d5f9f 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2089,6 +2089,7 @@ M: Marek Behún <kabel@kernel.org> S: Maintained W: https://www.turris.cz/ F: Documentation/ABI/testing/debugfs-moxtet +F: Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu F: Documentation/ABI/testing/sysfs-bus-moxtet-devices F: Documentation/ABI/testing/sysfs-firmware-turris-mox-rwtm F: Documentation/devicetree/bindings/arm/cznic,turris-omnia-mcu.yaml @@ -2102,10 +2103,12 @@ F: drivers/firmware/turris-mox-rwtm.c F: drivers/gpio/gpio-moxtet.c F: drivers/leds/leds-turris-omnia.c F: drivers/mailbox/armada-37xx-rwtm-mailbox.c +F: drivers/platform/cznic/ F: drivers/watchdog/armada_37xx_wdt.c F: include/dt-bindings/bus/moxtet.h F: include/linux/armada-37xx-rwtm-mailbox.h F: include/linux/moxtet.h +F: include/linux/turris-omnia-mcu-interface.h ARM/FARADAY FA526 PORT M: Hans Ulli Kroll <ulli.kroll@googlemail.com> diff --git a/drivers/platform/Kconfig b/drivers/platform/Kconfig index 868b20361769..fef907a94001 100644 --- a/drivers/platform/Kconfig +++ b/drivers/platform/Kconfig @@ -7,6 +7,8 @@ source "drivers/platform/goldfish/Kconfig" source "drivers/platform/chrome/Kconfig" +source "drivers/platform/cznic/Kconfig" + source "drivers/platform/mellanox/Kconfig" source "drivers/platform/olpc/Kconfig" diff --git a/drivers/platform/Makefile b/drivers/platform/Makefile index 41640172975a..cc66b5020a34 100644 --- a/drivers/platform/Makefile +++ b/drivers/platform/Makefile @@ -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/ diff --git a/drivers/platform/cznic/Kconfig b/drivers/platform/cznic/Kconfig new file mode 100644 index 000000000000..f8560ff9c1af --- /dev/null +++ b/drivers/platform/cznic/Kconfig @@ -0,0 +1,26 @@ +# SPDX-License-Identifier: GPL-2.0-only +# +# For a description of the syntax of this configuration file, +# see Documentation/kbuild/kconfig-language.rst. +# + +menuconfig CZNIC_PLATFORMS + bool "Platform support for CZ.NIC's Turris hardware" + depends on MACH_ARMADA_38X || COMPILE_TEST + 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 + 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. + +endif # CZNIC_PLATFORMS diff --git a/drivers/platform/cznic/Makefile b/drivers/platform/cznic/Makefile new file mode 100644 index 000000000000..4d0a9586538c --- /dev/null +++ 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 diff --git a/drivers/platform/cznic/turris-omnia-mcu-base.c b/drivers/platform/cznic/turris-omnia-mcu-base.c new file mode 100644 index 000000000000..2d3c3f9c68fc --- /dev/null +++ b/drivers/platform/cznic/turris-omnia-mcu-base.c @@ -0,0 +1,253 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * CZ.NIC's Turris Omnia MCU driver + * + * 2023 by Marek Behún <kabel@kernel.org> + */ + +#include <linux/hex.h> +#include <linux/module.h> + +#include "turris-omnia-mcu.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) + return ret; + + version[40] = '\0'; + 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; + + err = omnia_get_version_hash(mcu, bootloader, version); + if (err) + return err; + + return sysfs_emit(buf, "%s\n", version); +} + +static ssize_t fw_version_hash_application_show(struct device *dev, + struct device_attribute *a, + char *buf) +{ + return fw_version_hash_show(dev, buf, false); +} +static DEVICE_ATTR_RO(fw_version_hash_application); + +static ssize_t fw_version_hash_bootloader_show(struct device *dev, + struct device_attribute *a, + char *buf) +{ + return fw_version_hash_show(dev, buf, true); +} +static DEVICE_ATTR_RO(fw_version_hash_bootloader); + +static ssize_t fw_features_show(struct device *dev, struct device_attribute *a, + char *buf) +{ + struct omnia_mcu *mcu = i2c_get_clientdata(to_i2c_client(dev)); + + return sysfs_emit(buf, "%#x\n", mcu->features); +} +static DEVICE_ATTR_RO(fw_features); + +static ssize_t mcu_type_show(struct device *dev, struct device_attribute *a, + char *buf) +{ + struct omnia_mcu *mcu = i2c_get_clientdata(to_i2c_client(dev)); + + return sysfs_emit(buf, "%s\n", mcu->type); +} +static DEVICE_ATTR_RO(mcu_type); + +static ssize_t reset_selector_show(struct device *dev, + struct device_attribute *a, char *buf) +{ + int ret = omnia_cmd_read_u8(to_i2c_client(dev), CMD_GET_RESET); + + if (ret < 0) + return ret; + + return sysfs_emit(buf, "%d\n", ret); +} +static DEVICE_ATTR_RO(reset_selector); + +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, +}; + +ATTRIBUTE_GROUPS(omnia_mcu); + +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[41]; + 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); + return; + } + + dev_info(dev, "MCU %s firmware version hash: %s\n", type, version); +} + +static const char *omnia_status_to_mcu_type(uint16_t status) +{ + 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 void omnia_info_missing_feature(struct device *dev, const char *feature) +{ + dev_info(dev, + "Your board's MCU firmware does not support the %s feature.\n", + feature); +} + +static int omnia_mcu_read_features(struct omnia_mcu *mcu) +{ + static const struct { + uint16_t mask; + 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" }, + }; + struct device *dev = &mcu->client->dev; + bool suggest_fw_upgrade = false; + int status; + + status = omnia_cmd_read_u16(mcu->client, CMD_GET_STATUS_WORD); + if (status < 0) + return status; + + /* check whether MCU firmware supports the CMD_GET_FEAUTRES command */ + if (status & STS_FEATURES_SUPPORTED) { + int val = omnia_cmd_read_u16(mcu->client, CMD_GET_FEATURES); + + if (val < 0) + return val; + + mcu->features = val; + } else { + omnia_info_missing_feature(dev, "feature reading"); + suggest_fw_upgrade = true; + } + + mcu->type = omnia_status_to_mcu_type(status); + dev_info(dev, "MCU type %s%s\n", mcu->type, + (mcu->features & FEAT_PERIPH_MCU) ? + ", with peripheral resets wired" : ""); + + omnia_mcu_print_version_hash(mcu, true); + + if (mcu->features & FEAT_BOOTLOADER) + dev_warn(dev, + "MCU is running bootloader firmware. Was firmware upgrade interrupted?\n"); + else + omnia_mcu_print_version_hash(mcu, false); + + for (int i = 0; i < ARRAY_SIZE(features); ++i) { + if (!(mcu->features & features[i].mask)) { + omnia_info_missing_feature(dev, features[i].name); + suggest_fw_upgrade = true; + } + } + + if (suggest_fw_upgrade) + dev_info(dev, + "Consider upgrading MCU firmware with the omnia-mcutool utility.\n"); + + return 0; +} + +static int omnia_mcu_probe(struct i2c_client *client) +{ + struct device *dev = &client->dev; + struct omnia_mcu *mcu; + int ret; + + mcu = devm_kzalloc(dev, sizeof(*mcu), GFP_KERNEL); + if (!mcu) + return -ENOMEM; + + mcu->client = client; + i2c_set_clientdata(client, mcu); + + ret = omnia_mcu_read_features(mcu); + if (ret) { + dev_err(dev, "Cannot determine MCU supported features: %d\n", + ret); + return ret; + } + + if (!client->irq) { + dev_err(dev, "IRQ resource not found\n"); + return -ENODATA; + } + + return 0; +} + +static const struct of_device_id of_omnia_mcu_match[] = { + { .compatible = "cznic,turris-omnia-mcu", }, + {}, +}; + +static const struct i2c_device_id omnia_mcu_id[] = { + { "turris-omnia-mcu", 0 }, + { } +}; +MODULE_DEVICE_TABLE(i2c, omnia_mcu_id); + +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, + }, +}; + +module_i2c_driver(omnia_mcu_driver); + +MODULE_AUTHOR("Marek Behun <kabel@kernel.org>"); +MODULE_DESCRIPTION("CZ.NIC's Turris Omnia MCU"); +MODULE_LICENSE("GPL"); diff --git a/drivers/platform/cznic/turris-omnia-mcu.h b/drivers/platform/cznic/turris-omnia-mcu.h new file mode 100644 index 000000000000..a045b114e293 --- /dev/null +++ b/drivers/platform/cznic/turris-omnia-mcu.h @@ -0,0 +1,65 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * CZ.NIC's Turris Omnia MCU driver + * + * 2023 by Marek Behún <kabel@kernel.org> + */ + +#ifndef __DRIVERS_PLATFORM_CZNIC_TURRIS_OMNIA_MCU_H +#define __DRIVERS_PLATFORM_CZNIC_TURRIS_OMNIA_MCU_H + +#include <asm/byteorder.h> +#include <linux/i2c.h> +#include <linux/turris-omnia-mcu-interface.h> + +struct omnia_mcu { + struct i2c_client *client; + const char *type; + u16 features; +}; + +static inline int omnia_cmd_read(const struct i2c_client *client, u8 cmd, void *reply, + unsigned int len) +{ + 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 (likely(ret == ARRAY_SIZE(msgs))) + return len; + else if (ret < 0) + return ret; + else + return -EIO; +} + +static inline int omnia_cmd_read_u16(const struct i2c_client *client, u8 cmd) +{ + u16 reply; + int ret; + + ret = omnia_cmd_read(client, cmd, &reply, sizeof(reply)); + + return ret < 0 ? ret : le16_to_cpu(reply); +} + +static inline int omnia_cmd_read_u8(const struct i2c_client *client, u8 cmd) +{ + u8 reply; + int ret; + + ret = omnia_cmd_read(client, cmd, &reply, sizeof(reply)); + + return ret < 0 ? ret : reply; +} + +#endif /* __DRIVERS_PLATFORM_CZNIC_TURRIS_OMNIA_MCU_H */ diff --git a/include/linux/turris-omnia-mcu-interface.h b/include/linux/turris-omnia-mcu-interface.h new file mode 100644 index 000000000000..888dc74c5ec0 --- /dev/null +++ b/include/linux/turris-omnia-mcu-interface.h @@ -0,0 +1,194 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * CZ.NIC's Turris Omnia MCU I2C interface commands definitions + * + * 2023 by Marek Behún <kabel@kernel.org> + */ + +#ifndef __INCLUDE_LINUX_TURRIS_OMNIA_MCU_INTERFACE_H +#define __INCLUDE_LINUX_TURRIS_OMNIA_MCU_INTERFACE_H + +#include <linux/bits.h> + +enum commands_e { + CMD_GET_STATUS_WORD = 0x01, /* slave sends status word back */ + CMD_GENERAL_CONTROL = 0x02, + CMD_LED_MODE = 0x03, /* default/user */ + CMD_LED_STATE = 0x04, /* LED on/off */ + CMD_LED_COLOR = 0x05, /* LED number + RED + GREEN + BLUE */ + CMD_USER_VOLTAGE = 0x06, + CMD_SET_BRIGHTNESS = 0x07, + CMD_GET_BRIGHTNESS = 0x08, + CMD_GET_RESET = 0x09, + CMD_GET_FW_VERSION_APP = 0x0A, /* 20B git hash number */ + CMD_SET_WATCHDOG_STATE = 0x0B, /* 0 - disable + * 1 - enable / ping + * after boot watchdog is started + * with 2 minutes timeout + */ + + /* CMD_WATCHDOG_STATUS = 0x0C, not implemented anymore */ + + CMD_GET_WATCHDOG_STATE = 0x0D, + CMD_GET_FW_VERSION_BOOT = 0x0E, /* 20B git hash number */ + CMD_GET_FW_CHECKSUM = 0x0F, /* 4B length, 4B checksum */ + + /* available if FEATURES_SUPPORTED bit set in status word */ + CMD_GET_FEATURES = 0x10, + + /* available if EXT_CMD bit set in features */ + CMD_GET_EXT_STATUS_DWORD = 0x11, + CMD_EXT_CONTROL = 0x12, + CMD_GET_EXT_CONTROL_STATUS = 0x13, + + /* available if NEW_INT_API bit set in features */ + CMD_GET_INT_AND_CLEAR = 0x14, + CMD_GET_INT_MASK = 0x15, + CMD_SET_INT_MASK = 0x16, + + /* available if FLASHING bit set in features */ + CMD_FLASH = 0x19, + + /* available if WDT_PING bit set in features */ + CMD_SET_WDT_TIMEOUT = 0x20, + CMD_GET_WDT_TIMELEFT = 0x21, + + /* available if POWEROFF_WAKEUP bit set in features */ + CMD_SET_WAKEUP = 0x22, + CMD_GET_UPTIME_AND_WAKEUP = 0x23, + CMD_POWER_OFF = 0x24, + + /* available only at address 0x2b (led-controller) */ + /* available only if LED_GAMMA_CORRECTION bit set in features */ + CMD_SET_GAMMA_CORRECTION = 0x30, + CMD_GET_GAMMA_CORRECTION = 0x31, +}; + +enum flashing_commands_e { + FLASH_CMD_UNLOCK = 0x01, + FLASH_CMD_SIZE_AND_CSUM = 0x02, + FLASH_CMD_PROGRAM = 0x03, + FLASH_CMD_RESET = 0x04, +}; + +enum sts_word_e { + STS_MCU_TYPE_MASK = GENMASK(1, 0), + STS_MCU_TYPE_STM32 = 0 << 0, + STS_MCU_TYPE_GD32 = 1 << 0, + STS_MCU_TYPE_MKL = 2 << 0, + STS_FEATURES_SUPPORTED = BIT(2), + STS_USER_REGULATOR_NOT_SUPPORTED = BIT(3), + STS_CARD_DET = BIT(4), + STS_MSATA_IND = BIT(5), + STS_USB30_OVC = BIT(6), + STS_USB31_OVC = BIT(7), + STS_USB30_PWRON = BIT(8), + STS_USB31_PWRON = BIT(9), + STS_ENABLE_4V5 = BIT(10), + STS_BUTTON_MODE = BIT(11), + STS_BUTTON_PRESSED = BIT(12), + STS_BUTTON_COUNTER_MASK = GENMASK(15, 13) +}; + +enum ctl_byte_e { + CTL_LIGHT_RST = BIT(0), + CTL_HARD_RST = BIT(1), + /*CTL_RESERVED = BIT(2),*/ + CTL_USB30_PWRON = BIT(3), + CTL_USB31_PWRON = BIT(4), + CTL_ENABLE_4V5 = BIT(5), + CTL_BUTTON_MODE = BIT(6), + CTL_BOOTLOADER = BIT(7) +}; + +enum features_e { + FEAT_PERIPH_MCU = BIT(0), + FEAT_EXT_CMDS = BIT(1), + FEAT_WDT_PING = BIT(2), + FEAT_LED_STATE_EXT_MASK = GENMASK(4, 3), + FEAT_LED_STATE_EXT = 1 << 3, + FEAT_LED_STATE_EXT_V32 = 2 << 3, + FEAT_LED_GAMMA_CORRECTION = BIT(5), + FEAT_NEW_INT_API = BIT(6), + FEAT_BOOTLOADER = BIT(7), + FEAT_FLASHING = BIT(8), + FEAT_NEW_MESSAGE_API = BIT(9), + FEAT_BRIGHTNESS_INT = BIT(10), + FEAT_POWEROFF_WAKEUP = BIT(11), +}; + +enum ext_sts_dword_e { + EXT_STS_SFP_nDET = BIT(0), + EXT_STS_LED_STATES_MASK = GENMASK(31, 12), + EXT_STS_WLAN0_MSATA_LED = BIT(12), + EXT_STS_WLAN1_LED = BIT(13), + EXT_STS_WLAN2_LED = BIT(14), + EXT_STS_WPAN0_LED = BIT(15), + EXT_STS_WPAN1_LED = BIT(16), + EXT_STS_WPAN2_LED = BIT(17), + EXT_STS_WAN_LED0 = BIT(18), + EXT_STS_WAN_LED1 = BIT(19), + EXT_STS_LAN0_LED0 = BIT(20), + EXT_STS_LAN0_LED1 = BIT(21), + EXT_STS_LAN1_LED0 = BIT(22), + EXT_STS_LAN1_LED1 = BIT(23), + EXT_STS_LAN2_LED0 = BIT(24), + EXT_STS_LAN2_LED1 = BIT(25), + EXT_STS_LAN3_LED0 = BIT(26), + EXT_STS_LAN3_LED1 = BIT(27), + EXT_STS_LAN4_LED0 = BIT(28), + EXT_STS_LAN4_LED1 = BIT(29), + EXT_STS_LAN5_LED0 = BIT(30), + EXT_STS_LAN5_LED1 = BIT(31), +}; + +enum ext_ctl_e { + EXT_CTL_nRES_MMC = BIT(0), + EXT_CTL_nRES_LAN = BIT(1), + EXT_CTL_nRES_PHY = BIT(2), + EXT_CTL_nPERST0 = BIT(3), + EXT_CTL_nPERST1 = BIT(4), + EXT_CTL_nPERST2 = BIT(5), + EXT_CTL_PHY_SFP = BIT(6), + EXT_CTL_PHY_SFP_AUTO = BIT(7), + EXT_CTL_nVHV_CTRL = BIT(8), +}; + +enum int_e { + INT_CARD_DET = BIT(0), + INT_MSATA_IND = BIT(1), + INT_USB30_OVC = BIT(2), + INT_USB31_OVC = BIT(3), + INT_BUTTON_PRESSED = BIT(4), + INT_SFP_nDET = BIT(5), + INT_BRIGHTNESS_CHANGED = BIT(6), + + INT_LED_STATES_MASK = GENMASK(31, 12), + INT_WLAN0_MSATA_LED = BIT(12), + INT_WLAN1_LED = BIT(13), + INT_WLAN2_LED = BIT(14), + INT_WPAN0_LED = BIT(15), + INT_WPAN1_LED = BIT(16), + INT_WPAN2_LED = BIT(17), + INT_WAN_LED0 = BIT(18), + INT_WAN_LED1 = BIT(19), + INT_LAN0_LED0 = BIT(20), + INT_LAN0_LED1 = BIT(21), + INT_LAN1_LED0 = BIT(22), + INT_LAN1_LED1 = BIT(23), + INT_LAN2_LED0 = BIT(24), + INT_LAN2_LED1 = BIT(25), + INT_LAN3_LED0 = BIT(26), + INT_LAN3_LED1 = BIT(27), + INT_LAN4_LED0 = BIT(28), + INT_LAN4_LED1 = BIT(29), + INT_LAN5_LED0 = BIT(30), + INT_LAN5_LED1 = BIT(31), +}; + +enum cmd_poweroff_e { + CMD_POWER_OFF_POWERON_BUTTON = BIT(0), + CMD_POWER_OFF_MAGIC = 0xdead, +}; + +#endif /* __INCLUDE_LINUX_TURRIS_OMNIA_MCU_INTERFACE_H */
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. Signed-off-by: Marek Behún <kabel@kernel.org> --- .../sysfs-bus-i2c-devices-turris-omnia-mcu | 45 ++++ MAINTAINERS | 3 + drivers/platform/Kconfig | 2 + drivers/platform/Makefile | 1 + drivers/platform/cznic/Kconfig | 26 ++ drivers/platform/cznic/Makefile | 4 + .../platform/cznic/turris-omnia-mcu-base.c | 253 ++++++++++++++++++ drivers/platform/cznic/turris-omnia-mcu.h | 65 +++++ include/linux/turris-omnia-mcu-interface.h | 194 ++++++++++++++ 9 files changed, 593 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu 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.h create mode 100644 include/linux/turris-omnia-mcu-interface.h