by Marek Behún

[permalink] [raw]

Subject: Re: [PATCH v2 2/7] drivers: mfd: Add a driver for iEi WT61P803 PUZZLE MCU

On Sat, 26 Sep 2020 15:55:09 +0200
Luka Kovacic <[email protected]> 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.
>

Hi Luka,

this patch produces some checkpatch warnings, some of them are
reasonable.

My thought below:

> Signed-off-by: Luka Kovacic <[email protected]>
> Cc: Luka Perkov <[email protected]>
> Cc: Robert Marko <[email protected]>
> ---
> drivers/mfd/Kconfig | 8 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/iei-wt61p803-puzzle.c | 1069 +++++++++++++++++++++++
> include/linux/mfd/iei-wt61p803-puzzle.h | 69 ++
> 4 files changed, 1147 insertions(+)
> create mode 100644 drivers/mfd/iei-wt61p803-puzzle.c
> create mode 100644 include/linux/mfd/iei-wt61p803-puzzle.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 33df0837ab41..b1588845894e 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2118,5 +2118,13 @@ config SGI_MFD_IOC3
> If you have an SGI Origin, Octane, or a PCI IOC3 card,
> then say Y. Otherwise say N.
>
> +config MFD_IEI_WT61P803_PUZZLE
> + tristate "iEi WT61P803 PUZZLE MCU driver"
> + depends on SERIAL_DEV_BUS
> + help
> + iEi WT61P803 PUZZLE is a system power management microcontroller
> + used for fan control, temperature sensor reading, LED control
> + and system identification.
> +
> endmenu
> endif
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index a60e5f835283..33b88023a68d 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -236,6 +236,7 @@ obj-$(CONFIG_MFD_HI655X_PMIC) += hi655x-pmic.o
> obj-$(CONFIG_MFD_DLN2) += dln2.o
> obj-$(CONFIG_MFD_RT5033) += rt5033.o
> obj-$(CONFIG_MFD_SKY81452) += sky81452.o
> +obj-$(CONFIG_MFD_IEI_WT61P803_PUZZLE) += iei-wt61p803-puzzle.o
>
> intel-soc-pmic-objs := intel_soc_pmic_core.o intel_soc_pmic_crc.o
> obj-$(CONFIG_INTEL_SOC_PMIC) += intel-soc-pmic.o
> diff --git a/drivers/mfd/iei-wt61p803-puzzle.c b/drivers/mfd/iei-wt61p803-puzzle.c
> new file mode 100644
> index 000000000000..5cba010ac9b9
> --- /dev/null
> +++ b/drivers/mfd/iei-wt61p803-puzzle.c
> @@ -0,0 +1,1069 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* iEi WT61P803 PUZZLE MCU Driver
> + * System management microcontroller for fan control, temperature sensor reading,
> + * LED control and system identification on iEi Puzzle series ARM-based appliances.
> + *
> + * Copyright (C) 2020 Sartura Ltd.
> + * Author: Luka Kovacic <[email protected]>
> + */
> +
> +#include <asm/unaligned.h>
> +#include <linux/atomic.h>
> +#include <linux/delay.h>
> +#include <linux/delay.h>
> +#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>
> +#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)
> +#define IEI_WT61P803_PUZZLE_RESP_BUF_SIZE 512
> +
> +/* Use HZ as a timeout value throughout the driver */
> +#define IEI_WT61P803_PUZZLE_GENERAL_TIMEOUT HZ
> +
> +/**
> + * struct iei_wt61p803_puzzle_mcu_status - MCU flags state
> + *
> + * @ac_recovery_status_flag: AC Recovery Status Flag
> + * @power_loss_recovery: System recovery after power loss
> + * @power_status: System Power-on Method
> + */
> +struct iei_wt61p803_puzzle_mcu_status {
> + u8 ac_recovery_status_flag;
> + u8 power_loss_recovery;
> + u8 power_status;
> +};
> +
> +/**
> + * enum iei_wt61p803_puzzle_reply_state - State of the reply
> + * @FRAME_OK: The frame was completely processed/received
> + * @FRAME_PROCESSING: First bytes were received, but the frame isn't complete
> + * @FRAME_STRUCT_EMPTY: The frame struct is empty, no data was received
> + * @FRAME_TIMEOUT: The frame processing timed out, communication failed
> + *
> + * Describes the general state of the frame that is currently being received.
> + */
> +enum iei_wt61p803_puzzle_reply_state {
> + FRAME_OK = 0x00,
> + FRAME_PROCESSING = 0x01,
> + FRAME_STRUCT_EMPTY = 0xFF,
> + FRAME_TIMEOUT = 0xFE
> +};
> +
> +/**
> + * struct iei_wt61p803_puzzle_reply - MCU reply
> + *
> + * @size: Size of the MCU reply
> + * @data: Full MCU reply buffer
> + * @state: Current state of the packet
> + * @received: Was the response fullfilled
> + */
> +struct iei_wt61p803_puzzle_reply {
> + size_t size;
> + unsigned char *data;
> + u8 state;
> + struct completion received;
> +};
> +
> +/**
> + * struct iei_wt61p803_puzzle_mcu_version - MCU version status
> + *
> + * @version: Primary firmware version
> + * @build_info: Build date and time
> + * @bootloader_mode: Status of the MCU operation
> + * @protocol_version: MCU communication protocol version
> + * @serial_number: Device factory serial number
> + * @mac_address: Device factory MAC addresses
> + */
> +struct iei_wt61p803_puzzle_mcu_version {
> + const char *version;
> + const char *build_info;
> + bool bootloader_mode;
> + const char *protocol_version;
> + const char *serial_number;
> + const char *mac_address[8];

You use devm_kasprintf below for these, but such variables have limited
and often small length. The mac_address array could for example be:
char mac_address[8][18];
Can serial number be longer than 32 bytes? What about the other members?
I believe it would be better, to avoid unnecesarry allocations, to
change this.

> +};
> +
> +/**
> + * struct iei_wt61p803_puzzle - iEi WT61P803 PUZZLE MCU Driver
> + *
> + * @serdev: Pointer to underlying serdev device
> + * @kobj: Pointer to kobject (sysfs)
> + * @reply_lock: Reply mutex lock
> + * @bus_lock: Bus mutex lock
> + * @reply: Pointer to the iei_wt61p803_puzzle_reply struct
> + * @version: MCU version related data
> + * @status: MCU status related data
> + * @response_buffer Command response buffer allocation
> + * @lock General member mutex lock
> + */
> +struct iei_wt61p803_puzzle {
> + struct serdev_device *serdev;
> + struct kobject *kobj;
> + 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;
> +};
> +
> +unsigned char iei_wt61p803_puzzle_checksum(unsigned char *buf, size_t len)
> +{
> + unsigned char checksum = 0;
> + unsigned int i;
> +
> + for (i = 0; i < len; i++)
> + checksum ^= buf[i];
> +
> + return checksum;
> +}

This function should be static.
And maybe for the whole driver: s/unsigned char/u8/

