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

[Guenter Roeck]

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@xxxxxxxxxx>
> ---
>   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
>   	 */