Re: [PATCH] hwmon: max31790: support to config PWM as TACH

Guenter Roeck
2023-09-06T16:20:35Z

On 9/6/23 01:48, Delphine CC Chiu wrote:
> The PWM outputs of max31790 could be used as tachometer inputs by
> setting the fan configuration register, but the driver doesn't support
> to config the PWM outputs as tachometer inputs currently.
>
> Add a function to get properties of the setting of max31790 to config
> PWM outputs as tachometer inputs before initializing max31790.
> For example: set `pwm-as-tach = /bits/ 8 <2 5>` in DTS for max31790 and
> the driver will config PWMOUT2 and PWMOUT5 as TACH8 and TACH11.
>

Devicetree properties have to be documented in a property file
and have to be approved by a devicetree maintainer.

Personally I don't think this is the proper way of configuring this,
but I'll let devicetree maintainers decide.

> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@wiwynn.com>
> ---
> drivers/hwmon/max31790.c | 50 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 50 insertions(+)
>
> diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c
> index 0cd44c1e998a..0f8fe911539b 100644
> --- a/drivers/hwmon/max31790.c
> +++ b/drivers/hwmon/max31790.c
> @@ -480,6 +480,52 @@ static const struct hwmon_chip_info max31790_chip_info = {
> .info = max31790_info,
> };
>
> +static int max31790_config_pwm_as_tach(struct device *dev,
> + struct i2c_client *client)
> +{
> + struct device_node *np = dev->of_node;
> + int i, ret = 0, size, channel;
> + u8 pwm_index[NR_CHANNEL] = { 0 };
> + u8 fan_config;
> +
> + size = of_property_count_u8_elems(np, "pwm-as-tach");
> +
> + if ((size > 0) && (size <= NR_CHANNEL)) {

Please refrain from unnecessary ( ).

> + ret = of_property_read_u8_array(np, "pwm-as-tach", pwm_index,
> + size);
> + if (ret) {
> + dev_err(dev,
> + "Property 'pwm-as-tach' cannot be read.\n");
> + return ret;
> + }
> +
> + for (i = 0; i < size; i++) {
> + if ((pwm_index[i] == 0) ||
> + (pwm_index[i] > NR_CHANNEL)) {
> + continue;
> + }

Silently accepting bad data seems like a bad idea to me.

> +
> + channel = pwm_index[i] - 1;
> + fan_config = i2c_smbus_read_byte_data(
> + client, MAX31790_REG_FAN_CONFIG(channel));
> + if (fan_config < 0) {

An u8 is never < 0

> + dev_err(dev,
> + "Read fan config for channel %d failed.\n",
> + channel);
> + return fan_config;
> + }
> +
> + fan_config |= (MAX31790_FAN_CFG_CTRL_MON |
> + MAX31790_FAN_CFG_TACH_INPUT);

This assumes that the channel is configured as pwm.
What if the BIOS / ROMMON configured another channel which you want as
pwm channel as fan input channel ?

> + i2c_smbus_write_byte_data(
> + client, MAX31790_REG_FAN_CONFIG(channel),
> + fan_config);
> + }
> + }

Silently ignoring errors seems like a bad idea.

> +
> + return 0;
> +}
> +
> static int max31790_init_client(struct i2c_client *client,
> struct max31790_data *data)
> {
> @@ -521,6 +567,10 @@ static int max31790_probe(struct i2c_client *client)
> data->client = client;
> mutex_init(&data->update_lock);
>
> + err = max31790_config_pwm_as_tach(dev, client);
> + if (err)
> + dev_crit(dev, "Config PWM as TACH failed.\n");
> +
> /*
> * Initialize the max31790 chip
> */