> +
> +static int iei_wt61p803_puzzle_process_resp(struct iei_wt61p803_puzzle *mcu,
> + unsigned char *raw_resp_data, size_t size)
> +{
> + struct device *dev = &mcu->serdev->dev;
> +
> + unsigned char checksum;

Empty line between declarations?

> +
> + mutex_lock(&mcu->reply_lock);
> +
> + /* Check the incoming frame header */
> + if (!(raw_resp_data[0] == IEI_WT61P803_PUZZLE_CMD_HEADER_START ||
> + raw_resp_data[0] == IEI_WT61P803_PUZZLE_CMD_HEADER_START_OTHER ||
> + (raw_resp_data[0] == IEI_WT61P803_PUZZLE_CMD_HEADER_EEPROM &&
> + raw_resp_data[1] == IEI_WT61P803_PUZZLE_CMD_EEPROM_READ))) {
> +
> + /* Frame header is not correct, check whether to append */
> + if (mcu->reply->state != FRAME_PROCESSING) {
> + dev_err(dev, "Invalid frame header and state (0x%x)", mcu->reply->state);
> + mutex_unlock(&mcu->reply_lock);
> + return -EIO;
> + }
> +
> + /* Append the frame to existing data */
> + memcpy(mcu->reply->data+mcu->reply->size, raw_resp_data, size);
> + mcu->reply->size += size;
> + } else {
> + /* Start processing a new frame */
> + memcpy(mcu->reply->data, raw_resp_data, size);
> + mcu->reply->size = size;
> + mcu->reply->state = FRAME_PROCESSING;
> + }
> +
> + checksum = iei_wt61p803_puzzle_checksum(mcu->reply->data, mcu->reply->size-1);
> +
> + if (checksum != mcu->reply->data[mcu->reply->size-1]) {
> + /* The checksum isn't matched yet, wait for new frames */
> + mutex_unlock(&mcu->reply_lock);
> + return (int)size;
> + }
> +
> + /* Received all the data */
> + mcu->reply->state = FRAME_OK;
> + complete(&mcu->reply->received);
> +
> + mutex_unlock(&mcu->reply_lock);
> +
> + return (int)size;
> +}
> +
> +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);
> +
> + /* Return the number of processed bytes if function returns error */
> + if (ret < 0)
> + return (int)size;
> +
> + return ret;
> +}
> +
> +static const struct serdev_device_ops iei_wt61p803_puzzle_serdev_device_ops = {
> + .receive_buf = iei_wt61p803_puzzle_recv_buf,
> + .write_wakeup = serdev_device_write_wakeup,
> +};
> +
> +/**
> + * iei_wt61p803_puzzle_write_command_watchdog() - Watchdog of the normal cmd
> + * @mcu: Pointer to the iei_wt61p803_puzzle core MFD struct
> + * @cmd: Pointer to the char array to send (size should be content + 1 (xor))
> + * @size: Size of the cmd char array
> + * @reply_data: Pointer to the reply/response data array (should be allocated)
> + * @reply_size: Pointer to size_t (size of reply_data)
> + * @retry_count: Number of times to retry sending the command to the MCU
> + */
> +int iei_wt61p803_puzzle_write_command_watchdog(struct iei_wt61p803_puzzle *mcu,
> + unsigned char *cmd, size_t size, unsigned char *reply_data,
> + size_t *reply_size, int retry_count)
> +{
> + struct device *dev = &mcu->serdev->dev;
> + int ret, i;
> +
> + for (i = 0; i < retry_count; i++) {
> + ret = iei_wt61p803_puzzle_write_command(mcu, cmd, size,
> + reply_data, reply_size);
> +
> + if (ret != -ETIMEDOUT)
> + return ret;
> + }
> +
> + dev_err(dev, "%s: Command response timed out. Retries: %d", __func__,
> + retry_count);

The indentation here is weird, I would put the retry_count at the
position one to the right after dev_err(. But this is not important.

> +
> + return -ETIMEDOUT;
> +}
> +EXPORT_SYMBOL_GPL(iei_wt61p803_puzzle_write_command_watchdog);
> +
> +/**
> + * iei_wt61p803_puzzle_write_command() - Send a structured command to the MCU
> + * @mcu: Pointer to the iei_wt61p803_puzzle core MFD struct
> + * @cmd: Pointer to the char array to send (size should be content + 1 (xor))
> + * @size: Size of the cmd char array
> + * @reply_data: Pointer to the reply/response data array (should be allocated)
> + *
> + * Sends a structured command to the MCU.
> + */
> +int iei_wt61p803_puzzle_write_command(struct iei_wt61p803_puzzle *mcu,
> + unsigned char *cmd, size_t size, unsigned char *reply_data,
> + size_t *reply_size)
> +{
> + struct device *dev = &mcu->serdev->dev;
> + int ret;
> + int len = (int)size;

Some kernel maintainers like reverse christmas tree order of variable
declaration. I find it better to read. If you would like to try:
struct device *dev = &mcu->serdev->dev;
int len = (int)size;
int ret;

> +
> + if (size > IEI_WT61P803_PUZZLE_MAX_COMMAND_LENGTH)
> + return -EINVAL;
> +
> + cmd[len - 1] = iei_wt61p803_puzzle_checksum(cmd, size);
> +
> + mutex_lock(&mcu->bus_lock);
> + mutex_lock(&mcu->reply_lock);
> +
> + if (!mcu->reply) {
> + ret = -EFAULT;
> + goto exit;
> + }
> +
> + /* Initialize reply struct */
> + reinit_completion(&mcu->reply->received);
> + mcu->reply->state = FRAME_STRUCT_EMPTY;
> + mcu->reply->size = 0;
> + mutex_unlock(&mcu->reply_lock);
> +
> + ret = serdev_device_write(mcu->serdev, cmd, len, IEI_WT61P803_PUZZLE_GENERAL_TIMEOUT);
> +
> + if (ret < 0) {
> + mutex_unlock(&mcu->bus_lock);
> + return ret;
> + }
> +
> + if (!wait_for_completion_timeout(&mcu->reply->received,
> + IEI_WT61P803_PUZZLE_GENERAL_TIMEOUT)) {
> + dev_err(dev, "Command reply receive timeout\n");
> + mutex_lock(&mcu->reply_lock);
> + reinit_completion(&mcu->reply->received);
> + mcu->reply->state = FRAME_TIMEOUT;
> +
> + ret = -ETIMEDOUT;
> + goto exit;
> + }
> +
> + mutex_lock(&mcu->reply_lock);
> +
> + if (!mcu->reply) {
> + ret = -EFAULT;
> + goto exit;
> + }
> +
> + *reply_size = mcu->reply->size;
> + /* Copy the received data, as it will not be available after a new frame is received */
> + memcpy(reply_data, mcu->reply->data, mcu->reply->size);
> +
> + ret = 0;
> +exit:
> + mutex_unlock(&mcu->reply_lock);
> + mutex_unlock(&mcu->bus_lock);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(iei_wt61p803_puzzle_write_command);
> +
> +int iei_wt61p803_puzzle_buzzer(struct iei_wt61p803_puzzle *mcu, bool long_beep)
> +{
> + unsigned char buzzer_short_cmd[4] = {
> + IEI_WT61P803_PUZZLE_CMD_HEADER_START,
> + IEI_WT61P803_PUZZLE_CMD_FUNCTION_SINGLE,
> + '2'
> + }; /* Buzzer 0.5 sec */
> + unsigned char buzzer_long_cmd[4] = {
> + IEI_WT61P803_PUZZLE_CMD_HEADER_START,
> + IEI_WT61P803_PUZZLE_CMD_FUNCTION_SINGLE,
> + '3'
> + }; /* Buzzer 1.5 sec */
> + unsigned char *resp_buf = mcu->response_buffer;
> + size_t reply_size = 0;
> + int ret;
> +
> + mutex_lock(&mcu->lock);
> + ret = iei_wt61p803_puzzle_write_command(mcu,
> + long_beep ? buzzer_long_cmd : buzzer_short_cmd, 4,
> + resp_buf, &reply_size);
> + if (ret)
> + goto exit;
> +
> + if (reply_size != 3) {
> + ret = -EIO;
> + goto exit;
> + }
> +
> + 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)) {

Again weird indentation here, I would do

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 exit;
> + }
> +exit:
> + mutex_unlock(&mcu->lock);
> + return ret;
> +}
> +
> +int iei_wt61p803_puzzle_get_version(struct iei_wt61p803_puzzle *mcu)
> +{
> + struct device *dev = &mcu->serdev->dev;
> + unsigned char version_cmd[3] = {
> + IEI_WT61P803_PUZZLE_CMD_HEADER_START_OTHER,
> + IEI_WT61P803_PUZZLE_CMD_OTHER_VERSION
> + };
> + unsigned char build_info_cmd[3] = {
> + IEI_WT61P803_PUZZLE_CMD_HEADER_START_OTHER,
> + IEI_WT61P803_PUZZLE_CMD_OTHER_BUILD
> + };
> + unsigned char bootloader_mode_cmd[3] = {
> + IEI_WT61P803_PUZZLE_CMD_HEADER_START_OTHER,
> + IEI_WT61P803_PUZZLE_CMD_OTHER_BOOTLOADER_MODE
> + };
> + unsigned char protocol_version_cmd[3] = {
> + IEI_WT61P803_PUZZLE_CMD_HEADER_START_OTHER,
> + IEI_WT61P803_PUZZLE_CMD_OTHER_PROTOCOL_VERSION
> + };
> + unsigned char *rb = mcu->response_buffer;
> + size_t reply_size = 0;
> + int ret;
> +
> + mutex_lock(&mcu->lock);
> +
> + ret = iei_wt61p803_puzzle_write_command(mcu, version_cmd,
> + sizeof(version_cmd), rb, &reply_size);
> + if (ret)
> + goto err;
> + if (reply_size < 7) {
> + ret = -EIO;
> + goto err;
> + }
> + mcu->version.version = devm_kasprintf(dev, GFP_KERNEL, "v%c.%c%c%c",
> + rb[2], rb[3], rb[4], rb[5]);
> +
> + ret = iei_wt61p803_puzzle_write_command(mcu, build_info_cmd,
> + sizeof(build_info_cmd), rb, &reply_size);
> + if (ret)
> + goto err;
> + if (reply_size < 15) {
> + ret = -EIO;
> + goto err;
> + }
> + mcu->version.build_info = devm_kasprintf(dev, GFP_KERNEL,
> + "%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]);
> +
> + ret = iei_wt61p803_puzzle_write_command(mcu, bootloader_mode_cmd,
> + sizeof(bootloader_mode_cmd), rb, &reply_size);
> + if (ret)
> + goto err;
> + if (reply_size < 4) {
> + ret = -EIO;
> + goto err;
> + }
> + if (rb[2] == IEI_WT61P803_PUZZLE_CMD_OTHER_MODE_APPS)
> + mcu->version.bootloader_mode = false;
> + else if (rb[2] == IEI_WT61P803_PUZZLE_CMD_OTHER_MODE_BOOTLOADER)
> + mcu->version.bootloader_mode = true;
> +
> + ret = iei_wt61p803_puzzle_write_command(mcu, protocol_version_cmd,
> + sizeof(protocol_version_cmd), rb, &reply_size);
> + if (ret)
> + goto err;
> + if (reply_size < 9) {
> + ret = -EIO;
> + goto err;
> + }
> + mcu->version.protocol_version = devm_kasprintf(dev, GFP_KERNEL,
> + "v%c.%c%c%c%c%c",
> + rb[7], rb[6], rb[5], rb[4], rb[3], rb[2]);
> +err:
> + mutex_unlock(&mcu->lock);
> + return ret;
> +}
> +
> +int iei_wt61p803_puzzle_get_mcu_status(struct iei_wt61p803_puzzle *mcu)
> +{
> + unsigned char mcu_status_cmd[5] = {
> + IEI_WT61P803_PUZZLE_CMD_HEADER_START,
> + IEI_WT61P803_PUZZLE_CMD_FUNCTION_OTHER,
> + IEI_WT61P803_PUZZLE_CMD_FUNCTION_OTHER_STATUS,
> + IEI_WT61P803_PUZZLE_CMD_FUNCTION_OTHER_STATUS
> + };
> + unsigned char *resp_buf = mcu->response_buffer;
> + size_t reply_size = 0;
> + int ret;
> +
> + mutex_lock(&mcu->lock);
> + ret = iei_wt61p803_puzzle_write_command(mcu, mcu_status_cmd,
> + sizeof(mcu_status_cmd), resp_buf, &reply_size);
> + if (ret)
> + goto exit;
> + if (reply_size < 20) {
> + ret = -EIO;
> + goto exit;
> + }
> +
> + /* 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
> + */
> + if (resp_buf[0] == IEI_WT61P803_PUZZLE_CMD_HEADER_START &&
> + resp_buf[1] == IEI_WT61P803_PUZZLE_CMD_FUNCTION_OTHER &&
> + resp_buf[2] == IEI_WT61P803_PUZZLE_CMD_FUNCTION_OTHER_STATUS &&
> + resp_buf[3] == IEI_WT61P803_PUZZLE_CMD_FUNCTION_OTHER_STATUS) {
> + mcu->status.ac_recovery_status_flag = resp_buf[5];
> + mcu->status.power_loss_recovery = resp_buf[10];
> + mcu->status.power_status = resp_buf[19];
> + }
> +exit:
> + mutex_unlock(&mcu->lock);
> + return ret;
> +}
> +
> +int iei_wt61p803_puzzle_get_serial_number(struct iei_wt61p803_puzzle *mcu)
> +{
> + struct device *dev = &mcu->serdev->dev;
> + unsigned char serial_number_cmd[5] = {
> + IEI_WT61P803_PUZZLE_CMD_HEADER_EEPROM,
> + IEI_WT61P803_PUZZLE_CMD_EEPROM_READ,
> + 0x00,
> + 0x24
> + };
> + unsigned char *resp_buf = mcu->response_buffer;
> + size_t reply_size = 0;
> + int ret;
> +
> + mutex_lock(&mcu->lock);
> + ret = iei_wt61p803_puzzle_write_command(mcu, serial_number_cmd,
> + sizeof(serial_number_cmd), resp_buf, &reply_size);
> + if (ret)
> + goto err;
> +
> + mcu->version.serial_number = devm_kasprintf(dev, GFP_KERNEL, "%.*s",
> + (int)reply_size - 5, resp_buf + 4);
> +err:
> + mutex_unlock(&mcu->lock);
> + return ret;
> +
> +}
> +
> +int iei_wt61p803_puzzle_write_serial_number(struct iei_wt61p803_puzzle *mcu,
> + unsigned char serial_number[36])
> +{
> + struct device *dev = &mcu->serdev->dev;
> + unsigned char serial_number_header[4] = {
> + IEI_WT61P803_PUZZLE_CMD_HEADER_EEPROM,
> + IEI_WT61P803_PUZZLE_CMD_EEPROM_WRITE,
> + 0x00,
> + 0xC
> + };
> + unsigned char *resp_buf = mcu->response_buffer;
> + unsigned char serial_number_cmd[4+12+1]; /* header, serial number chunk, XOR checksum */
> + int ret, sn_counter;
> + size_t reply_size = 0;
> +
> + /* The MCU can only handle 22 byte messages, send the S/N in chunks */
> + mutex_lock(&mcu->lock);
> + for (sn_counter = 0; sn_counter < 3; sn_counter++) {
> + serial_number_header[2] = 0x0 + (0xC) * sn_counter;
> +
> + memcpy(serial_number_cmd, serial_number_header, 4);
> + memcpy(serial_number_cmd + 4, serial_number + (0xC) * sn_counter, 0xC);
> +
> + serial_number_cmd[sizeof(serial_number_cmd) - 1] = 0;
> +
> + ret = iei_wt61p803_puzzle_write_command(mcu, serial_number_cmd,
> + sizeof(serial_number_cmd), resp_buf, &reply_size);
> + if (ret)
> + goto err;
> + if (reply_size != 3) {
> + ret = -EIO;
> + goto err;
> + }
> + 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;
> + }
> + }
> +
> + mcu->version.serial_number = devm_kasprintf(dev, GFP_KERNEL, "%.*s",
> + 36, serial_number);
> +err:
> + mutex_unlock(&mcu->lock);
> + return ret;
> +}
> +
> +int iei_wt61p803_puzzle_get_mac_addresses(struct iei_wt61p803_puzzle *mcu)
> +{
> + struct device *dev = &mcu->serdev->dev;
> + unsigned char mac_address_cmd[5] = {
> + IEI_WT61P803_PUZZLE_CMD_HEADER_EEPROM,
> + IEI_WT61P803_PUZZLE_CMD_EEPROM_READ,
> + 0x00,
> + 0x11
> + };
> + unsigned char *resp_buf = mcu->response_buffer;
> + int ret, mac_counter;
> + size_t reply_size = 0;
> +
> + mutex_lock(&mcu->lock);
> + for (mac_counter = 0; mac_counter < 8; mac_counter++) {
> + mac_address_cmd[2] = 0x24 + (0x11) * mac_counter;
> + mac_address_cmd[4] = 0x00;
> +
> + ret = iei_wt61p803_puzzle_write_command(mcu, mac_address_cmd,
> + sizeof(mac_address_cmd), resp_buf, &reply_size);
> + if (ret)
> + continue;
> +
> + if (reply_size < 22) {
> + ret = -EIO;
> + goto err;
> + }
> +
> + mcu->version.mac_address[mac_counter] = devm_kasprintf(dev,
> + GFP_KERNEL, "%.*s", (int)reply_size - 5,
> + resp_buf + 4);
> + }
> +err:
> + mutex_unlock(&mcu->lock);
> + return ret;
> +}
> +
> +int iei_wt61p803_puzzle_write_mac_address(struct iei_wt61p803_puzzle *mcu,
> + unsigned char mac_address[17], int mac_address_idx)
> +{
> + struct device *dev = &mcu->serdev->dev;
> + unsigned char mac_address_header[4] = {
> + IEI_WT61P803_PUZZLE_CMD_HEADER_EEPROM,
> + IEI_WT61P803_PUZZLE_CMD_EEPROM_WRITE,
> + 0x00,
> + 0x11
> + };
> + unsigned char mac_address_cmd[4+17+1]; /* header, MAC address, XOR checksum*/
> + unsigned char *resp_buf = mcu->response_buffer;
> + size_t reply_size = 0;
> + int ret;
> +
> + if (!(mac_address_idx < 8))
> + return -EINVAL;
> +
> + mac_address_header[2] = 0x24 + (0x11) * mac_address_idx;
> +
> + /* 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);
> +
> + mac_address_cmd[sizeof(mac_address_cmd) - 1] = 0;
> +
> + mutex_lock(&mcu->lock);
> + ret = iei_wt61p803_puzzle_write_command(mcu, mac_address_cmd,
> + sizeof(mac_address_cmd), resp_buf, &reply_size);
> + if (ret)
> + goto err;
> + if (reply_size != 3) {
> + ret = -EIO;
> + goto err;
> + }
> + 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;
> + }
> +
> + mcu->version.mac_address[mac_address_idx] = devm_kasprintf(dev,
> + GFP_KERNEL, "%.*s", 17, mac_address);
> +err:
> + mutex_unlock(&mcu->lock);
> + return ret;
> +}
> +
> +int iei_wt61p803_puzzle_write_power_loss_recovery(struct iei_wt61p803_puzzle *mcu,
> + int power_loss_recovery_action)
> +{
> + unsigned char power_loss_recovery_cmd[5] = {
> + IEI_WT61P803_PUZZLE_CMD_HEADER_START,
> + IEI_WT61P803_PUZZLE_CMD_FUNCTION_OTHER,
> + IEI_WT61P803_PUZZLE_CMD_FUNCTION_OTHER_POWER_LOSS,
> + '0'
> + };
> + unsigned char *resp_buf = mcu->response_buffer;
> + size_t reply_size = 0;
> + unsigned char cmd_buf[2];
> + int ret;
> +
> + if (power_loss_recovery_action < 0 || power_loss_recovery_action > 4)
> + return -EINVAL;
> +
> + ret = snprintf(cmd_buf, sizeof(cmd_buf), "%d", power_loss_recovery_action);
> + if (ret < 0)
> + return ret;
> +
> + /* Modify the command with the action index */
> + power_loss_recovery_cmd[3] = cmd_buf[0];
> +
> + mutex_lock(&mcu->lock);
> + ret = iei_wt61p803_puzzle_write_command(mcu, power_loss_recovery_cmd,
> + sizeof(power_loss_recovery_cmd), resp_buf, &reply_size);
> + if (ret)
> + goto exit;
> + mcu->status.power_loss_recovery = power_loss_recovery_action;
> +exit:
> + mutex_unlock(&mcu->lock);
> + return ret;
> +}
> +
> +#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);
> +
> +static ssize_t build_info_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.build_info);
> +}
> +static DEVICE_ATTR_RO(build_info);
> +
> +static ssize_t bootloader_mode_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, "%d\n", mcu->version.bootloader_mode);
> +}
> +static DEVICE_ATTR_RO(bootloader_mode);
> +
> +static ssize_t protocol_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.protocol_version);
> +}
> +static DEVICE_ATTR_RO(protocol_version);
> +
> +static ssize_t serial_number_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);
> + int ret;
> +
> + mutex_lock(&mcu->lock);
> + ret = sprintf(buf, "%s\n", mcu->version.serial_number);
> + mutex_unlock(&mcu->lock);
> +
> + return ret;
> +}
> +static ssize_t serial_number_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct device *dev_container = sysfs_container(dev);
> + struct iei_wt61p803_puzzle *mcu = dev_get_drvdata(dev_container);
> + unsigned char serial_number[36];
> + int ret;
> +
> + /* Check whether the serial number is 36 characters long + null */
> + if ((int)count != 36 + 1)
> + return -EINVAL;
> +
> + /* Copy 36 characters from the buffer to serial_number */
> + memcpy(serial_number, (unsigned char *)buf, 36);
> +
> + ret = iei_wt61p803_puzzle_write_serial_number(mcu, serial_number);
> + if (ret)
> + return ret;
> +
> + return count;
> +}
> +static DEVICE_ATTR_RW(serial_number);
> +
> +static ssize_t mac_address_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);
> + int ret;
> +
> + mutex_lock(&mcu->lock);
> + if (!strcmp(attr->attr.name, "mac_address_0"))
> + ret = sprintf(buf, "%s\n", mcu->version.mac_address[0]);
> + else if (!strcmp(attr->attr.name, "mac_address_1"))
> + ret = sprintf(buf, "%s\n", mcu->version.mac_address[1]);
> + else if (!strcmp(attr->attr.name, "mac_address_2"))
> + ret = sprintf(buf, "%s\n", mcu->version.mac_address[2]);
> + else if (!strcmp(attr->attr.name, "mac_address_3"))
> + ret = sprintf(buf, "%s\n", mcu->version.mac_address[3]);
> + else if (!strcmp(attr->attr.name, "mac_address_4"))
> + ret = sprintf(buf, "%s\n", mcu->version.mac_address[4]);
> + else if (!strcmp(attr->attr.name, "mac_address_5"))
> + ret = sprintf(buf, "%s\n", mcu->version.mac_address[5]);
> + else if (!strcmp(attr->attr.name, "mac_address_6"))
> + ret = sprintf(buf, "%s\n", mcu->version.mac_address[6]);
> + else if (!strcmp(attr->attr.name, "mac_address_7"))
> + ret = sprintf(buf, "%s\n", mcu->version.mac_address[7]);
> + else
> + ret = sprintf(buf, "\n");

