Re: [PATCH v4 5/7] hwmon: add driver for the hwmon parts of qnap-mcu devices

["Heiko Stübner" <heiko@sntech.de>]

Hi Guenter,

Am Dienstag, 13. August 2024, 18:03:57 CEST schrieb Guenter Roeck:
> On 8/10/24 11:47, Heiko Stuebner wrote:

> > +static int qnap_mcu_hwmon_set_pwm(struct qnap_mcu_hwmon *hwm, u8 pwm)
> > +{
> > +	const u8 cmd[] = {
> > +		[0] = 0x40, /* @ */
> > +		[1] = 0x46, /* F */
> > +		[2] = 0x57, /* W */
> > +		[3] = 0x30, /* 0 ... fan-id? */
> > +		[4] = pwm
> > +	};
> > +
> > +	/* set the fan pwm */
> > +	return qnap_mcu_exec_with_ack(hwm->mcu, cmd, sizeof(cmd));
> > +}

> > +static int qnap_mcu_hwmon_get_cooling_data(struct device *dev, struct qnap_mcu_hwmon *hwm)
> > +{
> > +	struct fwnode_handle *fwnode;
> > +	int num, i, ret;
> > +
> > +	fwnode = device_get_named_child_node(dev->parent, "fan-0");
> 
> Is the dummy "-0" index mandated somewhere ?

I don't think it is. The node should just begin with "fan" I think.

I've added the -0 because from everything I've seen of the qnap software-
side, the mcu firmware can address multiple fans.

In the original firmware, everything is done in userspace wrt. the MCU,
and the fan commands in their abstraction layer allow for multiple fans.

Also there is that suspicious "0" in the command sequence when
getting/setting the fan pwm. And in the original device config this is
labeled as the first fan.

From everything I've seen, the MCU is not limited to the Rockchip-line
of devices and I do hope others will find this useful in the future,
so adding the "-0" was a better safe than sorry choice.

Because that way adding that theoretical 2nd fan in the future won't
cause too much trouble in the dt-binding.


> I don't care either way, it just seems odd. Either case,
> 
> Acked-by: Guenter Roeck <linux@roeck-us.net>
> 
> > +	if (!fwnode)
> > +		return 0;
> > +
> > +	/* if we found the fan-node, we're keeping it until device-unbind */
> > +	hwm->fan_node = fwnode;
> > +	ret = devm_add_action_or_reset(dev, devm_fan_node_release, hwm);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!fwnode_property_present(fwnode, "cooling-levels"))
> > +		return 0;
> > +
> 
> Side note: One could argue that a sub-node with no content does not really
> make sense and should be invalid.

I remember thinking about that, but didn't come to a real decision on my
own, hence left it as it was. So will follow your suggestion :-)


> > +	ret = fwnode_property_count_u32(fwnode, "cooling-levels");
> > +	if (ret <= 0) {
> > +		dev_err(dev, "Failed to count elements in 'cooling-levels'\n");
> > +		return ret ? : -EINVAL;
> > +	}
> > +
> > +	num = ret;
> 
> Another side note: Using ret here isn't necessary. I'd just have used
> 'num' directly.

will do

> 
> > +	hwm->fan_cooling_levels = devm_kcalloc(dev, num, sizeof(u32),
> > +					       GFP_KERNEL);
> > +	if (!hwm->fan_cooling_levels)
> > +		return -ENOMEM;
> > +
> > +	ret = fwnode_property_read_u32_array(fwnode, "cooling-levels",
> > +					     hwm->fan_cooling_levels, num);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to read 'cooling-levels'\n");
> > +		return ret;
> > +	}
> > +
> > +	for (i = 0; i < num; i++) {
> > +		if (hwm->fan_cooling_levels[i] > hwm->pwm_max) {
> > +			dev_err(dev, "fan state[%d]:%d > %d\n", i,
> > +				hwm->fan_cooling_levels[i], hwm->pwm_max);
> > +			return -EINVAL;
> 
> In case you send another version, you might want to consider using dev_err_probe().

ok will do.

I was probably way overthinking if I should not use dev_err_probe in a
function that is not a probe function (though of course part of the probe
process).


Thanks a lot for looking over the code once again
Heiko