Re: [PATCH 2/3] hwmon: (amc6821) Move reading fan data from OF to a function

[Quentin Schulz <quentin.schulz@cherry.de>]

Hi João Paulo,

On 5/30/25 7:46 PM, João Paulo Gonçalves wrote:
> From: João Paulo Gonçalves <joao.goncalves@toradex.com>
> 
> Move fan property reading from OF to a separate function. This keeps OF
> data handling separate from the code logic and makes it easier to add
> features like cooling device support that use the same fan node.
> 
> Signed-off-by: João Paulo Gonçalves <joao.goncalves@toradex.com>
> ---
>   drivers/hwmon/amc6821.c | 58 +++++++++++++++++++++++++++++++------------------
>   1 file changed, 37 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
> index 13a789cc85d24da282430eb2d4edf0003617fe6b..a969fad803ae1abb05113ce15f2476e83df029d9 100644
> --- a/drivers/hwmon/amc6821.c
> +++ b/drivers/hwmon/amc6821.c
> @@ -126,6 +126,7 @@ module_param(init, int, 0444);
>   struct amc6821_data {
>   	struct regmap *regmap;
>   	struct mutex update_lock;
> +	enum pwm_polarity of_pwm_polarity;

Do we actually need to keep the information about the OF polarity?

Are you trying to handle runtime modification of the pwminv module 
parameter where we could set it to -1 to force reading from the Device 
Tree again? This seems to be possible, c.f. https://lwn.net/Articles/85443/

Otherwise I would have said we just need to store the "computed" 
polarity, the output of amc6821_pwm_polarity instead of going through 
the logic every time we ask for the polarity.

Justify in the commit log why we want to keep the OF value instead of 
the resolved one (with the module param one).

[...]

> @@ -938,13 +935,21 @@ static const struct regmap_config amc6821_regmap_config = {
>   	.cache_type = REGCACHE_MAPLE,
>   };
>   
> +static void amc6821_of_fan_put(void *data)
> +{
> +	struct device_node *fan_np = data;
> +
> +	of_node_put(fan_np);
> +}
> +
>   static int amc6821_probe(struct i2c_client *client)
>   {
>   	struct device *dev = &client->dev;
>   	struct amc6821_data *data;
>   	struct device *hwmon_dev;
>   	struct regmap *regmap;
> -	int err;
> +	struct device_node *fan_np;
> +	int err = 0;
>   
>   	data = devm_kzalloc(dev, sizeof(struct amc6821_data), GFP_KERNEL);
>   	if (!data)
> @@ -956,6 +961,17 @@ static int amc6821_probe(struct i2c_client *client)
>   				     "Failed to initialize regmap\n");
>   	data->regmap = regmap;
>   
> +	fan_np = of_get_child_by_name(dev->of_node, "fan");
> +	if (fan_np)
> +		err = devm_add_action_or_reset(dev, amc6821_of_fan_put, fan_np);
> +

This seems a bit overkill to me. If I'm not mistaken, we should be able 
to do something simpler such as:

fan_np = of_get_child_by_name(dev->of_node, "fan");
if (fan_np) {
     amc6821_of_fan_read_data(data, fan_np);
     of_node_put(fan_np);
}

(not tested) what do you think? What made you pick the 
devm_add_action_or_reset here? What am I missing :)

Cheers,
Quentin