[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8a3b41a2-0ab2-6213-04a1-54ab7263c647@roeck-us.net>
Date: Wed, 6 Sep 2023 09:20:29 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Delphine CC Chiu <Delphine_CC_Chiu@...ynn.com>, patrick@...cx.xyz,
Jean Delvare <jdelvare@...e.com>
Cc: linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] hwmon: max31790: support to config PWM as TACH
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@...ynn.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
> */
Powered by blists - more mailing lists