Re: [PATCH v8 1/4] drivers: hwmon: Add max31760 fan speed controller driver

[Guenter Roeck]

On 9/9/22 06:37, Ibrahim Tilki wrote:
> MAX31760 is a precision fan speed controller with nonvolatile lookup table.
> Device has one internal and one external temperature sensor support.
> Controls two fans and measures their speeds. Generates hardware alerts when
> programmable max and critical temperatures are exceeded.
>
> Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@xxxxxxxxxx>
> Reviewed-by: Nurettin Bolucu <Nurettin.Bolucu@xxxxxxxxxx>

Since you'll have to resend this anyway to address the devicetree
failures:

[ ... ]

> +
> +static int max31760_write(struct device *dev, enum hwmon_sensor_types type,
> +			  u32 attr, int channel, long val)
> +{
> +	struct max31760_state *state = dev_get_drvdata(dev);
> +	unsigned int pwm_index;
> +	unsigned int reg_temp;
> +	int temp;
> +	u8 reg_val[2];
> +
> +	switch (type) {
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_max:
> +			reg_temp = REG_TEMP_MAX(channel);
> +			break;
> +		case hwmon_temp_crit:
> +			reg_temp = REG_TEMP_CRIT(channel);
> +			break;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +
> +		temp = TEMP11_TO_REG(val);
> +		reg_val[0] = temp >> 8;
> +		reg_val[1] = temp & 0xFF;
> +
> +		return regmap_bulk_write(state->regmap, reg_temp, reg_val, 2);
> +	case hwmon_fan:
> +		switch (attr) {
> +		case hwmon_fan_enable:
> +			if (val == 0)
> +				return regmap_clear_bits(state->regmap, REG_CR3, BIT(channel));
> +
> +			if (val == 1)
> +				return regmap_set_bits(state->regmap, REG_CR3, BIT(channel));
> +
> +			return -EOPNOTSUPP;

This should return -EINVAL.

> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +	case hwmon_pwm:
> +		switch (attr) {
> +		case hwmon_pwm_input:
> +			if (val < 0 || val > 255)
> +				return -EINVAL;
> +
> +			return regmap_write(state->regmap, REG_PWMR, val);
> +		case hwmon_pwm_enable:
> +			if (val == 1)
> +				return regmap_set_bits(state->regmap, REG_CR2, CR2_DFC);
> +
> +			if (val == 2)
> +				return regmap_clear_bits(state->regmap, REG_CR2, CR2_DFC);
> +
> +			return -EOPNOTSUPP;

This should return -EINVAL (the _operation_ is supported, after all,
but the written value is invalid).

Thanks,
Guenter