WHYYYYY?

int idx;

if (strlen(attr->attr.name) != 13)
return -EIO;

idx = attr->attr.name[12] - '0';
if (idx < 0 || idx > 7)
return -EIO;
ret = sprintf(buf, "%s\n", mcu->version.mac_address[idx]);

> + mutex_unlock(&mcu->lock);
> +
> + return ret;
> +}
> +static ssize_t mac_address_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct device *dev_container = sysfs_container(dev);
> + struct iei_wt61p803_puzzle *mcu = dev_get_drvdata(dev_container);
> + unsigned char mac_address[17];
> + int ret;
> +
> + if ((int)count != 17 + 1)
> + return -EINVAL;
> +
> + memcpy(mac_address, (unsigned char *)buf, 17);
> +
> + if (!strcmp(attr->attr.name, "mac_address_0"))
> + ret = iei_wt61p803_puzzle_write_mac_address(mcu, mac_address, 0);
> + else if (!strcmp(attr->attr.name, "mac_address_1"))
> + ret = iei_wt61p803_puzzle_write_mac_address(mcu, mac_address, 1);
> + else if (!strcmp(attr->attr.name, "mac_address_2"))
> + ret = iei_wt61p803_puzzle_write_mac_address(mcu, mac_address, 2);
> + else if (!strcmp(attr->attr.name, "mac_address_3"))
> + ret = iei_wt61p803_puzzle_write_mac_address(mcu, mac_address, 3);
> + else if (!strcmp(attr->attr.name, "mac_address_4"))
> + ret = iei_wt61p803_puzzle_write_mac_address(mcu, mac_address, 4);
> + else if (!strcmp(attr->attr.name, "mac_address_5"))
> + ret = iei_wt61p803_puzzle_write_mac_address(mcu, mac_address, 5);
> + else if (!strcmp(attr->attr.name, "mac_address_6"))
> + ret = iei_wt61p803_puzzle_write_mac_address(mcu, mac_address, 6);
> + else if (!strcmp(attr->attr.name, "mac_address_7"))
> + ret = iei_wt61p803_puzzle_write_mac_address(mcu, mac_address, 7);
> + if (ret)
> + return ret;
> +

