Messages in this thread | | | Date | Wed, 6 Sep 2023 09:20:29 -0700 | From | Guenter Roeck <> | 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@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 > */
| |