[PATCH v1 0/1] hwmon: max31790: support to config PWM as TACH

[PATCH v1 0/1] hwmon: max31790: support to config PWM as TACH

[Delphine CC Chiu]

v1 - Support to config PWM as TACH in max31790 driver

Delphine CC Chiu (1):
  hwmon: max31790: support to config PWM as TACH

 drivers/hwmon/max31790.c | 50 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

-- 
2.25.1

[PATCH v1 1/1] hwmon: max31790: support to config PWM as TACH

[Delphine CC Chiu]

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.

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)) {
+		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;
+			}
+
+			channel = pwm_index[i] - 1;
+			fan_config = i2c_smbus_read_byte_data(
+				client, MAX31790_REG_FAN_CONFIG(channel));
+			if (fan_config < 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);
+			i2c_smbus_write_byte_data(
+				client, MAX31790_REG_FAN_CONFIG(channel),
+				fan_config);
+		}
+	}
+
+	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
 	 */
-- 
2.25.1

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

[Delphine CC Chiu]

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.

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)) {
+		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;
+			}
+
+			channel = pwm_index[i] - 1;
+			fan_config = i2c_smbus_read_byte_data(
+				client, MAX31790_REG_FAN_CONFIG(channel));
+			if (fan_config < 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);
+			i2c_smbus_write_byte_data(
+				client, MAX31790_REG_FAN_CONFIG(channel),
+				fan_config);
+		}
+	}
+
+	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
 	 */
-- 
2.25.1

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

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

[Delphine_CC_Chiu/WYHQ/Wiwynn]

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Thursday, September 7, 2023 12:20 AM
> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>;
> patrick@stwcx.xyz; Jean Delvare <jdelvare@suse.com>
> Cc: linux-hwmon@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] hwmon: max31790: support to config PWM as TACH
> 
>   Security Reminder: Please be aware that this email is sent by an external
> sender.
> 
> 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.
> 
Thanks for your reply.
We will add document of max31790 in v2 patch.

> > 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 ( ).
> 
Will remove in the v2 patch.

> > +             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.
> 
Add error handling in v2 patch.

> > +
> > +                     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
> 
Use integer to get the return value first and set to fan_config in v2 patch.

> > +                             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 ?
> 
This will config the channel as TACH.
Could you provide more information about the scenario you mentioned?
In our system, there is only BMC that will set the config of fan device.
> > +                     i2c_smbus_write_byte_data(
> > +                             client,
> MAX31790_REG_FAN_CONFIG(channel),
> > +                             fan_config);
> > +             }
> > +     }
> 
> Silently ignoring errors seems like a bad idea.
> 
Add error handling in v2 patch.

> > +
> > +     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
> >        */

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

[Guenter Roeck]

On 9/13/23 01:51, Delphine_CC_Chiu/WYHQ/Wiwynn wrote:

[ ... ]

>>
>> 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 ?
>>
> This will config the channel as TACH.
> Could you provide more information about the scenario you mentioned?
> In our system, there is only BMC that will set the config of fan device.

Please keep in mind that upstream code is not intended to only support
your specific use case, but the use case of others as well. Your system
(the one you have today) may only use the chip for fan speed
measurement. Others may use it use it to control fans, or your hardware
might tomorrow build another system where the chip us used to control
the fans as well. Those use cases should be covered by your patch as well.

Guenter

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