Similar to above.

> + return count;
> +}
> +static DEVICE_ATTR(mac_address_0, 0644, mac_address_show, mac_address_store);
> +static DEVICE_ATTR(mac_address_1, 0644, mac_address_show, mac_address_store);
> +static DEVICE_ATTR(mac_address_2, 0644, mac_address_show, mac_address_store);
> +static DEVICE_ATTR(mac_address_3, 0644, mac_address_show, mac_address_store);
> +static DEVICE_ATTR(mac_address_4, 0644, mac_address_show, mac_address_store);
> +static DEVICE_ATTR(mac_address_5, 0644, mac_address_show, mac_address_store);
> +static DEVICE_ATTR(mac_address_6, 0644, mac_address_show, mac_address_store);
> +static DEVICE_ATTR(mac_address_7, 0644, mac_address_show, mac_address_store);
> +
> +static ssize_t ac_recovery_status_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);
> +
> + int ret;
> +
> + ret = iei_wt61p803_puzzle_get_mcu_status(mcu);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&mcu->lock);
> + ret = sprintf(buf, "%x\n", mcu->status.ac_recovery_status_flag);
> + mutex_unlock(&mcu->lock);
> +
> + return ret;
> +}
> +static DEVICE_ATTR_RO(ac_recovery_status);
> +
> +static ssize_t power_loss_recovery_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);
> +
> + int ret;
> +
> + ret = iei_wt61p803_puzzle_get_mcu_status(mcu);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&mcu->lock);
> + ret = sprintf(buf, "%x\n", mcu->status.power_loss_recovery);
> + mutex_unlock(&mcu->lock);
> +
> + return ret;
> +}
> +static ssize_t power_loss_recovery_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct device *dev_container = sysfs_container(dev);
> + struct iei_wt61p803_puzzle *mcu = dev_get_drvdata(dev_container);
> +
> + int ret;
> + long power_loss_recovery_action = 0;
> +
> + ret = kstrtol(buf, 10, &power_loss_recovery_action);
> + if (ret)
> + return ret;
> +
> + ret = iei_wt61p803_puzzle_write_power_loss_recovery(mcu,
> + (int)power_loss_recovery_action);
> + if (ret)
> + return ret;
> +
> + return count;
> +}
> +static DEVICE_ATTR_RW(power_loss_recovery);
> +
> +static ssize_t power_status_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);
> +
> + int ret;
> +
> + ret = iei_wt61p803_puzzle_get_mcu_status(mcu);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&mcu->lock);
> + ret = sprintf(buf, "%x\n", mcu->status.power_status);
> + mutex_unlock(&mcu->lock);
> +
> + return ret;
> +}
> +static DEVICE_ATTR_RO(power_status);
> +
> +static struct attribute *iei_wt61p803_puzzle_attrs[] = {
> + &dev_attr_version.attr,
> + &dev_attr_build_info.attr,
> + &dev_attr_bootloader_mode.attr,
> + &dev_attr_protocol_version.attr,
> + &dev_attr_serial_number.attr,
> + &dev_attr_mac_address_0.attr,
> + &dev_attr_mac_address_1.attr,
> + &dev_attr_mac_address_2.attr,
> + &dev_attr_mac_address_3.attr,
> + &dev_attr_mac_address_4.attr,
> + &dev_attr_mac_address_5.attr,
> + &dev_attr_mac_address_6.attr,
> + &dev_attr_mac_address_7.attr,
> + &dev_attr_ac_recovery_status.attr,
> + &dev_attr_power_loss_recovery.attr,
> + &dev_attr_power_status.attr,
> + NULL
> +};
> +ATTRIBUTE_GROUPS(iei_wt61p803_puzzle);
> +
> +static int iei_wt61p803_puzzle_sysfs_create(struct device *dev,
> + struct iei_wt61p803_puzzle *mcu)
> +{
> + int ret;
> +
> + mcu->kobj = kobject_create_and_add("iei_wt61p803_puzzle_core", &dev->kobj);
> + if (!mcu->kobj)
> + return -ENOMEM;
> +
> + ret = sysfs_create_groups(mcu->kobj, iei_wt61p803_puzzle_groups);
> + if (ret) {
> + kobject_del(mcu->kobj);
> + kobject_put(mcu->kobj);
> + mcu->kobj = NULL;
> + }
> +
> + return ret;
> +}
> +
> +static int iei_wt61p803_puzzle_sysfs_remove(struct device *dev,
> + struct iei_wt61p803_puzzle *mcu)
> +{
> + /* Remove sysfs groups */
> + sysfs_remove_groups(mcu->kobj, iei_wt61p803_puzzle_groups);
> +
> + /* Remove the kobject */
> + kobject_del(mcu->kobj);
> + kobject_put(mcu->kobj);
> + mcu->kobj = NULL;
> +
> + return 0;
> +}
> +
> +static int iei_wt61p803_puzzle_probe(struct serdev_device *serdev)
> +{
> + struct device *dev = &serdev->dev;
> + struct iei_wt61p803_puzzle *mcu;
> + u32 baud;
> + int ret;
> +
> + /* Read the baud rate from 'current-speed', because the MCU supports different rates */
> + if (device_property_read_u32(dev, "current-speed", &baud)) {
> + dev_err(dev,
> + "'current-speed' is not specified in device node\n");
> + return -EINVAL;
> + }
> + dev_info(dev, "Driver baud rate: %d", baud);
> +
> + /* Allocate the memory */
> + mcu = devm_kzalloc(dev, sizeof(*mcu), GFP_KERNEL);
> + if (!mcu)
> + return -ENOMEM;
> +
> + mcu->reply = devm_kzalloc(dev, sizeof(*mcu->reply), GFP_KERNEL);
> + if (!mcu->reply)
> + return -ENOMEM;
> +
> + mcu->reply->data = devm_kzalloc(dev, IEI_WT61P803_PUZZLE_RESP_BUF_SIZE,
> + GFP_KERNEL);
> + if (!mcu->reply->data)
> + return -ENOMEM;
> +
> + mcu->response_buffer = devm_kzalloc(dev, IEI_WT61P803_PUZZLE_BUF_SIZE,
> + GFP_KERNEL);
> + if (!mcu->response_buffer)
> + return -ENOMEM;
> +
> + /* Initialize device struct data */
> + mcu->serdev = serdev;
> + init_completion(&mcu->reply->received);
> + mutex_init(&mcu->reply_lock);
> + mutex_init(&mcu->bus_lock);
> + mutex_init(&mcu->lock);
> +
> + /* Setup UART interface */
> + serdev_device_set_drvdata(serdev, mcu);
> + serdev_device_set_client_ops(serdev, &iei_wt61p803_puzzle_serdev_device_ops);
> + ret = devm_serdev_device_open(dev, serdev);
> + if (ret)
> + return ret;
> + serdev_device_set_baudrate(serdev, baud);
> + serdev_device_set_flow_control(serdev, false);
> + ret = serdev_device_set_parity(serdev, SERDEV_PARITY_NONE);
> + if (ret) {
> + dev_err(dev, "Failed to set parity");
> + return ret;
> + }
> +
> + ret = iei_wt61p803_puzzle_get_version(mcu);
> + if (ret)
> + return ret;
> +
> + ret = iei_wt61p803_puzzle_get_mac_addresses(mcu);
> + if (ret)
> + return ret;
> +
> + ret = iei_wt61p803_puzzle_get_serial_number(mcu);
> + if (ret)
> + return ret;
> +
> + 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);
> +
> + if (device_property_read_bool(dev, "enable-beep")) {
> + ret = iei_wt61p803_puzzle_buzzer(mcu, false);
> + if (ret)
> + return ret;
> + }
> +
> + ret = iei_wt61p803_puzzle_sysfs_create(dev, mcu);
> +
> + return devm_of_platform_populate(dev);
> +}
> +
> +static void iei_wt61p803_puzzle_remove(struct serdev_device *serdev)
> +{
> + struct device *dev = &serdev->dev;
> + struct iei_wt61p803_puzzle *mcu = dev_get_drvdata(dev);
> +
> + iei_wt61p803_puzzle_sysfs_remove(dev, mcu);
> +}
> +
> +static const struct of_device_id iei_wt61p803_puzzle_dt_ids[] = {
> + { .compatible = "iei,wt61p803-puzzle" },
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(of, iei_wt61p803_puzzle_dt_ids);
> +
> +static struct serdev_device_driver iei_wt61p803_puzzle_drv = {
> + .probe = iei_wt61p803_puzzle_probe,
> + .remove = iei_wt61p803_puzzle_remove,
> + .driver = {
> + .name = "iei-wt61p803-puzzle",
> + .of_match_table = iei_wt61p803_puzzle_dt_ids,
> + },
> +};
> +
> +module_serdev_device_driver(iei_wt61p803_puzzle_drv);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Luka Kovacic <[email protected]>");
> +MODULE_DESCRIPTION("iEi WT61P803 PUZZLE MCU Driver");
> diff --git a/include/linux/mfd/iei-wt61p803-puzzle.h b/include/linux/mfd/iei-wt61p803-puzzle.h
> new file mode 100644
> index 000000000000..633ceb1d00e3
> --- /dev/null
> +++ b/include/linux/mfd/iei-wt61p803-puzzle.h
> @@ -0,0 +1,69 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* iEi WT61P803 PUZZLE MCU Driver
> + * System management microcontroller for fan control, temperature sensor reading,
> + * LED control and system identification on iEi Puzzle series ARM-based appliances.
> + *
> + * Copyright (C) 2020 Sartura Ltd.
> + * Author: Luka Kovacic <[email protected]>
> + */
> +
> +#ifndef _MFD_IEI_WT61P803_PUZZLE_H_
> +#define _MFD_IEI_WT61P803_PUZZLE_H_
> +
> +#define IEI_WT61P803_PUZZLE_BUF_SIZE 512
> +
> +/* Command magic numbers */
> +#define IEI_WT61P803_PUZZLE_CMD_HEADER_START 0x40 /* @ */
> +#define IEI_WT61P803_PUZZLE_CMD_HEADER_START_OTHER 0x25 /* % */
> +#define IEI_WT61P803_PUZZLE_CMD_HEADER_EEPROM 0xF7
> +
> +#define IEI_WT61P803_PUZZLE_CMD_RESPONSE_OK 0x30 /* 0 */
> +#define IEI_WT61P803_PUZZLE_CHECKSUM_RESPONSE_OK 0x70
> +
> +#define IEI_WT61P803_PUZZLE_CMD_EEPROM_READ 0xA1
> +#define IEI_WT61P803_PUZZLE_CMD_EEPROM_WRITE 0xA0
> +
> +#define IEI_WT61P803_PUZZLE_CMD_OTHER_VERSION 0x56 /* V */
> +#define IEI_WT61P803_PUZZLE_CMD_OTHER_BUILD 0x42 /* B */
> +#define IEI_WT61P803_PUZZLE_CMD_OTHER_BOOTLOADER_MODE 0x4D /* M */
> +#define IEI_WT61P803_PUZZLE_CMD_OTHER_MODE_BOOTLOADER 0x30
> +#define IEI_WT61P803_PUZZLE_CMD_OTHER_MODE_APPS 0x31
> +#define IEI_WT61P803_PUZZLE_CMD_OTHER_PROTOCOL_VERSION 0x50 /* P */
> +
> +#define IEI_WT61P803_PUZZLE_CMD_FUNCTION_SINGLE 0x43 /* C */
> +#define IEI_WT61P803_PUZZLE_CMD_FUNCTION_OTHER 0x4F /* O */
> +#define IEI_WT61P803_PUZZLE_CMD_FUNCTION_OTHER_STATUS 0x53 /* S */
> +#define IEI_WT61P803_PUZZLE_CMD_FUNCTION_OTHER_POWER_LOSS 0x41 /* A */
> +
> +#define IEI_WT61P803_PUZZLE_CMD_LED 0x52 /* R */
> +#define IEI_WT61P803_PUZZLE_CMD_LED_POWER 0x31 /* 1 */
> +
> +#define IEI_WT61P803_PUZZLE_CMD_TEMP 0x54 /* T */
> +#define IEI_WT61P803_PUZZLE_CMD_TEMP_ALL 0x41 /* A */
> +
> +#define IEI_WT61P803_PUZZLE_CMD_FAN 0x46 /* F */
> +#define IEI_WT61P803_PUZZLE_CMD_FAN_PWM_0 0x30
> +#define IEI_WT61P803_PUZZLE_CMD_FAN_PWM_1 0x31
> +#define IEI_WT61P803_PUZZLE_CMD_FAN_PWM_READ 0x5A /* Z */
> +#define IEI_WT61P803_PUZZLE_CMD_FAN_PWM_WRITE 0x57 /* W */
> +#define IEI_WT61P803_PUZZLE_CMD_FAN_RPM_0 0x41 /* A */
> +#define IEI_WT61P803_PUZZLE_CMD_FAN_RPM_1 0x42 /* B */
> +#define IEI_WT61P803_PUZZLE_CMD_FAN_RPM_2 0x43 /* C */
> +#define IEI_WT61P803_PUZZLE_CMD_FAN_RPM_3 0x44 /* D */
> +#define IEI_WT61P803_PUZZLE_CMD_FAN_RPM_4 0x45 /* E */
> +#define IEI_WT61P803_PUZZLE_CMD_FAN_RPM_5 0x46 /* F */
> +
> +struct iei_wt61p803_puzzle_mcu_version;
> +struct iei_wt61p803_puzzle_reply;
> +struct iei_wt61p803_puzzle;
> +
> +int iei_wt61p803_puzzle_write_command_watchdog(struct iei_wt61p803_puzzle *mcu,
> + unsigned char *cmd, size_t size,
> + unsigned char *reply_data, size_t *reply_size,
> + int retry_count);
> +
> +int iei_wt61p803_puzzle_write_command(struct iei_wt61p803_puzzle *mcu,
> + unsigned char *cmd, size_t size,
> + unsigned char *reply_data, size_t *reply_size);
> +
> +#endif /* _MFD_IEI_WT61P803_PUZZLE_H_ */

Marek

2020-09-26 22:59:50

On Wed, Sep 30, 2020 at 10:31 PM Guenter Roeck <[email protected]> wrote:
>
> On Sat, Sep 26, 2020 at 03:55:10PM +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 <[email protected]>
> > Cc: Luka Perkov <[email protected]>
> > Cc: Robert Marko <[email protected]>
> > ---
> > drivers/hwmon/Kconfig | 8 +
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/iei-wt61p803-puzzle-hwmon.c | 511 ++++++++++++++++++++++
> > 3 files changed, 520 insertions(+)
> > create mode 100644 drivers/hwmon/iei-wt61p803-puzzle-hwmon.c
> >
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 8dc28b26916e..ff279df9bf40 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..2691b943936b
> > --- /dev/null
> > +++ b/drivers/hwmon/iei-wt61p803-puzzle-hwmon.c
> > @@ -0,0 +1,511 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* iEi WT61P803 PUZZLE MCU HWMON Driver
> > + *
> > + * Copyright (C) 2020 Sartura Ltd.
> > + * Author: Luka Kovacic <[email protected]>
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/hwmon-sysfs.h>
> > +#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_NUM 2
> > +#define IEI_WT61P803_PUZZLE_HWMON_MAX_FAN_NUM 5
> > +#define IEI_WT61P803_PUZZLE_HWMON_MAX_PWM_NUM 2
> > +#define IEI_WT61P803_PUZZLE_HWMON_MAX_PWM_VAL 255
> > +
> > +/**
> > + * struct iei_wt61p803_puzzle_thermal_cooling_device - Thermal cooling device instance
> > + *
> > + * @mcu_hwmon: MCU HWMON struct pointer
> > + * @tcdev: Thermal cooling device pointer
> > + * @name: Thermal cooling device name
> > + * @pwm_channel: PWM channel (0 or 1)
> > + * @cooling_levels: Thermal cooling device cooling levels
> > + */
> > +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
> > + * @lock General member lock
> > + * @response_buffer Global MCU response buffer allocation
> > + * @temp_sensor_val: Temperature sensor values
> > + * @fan_speed_val: FAN speed (RPM) values
> > + * @pwm_val: PWM values (0-255)
> > + * @thermal_cooling_dev_present: Per-channel thermal cooling device control
> > + * @cdev: Per-channel thermal cooling device private structure
> > + */
> > +struct iei_wt61p803_puzzle_hwmon {
> > + struct iei_wt61p803_puzzle *mcu;
> > + struct mutex lock;
> > + unsigned char *response_buffer;
> > + int temp_sensor_val[IEI_WT61P803_PUZZLE_HWMON_MAX_TEMP_NUM];
> > + int fan_speed_val[IEI_WT61P803_PUZZLE_HWMON_MAX_FAN_NUM];
> > + int pwm_val[IEI_WT61P803_PUZZLE_HWMON_MAX_PWM_NUM];
> > + bool thermal_cooling_dev_present[IEI_WT61P803_PUZZLE_HWMON_MAX_PWM_NUM];
> > + struct iei_wt61p803_puzzle_thermal_cooling_device
> > + *cdev[IEI_WT61P803_PUZZLE_HWMON_MAX_PWM_NUM];
> > +};
> > +
> > +#define raw_temp_to_milidegree_celsius(x) ((int)((x - 0x80)*1000))
>
> Spaces around '*', please, and (x). checkpatch --strict will tell about it.
>
> > +static int iei_wt61p803_puzzle_read_temp_sensor
> > +(struct iei_wt61p803_puzzle_hwmon *mcu_hwmon, int channel, int *value)
>
> Odd multi-line alignment. Please use either
>
> static int
> iei_wt61p803_puzzle_read_temp_sensor(struct iei_wt61p803_puzzle_hwmon *mcu_hwmon, int channel,
> int *value)
>
> or
>
> static int iei_wt61p803_puzzle_read_temp_sensor(struct iei_wt61p803_puzzle_hwmon *mcu_hwmon,
> int channel, int *value)
>
> There are lots of those in this driver. Please run
> checkpatch --strict and fix what it reports.
>
> > +{
> > + int ret;
> > + size_t reply_size = 0;
> > + unsigned char *resp_buf = mcu_hwmon->response_buffer;
> > + unsigned char temp_sensor_ntc_cmd[4] = {
> > + IEI_WT61P803_PUZZLE_CMD_HEADER_START,
> > + IEI_WT61P803_PUZZLE_CMD_TEMP,
> > + IEI_WT61P803_PUZZLE_CMD_TEMP_ALL
> > + };
> > +
> > + if (channel > 1 && channel < 0)
> > + return -EINVAL;
>
> Unnecessary range check.
>
> > +
> > + mutex_lock(&mcu_hwmon->lock);
> > + ret = iei_wt61p803_puzzle_write_command(mcu_hwmon->mcu,
> > + temp_sensor_ntc_cmd, sizeof(temp_sensor_ntc_cmd),
> > + resp_buf, &reply_size);
>
> if (ret < 0)
> goto unlock;
>
> > + if (!ret) {
> > + /* Check the number of NTC values (should be 0x32/'2') */
> > + if (resp_buf[3] == 0x32) {
> > + /* Write values to the struct */
> > + mcu_hwmon->temp_sensor_val[0] =
> > + raw_temp_to_milidegree_celsius(resp_buf[4]);
> > + mcu_hwmon->temp_sensor_val[1] =
> > + raw_temp_to_milidegree_celsius(resp_buf[5]);
>
> What is the point of storing the return values in mcu_hwmon->temp_sensor_val[] ?
>
> > + }
> > +
>
> Unnecessary empty line. checkpatch --strict reports this.
>
> > + }
> > + *value = mcu_hwmon->temp_sensor_val[channel];
>
> unlock:
>
> > + mutex_unlock(&mcu_hwmon->lock);
> > +
> > + return ret;
> > +}
> > +
> > +#define raw_fan_val_to_rpm(x, y) ((int)(((x)<<8|(y))/2)*60)
> > +static int iei_wt61p803_puzzle_read_fan_speed
> > +(struct iei_wt61p803_puzzle_hwmon *mcu_hwmon, int channel, int *value)
> > +{
> > + int ret;
> > + size_t reply_size = 0;
> > + unsigned char *resp_buf = mcu_hwmon->response_buffer;
> > + unsigned char fan_speed_cmd[4] = {
> > + IEI_WT61P803_PUZZLE_CMD_HEADER_START,
> > + IEI_WT61P803_PUZZLE_CMD_FAN,
> > + IEI_WT61P803_PUZZLE_CMD_FAN_RPM_0
> > + };
> > +
> > + switch (channel) {
> > + case 0:
> > + fan_speed_cmd[2] = IEI_WT61P803_PUZZLE_CMD_FAN_RPM_0;
> > + break;
> > + case 1:
> > + fan_speed_cmd[2] = IEI_WT61P803_PUZZLE_CMD_FAN_RPM_1;
> > + break;
> > + case 2:
> > + fan_speed_cmd[2] = IEI_WT61P803_PUZZLE_CMD_FAN_RPM_2;
> > + break;
> > + case 3:
> > + fan_speed_cmd[2] = IEI_WT61P803_PUZZLE_CMD_FAN_RPM_3;
> > + break;
> > + case 4:
> > + fan_speed_cmd[2] = IEI_WT61P803_PUZZLE_CMD_FAN_RPM_4;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
>
> This would be much simpler written as
>
> static const u8 fan_speed_cmds[] = {
> IEI_WT61P803_PUZZLE_CMD_FAN_RPM_0,
> IEI_WT61P803_PUZZLE_CMD_FAN_RPM_1,
> IEI_WT61P803_PUZZLE_CMD_FAN_RPM_2,
> IEI_WT61P803_PUZZLE_CMD_FAN_RPM_3,
> IEI_WT61P803_PUZZLE_CMD_FAN_RPM_4
> };
> ...
>
> fan_speed_cmd[2] = fan_speed_cmds[channel];
>
> > +
> > + mutex_lock(&mcu_hwmon->lock);
> > + ret = iei_wt61p803_puzzle_write_command(mcu_hwmon->mcu, fan_speed_cmd,
> > + sizeof(fan_speed_cmd), resp_buf, &reply_size);
>
> if (ret < 0)
> goto unlock;
>
> > + if (!ret)
> > + mcu_hwmon->fan_speed_val[channel] = raw_fan_val_to_rpm(resp_buf[3],
> > + resp_buf[4]);
> > +
> > + *value = mcu_hwmon->fan_speed_val[channel];
>
> What exactly is the point of storing the result in
> mcu_hwmon->fan_speed_val[channel] ?
>
> I won't comment about similar code again, but please drop any such
> unnecessary arrays.
>
> > + mutex_unlock(&mcu_hwmon->lock);
> > +
> > + return 0;
> > +}
> > +
> > +static int iei_wt61p803_puzzle_write_pwm_channel
> > +(struct iei_wt61p803_puzzle_hwmon *mcu_hwmon, int channel, long pwm_set_val)
> > +{
> > + int ret;
> > + size_t reply_size = 0;
> > + unsigned char *resp_buf = mcu_hwmon->response_buffer;
> > + unsigned char pwm_set_cmd[6] = {
> > + IEI_WT61P803_PUZZLE_CMD_HEADER_START,
> > + IEI_WT61P803_PUZZLE_CMD_FAN,
> > + IEI_WT61P803_PUZZLE_CMD_FAN_PWM_WRITE,
> > + IEI_WT61P803_PUZZLE_CMD_FAN_PWM_0,
> > + 0x00
> > + };
> > +
> > + switch (channel) {
> > + case 0:
> > + pwm_set_cmd[3] = IEI_WT61P803_PUZZLE_CMD_FAN_PWM_0;
> > + break;
> > + case 1:
> > + pwm_set_cmd[3] = IEI_WT61P803_PUZZLE_CMD_FAN_PWM_1;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
>
> Same as above - it would be much simpler to have IEI_WT61P803_PUZZLE_CMD_FAN_PWM_0
> and IEI_WT61P803_PUZZLE_CMD_FAN_PWM_1 in an array and get it from there.
> The range check is unnecessary.
>
> > +
> > + if (pwm_set_val < 0 || pwm_set_val > IEI_WT61P803_PUZZLE_HWMON_MAX_PWM_VAL)
> > + return -EINVAL;
> > +
> > + /* Add the PWM value to the command */
> > + pwm_set_cmd[4] = (char)pwm_set_val;
>
> I think this typecast is quite unnecessary. Besides, it is wrong, since the
> value is an unsigned char.
>
> > +
> > + mutex_lock(&mcu_hwmon->lock);
> > + ret = iei_wt61p803_puzzle_write_command(mcu_hwmon->mcu, pwm_set_cmd,
> > + sizeof(pwm_set_cmd), resp_buf, &reply_size);
>
> if (ret < 0)
> goto unlock;
>
> > + if (!ret) {
> > + /* Store the PWM value */
>
> What for ?
>
> Ah yes, I think I finally get it: All this odd handling is to be able to ignore
> errors. But that is not acceptable. If there is an error, report it to the user.
> You can't really claim no error to the user when the value wasn't stored.
>
> > + 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)
> > + mcu_hwmon->pwm_val[channel] = (int)pwm_set_val;
> > + }
> > + mutex_unlock(&mcu_hwmon->lock);
> > +
> > + return 0;
> > +}
> > +
> > +static int iei_wt61p803_puzzle_read_pwm_channel
> > +(struct iei_wt61p803_puzzle_hwmon *mcu_hwmon, int channel, int *value)
> > +{
> > + int ret;
> > + size_t reply_size = 0;
> > + unsigned char *resp_buf = mcu_hwmon->response_buffer;
> > + unsigned char pwm_get_cmd[5] = {
> > + IEI_WT61P803_PUZZLE_CMD_HEADER_START,
> > + IEI_WT61P803_PUZZLE_CMD_FAN,
> > + IEI_WT61P803_PUZZLE_CMD_FAN_PWM_READ,
> > + IEI_WT61P803_PUZZLE_CMD_FAN_PWM_0
> > + };
> > +
> > + switch (channel) {
> > + case 0:
> > + pwm_get_cmd[3] = IEI_WT61P803_PUZZLE_CMD_FAN_PWM_0;
> > + break;
> > + case 1:
> > + pwm_get_cmd[3] = IEI_WT61P803_PUZZLE_CMD_FAN_PWM_1;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
>
> Same comments as before.
>
> > +
> > + mutex_lock(&mcu_hwmon->lock);
> > + ret = iei_wt61p803_puzzle_write_command(mcu_hwmon->mcu, pwm_get_cmd,
> > + sizeof(pwm_get_cmd), resp_buf, &reply_size);
> > + if (!ret) {
> > + /* Store the PWM value */
> > + if (resp_buf[2] == IEI_WT61P803_PUZZLE_CMD_FAN_PWM_READ)
> > + mcu_hwmon->pwm_val[channel] = (int)resp_buf[3];
> > + }
>
> Same comments as before.
>
> > + *value = mcu_hwmon->pwm_val[channel];
> > + mutex_unlock(&mcu_hwmon->lock);
> > +
> > + 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);
> > + int ret, value;
> > +
> > + switch (type) {
> > + case hwmon_pwm:
> > + if (attr != hwmon_pwm_input)
> > + return -ENODEV;
> > + ret = iei_wt61p803_puzzle_read_pwm_channel(mcu_hwmon, channel, &value);
> > + if (ret)
> > + return ret;
> > + *val = (long)value;
> > + return ret;
> > + case hwmon_fan:
> > + if (attr != hwmon_fan_input)
> > + return -ENODEV;
> > + ret = iei_wt61p803_puzzle_read_fan_speed(mcu_hwmon, channel, &value);
> > + if (ret)
> > + return ret;
> > + *val = (long)value;
>
> Unncecssary typecast. Plase check all typecasts and keep only those which
> are really needed (if there are any).
>
> > + return ret;
>
> ret is 0 here.
>
> > + case hwmon_temp:
> > + if (attr != hwmon_temp_input)
> > + return -ENODEV;
> > + ret = iei_wt61p803_puzzle_read_temp_sensor(mcu_hwmon, channel, &value);
> > + if (ret)
> > + return ret;
> > + *val = (long)value;
> > + return ret;
>
> ret is 0 here. That is sprinkled through the code. Please
> replace with "return 0;" everywhere.
>
> > + default:
> > + return -ENODEV;
> > + }
> > +}
> > +
> > +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);
> > +
> > + switch (type) {
> > + case hwmon_pwm:
> > + if (attr != hwmon_pwm_input)
> > + return -ENODEV;
> > + if (mcu_hwmon->thermal_cooling_dev_present[channel]) {
> > + /*
> > + * The Thermal Framework has already claimed this specific PWM
> > + * channel.
> > + */
> > + return -EBUSY;
> > + }
> This won't happen (the attribute is read-only in this case), and the check is
> therefore unnecessary and just adds confusion.
>
> > + return iei_wt61p803_puzzle_write_pwm_channel(mcu_hwmon, channel, val);
> > + default:
> > + return -ENODEV;
> > + }
>
> Unless there is a plan to make other types writable, this switch statement
> is unnecessary. Only pwm attributes are writeable, after all, so the code
> won't be called for anything else.
>
> > +}
> > +
> > +static umode_t iei_wt61p803_puzzle_is_visible(const void *data,
> > + enum hwmon_sensor_types type, u32 attr, int channel)
> > +{
> > + const struct iei_wt61p803_puzzle_hwmon *mcu_hwmon = data;
> > +
> > + switch (type) {
> > + case hwmon_pwm:
> > + switch (attr) {
> > + case hwmon_pwm_input:
> > + if (mcu_hwmon->thermal_cooling_dev_present[channel])
> > + return 0444;
> > + return 0644;
> > + default:
> > + return 0;
> > + }
> > + case hwmon_fan:
> > + switch (attr) {
> > + case hwmon_fan_input:
> > + return 0444;
> > + default:
> > + return 0;
> > + }
> > + case hwmon_temp:
> > + switch (attr) {
> > + case hwmon_temp_input:
> > + return 0444;
> > + default:
> > + return 0;
> > + }
> > + default:
> > + 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;
> > +
> > + int ret, value;
> > +
> > + if (!mcu_hwmon)
> > + return -EINVAL;
> > +
> > + ret = iei_wt61p803_puzzle_read_pwm_channel(mcu_hwmon,
> > + cdev->pwm_channel, &value);
> > + if (ret)
> > + return ret;
> > +
> > + *state = (unsigned long)value;
> > +
> > + return 0;
> > +}
>
> Missing empty line. checkpatch --strict reports this.
>
> > +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;
> > +
> > + if (!mcu_hwmon)
> > + return -EINVAL;
> > +
> > + 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)
> > +{
> > + u32 pwm_channel;
> > + int ret, num_levels;
> > +
> > + struct iei_wt61p803_puzzle_thermal_cooling_device *cdev;
> > +
> > + ret = fwnode_property_read_u32(child, "reg", &pwm_channel);
> > + if (ret)
> > + return ret;
> > +
> > + mutex_lock(&mcu_hwmon->lock);
> > + mcu_hwmon->thermal_cooling_dev_present[pwm_channel] = true;
> > + mutex_unlock(&mcu_hwmon->lock);
> > +
> > + num_levels = fwnode_property_read_u8_array(child, "cooling-levels", NULL, 0);
> > + if (num_levels > 0) {
> > + cdev = devm_kzalloc(dev, sizeof(*cdev), GFP_KERNEL);
> > + if (!cdev)
> > + return -ENOMEM;
> > +
> > + cdev->cooling_levels = devm_kzalloc(dev, num_levels, 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;
> > +
> > + mutex_lock(&mcu_hwmon->lock);
> > + mcu_hwmon->cdev[pwm_channel] = cdev;
> > + mutex_unlock(&mcu_hwmon->lock);
> > + }
> > + return 0;
> > +}
> > +
> > +static int iei_wt61p803_puzzle_hwmon_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct fwnode_handle *child;
> > + struct iei_wt61p803_puzzle_hwmon *mcu_hwmon;
> > + struct iei_wt61p803_puzzle *mcu = dev_get_drvdata(dev->parent);
> > + 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;
> > + mutex_init(&mcu_hwmon->lock);
> > + 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);
> > +
> > + /* 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 <[email protected]>");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.26.2
> >

Hello Guenter,

Thanks for reviewing the patch.
I'll resolve the code styling issues, fix the error handling, and
other issues you
have pointed out.

Kind regards,
Luka