[no subject]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxx>
> 
> > +	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






[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux