| Message ID | 20201025005916.64747-1-luka.kovacic@sartura.hr |
|---|---|
| Headers | show |
| Series | Add support for the IEI WT61P803 PUZZLE MCU | expand |
On Sun, Oct 25, 2020 at 3:59 AM Luka Kovacic <luka.kovacic@sartura.hr> wrote: > > Add a driver for the IEI WT61P803 PUZZLE microcontroller, used in some > IEI Puzzle series devices. The microcontroller controls system power, > temperature sensors, fans and LEDs. > > This driver implements the core functionality for device communication > over the system serial (serdev bus). It handles MCU messages and the > internal MCU properties. Some properties can be managed over sysfs. ... > +#include <asm/unaligned.h> asm/* usually go after linux/*. If you get a comment against one place in your series it implies to check the other potential places to address. > +#include <linux/atomic.h> > +#include <linux/delay.h> > +#include <linux/delay.h> Delay should delay :-) > +#include <linux/export.h> > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/mfd/iei-wt61p803-puzzle.h> > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/of_device.h> Don't see a user of this, but of_platform.h seems to be missed. > +#include <linux/property.h> > +#include <linux/sched.h> > +#include <linux/serdev.h> > +#include <linux/slab.h> > +#include <linux/sysfs.h> ... > +#define IEI_WT61P803_PUZZLE_MAX_COMMAND_LENGTH (20 + 2) Since it uses formula, can you add a comment explaining what is the meaning of each argument? ... > +enum iei_wt61p803_puzzle_reply_state { > + FRAME_OK = 0x00, > + FRAME_PROCESSING = 0x01, > + FRAME_STRUCT_EMPTY = 0xFF, > + FRAME_TIMEOUT = 0xFE Hmm, why not ordered? > +}; ... > +struct iei_wt61p803_puzzle_mcu_version { > + char version[IEI_WT61P803_PUZZLE_VERSION_VERSION_LENGTH + 1]; > + char build_info[IEI_WT61P803_PUZZLE_VERSION_BUILD_INFO_LENGTH + 1]; > + bool bootloader_mode; > + char protocol_version[IEI_WT61P803_PUZZLE_VERSION_PROTOCOL_VERSION_LENGTH + 1]; > + char serial_number[IEI_WT61P803_PUZZLE_VERSION_SN_LENGTH + 1]; > + char mac_address[8][IEI_WT61P803_PUZZLE_VERSION_MAC_LENGTH + 1]; Perhaps additional constant to include (presumably) NUL ? Also, what about 8? > +}; ... > +struct iei_wt61p803_puzzle { > + struct serdev_device *serdev; > + struct kobject *kobj; It's quite strange you need this, > + struct mutex reply_lock; > + struct mutex bus_lock; > + struct iei_wt61p803_puzzle_reply *reply; > + struct iei_wt61p803_puzzle_mcu_version version; > + struct iei_wt61p803_puzzle_mcu_status status; > + unsigned char *response_buffer; > + struct mutex lock; > +}; ... > +static int iei_wt61p803_puzzle_recv_buf(struct serdev_device *serdev, > + const unsigned char *data, size_t size) > +{ > + struct iei_wt61p803_puzzle *mcu = serdev_device_get_drvdata(serdev); > + int ret; > + > + ret = iei_wt61p803_puzzle_process_resp(mcu, (unsigned char *)data, size); Dropping const, why? > + /* Return the number of processed bytes if function returns error */ > + if (ret < 0) > + return (int)size; Will be interesting result, maybe you wanted other way around? > + return ret; > +} ... > + dev_err(dev, "%s: Command response timed out. Retries: %d", __func__, retry_count); Drop __func__, it should not be critical for properly formulated messages (for debug Dynamic Debug may take care of this at run time). > + return -ETIMEDOUT; ... > + struct device *dev = &mcu->serdev->dev; > + int ret; > + int len = (int)size; Why len can't be size_t? Can it be also organized in reversed xmas tree order? ... > + ret = serdev_device_write(mcu->serdev, cmd, len, IEI_WT61P803_PUZZLE_GENERAL_TIMEOUT); > + Not a competition for LOCs, please drop unneeded blank lines here and there. > + if (ret < 0) { > + mutex_unlock(&mcu->bus_lock); > + return ret; > + } > + if (!mcu->reply) { > + ret = -EFAULT; Why this error code? > + goto exit; > + } ... > +exit: Perhaps exit_unlock: ? > + mutex_unlock(&mcu->lock); > + return ret; ... > + sprintf(mcu->version.version, "v%c.%c%c%c", rb[2], rb[3], rb[4], rb[5]); Can be '%.3s' for the second part, but it's up to you. ... > + sprintf(mcu->version.build_info, "%c%c/%c%c/%c%c%c%c %c%c:%c%c", > + rb[8], rb[9], rb[6], rb[7], rb[2], > + rb[3], rb[4], rb[5], rb[10], rb[11], > + rb[12], rb[13]); Ditto. ... > + sprintf(mcu->version.protocol_version, "v%c.%c%c%c%c%c", > + rb[7], rb[6], rb[5], rb[4], rb[3], rb[2]); Ditto. ... > +err: err_unlock: ? > + mutex_unlock(&mcu->lock); > + return ret; ... > + /* Response format: > + * (IDX RESPONSE) > + * 0 @ > + * 1 O > + * 2 S > + * 3 S > + * ... > + * 5 AC Recovery Status Flag > + * ... > + * 10 Power Loss Recovery > + * ... > + * 19 Power Status (system power on method) > + * 20 XOR checksum > + */ Shouldn't be rather defined data structure for response? ... > + size_t reply_size = 0; Dummy? ... > + sprintf(mcu->version.serial_number, "%.*s", > + IEI_WT61P803_PUZZLE_VERSION_SN_LENGTH, resp_buf + 4); Shouldn't you check for reply_size to be big enough? ... > + serial_number_header[2] = 0x0 + (0xC) * sn_counter; Why capital, why in parentheses? ... > + memcpy(serial_number_cmd + 4, serial_number + (0xC) * sn_counter, 0xC); Ditto. ... > + serial_number_cmd[sizeof(serial_number_cmd) - 1] = 0; You defined X+1 to then use sizeof() -1? Hmm... ... > + if (!(resp_buf[0] == IEI_WT61P803_PUZZLE_CMD_HEADER_START && > + resp_buf[1] == IEI_WT61P803_PUZZLE_CMD_RESPONSE_OK && > + resp_buf[2] == IEI_WT61P803_PUZZLE_CHECKSUM_RESPONSE_OK)) { > + ret = -EPROTO; > + goto err; > + } I think it would be better to define data structure for replies and then check would be as simple as memcmp(). ... > + if (reply_size < 22) { Looking at the code organisation it seems to me like if (reply_size < sizeof(struct_of_this_type_of_reply)). > + ret = -EIO; > + goto err; > + } ... > + mac_address_header[2] = 0x24 + (0x11) * mac_address_idx; Why in parentheses? ... > + /* Concat mac_address_header, mac_address to mac_address_cmd */ > + memcpy(mac_address_cmd, mac_address_header, 4); > + memcpy(mac_address_cmd + 4, mac_address, 17); Yeah, much easier to use specific field names instead of this 4 / + 4, 17, ... ... > + ret = snprintf(cmd_buf, sizeof(cmd_buf), "%d", power_loss_recovery_action); > + if (ret < 0) > + return ret; ... > + power_loss_recovery_cmd[3] = cmd_buf[0]; One decimal (most significant) digit?! Isn't it a bit ambiguous? ... > +#define sysfs_container(dev) \ > + (container_of((dev)->kobj.parent, struct device, kobj)) > + > +static ssize_t version_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + struct device *dev_container = sysfs_container(dev); > + struct iei_wt61p803_puzzle *mcu = dev_get_drvdata(dev_container); > + > + return sprintf(buf, "%s\n", mcu->version.version); > +} > +static DEVICE_ATTR_RO(version); I believe we have better approach than this. dev_groups, for example. ... > + if ((int)count != IEI_WT61P803_PUZZLE_VERSION_SN_LENGTH + 1) > + return -EINVAL; You need to revisit all of these strange castings here and there. It should be really rear to have explicit castings in C. ... > + memcpy(serial_number, (unsigned char *)buf, IEI_WT61P803_PUZZLE_VERSION_SN_LENGTH); This casting is not need. Basically any casting from or to void * is not needed. ... > + dev_info(dev, "Driver baud rate: %d", baud); Why being so noisy, how does it help user? Doesn't serdev has a facility to show this rather basic stuff? ... > + dev_info(dev, "MCU version: %s", mcu->version.version); > + dev_info(dev, "MCU firmware build info: %s", mcu->version.build_info); > + dev_info(dev, "MCU in bootloader mode: %s", > + mcu->version.bootloader_mode ? "true" : "false"); > + dev_info(dev, "MCU protocol version: %s", mcu->version.protocol_version); How all of this can be useful for *working* case? ... > + ret = iei_wt61p803_puzzle_sysfs_create(dev, mcu); No check? ... Have I missed ABI documentation?
Hi! > Add support for the IEI WT61P803 PUZZLE LED driver. > Currently only the front panel power LED is supported, > since it is the only LED on this board wired through the > MCU. > > The LED is wired directly to the on-board MCU controller > and is toggled using an MCU command. > > Support for more LEDs is going to be added in case more > boards implement this microcontroller, as LEDs use many > different GPIOs. Not too bad. > This driver depends on the IEI WT61P803 PUZZLE MFD driver. > > Signed-off-by: Luka Kovacic <luka.kovacic@sartura.hr> > Cc: Luka Perkov <luka.perkov@sartura.hr> > Cc: Robert Marko <robert.marko@sartura.hr> > --- > drivers/leds/Kconfig | 8 ++ > drivers/leds/Makefile | 1 + > drivers/leds/leds-iei-wt61p803-puzzle.c | 161 ++++++++++++++++++++++++ > 3 files changed, 170 insertions(+) Can you put it into drivers/leds/simple? You'll have to create it. > +++ b/drivers/leds/leds-iei-wt61p803-puzzle.c > @@ -0,0 +1,161 @@ > +// SPDX-License-Identifier: GPL-2.0-only Make sure this is consistent with MODULE_LICENSE("GPL");. GPLv2+ would be nicer if you can. > + struct mutex lock; Mutex is _way_ overkill for this. Please check that locking provided by LED core is not sufficient. If not, please use atomic_t or something. Best regards, Pavel
Hello Pavel, On Thu, Oct 29, 2020 at 6:58 PM Pavel Machek <pavel@ucw.cz> wrote: > > Hi! > > > Add support for the IEI WT61P803 PUZZLE LED driver. > > Currently only the front panel power LED is supported, > > since it is the only LED on this board wired through the > > MCU. > > > > The LED is wired directly to the on-board MCU controller > > and is toggled using an MCU command. > > > > Support for more LEDs is going to be added in case more > > boards implement this microcontroller, as LEDs use many > > different GPIOs. > > Not too bad. > > > This driver depends on the IEI WT61P803 PUZZLE MFD driver. > > > > Signed-off-by: Luka Kovacic <luka.kovacic@sartura.hr> > > Cc: Luka Perkov <luka.perkov@sartura.hr> > > Cc: Robert Marko <robert.marko@sartura.hr> > > --- > > drivers/leds/Kconfig | 8 ++ > > drivers/leds/Makefile | 1 + > > drivers/leds/leds-iei-wt61p803-puzzle.c | 161 ++++++++++++++++++++++++ > > 3 files changed, 170 insertions(+) > > Can you put it into drivers/leds/simple? You'll have to create it. Sure, I'll move the driver there. > > > +++ b/drivers/leds/leds-iei-wt61p803-puzzle.c > > @@ -0,0 +1,161 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > Make sure this is consistent with MODULE_LICENSE("GPL");. GPLv2+ would > be nicer if you can. Okay, I'll see what I can do... Although isn't it okay to use either GPL-2.0-only or GPL-2.0+ with MODULE_LICENSE("GPL") as described in Documentation/process/license-rules.rst on line 441? > > > + struct mutex lock; > > Mutex is _way_ overkill for this. Please check that locking provided > by LED core is not sufficient. If not, please use atomic_t or > something. Ok. > > Best regards, > Pavel > -- > http://www.livejournal.com/~pavelmachek Kind regards, Luka
Hello Pavel, On Thu, Oct 29, 2020 at 7:01 PM Pavel Machek <pavel@ucw.cz> wrote: > > Hi! > > > +What: /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/power_status > > +Date: September 2020 > > +Contact: Luka Kovacic <luka.kovacic@sartura.hr> > > +Description: (RO) Power status indicates the host platform power on method. > > + Value mapping (bitwise list): > > + 0x80 - Null > > + 0x40 - Firmware flag > > + 0x20 - Power loss detection flag (powered off) > > + 0x10 - Power loss detection flag (AC mode) > > + 0x08 - Button power on > > + 0x04 - WOL power on > > + 0x02 - RTC alarm power on > > + 0x01 - AC recover power on > > It would be nice to put this into standard place somewhere. Many > machines will want to expose this information. As this is specific to this microcontroller and to how it encodes these values, I don't see a need to change this. This isn't used anywhere else. > > If not, at least spell out WoL, as it is not that common of acronym. Okay. > > > +What: /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/ac_recovery_status > > +Date: September 2020 > > +Contact: Luka Kovacic <luka.kovacic@sartura.hr> > > +Description: (RO) Host platform AC recovery status value > > I can not tell what this is from documentation... I'll expand the description. > > Best regards, > Pavel > > -- > http://www.livejournal.com/~pavelmachek Kind regards, Luka
Hello Andy, On Mon, Oct 26, 2020 at 11:54 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Sun, Oct 25, 2020 at 3:59 AM Luka Kovacic <luka.kovacic@sartura.hr> wrote: > > > > Add a driver for the IEI WT61P803 PUZZLE microcontroller, used in some > > IEI Puzzle series devices. The microcontroller controls system power, > > temperature sensors, fans and LEDs. > > > > This driver implements the core functionality for device communication > > over the system serial (serdev bus). It handles MCU messages and the > > internal MCU properties. Some properties can be managed over sysfs. > > ... > > > +#include <asm/unaligned.h> > > asm/* usually go after linux/*. > If you get a comment against one place in your series it implies to > check the other potential places to address. Okay. > > > +#include <linux/atomic.h> > > > +#include <linux/delay.h> > > +#include <linux/delay.h> > > Delay should delay :-) It certainly will :) > > > +#include <linux/export.h> > > +#include <linux/init.h> > > +#include <linux/kernel.h> > > +#include <linux/mfd/iei-wt61p803-puzzle.h> > > +#include <linux/mod_devicetable.h> > > +#include <linux/module.h> > > > +#include <linux/of_device.h> > > Don't see a user of this, but of_platform.h seems to be missed. Okay, I'll add it. I'm still using devm_of_platform_populate() in iei_wt61p803_puzzle_probe(). > > > +#include <linux/property.h> > > +#include <linux/sched.h> > > +#include <linux/serdev.h> > > +#include <linux/slab.h> > > +#include <linux/sysfs.h> > > ... > > > +#define IEI_WT61P803_PUZZLE_MAX_COMMAND_LENGTH (20 + 2) > > Since it uses formula, can you add a comment explaining what is the > meaning of each argument? Ok. > > ... > > > +enum iei_wt61p803_puzzle_reply_state { > > + FRAME_OK = 0x00, > > + FRAME_PROCESSING = 0x01, > > + FRAME_STRUCT_EMPTY = 0xFF, > > + FRAME_TIMEOUT = 0xFE > > Hmm, why not ordered? I'll order it by value. > > > +}; > > ... > > > +struct iei_wt61p803_puzzle_mcu_version { > > + char version[IEI_WT61P803_PUZZLE_VERSION_VERSION_LENGTH + 1]; > > + char build_info[IEI_WT61P803_PUZZLE_VERSION_BUILD_INFO_LENGTH + 1]; > > + bool bootloader_mode; > > + char protocol_version[IEI_WT61P803_PUZZLE_VERSION_PROTOCOL_VERSION_LENGTH + 1]; > > + char serial_number[IEI_WT61P803_PUZZLE_VERSION_SN_LENGTH + 1]; > > + char mac_address[8][IEI_WT61P803_PUZZLE_VERSION_MAC_LENGTH + 1]; > > Perhaps additional constant to include (presumably) NUL ? I wouldn't separate this into a constant, I can add a comment explaining this is NUL. > > Also, what about 8? Okay, I can add a constant for the number of MAC addresses. > > > +}; > > ... > > > +struct iei_wt61p803_puzzle { > > + struct serdev_device *serdev; > > > + struct kobject *kobj; > > It's quite strange you need this, This is used in iei_wt61p803_puzzle_sysfs_create() and iei_wt61p803_puzzle_sysfs_remove() to clean up afterwards. > > > + struct mutex reply_lock; > > + struct mutex bus_lock; > > + struct iei_wt61p803_puzzle_reply *reply; > > + struct iei_wt61p803_puzzle_mcu_version version; > > + struct iei_wt61p803_puzzle_mcu_status status; > > + unsigned char *response_buffer; > > + struct mutex lock; > > +}; > > ... > > > +static int iei_wt61p803_puzzle_recv_buf(struct serdev_device *serdev, > > + const unsigned char *data, size_t size) > > +{ > > + struct iei_wt61p803_puzzle *mcu = serdev_device_get_drvdata(serdev); > > + int ret; > > + > > + ret = iei_wt61p803_puzzle_process_resp(mcu, (unsigned char *)data, size); > > Dropping const, why? I copy the content, so I can remove the cast and change the parameter iei_wt61p803_puzzle_process_resp() accepts to const unsigned char *. > > > + /* Return the number of processed bytes if function returns error */ > > + if (ret < 0) > > > + return (int)size; > > Will be interesting result, maybe you wanted other way around? That is intentional. A single frame is concatenated in the iei_wt61p803_puzzle_process_resp() function. In case we find ourselves in an unknown state, an error is returned there. We want to discard the remaining incoming data, since the frame this data belongs to is broken anyway. > > > + return ret; > > +} > > ... > > > + dev_err(dev, "%s: Command response timed out. Retries: %d", __func__, retry_count); > > Drop __func__, it should not be critical for properly formulated > messages (for debug Dynamic Debug may take care of this at run time). Okay. > > > > + return -ETIMEDOUT; > > ... > > > + struct device *dev = &mcu->serdev->dev; > > + int ret; > > > + int len = (int)size; > > Why len can't be size_t? I'll check how I can improve this. > > Can it be also organized in reversed xmas tree order? Ok. > > ... > > > + ret = serdev_device_write(mcu->serdev, cmd, len, IEI_WT61P803_PUZZLE_GENERAL_TIMEOUT); > > > + > > Not a competition for LOCs, please drop unneeded blank lines here and there. Ok. > > > + if (ret < 0) { > > + mutex_unlock(&mcu->bus_lock); > > + return ret; > > + } > > > + if (!mcu->reply) { > > + ret = -EFAULT; > > Why this error code? Maybe ENOMEM is more appropriate here... > > > + goto exit; > > + } > > ... > > > +exit: > > Perhaps > exit_unlock: > ? > > > + mutex_unlock(&mcu->lock); > > + return ret; > > ... > > > + sprintf(mcu->version.version, "v%c.%c%c%c", rb[2], rb[3], rb[4], rb[5]); > > Can be '%.3s' for the second part, but it's up to you. Okay, I agree, that would look better. > > ... > > > + sprintf(mcu->version.build_info, "%c%c/%c%c/%c%c%c%c %c%c:%c%c", > > + rb[8], rb[9], rb[6], rb[7], rb[2], > > + rb[3], rb[4], rb[5], rb[10], rb[11], > > + rb[12], rb[13]); > > Ditto. > > ... > > > + sprintf(mcu->version.protocol_version, "v%c.%c%c%c%c%c", > > + rb[7], rb[6], rb[5], rb[4], rb[3], rb[2]); > > Ditto. > > ... > > > +err: > > err_unlock: ? I use goto only in case there is also a mutex to unlock, so I don't see why to change this. > > > + mutex_unlock(&mcu->lock); > > + return ret; > > ... > > > + /* Response format: > > + * (IDX RESPONSE) > > + * 0 @ > > + * 1 O > > + * 2 S > > + * 3 S > > + * ... > > + * 5 AC Recovery Status Flag > > + * ... > > + * 10 Power Loss Recovery > > + * ... > > + * 19 Power Status (system power on method) > > + * 20 XOR checksum > > + */ > > Shouldn't be rather defined data structure for response? Every response, apart from the standard headers and a checksum at the end is completely different and I don't see a good way to standardize that in some other way. > > ... > > > + size_t reply_size = 0; > > Dummy? > > ... > > > + sprintf(mcu->version.serial_number, "%.*s", > > + IEI_WT61P803_PUZZLE_VERSION_SN_LENGTH, resp_buf + 4); > > Shouldn't you check for reply_size to be big enough? I'll add a check. > > ... > > > + serial_number_header[2] = 0x0 + (0xC) * sn_counter; > > Why capital, why in parentheses? > > ... > > > + memcpy(serial_number_cmd + 4, serial_number + (0xC) * sn_counter, 0xC); > > Ditto. I'll change these to lower case and remove the parentheses. > > ... > > > + serial_number_cmd[sizeof(serial_number_cmd) - 1] = 0; > > You defined X+1 to then use sizeof() -1? Hmm... This was used for the XOR checksum. I'll remove this statement as it is not needed anymore. > > ... > > > + if (!(resp_buf[0] == IEI_WT61P803_PUZZLE_CMD_HEADER_START && > > + resp_buf[1] == IEI_WT61P803_PUZZLE_CMD_RESPONSE_OK && > > + resp_buf[2] == IEI_WT61P803_PUZZLE_CHECKSUM_RESPONSE_OK)) { > > + ret = -EPROTO; > > + goto err; > > + } > > I think it would be better to define data structure for replies and > then check would be as simple as memcmp(). I'd keep this as is, because the replies are different a lot of the times. Especially when the reply isn't just an ACK. > > ... > > > + if (reply_size < 22) { > > Looking at the code organisation it seems to me like if (reply_size < > sizeof(struct_of_this_type_of_reply)). > > > + ret = -EIO; > > + goto err; > > + } > > ... > > > + mac_address_header[2] = 0x24 + (0x11) * mac_address_idx; > > Why in parentheses? > > ... > > > + /* Concat mac_address_header, mac_address to mac_address_cmd */ > > + memcpy(mac_address_cmd, mac_address_header, 4); > > + memcpy(mac_address_cmd + 4, mac_address, 17); > > Yeah, much easier to use specific field names instead of this 4 / + 4, 17, ... Ok, I'll convert this to: memcpy(mac_address_cmd, mac_address_header, sizeof(mac_address_header)); ... > > ... > > > + ret = snprintf(cmd_buf, sizeof(cmd_buf), "%d", power_loss_recovery_action); > > + if (ret < 0) > > + return ret; > > ... > > > + power_loss_recovery_cmd[3] = cmd_buf[0]; > > One decimal (most significant) digit?! Isn't it a bit ambiguous? The power_loss_recovery_action can only have a value of 0 - 4. My understanding is that if I give snprintf a buffer of size 1, it will truncate the one character to make space for NUL. > > ... > > > +#define sysfs_container(dev) \ > > + (container_of((dev)->kobj.parent, struct device, kobj)) > > + > > +static ssize_t version_show(struct device *dev, struct device_attribute *attr, > > + char *buf) > > +{ > > + struct device *dev_container = sysfs_container(dev); > > + struct iei_wt61p803_puzzle *mcu = dev_get_drvdata(dev_container); > > + > > + return sprintf(buf, "%s\n", mcu->version.version); > > +} > > +static DEVICE_ATTR_RO(version); > > I believe we have better approach than this. dev_groups, for example. Ok, I'll check how I could improve it. > > ... > > > + if ((int)count != IEI_WT61P803_PUZZLE_VERSION_SN_LENGTH + 1) > > + return -EINVAL; > > You need to revisit all of these strange castings here and there. It > should be really rear to have explicit castings in C. > > ... > > > + memcpy(serial_number, (unsigned char *)buf, IEI_WT61P803_PUZZLE_VERSION_SN_LENGTH); > > This casting is not need. Basically any casting from or to void * is not needed. Ok. > > ... > > > + dev_info(dev, "Driver baud rate: %d", baud); > > Why being so noisy, how does it help user? Doesn't serdev has a > facility to show this rather basic stuff? > > ... > > > + dev_info(dev, "MCU version: %s", mcu->version.version); > > + dev_info(dev, "MCU firmware build info: %s", mcu->version.build_info); > > + dev_info(dev, "MCU in bootloader mode: %s", > > + mcu->version.bootloader_mode ? "true" : "false"); > > + dev_info(dev, "MCU protocol version: %s", mcu->version.protocol_version); > > How all of this can be useful for *working* case? I can reduce this, but I'd just like to log the baud rate and the firmware build info. These two could be useful in a kernel log, if something doesn't work. > > ... > > > + ret = iei_wt61p803_puzzle_sysfs_create(dev, mcu); > > No check? I'll add a check. > > ... > > Have I missed ABI documentation? The ABI documentation is in a separate patch (Documentation/ABI/testing/sysfs-driver-iei-wt61p803-puzzle). > > -- > With Best Regards, > Andy Shevchenko Kind regards, Luka
On Sun, Nov 1, 2020 at 3:22 PM Luka Kovacic <luka.kovacic@sartura.hr> wrote: > On Mon, Oct 26, 2020 at 11:54 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Sun, Oct 25, 2020 at 3:59 AM Luka Kovacic <luka.kovacic@sartura.hr> wrote: ... > > > +#include <linux/of_device.h> > > > > Don't see a user of this, but of_platform.h seems to be missed. > > Okay, I'll add it. > I'm still using devm_of_platform_populate() in iei_wt61p803_puzzle_probe(). Yes, and that's why I have mentioned of_platform.h above. ... > > > + struct kobject *kobj; > > > > It's quite strange you need this, > > This is used in iei_wt61p803_puzzle_sysfs_create() and > iei_wt61p803_puzzle_sysfs_remove() to clean up afterwards. I didn't get why you need this in the first place. ... > > > + /* Return the number of processed bytes if function returns error */ > > > + if (ret < 0) > > > > > + return (int)size; > > > > Will be interesting result, maybe you wanted other way around? > > That is intentional. > A single frame is concatenated in the iei_wt61p803_puzzle_process_resp() > function. In case we find ourselves in an unknown state, an error is > returned there. > > We want to discard the remaining incoming data, since the frame this > data belongs > to is broken anyway. Elaborate in the comment. > > > + return ret; ... > > > +err: > > > > err_unlock: ? > > I use goto only in case there is also a mutex to unlock, so I don't see why > to change this. The comment was about clarification of what is done at this label. > > > + mutex_unlock(&mcu->lock); > > > + return ret; ... > > > + /* Response format: > > > + * (IDX RESPONSE) > > > + * 0 @ > > > + * 1 O > > > + * 2 S > > > + * 3 S > > > + * ... > > > + * 5 AC Recovery Status Flag > > > + * ... > > > + * 10 Power Loss Recovery > > > + * ... > > > + * 19 Power Status (system power on method) > > > + * 20 XOR checksum > > > + */ > > > > Shouldn't be rather defined data structure for response? > > Every response, apart from the standard headers and a checksum > at the end is completely different and I don't see a good way to > standardize that in some other way. And that's my point. Provide data structures for all responses you are taking care of. It will be way better documentation and understanding of this IPC. ... > > > + if (!(resp_buf[0] == IEI_WT61P803_PUZZLE_CMD_HEADER_START && > > > + resp_buf[1] == IEI_WT61P803_PUZZLE_CMD_RESPONSE_OK && > > > + resp_buf[2] == IEI_WT61P803_PUZZLE_CHECKSUM_RESPONSE_OK)) { > > > + ret = -EPROTO; > > > + goto err; > > > + } > > > > I think it would be better to define data structure for replies and > > then check would be as simple as memcmp(). > > I'd keep this as is, because the replies are different a lot of the times. > Especially when the reply isn't just an ACK. How do you know the type of the reply? Can't you provide a data structure which will have necessary fields to recognize this? ... > > > + power_loss_recovery_cmd[3] = cmd_buf[0]; > > > > One decimal (most significant) digit?! Isn't it a bit ambiguous? > > The power_loss_recovery_action can only have a value of 0 - 4. > My understanding is that if I give snprintf a buffer of size 1, it will > truncate the one character to make space for NUL. Why to bother with snprintf()? hex_asc[] would be sufficient. But my point that the code is fragile. If it ever gets 15, you will get 1. ... > I can reduce this, but I'd just like to log the baud rate and the > firmware build info. > These two could be useful in a kernel log, if something doesn't work. FW build info is definitely good to have, but don't you have other means to retrieve baud rate?
Hello On 11/1/20 3:56 AM, Luka Kovacic wrote: > Hello Pavel, > > On Thu, Oct 29, 2020 at 7:01 PM Pavel Machek <pavel@ucw.cz> wrote: >> Hi! >> >>> +What: /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/power_status >>> +Date: September 2020 >>> +Contact: Luka Kovacic <luka.kovacic@sartura.hr> >>> +Description: (RO) Power status indicates the host platform power on method. >>> + Value mapping (bitwise list): >>> + 0x80 - Null >>> + 0x40 - Firmware flag >>> + 0x20 - Power loss detection flag (powered off) >>> + 0x10 - Power loss detection flag (AC mode) >>> + 0x08 - Button power on >>> + 0x04 - WOL power on >>> + 0x02 - RTC alarm power on >>> + 0x01 - AC recover power on >> It would be nice to put this into standard place somewhere. Many >> machines will want to expose this information. > As this is specific to this microcontroller and to how it encodes > these values, I don't see a need to change this. > This isn't used anywhere else. > >> If not, at least spell out WoL, as it is not that common of acronym. > Okay. WoL is a very common acronym especially in the networking space But the overall this section does not make sense The description says that it indicates platform power on method but what is NULL power on? There are flags for power loss detection. Does the RTC mean that the processor real time clock woke up the uC? Or that the internal RTC woke up the controller? And for the /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/ac_recovery_status what are those values? It seems like some ABI's are documented well with formats and others are just described without a format. For instance /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/version the format of this version is not described but /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/build_info is. Dan
On Mon 2020-11-02 12:29:59, Dan Murphy wrote: > Hello > > On 11/1/20 3:56 AM, Luka Kovacic wrote: > > Hello Pavel, > > > > On Thu, Oct 29, 2020 at 7:01 PM Pavel Machek <pavel@ucw.cz> wrote: > > > Hi! > > > > > > > +What: /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/power_status > > > > +Date: September 2020 > > > > +Contact: Luka Kovacic <luka.kovacic@sartura.hr> > > > > +Description: (RO) Power status indicates the host platform power on method. > > > > + Value mapping (bitwise list): > > > > + 0x80 - Null > > > > + 0x40 - Firmware flag > > > > + 0x20 - Power loss detection flag (powered off) > > > > + 0x10 - Power loss detection flag (AC mode) > > > > + 0x08 - Button power on > > > > + 0x04 - WOL power on > > > > + 0x02 - RTC alarm power on > > > > + 0x01 - AC recover power on > > > It would be nice to put this into standard place somewhere. Many > > > machines will want to expose this information. > > As this is specific to this microcontroller and to how it encodes > > these values, I don't see a need to change this. > > This isn't used anywhere else. > > > > > If not, at least spell out WoL, as it is not that common of acronym. > > Okay. > > WoL is a very common acronym especially in the networking space WoL is common. WOL is not. Better spell it out. Best regards, Pavel
Hello On 11/2/20 1:03 PM, Pavel Machek wrote: > On Mon 2020-11-02 12:29:59, Dan Murphy wrote: >> Hello >> >> On 11/1/20 3:56 AM, Luka Kovacic wrote: >>> Hello Pavel, >>> >>> On Thu, Oct 29, 2020 at 7:01 PM Pavel Machek <pavel@ucw.cz> wrote: >>>> Hi! >>>> >>>>> +What: /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/power_status >>>>> +Date: September 2020 >>>>> +Contact: Luka Kovacic <luka.kovacic@sartura.hr> >>>>> +Description: (RO) Power status indicates the host platform power on method. >>>>> + Value mapping (bitwise list): >>>>> + 0x80 - Null >>>>> + 0x40 - Firmware flag >>>>> + 0x20 - Power loss detection flag (powered off) >>>>> + 0x10 - Power loss detection flag (AC mode) >>>>> + 0x08 - Button power on >>>>> + 0x04 - WOL power on >>>>> + 0x02 - RTC alarm power on >>>>> + 0x01 - AC recover power on >>>> It would be nice to put this into standard place somewhere. Many >>>> machines will want to expose this information. >>> As this is specific to this microcontroller and to how it encodes >>> these values, I don't see a need to change this. >>> This isn't used anywhere else. >>> >>>> If not, at least spell out WoL, as it is not that common of acronym. >>> Okay. >> WoL is a very common acronym especially in the networking space > WoL is common. WOL is not. Better spell it out. Agreed. Especially if WOL does not mean Wake On Lan Dan
Hello, On Mon, Nov 2, 2020 at 7:30 PM Dan Murphy <dmurphy@ti.com> wrote: > > Hello > > On 11/1/20 3:56 AM, Luka Kovacic wrote: > > Hello Pavel, > > > > On Thu, Oct 29, 2020 at 7:01 PM Pavel Machek <pavel@ucw.cz> wrote: > >> Hi! > >> > >>> +What: /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/power_status > >>> +Date: September 2020 > >>> +Contact: Luka Kovacic <luka.kovacic@sartura.hr> > >>> +Description: (RO) Power status indicates the host platform power on method. > >>> + Value mapping (bitwise list): > >>> + 0x80 - Null > >>> + 0x40 - Firmware flag > >>> + 0x20 - Power loss detection flag (powered off) > >>> + 0x10 - Power loss detection flag (AC mode) > >>> + 0x08 - Button power on > >>> + 0x04 - WOL power on > >>> + 0x02 - RTC alarm power on > >>> + 0x01 - AC recover power on > >> It would be nice to put this into standard place somewhere. Many > >> machines will want to expose this information. > > As this is specific to this microcontroller and to how it encodes > > these values, I don't see a need to change this. > > This isn't used anywhere else. > > > >> If not, at least spell out WoL, as it is not that common of acronym. > > Okay. > > WoL is a very common acronym especially in the networking space By WOL I meant Wake-on-LAN, I will spell out the whole acronym. > > But the overall this section does not make sense > > The description says that it indicates platform power on method but what > is NULL power on? There are flags for power loss detection. I will clarify the value mapping and try to replicate some of these states so I can write a better description. > > Does the RTC mean that the processor real time clock woke up the uC? Or > that the internal RTC woke up the controller? These are all related to the platform as a whole. So the Marvell SoC and all of the required peripherals are turned on. > > And for the > /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/ac_recovery_status > what are those values? These values indicate whether the board has been shut down gracefully and whether it has been powered on automatically (when power came back) or by pressing the power button. I will also extend the documentation with the value mapping for this. > > It seems like some ABI's are documented well with formats and others are > just described without a format. > > For instance > > /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/version the format > of this version is not described but > /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/build_info is. I left out the version format descriptions as they are in the recognizable format and all of them are quite arbitrary (e.g. v1.000). > > > Dan > Kind regards, Luka
Hello Andy, On Mon, Nov 2, 2020 at 12:18 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Sun, Nov 1, 2020 at 3:22 PM Luka Kovacic <luka.kovacic@sartura.hr> wrote: > > On Mon, Oct 26, 2020 at 11:54 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Sun, Oct 25, 2020 at 3:59 AM Luka Kovacic <luka.kovacic@sartura.hr> wrote: > > ... > > > > > +#include <linux/of_device.h> > > > > > > Don't see a user of this, but of_platform.h seems to be missed. > > > > Okay, I'll add it. > > I'm still using devm_of_platform_populate() in iei_wt61p803_puzzle_probe(). > > Yes, and that's why I have mentioned of_platform.h above. > > ... > > > > > + struct kobject *kobj; > > > > > > It's quite strange you need this, > > > > This is used in iei_wt61p803_puzzle_sysfs_create() and > > iei_wt61p803_puzzle_sysfs_remove() to clean up afterwards. > > I didn't get why you need this in the first place. > > ... > > > > > + /* Return the number of processed bytes if function returns error */ > > > > + if (ret < 0) > > > > > > > + return (int)size; > > > > > > Will be interesting result, maybe you wanted other way around? > > > > That is intentional. > > A single frame is concatenated in the iei_wt61p803_puzzle_process_resp() > > function. In case we find ourselves in an unknown state, an error is > > returned there. > > > > We want to discard the remaining incoming data, since the frame this > > data belongs > > to is broken anyway. > > Elaborate in the comment. Okay. > > > > > + return ret; > > ... > > > > > +err: > > > > > > err_unlock: ? > > > > I use goto only in case there is also a mutex to unlock, so I don't see why > > to change this. > > The comment was about clarification of what is done at this label. Ok, I understand. I will change the labels where mutexes are unlocked to indicate this as you suggested. > > > > > + mutex_unlock(&mcu->lock); > > > > + return ret; > > ... > > > > > + /* Response format: > > > > + * (IDX RESPONSE) > > > > + * 0 @ > > > > + * 1 O > > > > + * 2 S > > > > + * 3 S > > > > + * ... > > > > + * 5 AC Recovery Status Flag > > > > + * ... > > > > + * 10 Power Loss Recovery > > > > + * ... > > > > + * 19 Power Status (system power on method) > > > > + * 20 XOR checksum > > > > + */ > > > > > > Shouldn't be rather defined data structure for response? > > > > Every response, apart from the standard headers and a checksum > > at the end is completely different and I don't see a good way to > > standardize that in some other way. > > And that's my point. Provide data structures for all responses you are > taking care of. > It will be way better documentation and understanding of this IPC. Okay, I'll improve handling of these in the next patchset. Should I make a generic header structure for the common parts and define the common responses somewhere centrally? Then I can check those just as you suggested. For the variable ones I can reuse the generic header structure and just use the specific values as I would do normally. > > ... > > > > > + if (!(resp_buf[0] == IEI_WT61P803_PUZZLE_CMD_HEADER_START && > > > > + resp_buf[1] == IEI_WT61P803_PUZZLE_CMD_RESPONSE_OK && > > > > + resp_buf[2] == IEI_WT61P803_PUZZLE_CHECKSUM_RESPONSE_OK)) { > > > > + ret = -EPROTO; > > > > + goto err; > > > > + } > > > > > > I think it would be better to define data structure for replies and > > > then check would be as simple as memcmp(). > > > > I'd keep this as is, because the replies are different a lot of the times. > > Especially when the reply isn't just an ACK. > > How do you know the type of the reply? Can't you provide a data > structure which will have necessary fields to recognize this? > It can be recognized by the specific header of the reply. I will separate the header and the checksum into some kind of a generic structure, but as the content itself is just an arbitrary array of characters I cannot generalize that sensibly for every type of a reply there is. Anyway, I agree it would be good to define the common responses... > ... > > > > > + power_loss_recovery_cmd[3] = cmd_buf[0]; > > > > > > One decimal (most significant) digit?! Isn't it a bit ambiguous? > > > > The power_loss_recovery_action can only have a value of 0 - 4. > > My understanding is that if I give snprintf a buffer of size 1, it will > > truncate the one character to make space for NUL. > > Why to bother with snprintf()? hex_asc[] would be sufficient. But my > point that the code is fragile. If it ever gets 15, you will get 1. Ok, I'll simplify this. > > ... > > > I can reduce this, but I'd just like to log the baud rate and the > > firmware build info. > > > These two could be useful in a kernel log, if something doesn't work. > > FW build info is definitely good to have, but don't you have other > means to retrieve baud rate? The normal boot log is completely clean, there are no serdev bus specific messages. The baud rate is defined in the device tree, so if something goes wrong it should be sufficient to only print the baud rate then. Can I output the versions, the firmware build info and only print the baud rate when an error occurs? > > -- > With Best Regards, > Andy Shevchenko
On Tue, Nov 3, 2020 at 1:15 AM Luka Kovacic <luka.kovacic@sartura.hr> wrote: > On Mon, Nov 2, 2020 at 12:18 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Sun, Nov 1, 2020 at 3:22 PM Luka Kovacic <luka.kovacic@sartura.hr> wrote: > > > On Mon, Oct 26, 2020 at 11:54 PM Andy Shevchenko > > > <andy.shevchenko@gmail.com> wrote: > > > > On Sun, Oct 25, 2020 at 3:59 AM Luka Kovacic <luka.kovacic@sartura.hr> wrote: ... > > > > > + /* Response format: > > > > > + * (IDX RESPONSE) > > > > > + * 0 @ > > > > > + * 1 O > > > > > + * 2 S > > > > > + * 3 S > > > > > + * ... > > > > > + * 5 AC Recovery Status Flag > > > > > + * ... > > > > > + * 10 Power Loss Recovery > > > > > + * ... > > > > > + * 19 Power Status (system power on method) > > > > > + * 20 XOR checksum > > > > > + */ > > > > > > > > Shouldn't be rather defined data structure for response? > > > > > > Every response, apart from the standard headers and a checksum > > > at the end is completely different and I don't see a good way to > > > standardize that in some other way. > > > > And that's my point. Provide data structures for all responses you are > > taking care of. > > It will be way better documentation and understanding of this IPC. > > Okay, I'll improve handling of these in the next patchset. > Should I make a generic header structure for the common parts and > define the common responses somewhere centrally? Yes, something like TCP/IP headers have. This will immediately show how good/bad was design of this IPC protocol (as a side effect, but gives a good hint on how layers of messages are looking ) > Then I can check those just as you suggested. > > For the variable ones I can reuse the generic header structure and just > use the specific values as I would do normally. ... > > > > > + if (!(resp_buf[0] == IEI_WT61P803_PUZZLE_CMD_HEADER_START && > > > > > + resp_buf[1] == IEI_WT61P803_PUZZLE_CMD_RESPONSE_OK && > > > > > + resp_buf[2] == IEI_WT61P803_PUZZLE_CHECKSUM_RESPONSE_OK)) { > > > > > + ret = -EPROTO; > > > > > + goto err; > > > > > + } > > > > > > > > I think it would be better to define data structure for replies and > > > > then check would be as simple as memcmp(). > > > > > > I'd keep this as is, because the replies are different a lot of the times. > > > Especially when the reply isn't just an ACK. > > > > How do you know the type of the reply? Can't you provide a data > > structure which will have necessary fields to recognize this? > > > > It can be recognized by the specific header of the reply. > I will separate the header and the checksum into some kind of a generic > structure, but as the content itself is just an arbitrary array of characters > I cannot generalize that sensibly for every type of a reply there is. > > Anyway, I agree it would be good to define the common responses... Yep, something to look like a structure with a payload. ... > Can I output the versions, the firmware build info and only print the baud > rate when an error occurs? What you think is crucial. I'm not against it, I'm just pointing to the way of shrinking as much as possible. Otherwise, move messages to debug level (but that shouldn't be many in the production driver).
On Sun, Oct 25, 2020 at 02:59:13AM +0200, Luka Kovacic wrote: > Add the IEI WT61P803 PUZZLE HWMON driver, that handles the fan speed > control via PWM, reading fan speed and reading on-board temperature > sensors. > > The driver registers a HWMON device and a simple thermal cooling device to > enable in-kernel fan management. > > This driver depends on the IEI WT61P803 PUZZLE MFD driver. > > Signed-off-by: Luka Kovacic <luka.kovacic@sartura.hr> > Cc: Luka Perkov <luka.perkov@sartura.hr> > Cc: Robert Marko <robert.marko@sartura.hr> > --- > drivers/hwmon/Kconfig | 8 + > drivers/hwmon/Makefile | 1 + > drivers/hwmon/iei-wt61p803-puzzle-hwmon.c | 412 ++++++++++++++++++++++ This requires documentation (Documentation/hwmon/iei-wt61p803-puzzle.rst). > 3 files changed, 421 insertions(+) > create mode 100644 drivers/hwmon/iei-wt61p803-puzzle-hwmon.c > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index 8dc28b26916e..e0469384af2a 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -722,6 +722,14 @@ config SENSORS_IBMPOWERNV > This driver can also be built as a module. If so, the module > will be called ibmpowernv. > > +config SENSORS_IEI_WT61P803_PUZZLE_HWMON > + tristate "IEI WT61P803 PUZZLE MFD HWMON Driver" > + depends on MFD_IEI_WT61P803_PUZZLE > + help > + The IEI WT61P803 PUZZLE MFD HWMON Driver handles reading fan speed > + and writing fan PWM values. It also supports reading on-board > + temperature sensors. > + > config SENSORS_IIO_HWMON > tristate "Hwmon driver that uses channels specified via iio maps" > depends on IIO > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index a8f4b35b136b..b0afb2d6896f 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -83,6 +83,7 @@ obj-$(CONFIG_SENSORS_HIH6130) += hih6130.o > obj-$(CONFIG_SENSORS_ULTRA45) += ultra45_env.o > obj-$(CONFIG_SENSORS_I5500) += i5500_temp.o > obj-$(CONFIG_SENSORS_I5K_AMB) += i5k_amb.o > +obj-$(CONFIG_SENSORS_IEI_WT61P803_PUZZLE_HWMON) += iei-wt61p803-puzzle-hwmon.o > obj-$(CONFIG_SENSORS_IBMAEM) += ibmaem.o > obj-$(CONFIG_SENSORS_IBMPEX) += ibmpex.o > obj-$(CONFIG_SENSORS_IBMPOWERNV)+= ibmpowernv.o > diff --git a/drivers/hwmon/iei-wt61p803-puzzle-hwmon.c b/drivers/hwmon/iei-wt61p803-puzzle-hwmon.c > new file mode 100644 > index 000000000000..61b06c9e61df > --- /dev/null > +++ b/drivers/hwmon/iei-wt61p803-puzzle-hwmon.c > @@ -0,0 +1,412 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* IEI WT61P803 PUZZLE MCU HWMON Driver > + * > + * Copyright (C) 2020 Sartura Ltd. > + * Author: Luka Kovacic <luka.kovacic@sartura.hr> > + */ > + > +#include <linux/err.h> > +#include <linux/hwmon-sysfs.h> Unnecessary include. > +#include <linux/hwmon.h> > +#include <linux/interrupt.h> > +#include <linux/irq.h> > +#include <linux/math64.h> > +#include <linux/mfd/iei-wt61p803-puzzle.h> > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/property.h> > +#include <linux/slab.h> > +#include <linux/thermal.h> > + > +#define IEI_WT61P803_PUZZLE_HWMON_MAX_TEMP 2 > +#define IEI_WT61P803_PUZZLE_HWMON_MAX_FAN 5 > +#define IEI_WT61P803_PUZZLE_HWMON_MAX_PWM 2 > +#define IEI_WT61P803_PUZZLE_HWMON_MAX_PWM_VAL 255 > + > +/** > + * struct iei_wt61p803_puzzle_thermal_cooling_device - Thermal cooling device instance > + * @mcu_hwmon: Parent driver struct pointer > + * @tcdev: Thermal cooling device pointer > + * @name: Thermal cooling device name > + * @pwm_channel: Controlled PWM channel (0 or 1) > + * @cooling_levels: Thermal cooling device cooling levels (DT) > + */ > +struct iei_wt61p803_puzzle_thermal_cooling_device { > + struct iei_wt61p803_puzzle_hwmon *mcu_hwmon; > + struct thermal_cooling_device *tcdev; > + char name[THERMAL_NAME_LENGTH]; > + int pwm_channel; > + u8 *cooling_levels; > +}; > + > +/** > + * struct iei_wt61p803_puzzle_hwmon - MCU HWMON Driver > + * @mcu: MCU struct pointer > + * @response_buffer Global MCU response buffer allocation > + * @thermal_cooling_dev_present: Per-channel thermal cooling device control indicator > + * @cdev: Per-channel thermal cooling device private structure > + */ > +struct iei_wt61p803_puzzle_hwmon { > + struct iei_wt61p803_puzzle *mcu; > + unsigned char *response_buffer; > + bool thermal_cooling_dev_present[IEI_WT61P803_PUZZLE_HWMON_MAX_PWM]; > + struct iei_wt61p803_puzzle_thermal_cooling_device > + *cdev[IEI_WT61P803_PUZZLE_HWMON_MAX_PWM]; > +}; > + > +#define raw_temp_to_milidegree_celsius(x) (((x) - 0x80) * 1000) > +static int iei_wt61p803_puzzle_read_temp_sensor(struct iei_wt61p803_puzzle_hwmon *mcu_hwmon, > + int channel, long *value) > +{ > + unsigned char *resp_buf = mcu_hwmon->response_buffer; > + static unsigned char temp_sensor_ntc_cmd[4] = { > + IEI_WT61P803_PUZZLE_CMD_HEADER_START, > + IEI_WT61P803_PUZZLE_CMD_TEMP, > + IEI_WT61P803_PUZZLE_CMD_TEMP_ALL, > + }; > + size_t reply_size = 0; > + int ret; > + > + ret = iei_wt61p803_puzzle_write_command(mcu_hwmon->mcu, temp_sensor_ntc_cmd, > + sizeof(temp_sensor_ntc_cmd), resp_buf, > + &reply_size); > + > + if (ret) > + return ret; > + > + if (reply_size != 7) > + return -EIO; > + > + /* Check the number of NTC values */ > + if (resp_buf[3] != '2') > + return -EIO; resp_buf always points to mcu_hwmon->response_buffer, yet the use of that buffer is not mutex protected. This will result in race conditions all over the place if there is more than one reader/writer. Guenter > + > + *value = raw_temp_to_milidegree_celsius(resp_buf[4 + channel]); > + > + return 0; > +} > + > +#define raw_fan_val_to_rpm(x, y) ((((x) << 8 | (y)) / 2) * 60) > +static int iei_wt61p803_puzzle_read_fan_speed(struct iei_wt61p803_puzzle_hwmon *mcu_hwmon, > + int channel, long *value) > +{ > + unsigned char *resp_buf = mcu_hwmon->response_buffer; > + unsigned char fan_speed_cmd[4] = {}; > + size_t reply_size = 0; > + int ret; > + > + fan_speed_cmd[0] = IEI_WT61P803_PUZZLE_CMD_HEADER_START; > + fan_speed_cmd[1] = IEI_WT61P803_PUZZLE_CMD_FAN; > + fan_speed_cmd[2] = IEI_WT61P803_PUZZLE_CMD_FAN_RPM(channel); > + > + ret = iei_wt61p803_puzzle_write_command(mcu_hwmon->mcu, fan_speed_cmd, > + sizeof(fan_speed_cmd), resp_buf, > + &reply_size); > + if (ret) > + return ret; > + > + if (reply_size != 7) > + return -EIO; > + > + *value = raw_fan_val_to_rpm(resp_buf[3], resp_buf[4]); > + > + return 0; > +} > + > +static int iei_wt61p803_puzzle_write_pwm_channel(struct iei_wt61p803_puzzle_hwmon *mcu_hwmon, > + int channel, long pwm_set_val) > +{ > + unsigned char *resp_buf = mcu_hwmon->response_buffer; > + unsigned char pwm_set_cmd[6] = {}; > + size_t reply_size = 0; > + int ret; > + > + pwm_set_cmd[0] = IEI_WT61P803_PUZZLE_CMD_HEADER_START; > + pwm_set_cmd[1] = IEI_WT61P803_PUZZLE_CMD_FAN; > + pwm_set_cmd[2] = IEI_WT61P803_PUZZLE_CMD_FAN_PWM_WRITE; > + pwm_set_cmd[3] = IEI_WT61P803_PUZZLE_CMD_FAN_PWM(channel); > + pwm_set_cmd[4] = (unsigned char)pwm_set_val; > + > + ret = iei_wt61p803_puzzle_write_command(mcu_hwmon->mcu, pwm_set_cmd, > + sizeof(pwm_set_cmd), resp_buf, > + &reply_size); > + if (ret) > + return ret; > + > + if (reply_size != 3) > + return -EIO; > + > + if (!(resp_buf[0] == IEI_WT61P803_PUZZLE_CMD_HEADER_START && > + resp_buf[1] == IEI_WT61P803_PUZZLE_CMD_RESPONSE_OK && > + resp_buf[2] == IEI_WT61P803_PUZZLE_CHECKSUM_RESPONSE_OK)) > + return -EIO; > + > + return 0; > +} > + > +static int iei_wt61p803_puzzle_read_pwm_channel(struct iei_wt61p803_puzzle_hwmon *mcu_hwmon, > + int channel, long *value) > +{ > + unsigned char *resp_buf = mcu_hwmon->response_buffer; > + unsigned char pwm_get_cmd[5] = {}; > + size_t reply_size = 0; > + int ret; > + > + pwm_get_cmd[0] = IEI_WT61P803_PUZZLE_CMD_HEADER_START; > + pwm_get_cmd[1] = IEI_WT61P803_PUZZLE_CMD_FAN; > + pwm_get_cmd[2] = IEI_WT61P803_PUZZLE_CMD_FAN_PWM_READ; > + pwm_get_cmd[3] = IEI_WT61P803_PUZZLE_CMD_FAN_PWM(channel); > + > + ret = iei_wt61p803_puzzle_write_command(mcu_hwmon->mcu, pwm_get_cmd, > + sizeof(pwm_get_cmd), resp_buf, > + &reply_size); > + if (ret) > + return ret; > + > + if (reply_size != 5) > + return -EIO; > + > + if (resp_buf[2] != IEI_WT61P803_PUZZLE_CMD_FAN_PWM_READ) > + return -EIO; > + > + *value = resp_buf[3]; > + > + return 0; > +} > + > +static int iei_wt61p803_puzzle_read(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int channel, long *val) > +{ > + struct iei_wt61p803_puzzle_hwmon *mcu_hwmon = dev_get_drvdata(dev->parent); > + > + switch (type) { > + case hwmon_pwm: > + return iei_wt61p803_puzzle_read_pwm_channel(mcu_hwmon, channel, val); > + case hwmon_fan: > + return iei_wt61p803_puzzle_read_fan_speed(mcu_hwmon, channel, val); > + case hwmon_temp: > + return iei_wt61p803_puzzle_read_temp_sensor(mcu_hwmon, channel, val); > + default: > + return -EINVAL; > + } > +} > + > +static int iei_wt61p803_puzzle_write(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int channel, long val) > +{ > + struct iei_wt61p803_puzzle_hwmon *mcu_hwmon = dev_get_drvdata(dev->parent); > + > + if (attr != hwmon_pwm_input) > + return -EINVAL; This check is unnecesary. > + if (mcu_hwmon->thermal_cooling_dev_present[channel]) { > + /* > + * The Thermal Framework has already claimed this specific PWM > + * channel. > + */ > + return -EBUSY; > + } The sensor should not be marked as writeable in this case. iei_wt61p803_puzzle_is_visible() should check for the condition and mark the affected attribute(s) read-only. > + return iei_wt61p803_puzzle_write_pwm_channel(mcu_hwmon, channel, val); > +} > + > +static umode_t iei_wt61p803_puzzle_is_visible(const void *data, enum hwmon_sensor_types type, > + u32 attr, int channel) > +{ > + switch (type) { > + case hwmon_pwm: > + if (attr == hwmon_pwm_input) > + return 0644; > + break; > + case hwmon_fan: > + if (attr == hwmon_fan_input) > + return 0444; > + break; > + case hwmon_temp: > + if (attr == hwmon_temp_input) > + return 0444; > + break; > + default: > + return 0; > + } > + > + return 0; > +} > + > +static const struct hwmon_ops iei_wt61p803_puzzle_hwmon_ops = { > + .is_visible = iei_wt61p803_puzzle_is_visible, > + .read = iei_wt61p803_puzzle_read, > + .write = iei_wt61p803_puzzle_write, > +}; > + > +static const struct hwmon_channel_info *iei_wt61p803_puzzle_info[] = { > + HWMON_CHANNEL_INFO(pwm, > + HWMON_PWM_INPUT, > + HWMON_PWM_INPUT), > + HWMON_CHANNEL_INFO(fan, > + HWMON_F_INPUT, > + HWMON_F_INPUT, > + HWMON_F_INPUT, > + HWMON_F_INPUT, > + HWMON_F_INPUT), > + HWMON_CHANNEL_INFO(temp, > + HWMON_T_INPUT, > + HWMON_T_INPUT), > + NULL > +}; > + > +static const struct hwmon_chip_info iei_wt61p803_puzzle_chip_info = { > + .ops = &iei_wt61p803_puzzle_hwmon_ops, > + .info = iei_wt61p803_puzzle_info, > +}; > + > +static int iei_wt61p803_puzzle_get_max_state(struct thermal_cooling_device *tcdev, > + unsigned long *state) > +{ > + *state = IEI_WT61P803_PUZZLE_HWMON_MAX_PWM_VAL; > + > + return 0; > +} > + > +static int iei_wt61p803_puzzle_get_cur_state(struct thermal_cooling_device *tcdev, > + unsigned long *state) > +{ > + struct iei_wt61p803_puzzle_thermal_cooling_device *cdev = tcdev->devdata; > + struct iei_wt61p803_puzzle_hwmon *mcu_hwmon = cdev->mcu_hwmon; > + long value; > + int ret; > + > + ret = iei_wt61p803_puzzle_read_pwm_channel(mcu_hwmon, cdev->pwm_channel, &value); > + if (ret) > + return ret; > + > + *state = value; > + > + return 0; > +} > + > +static int iei_wt61p803_puzzle_set_cur_state(struct thermal_cooling_device *tcdev, > + unsigned long state) > +{ > + struct iei_wt61p803_puzzle_thermal_cooling_device *cdev = tcdev->devdata; > + struct iei_wt61p803_puzzle_hwmon *mcu_hwmon = cdev->mcu_hwmon; > + > + return iei_wt61p803_puzzle_write_pwm_channel(mcu_hwmon, cdev->pwm_channel, state); > +} > + > +static const struct thermal_cooling_device_ops iei_wt61p803_puzzle_cooling_ops = { > + .get_max_state = iei_wt61p803_puzzle_get_max_state, > + .get_cur_state = iei_wt61p803_puzzle_get_cur_state, > + .set_cur_state = iei_wt61p803_puzzle_set_cur_state, > +}; > + > +static int iei_wt61p803_puzzle_enable_thermal_cooling_dev(struct device *dev, > + struct fwnode_handle *child, > + struct iei_wt61p803_puzzle_hwmon *mcu_hwmon) > +{ > + struct iei_wt61p803_puzzle_thermal_cooling_device *cdev; > + u32 pwm_channel; > + u8 num_levels; > + int ret; > + > + ret = fwnode_property_read_u32(child, "reg", &pwm_channel); > + if (ret) > + return ret; > + > + mcu_hwmon->thermal_cooling_dev_present[pwm_channel] = true; > + > + num_levels = fwnode_property_count_u8(child, "cooling-levels"); > + if (!num_levels) > + return -EINVAL; > + > + cdev = devm_kzalloc(dev, sizeof(*cdev), GFP_KERNEL); > + if (!cdev) > + return -ENOMEM; > + > + cdev->cooling_levels = devm_kmalloc_array(dev, num_levels, sizeof(u8), GFP_KERNEL); > + if (!cdev->cooling_levels) > + return -ENOMEM; > + > + ret = fwnode_property_read_u8_array(child, "cooling-levels", > + cdev->cooling_levels, > + num_levels); > + if (ret) { > + dev_err(dev, "Couldn't read property 'cooling-levels'"); > + return ret; > + } > + > + snprintf(cdev->name, THERMAL_NAME_LENGTH, "iei_wt61p803_puzzle_%d", pwm_channel); > + > + cdev->tcdev = devm_thermal_of_cooling_device_register(dev, NULL, cdev->name, cdev, > + &iei_wt61p803_puzzle_cooling_ops); > + if (IS_ERR(cdev->tcdev)) > + return PTR_ERR(cdev->tcdev); > + > + cdev->mcu_hwmon = mcu_hwmon; > + cdev->pwm_channel = pwm_channel; > + > + mcu_hwmon->cdev[pwm_channel] = cdev; > + > + return 0; > +} > + > +static int iei_wt61p803_puzzle_hwmon_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct iei_wt61p803_puzzle *mcu = dev_get_drvdata(dev->parent); > + struct iei_wt61p803_puzzle_hwmon *mcu_hwmon; > + struct fwnode_handle *child; > + struct device *hwmon_dev; > + int ret; > + > + mcu_hwmon = devm_kzalloc(dev, sizeof(*mcu_hwmon), GFP_KERNEL); > + if (!mcu_hwmon) > + return -ENOMEM; > + > + mcu_hwmon->response_buffer = devm_kzalloc(dev, IEI_WT61P803_PUZZLE_BUF_SIZE, GFP_KERNEL); > + if (!mcu_hwmon->response_buffer) > + return -ENOMEM; > + > + mcu_hwmon->mcu = mcu; > + platform_set_drvdata(pdev, mcu_hwmon); > + > + hwmon_dev = devm_hwmon_device_register_with_info(dev, "iei_wt61p803_puzzle", > + mcu_hwmon, > + &iei_wt61p803_puzzle_chip_info, > + NULL); > + > + if (IS_ERR(hwmon_dev)) > + return PTR_ERR(hwmon_dev); > + > + /* Control fans via PWM lines via Linux Kernel */ > + if (IS_ENABLED(CONFIG_THERMAL)) { > + device_for_each_child_node(dev, child) { > + ret = iei_wt61p803_puzzle_enable_thermal_cooling_dev(dev, child, mcu_hwmon); > + if (ret) { > + dev_err(dev, "Enabling the PWM fan failed\n"); > + fwnode_handle_put(child); > + return ret; > + } > + } > + } > + return 0; > +} > + > +static const struct of_device_id iei_wt61p803_puzzle_hwmon_id_table[] = { > + { .compatible = "iei,wt61p803-puzzle-hwmon" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, iei_wt61p803_puzzle_hwmon_id_table); > + > +static struct platform_driver iei_wt61p803_puzzle_hwmon_driver = { > + .driver = { > + .name = "iei-wt61p803-puzzle-hwmon", > + .of_match_table = iei_wt61p803_puzzle_hwmon_id_table, > + }, > + .probe = iei_wt61p803_puzzle_hwmon_probe, > +}; > + > +module_platform_driver(iei_wt61p803_puzzle_hwmon_driver); > + > +MODULE_DESCRIPTION("IEI WT61P803 PUZZLE MCU HWMON Driver"); > +MODULE_AUTHOR("Luka Kovacic <luka.kovacic@sartura.hr>"); > +MODULE_LICENSE("GPL");
Hello Guenter, On Sun, Nov 8, 2020 at 5:51 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On Sun, Oct 25, 2020 at 02:59:13AM +0200, Luka Kovacic wrote: > > Add the IEI WT61P803 PUZZLE HWMON driver, that handles the fan speed > > control via PWM, reading fan speed and reading on-board temperature > > sensors. > > > > The driver registers a HWMON device and a simple thermal cooling device to > > enable in-kernel fan management. > > > > This driver depends on the IEI WT61P803 PUZZLE MFD driver. > > > > Signed-off-by: Luka Kovacic <luka.kovacic@sartura.hr> > > Cc: Luka Perkov <luka.perkov@sartura.hr> > > Cc: Robert Marko <robert.marko@sartura.hr> > > --- > > drivers/hwmon/Kconfig | 8 + > > drivers/hwmon/Makefile | 1 + > > drivers/hwmon/iei-wt61p803-puzzle-hwmon.c | 412 ++++++++++++++++++++++ > > This requires documentation (Documentation/hwmon/iei-wt61p803-puzzle.rst). Okay, I'll add it. > > > > 3 files changed, 421 insertions(+) > > create mode 100644 drivers/hwmon/iei-wt61p803-puzzle-hwmon.c > > > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > > index 8dc28b26916e..e0469384af2a 100644 > > --- a/drivers/hwmon/Kconfig > > +++ b/drivers/hwmon/Kconfig > > @@ -722,6 +722,14 @@ config SENSORS_IBMPOWERNV > > This driver can also be built as a module. If so, the module > > will be called ibmpowernv. > > > > +config SENSORS_IEI_WT61P803_PUZZLE_HWMON > > + tristate "IEI WT61P803 PUZZLE MFD HWMON Driver" > > + depends on MFD_IEI_WT61P803_PUZZLE > > + help > > + The IEI WT61P803 PUZZLE MFD HWMON Driver handles reading fan speed > > + and writing fan PWM values. It also supports reading on-board > > + temperature sensors. > > + > > config SENSORS_IIO_HWMON > > tristate "Hwmon driver that uses channels specified via iio maps" > > depends on IIO > > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > > index a8f4b35b136b..b0afb2d6896f 100644 > > --- a/drivers/hwmon/Makefile > > +++ b/drivers/hwmon/Makefile > > @@ -83,6 +83,7 @@ obj-$(CONFIG_SENSORS_HIH6130) += hih6130.o > > obj-$(CONFIG_SENSORS_ULTRA45) += ultra45_env.o > > obj-$(CONFIG_SENSORS_I5500) += i5500_temp.o > > obj-$(CONFIG_SENSORS_I5K_AMB) += i5k_amb.o > > +obj-$(CONFIG_SENSORS_IEI_WT61P803_PUZZLE_HWMON) += iei-wt61p803-puzzle-hwmon.o > > obj-$(CONFIG_SENSORS_IBMAEM) += ibmaem.o > > obj-$(CONFIG_SENSORS_IBMPEX) += ibmpex.o > > obj-$(CONFIG_SENSORS_IBMPOWERNV)+= ibmpowernv.o > > diff --git a/drivers/hwmon/iei-wt61p803-puzzle-hwmon.c b/drivers/hwmon/iei-wt61p803-puzzle-hwmon.c > > new file mode 100644 > > index 000000000000..61b06c9e61df > > --- /dev/null > > +++ b/drivers/hwmon/iei-wt61p803-puzzle-hwmon.c > > @@ -0,0 +1,412 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* IEI WT61P803 PUZZLE MCU HWMON Driver > > + * > > + * Copyright (C) 2020 Sartura Ltd. > > + * Author: Luka Kovacic <luka.kovacic@sartura.hr> > > + */ > > + > > +#include <linux/err.h> > > +#include <linux/hwmon-sysfs.h> > > Unnecessary include. > > > +#include <linux/hwmon.h> > > +#include <linux/interrupt.h> > > +#include <linux/irq.h> > > +#include <linux/math64.h> > > +#include <linux/mfd/iei-wt61p803-puzzle.h> > > +#include <linux/mod_devicetable.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/property.h> > > +#include <linux/slab.h> > > +#include <linux/thermal.h> > > + > > +#define IEI_WT61P803_PUZZLE_HWMON_MAX_TEMP 2 > > +#define IEI_WT61P803_PUZZLE_HWMON_MAX_FAN 5 > > +#define IEI_WT61P803_PUZZLE_HWMON_MAX_PWM 2 > > +#define IEI_WT61P803_PUZZLE_HWMON_MAX_PWM_VAL 255 > > + > > +/** > > + * struct iei_wt61p803_puzzle_thermal_cooling_device - Thermal cooling device instance > > + * @mcu_hwmon: Parent driver struct pointer > > + * @tcdev: Thermal cooling device pointer > > + * @name: Thermal cooling device name > > + * @pwm_channel: Controlled PWM channel (0 or 1) > > + * @cooling_levels: Thermal cooling device cooling levels (DT) > > + */ > > +struct iei_wt61p803_puzzle_thermal_cooling_device { > > + struct iei_wt61p803_puzzle_hwmon *mcu_hwmon; > > + struct thermal_cooling_device *tcdev; > > + char name[THERMAL_NAME_LENGTH]; > > + int pwm_channel; > > + u8 *cooling_levels; > > +}; > > + > > +/** > > + * struct iei_wt61p803_puzzle_hwmon - MCU HWMON Driver > > + * @mcu: MCU struct pointer > > + * @response_buffer Global MCU response buffer allocation > > + * @thermal_cooling_dev_present: Per-channel thermal cooling device control indicator > > + * @cdev: Per-channel thermal cooling device private structure > > + */ > > +struct iei_wt61p803_puzzle_hwmon { > > + struct iei_wt61p803_puzzle *mcu; > > + unsigned char *response_buffer; > > + bool thermal_cooling_dev_present[IEI_WT61P803_PUZZLE_HWMON_MAX_PWM]; > > + struct iei_wt61p803_puzzle_thermal_cooling_device > > + *cdev[IEI_WT61P803_PUZZLE_HWMON_MAX_PWM]; > > +}; > > + > > +#define raw_temp_to_milidegree_celsius(x) (((x) - 0x80) * 1000) > > +static int iei_wt61p803_puzzle_read_temp_sensor(struct iei_wt61p803_puzzle_hwmon *mcu_hwmon, > > + int channel, long *value) > > +{ > > + unsigned char *resp_buf = mcu_hwmon->response_buffer; > > + static unsigned char temp_sensor_ntc_cmd[4] = { > > + IEI_WT61P803_PUZZLE_CMD_HEADER_START, > > + IEI_WT61P803_PUZZLE_CMD_TEMP, > > + IEI_WT61P803_PUZZLE_CMD_TEMP_ALL, > > + }; > > + size_t reply_size = 0; > > + int ret; > > + > > + ret = iei_wt61p803_puzzle_write_command(mcu_hwmon->mcu, temp_sensor_ntc_cmd, > > + sizeof(temp_sensor_ntc_cmd), resp_buf, > > + &reply_size); > > + > > + if (ret) > > + return ret; > > + > > + if (reply_size != 7) > > + return -EIO; > > + > > + /* Check the number of NTC values */ > > + if (resp_buf[3] != '2') > > + return -EIO; > > resp_buf always points to mcu_hwmon->response_buffer, yet the use of that buffer is > not mutex protected. This will result in race conditions all over the place > if there is more than one reader/writer. Okay, I can add a mutex for the buffer. > > Guenter > > > + > > + *value = raw_temp_to_milidegree_celsius(resp_buf[4 + channel]); > > + > > + return 0; > > +} > > + > > +#define raw_fan_val_to_rpm(x, y) ((((x) << 8 | (y)) / 2) * 60) > > +static int iei_wt61p803_puzzle_read_fan_speed(struct iei_wt61p803_puzzle_hwmon *mcu_hwmon, > > + int channel, long *value) > > +{ > > + unsigned char *resp_buf = mcu_hwmon->response_buffer; > > + unsigned char fan_speed_cmd[4] = {}; > > + size_t reply_size = 0; > > + int ret; > > + > > + fan_speed_cmd[0] = IEI_WT61P803_PUZZLE_CMD_HEADER_START; > > + fan_speed_cmd[1] = IEI_WT61P803_PUZZLE_CMD_FAN; > > + fan_speed_cmd[2] = IEI_WT61P803_PUZZLE_CMD_FAN_RPM(channel); > > + > > + ret = iei_wt61p803_puzzle_write_command(mcu_hwmon->mcu, fan_speed_cmd, > > + sizeof(fan_speed_cmd), resp_buf, > > + &reply_size); > > + if (ret) > > + return ret; > > + > > + if (reply_size != 7) > > + return -EIO; > > + > > + *value = raw_fan_val_to_rpm(resp_buf[3], resp_buf[4]); > > + > > + return 0; > > +} > > + > > +static int iei_wt61p803_puzzle_write_pwm_channel(struct iei_wt61p803_puzzle_hwmon *mcu_hwmon, > > + int channel, long pwm_set_val) > > +{ > > + unsigned char *resp_buf = mcu_hwmon->response_buffer; > > + unsigned char pwm_set_cmd[6] = {}; > > + size_t reply_size = 0; > > + int ret; > > + > > + pwm_set_cmd[0] = IEI_WT61P803_PUZZLE_CMD_HEADER_START; > > + pwm_set_cmd[1] = IEI_WT61P803_PUZZLE_CMD_FAN; > > + pwm_set_cmd[2] = IEI_WT61P803_PUZZLE_CMD_FAN_PWM_WRITE; > > + pwm_set_cmd[3] = IEI_WT61P803_PUZZLE_CMD_FAN_PWM(channel); > > + pwm_set_cmd[4] = (unsigned char)pwm_set_val; > > + > > + ret = iei_wt61p803_puzzle_write_command(mcu_hwmon->mcu, pwm_set_cmd, > > + sizeof(pwm_set_cmd), resp_buf, > > + &reply_size); > > + if (ret) > > + return ret; > > + > > + if (reply_size != 3) > > + return -EIO; > > + > > + if (!(resp_buf[0] == IEI_WT61P803_PUZZLE_CMD_HEADER_START && > > + resp_buf[1] == IEI_WT61P803_PUZZLE_CMD_RESPONSE_OK && > > + resp_buf[2] == IEI_WT61P803_PUZZLE_CHECKSUM_RESPONSE_OK)) > > + return -EIO; > > + > > + return 0; > > +} > > + > > +static int iei_wt61p803_puzzle_read_pwm_channel(struct iei_wt61p803_puzzle_hwmon *mcu_hwmon, > > + int channel, long *value) > > +{ > > + unsigned char *resp_buf = mcu_hwmon->response_buffer; > > + unsigned char pwm_get_cmd[5] = {}; > > + size_t reply_size = 0; > > + int ret; > > + > > + pwm_get_cmd[0] = IEI_WT61P803_PUZZLE_CMD_HEADER_START; > > + pwm_get_cmd[1] = IEI_WT61P803_PUZZLE_CMD_FAN; > > + pwm_get_cmd[2] = IEI_WT61P803_PUZZLE_CMD_FAN_PWM_READ; > > + pwm_get_cmd[3] = IEI_WT61P803_PUZZLE_CMD_FAN_PWM(channel); > > + > > + ret = iei_wt61p803_puzzle_write_command(mcu_hwmon->mcu, pwm_get_cmd, > > + sizeof(pwm_get_cmd), resp_buf, > > + &reply_size); > > + if (ret) > > + return ret; > > + > > + if (reply_size != 5) > > + return -EIO; > > + > > + if (resp_buf[2] != IEI_WT61P803_PUZZLE_CMD_FAN_PWM_READ) > > + return -EIO; > > + > > + *value = resp_buf[3]; > > + > > + return 0; > > +} > > + > > +static int iei_wt61p803_puzzle_read(struct device *dev, enum hwmon_sensor_types type, > > + u32 attr, int channel, long *val) > > +{ > > + struct iei_wt61p803_puzzle_hwmon *mcu_hwmon = dev_get_drvdata(dev->parent); > > + > > + switch (type) { > > + case hwmon_pwm: > > + return iei_wt61p803_puzzle_read_pwm_channel(mcu_hwmon, channel, val); > > + case hwmon_fan: > > + return iei_wt61p803_puzzle_read_fan_speed(mcu_hwmon, channel, val); > > + case hwmon_temp: > > + return iei_wt61p803_puzzle_read_temp_sensor(mcu_hwmon, channel, val); > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static int iei_wt61p803_puzzle_write(struct device *dev, enum hwmon_sensor_types type, > > + u32 attr, int channel, long val) > > +{ > > + struct iei_wt61p803_puzzle_hwmon *mcu_hwmon = dev_get_drvdata(dev->parent); > > + > > + if (attr != hwmon_pwm_input) > > + return -EINVAL; > > This check is unnecesary. Ok, I'll remove it. > > > + if (mcu_hwmon->thermal_cooling_dev_present[channel]) { > > + /* > > + * The Thermal Framework has already claimed this specific PWM > > + * channel. > > + */ > > + return -EBUSY; > > + } > > The sensor should not be marked as writeable in this case. > iei_wt61p803_puzzle_is_visible() should check for the condition and > mark the affected attribute(s) read-only. Ok, I'll move the condition to iei_wt61p803_puzzle_is_visible(). > > > + return iei_wt61p803_puzzle_write_pwm_channel(mcu_hwmon, channel, val); > > +} > > + > > +static umode_t iei_wt61p803_puzzle_is_visible(const void *data, enum hwmon_sensor_types type, > > + u32 attr, int channel) > > +{ > > + switch (type) { > > + case hwmon_pwm: > > + if (attr == hwmon_pwm_input) > > + return 0644; > > + break; > > + case hwmon_fan: > > + if (attr == hwmon_fan_input) > > + return 0444; > > + break; > > + case hwmon_temp: > > + if (attr == hwmon_temp_input) > > + return 0444; > > + break; > > + default: > > + return 0; > > + } > > + > > + return 0; > > +} > > + > > +static const struct hwmon_ops iei_wt61p803_puzzle_hwmon_ops = { > > + .is_visible = iei_wt61p803_puzzle_is_visible, > > + .read = iei_wt61p803_puzzle_read, > > + .write = iei_wt61p803_puzzle_write, > > +}; > > + > > +static const struct hwmon_channel_info *iei_wt61p803_puzzle_info[] = { > > + HWMON_CHANNEL_INFO(pwm, > > + HWMON_PWM_INPUT, > > + HWMON_PWM_INPUT), > > + HWMON_CHANNEL_INFO(fan, > > + HWMON_F_INPUT, > > + HWMON_F_INPUT, > > + HWMON_F_INPUT, > > + HWMON_F_INPUT, > > + HWMON_F_INPUT), > > + HWMON_CHANNEL_INFO(temp, > > + HWMON_T_INPUT, > > + HWMON_T_INPUT), > > + NULL > > +}; > > + > > +static const struct hwmon_chip_info iei_wt61p803_puzzle_chip_info = { > > + .ops = &iei_wt61p803_puzzle_hwmon_ops, > > + .info = iei_wt61p803_puzzle_info, > > +}; > > + > > +static int iei_wt61p803_puzzle_get_max_state(struct thermal_cooling_device *tcdev, > > + unsigned long *state) > > +{ > > + *state = IEI_WT61P803_PUZZLE_HWMON_MAX_PWM_VAL; > > + > > + return 0; > > +} > > + > > +static int iei_wt61p803_puzzle_get_cur_state(struct thermal_cooling_device *tcdev, > > + unsigned long *state) > > +{ > > + struct iei_wt61p803_puzzle_thermal_cooling_device *cdev = tcdev->devdata; > > + struct iei_wt61p803_puzzle_hwmon *mcu_hwmon = cdev->mcu_hwmon; > > + long value; > > + int ret; > > + > > + ret = iei_wt61p803_puzzle_read_pwm_channel(mcu_hwmon, cdev->pwm_channel, &value); > > + if (ret) > > + return ret; > > + > > + *state = value; > > + > > + return 0; > > +} > > + > > +static int iei_wt61p803_puzzle_set_cur_state(struct thermal_cooling_device *tcdev, > > + unsigned long state) > > +{ > > + struct iei_wt61p803_puzzle_thermal_cooling_device *cdev = tcdev->devdata; > > + struct iei_wt61p803_puzzle_hwmon *mcu_hwmon = cdev->mcu_hwmon; > > + > > + return iei_wt61p803_puzzle_write_pwm_channel(mcu_hwmon, cdev->pwm_channel, state); > > +} > > + > > +static const struct thermal_cooling_device_ops iei_wt61p803_puzzle_cooling_ops = { > > + .get_max_state = iei_wt61p803_puzzle_get_max_state, > > + .get_cur_state = iei_wt61p803_puzzle_get_cur_state, > > + .set_cur_state = iei_wt61p803_puzzle_set_cur_state, > > +}; > > + > > +static int iei_wt61p803_puzzle_enable_thermal_cooling_dev(struct device *dev, > > + struct fwnode_handle *child, > > + struct iei_wt61p803_puzzle_hwmon *mcu_hwmon) > > +{ > > + struct iei_wt61p803_puzzle_thermal_cooling_device *cdev; > > + u32 pwm_channel; > > + u8 num_levels; > > + int ret; > > + > > + ret = fwnode_property_read_u32(child, "reg", &pwm_channel); > > + if (ret) > > + return ret; > > + > > + mcu_hwmon->thermal_cooling_dev_present[pwm_channel] = true; > > + > > + num_levels = fwnode_property_count_u8(child, "cooling-levels"); > > + if (!num_levels) > > + return -EINVAL; > > + > > + cdev = devm_kzalloc(dev, sizeof(*cdev), GFP_KERNEL); > > + if (!cdev) > > + return -ENOMEM; > > + > > + cdev->cooling_levels = devm_kmalloc_array(dev, num_levels, sizeof(u8), GFP_KERNEL); > > + if (!cdev->cooling_levels) > > + return -ENOMEM; > > + > > + ret = fwnode_property_read_u8_array(child, "cooling-levels", > > + cdev->cooling_levels, > > + num_levels); > > + if (ret) { > > + dev_err(dev, "Couldn't read property 'cooling-levels'"); > > + return ret; > > + } > > + > > + snprintf(cdev->name, THERMAL_NAME_LENGTH, "iei_wt61p803_puzzle_%d", pwm_channel); > > + > > + cdev->tcdev = devm_thermal_of_cooling_device_register(dev, NULL, cdev->name, cdev, > > + &iei_wt61p803_puzzle_cooling_ops); > > + if (IS_ERR(cdev->tcdev)) > > + return PTR_ERR(cdev->tcdev); > > + > > + cdev->mcu_hwmon = mcu_hwmon; > > + cdev->pwm_channel = pwm_channel; > > + > > + mcu_hwmon->cdev[pwm_channel] = cdev; > > + > > + return 0; > > +} > > + > > +static int iei_wt61p803_puzzle_hwmon_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct iei_wt61p803_puzzle *mcu = dev_get_drvdata(dev->parent); > > + struct iei_wt61p803_puzzle_hwmon *mcu_hwmon; > > + struct fwnode_handle *child; > > + struct device *hwmon_dev; > > + int ret; > > + > > + mcu_hwmon = devm_kzalloc(dev, sizeof(*mcu_hwmon), GFP_KERNEL); > > + if (!mcu_hwmon) > > + return -ENOMEM; > > + > > + mcu_hwmon->response_buffer = devm_kzalloc(dev, IEI_WT61P803_PUZZLE_BUF_SIZE, GFP_KERNEL); > > + if (!mcu_hwmon->response_buffer) > > + return -ENOMEM; > > + > > + mcu_hwmon->mcu = mcu; > > + platform_set_drvdata(pdev, mcu_hwmon); > > + > > + hwmon_dev = devm_hwmon_device_register_with_info(dev, "iei_wt61p803_puzzle", > > + mcu_hwmon, > > + &iei_wt61p803_puzzle_chip_info, > > + NULL); > > + > > + if (IS_ERR(hwmon_dev)) > > + return PTR_ERR(hwmon_dev); > > + > > + /* Control fans via PWM lines via Linux Kernel */ > > + if (IS_ENABLED(CONFIG_THERMAL)) { > > + device_for_each_child_node(dev, child) { > > + ret = iei_wt61p803_puzzle_enable_thermal_cooling_dev(dev, child, mcu_hwmon); > > + if (ret) { > > + dev_err(dev, "Enabling the PWM fan failed\n"); > > + fwnode_handle_put(child); > > + return ret; > > + } > > + } > > + } > > + return 0; > > +} > > + > > +static const struct of_device_id iei_wt61p803_puzzle_hwmon_id_table[] = { > > + { .compatible = "iei,wt61p803-puzzle-hwmon" }, > > + {} > > +}; > > +MODULE_DEVICE_TABLE(of, iei_wt61p803_puzzle_hwmon_id_table); > > + > > +static struct platform_driver iei_wt61p803_puzzle_hwmon_driver = { > > + .driver = { > > + .name = "iei-wt61p803-puzzle-hwmon", > > + .of_match_table = iei_wt61p803_puzzle_hwmon_id_table, > > + }, > > + .probe = iei_wt61p803_puzzle_hwmon_probe, > > +}; > > + > > +module_platform_driver(iei_wt61p803_puzzle_hwmon_driver); > > + > > +MODULE_DESCRIPTION("IEI WT61P803 PUZZLE MCU HWMON Driver"); > > +MODULE_AUTHOR("Luka Kovacic <luka.kovacic@sartura.hr>"); > > +MODULE_LICENSE("GPL"); Kind regards, Luka
This patchset adds support for the IEI WT61P803 PUZZLE microcontroller, which enables some board specific features like fan and LED control, system power management and temperature sensor reading on some IEI Puzzle series boards. The first board to use this functionality is IEI Puzzle-M801 1U Rackmount Network Appliance and is since v4 sent separately, as a standalone patch. Changes for v2: - Use LAAs for local-mac-address and match reg values - Code styling changes - Error handling moved to the end of the function - Define all magic numbers in the main header file - Convert the driver to make it OF independent - Refactor hwmon to use devm_hwmon_device_register_with_info() - Reduce the number of mutex locks - Allocate memory once for the response buffer - Reduce managed memory allocations Changes for v3: - Move iei-wt61p803-puzzle driver sysfs interface documentation to testing - Change some internal functions to static - Sync dt-bindings examples with the IEI Puzzle-M801 board dts - Remove obsolete device tree properties and correct LED functions - Reverse christmas tree variable declaration order, where possible - MAC address sysfs function rewrite - Fixed struct members size, where reasonable (MFD driver) - Add an error check for hwmon_dev - Use devm_led_classdev_register_ext() in the LED driver Changes for v4: - Clean up sensible checks reported by checkpatch --strict - Document the mutex lock usage in the LED driver - Fix error handling and code styling issues in the HWMON driver - Break up the patchset and send the IEI Puzzle-M801 board support patch separately Changes for v5: - Remove the return before goto to also fwnode_handle_put(child) when ret is 0 (LED driver) - Change unsigned char arrays to static where applicable - Fix unconventional line indentations - Remove unnecessary checks in the HWMON driver - Remove unnecessary type casts - Clear up command array assignments, where the command array is modified before it is sent - Resolve a checksum calculation issue - Add Luka Perkov to MAINTAINERS Changes for v6: - Use the container_of() macro to get the led_cdev parent struct - Use %u instead of %lu in a printf() (LED driver) Changes for v7: - Use the correct vendor title (IEI instead of iEi) - Add missing properties to dt-bindings and fix styling issues - Styling changes in the IEI WT61P803 PUZZLE HWMON driver - Add missing commas in array definitions - Check reply_size, where possible - Clean up kernel-doc comments Luka Kovacic (6): dt-bindings: Add IEI vendor prefix and IEI WT61P803 PUZZLE driver bindings drivers: mfd: Add a driver for IEI WT61P803 PUZZLE MCU drivers: hwmon: Add the IEI WT61P803 PUZZLE HWMON driver drivers: leds: Add the IEI WT61P803 PUZZLE LED driver Documentation/ABI: Add iei-wt61p803-puzzle driver sysfs interface documentation MAINTAINERS: Add an entry for the IEI WT61P803 PUZZLE driver .../testing/sysfs-driver-iei-wt61p803-puzzle | 55 + .../hwmon/iei,wt61p803-puzzle-hwmon.yaml | 53 + .../leds/iei,wt61p803-puzzle-leds.yaml | 45 + .../bindings/mfd/iei,wt61p803-puzzle.yaml | 83 ++ .../devicetree/bindings/vendor-prefixes.yaml | 2 + MAINTAINERS | 14 + drivers/hwmon/Kconfig | 8 + drivers/hwmon/Makefile | 1 + drivers/hwmon/iei-wt61p803-puzzle-hwmon.c | 412 +++++++ drivers/leds/Kconfig | 8 + drivers/leds/Makefile | 1 + drivers/leds/leds-iei-wt61p803-puzzle.c | 161 +++ drivers/mfd/Kconfig | 8 + drivers/mfd/Makefile | 1 + drivers/mfd/iei-wt61p803-puzzle.c | 1039 +++++++++++++++++ include/linux/mfd/iei-wt61p803-puzzle.h | 66 ++ 16 files changed, 1957 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-driver-iei-wt61p803-puzzle create mode 100644 Documentation/devicetree/bindings/hwmon/iei,wt61p803-puzzle-hwmon.yaml create mode 100644 Documentation/devicetree/bindings/leds/iei,wt61p803-puzzle-leds.yaml create mode 100644 Documentation/devicetree/bindings/mfd/iei,wt61p803-puzzle.yaml create mode 100644 drivers/hwmon/iei-wt61p803-puzzle-hwmon.c create mode 100644 drivers/leds/leds-iei-wt61p803-puzzle.c create mode 100644 drivers/mfd/iei-wt61p803-puzzle.c create mode 100644 include/linux/mfd/iei-wt61p803-puzzle.h