[Delphine_CC_Chiu/WYHQ/Wiwynn]

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Wednesday, September 13, 2023 9:37 PM
> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>;
> patrick@stwcx.xyz; Jean Delvare <jdelvare@suse.com>
> Cc: linux-hwmon@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] hwmon: max31790: support to config PWM as TACH
> 
>   Security Reminder: Please be aware that this email is sent by an external
> sender.
> 
> On 9/13/23 01:51, Delphine_CC_Chiu/WYHQ/Wiwynn wrote:
> 
> [ ... ]
> 
> >>
> >> 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 ?
> >>
> > This will config the channel as TACH.
> > Could you provide more information about the scenario you mentioned?
> > In our system, there is only BMC that will set the config of fan device.
> 
> Please keep in mind that upstream code is not intended to only support your
> specific use case, but the use case of others as well. Your system (the one you
> have today) may only use the chip for fan speed measurement. Others may
> use it use it to control fans, or your hardware might tomorrow build another
> system where the chip us used to control the fans as well. Those use cases
> should be covered by your patch as well.
> 
> Guenter
Hi Guenter,
After our internal discussion, we thought that the PWM output channel of
max31790 should either be PWM output or TACH input in hardware design.
And the property "pwm-as-tach" is optional, if users don't want to config
any PWM output channel as TACH input, then they don't need to add this
property in DTS.
Would like to know is this meet your expectations?

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

[kernel test robot]

Hi Delphine,

kernel test robot noticed the following build warnings:

[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on linus/master v6.5 next-20230908]
[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#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Delphine-CC-Chiu/hwmon-max31790-support-to-config-PWM-as-TACH/20230906-165344
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link:    https://lore.kernel.org/r/20230906084837.3043030-3-Delphine_CC_Chiu%40wiwynn.com
patch subject: [PATCH] hwmon: max31790: support to config PWM as TACH
config: x86_64-randconfig-161-20230907 (https://download.01.org/0day-ci/archive/20230909/202309090228.2bSFmAO8-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230909/202309090228.2bSFmAO8-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309090228.2bSFmAO8-lkp@intel.com/

smatch warnings:
drivers/hwmon/max31790.c:511 max31790_config_pwm_as_tach() warn: unsigned 'fan_config' is never less than zero.

vim +/fan_config +511 drivers/hwmon/max31790.c

   482	
   483	static int max31790_config_pwm_as_tach(struct device *dev,
   484					       struct i2c_client *client)
   485	{
   486		struct device_node *np = dev->of_node;
   487		int i, ret = 0, size, channel;
   488		u8 pwm_index[NR_CHANNEL] = { 0 };
   489		u8 fan_config;
   490	
   491		size = of_property_count_u8_elems(np, "pwm-as-tach");
   492	
   493		if ((size > 0) && (size <= NR_CHANNEL)) {
   494			ret = of_property_read_u8_array(np, "pwm-as-tach", pwm_index,
   495							size);
   496			if (ret) {
   497				dev_err(dev,
   498					"Property 'pwm-as-tach' cannot be read.\n");
   499				return ret;
   500			}
   501	
   502			for (i = 0; i < size; i++) {
   503				if ((pwm_index[i] == 0) ||
   504				    (pwm_index[i] > NR_CHANNEL)) {
   505					continue;
   506				}
   507	
   508				channel = pwm_index[i] - 1;
   509				fan_config = i2c_smbus_read_byte_data(
   510					client, MAX31790_REG_FAN_CONFIG(channel));
 > 511				if (fan_config < 0) {
   512					dev_err(dev,
   513						"Read fan config for channel %d failed.\n",
   514						channel);
   515					return fan_config;
   516				}
   517	
   518				fan_config |= (MAX31790_FAN_CFG_CTRL_MON |
   519					       MAX31790_FAN_CFG_TACH_INPUT);
   520				i2c_smbus_write_byte_data(
   521					client, MAX31790_REG_FAN_CONFIG(channel),
   522					fan_config);
   523			}
   524		}
   525	
   526		return 0;
   527	}
   528	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 0/1] hwmon: max31790: support to config PWM as TACH

[Guenter Roeck]

On 9/6/23 01:48, Delphine CC Chiu wrote:
> v1 - Support to config PWM as TACH in max31790 driver
> 
> Delphine CC Chiu (1):
>    hwmon: max31790: support to config PWM as TACH
> 
>   drivers/hwmon/max31790.c | 50 ++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 50 insertions(+)
> 

An introduction email is not necessary for a single patch.
_If_ you insist sending it anyway, it should contain the rationale.

Please do not send both numbered and unnumbered patches like you
did. I have no idea which one is current.

Guenter