* Re: [PATCH v2 3/7] drivers: hwmon: Add the iEi WT61P803 PUZZLE HWMON driver
@ 2020-09-30 20:31 Guenter Roeck
2020-10-06 10:00 ` Luka Kovacic
0 siblings, 1 reply; 4+ messages in thread
From: Guenter Roeck @ 2020-09-30 20:31 UTC (permalink / raw)
To: Luka Kovacic
Cc: linux-kernel, linux-hwmon, linux-arm-kernel, linux-leds,
lee.jones, pavel, dmurphy, robh+dt, jdelvare, andrew, jason,
gregory.clement, luka.perkov, robert.marko
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 <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 | 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 <luka.kovacic@sartura.hr>
> + */
> +
> +#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 <luka.kovacic@sartura.hr>");
> +MODULE_LICENSE("GPL");
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 3/7] drivers: hwmon: Add the iEi WT61P803 PUZZLE HWMON driver
2020-09-30 20:31 [PATCH v2 3/7] drivers: hwmon: Add the iEi WT61P803 PUZZLE HWMON driver Guenter Roeck
@ 2020-10-06 10:00 ` Luka Kovacic
0 siblings, 0 replies; 4+ messages in thread
From: Luka Kovacic @ 2020-10-06 10:00 UTC (permalink / raw)
To: Guenter Roeck
Cc: Linux Kernel Mailing List, linux-hwmon, linux-arm Mailing List,
Linux LED Subsystem, Lee Jones, Pavel Machek, Dan Murphy,
Rob Herring, Jean Delvare, Andrew Lunn, Jason Cooper,
Gregory Clement, Luka Perkov, Robert Marko
On Wed, Sep 30, 2020 at 10:31 PM Guenter Roeck <linux@roeck-us.net> 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 <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 | 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 <luka.kovacic@sartura.hr>
> > + */
> > +
> > +#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 <luka.kovacic@sartura.hr>");
> > +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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 3/7] drivers: hwmon: Add the iEi WT61P803 PUZZLE HWMON driver
2020-09-26 13:55 ` [PATCH v2 3/7] drivers: hwmon: Add the iEi WT61P803 PUZZLE HWMON driver Luka Kovacic
@ 2020-09-26 22:50 ` kernel test robot
0 siblings, 0 replies; 4+ messages in thread
From: kernel test robot @ 2020-09-26 22:50 UTC (permalink / raw)
To: Luka Kovacic, linux-kernel, linux-hwmon, linux-arm-kernel, linux-leds
Cc: kbuild-all, lee.jones, pavel, dmurphy, robh+dt, jdelvare, linux
[-- Attachment #1: Type: text/plain, Size: 3410 bytes --]
Hi Luka,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on hwmon/hwmon-next]
[also build test WARNING on lee-mfd/for-mfd-next pavel-linux-leds/for-next linus/master v5.9-rc6 next-20200925]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Luka-Kovacic/Add-support-for-the-iEi-Puzzle-M801-board/20200926-215756
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: nios2-allyesconfig (attached as .config)
compiler: nios2-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/a453994a7710920ee06d179274597737ff37af27
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Luka-Kovacic/Add-support-for-the-iEi-Puzzle-M801-board/20200926-215756
git checkout a453994a7710920ee06d179274597737ff37af27
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=nios2
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/hwmon/iei-wt61p803-puzzle-hwmon.c: In function 'iei_wt61p803_puzzle_hwmon_probe':
>> drivers/hwmon/iei-wt61p803-puzzle-hwmon.c:457:17: warning: variable 'hwmon_dev' set but not used [-Wunused-but-set-variable]
457 | struct device *hwmon_dev;
| ^~~~~~~~~
vim +/hwmon_dev +457 drivers/hwmon/iei-wt61p803-puzzle-hwmon.c
450
451 static int iei_wt61p803_puzzle_hwmon_probe(struct platform_device *pdev)
452 {
453 struct device *dev = &pdev->dev;
454 struct fwnode_handle *child;
455 struct iei_wt61p803_puzzle_hwmon *mcu_hwmon;
456 struct iei_wt61p803_puzzle *mcu = dev_get_drvdata(dev->parent);
> 457 struct device *hwmon_dev;
458 int ret;
459
460 mcu_hwmon = devm_kzalloc(dev, sizeof(*mcu_hwmon), GFP_KERNEL);
461 if (!mcu_hwmon)
462 return -ENOMEM;
463
464 mcu_hwmon->response_buffer = devm_kzalloc(dev,
465 IEI_WT61P803_PUZZLE_BUF_SIZE, GFP_KERNEL);
466 if (!mcu_hwmon->response_buffer)
467 return -ENOMEM;
468
469 mcu_hwmon->mcu = mcu;
470 mutex_init(&mcu_hwmon->lock);
471 platform_set_drvdata(pdev, mcu_hwmon);
472
473 hwmon_dev = devm_hwmon_device_register_with_info(dev,
474 "iei_wt61p803_puzzle",
475 mcu_hwmon,
476 &iei_wt61p803_puzzle_chip_info,
477 NULL);
478
479 /* Control fans via PWM lines via Linux Kernel */
480 if (IS_ENABLED(CONFIG_THERMAL)) {
481 device_for_each_child_node(dev, child) {
482 ret = iei_wt61p803_puzzle_enable_thermal_cooling_dev(dev, child, mcu_hwmon);
483 if (ret) {
484 dev_err(dev, "Enabling the PWM fan failed\n");
485 fwnode_handle_put(child);
486 return ret;
487 }
488 }
489 }
490 return 0;
491 }
492
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 57189 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2 3/7] drivers: hwmon: Add the iEi WT61P803 PUZZLE HWMON driver
2020-09-26 13:55 [PATCH v2 0/7] Add support for the iEi Puzzle-M801 board Luka Kovacic
@ 2020-09-26 13:55 ` Luka Kovacic
2020-09-26 22:50 ` kernel test robot
0 siblings, 1 reply; 4+ messages in thread
From: Luka Kovacic @ 2020-09-26 13:55 UTC (permalink / raw)
To: linux-kernel, linux-hwmon, linux-arm-kernel, linux-leds
Cc: lee.jones, pavel, dmurphy, robh+dt, jdelvare, linux, andrew,
jason, gregory.clement, luka.perkov, robert.marko, Luka Kovacic
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 | 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 <luka.kovacic@sartura.hr>
+ */
+
+#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))
+static int iei_wt61p803_puzzle_read_temp_sensor
+(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 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;
+
+ 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) {
+ /* 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]);
+ }
+
+ }
+ *value = mcu_hwmon->temp_sensor_val[channel];
+ 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;
+ }
+
+ 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)
+ mcu_hwmon->fan_speed_val[channel] = raw_fan_val_to_rpm(resp_buf[3],
+ resp_buf[4]);
+
+ *value = mcu_hwmon->fan_speed_val[channel];
+ 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;
+ }
+
+ 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;
+
+ 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) {
+ /* Store the PWM value */
+ 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;
+ }
+
+ 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];
+ }
+ *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;
+ return ret;
+ 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;
+ 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;
+ }
+ return iei_wt61p803_puzzle_write_pwm_channel(mcu_hwmon, channel, val);
+ default:
+ return -ENODEV;
+ }
+}
+
+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;
+}
+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 <luka.kovacic@sartura.hr>");
+MODULE_LICENSE("GPL");
--
2.26.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-10-06 10:00 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-30 20:31 [PATCH v2 3/7] drivers: hwmon: Add the iEi WT61P803 PUZZLE HWMON driver Guenter Roeck
2020-10-06 10:00 ` Luka Kovacic
-- strict thread matches above, loose matches on Subject: below --
2020-09-26 13:55 [PATCH v2 0/7] Add support for the iEi Puzzle-M801 board Luka Kovacic
2020-09-26 13:55 ` [PATCH v2 3/7] drivers: hwmon: Add the iEi WT61P803 PUZZLE HWMON driver Luka Kovacic
2020-09-26 22:50 ` kernel test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).