Re: [PATCH v6 2/2] hwmon: Add driver for I2C chip Nuvoton NCT7363Y

[Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>]

Le 22/10/2024 à 07:29, baneric926@xxxxxxxxx a écrit :
> From: Ban Feng <kcfeng0@xxxxxxxxxxx>
>
> The NCT7363Y is a fan controller which provides up to 16
> independent FAN input monitors. It can report each FAN input count
> values. The NCT7363Y also provides up to 16 independent PWM
> outputs. Each PWM can output specific PWM signal by manual mode to
> control the FAN duty outside.
>
> Signed-off-by: Ban Feng <kcfeng0@xxxxxxxxxxx>
> ---

Hi,

a few nitpick, should there be a v7 and if they make sense to you.

> +static const struct of_device_id nct7363_of_match[] = {
> +	{ .compatible = "nuvoton,nct7363", },
> +	{ .compatible = "nuvoton,nct7362", },
> +	{ },

Usually, a comma is not added after a terminator entry.

> +};
> +MODULE_DEVICE_TABLE(of, nct7363_of_match);
> +
> +struct nct7363_data {
> +	struct regmap		*regmap;
> +
> +	u16			fanin_mask;
> +	u16			pwm_mask;
> +};
> +
> +static int nct7363_read_fan(struct device *dev, u32 attr, int channel,
> +			    long *val)
> +{
> +	struct nct7363_data *data = dev_get_drvdata(dev);
> +	unsigned int reg;
> +	u8 regval[2];
> +	int ret = 0;

No need to init.

> +	u16 cnt;
> +
> +	switch (attr) {
> +	case hwmon_fan_input:
> +		/*
> +		 * High-byte register should be read first to latch
> +		 * synchronous low-byte value
> +		 */
> +		ret = regmap_bulk_read(data->regmap,
> +				       NCT7363_REG_FANINX_HVAL(channel),
> +				       &regval, 2);
> +		if (ret)
> +			return ret;
> +
> +		cnt = (regval[0] << 5) | (regval[1] & NCT7363_FANINX_LVAL_MASK);
> +		*val = fan_from_reg(cnt);
> +		return 0;
> +	case hwmon_fan_min:
> +		ret = regmap_bulk_read(data->regmap,
> +				       NCT7363_REG_FANINX_HL(channel),
> +				       &regval, 2);
> +		if (ret)
> +			return ret;
> +
> +		cnt = (regval[0] << 5) | (regval[1] & NCT7363_FANINX_LVAL_MASK);
> +		*val = fan_from_reg(cnt);
> +		return 0;
> +	case hwmon_fan_alarm:
> +		ret = regmap_read(data->regmap,
> +				  NCT7363_REG_LSRS(channel), &reg);
> +		if (ret)
> +			return ret;
> +
> +		*val = (long)ALARM_SEL(reg, channel) > 0 ? 1 : 0;
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}

...

> +static int nct7363_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct device_node *child;
> +	struct nct7363_data *data;
> +	struct device *hwmon_dev;
> +	int ret;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->regmap = devm_regmap_init_i2c(client, &nct7363_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return PTR_ERR(data->regmap);
> +
> +	for_each_child_of_node(dev->of_node, child) {

for_each_child_of_node_scoped() to slightly simplify code and save a few 
lines?

> +		ret = nct7363_present_pwm_fanin(dev, child, data);
> +		if (ret) {
> +			of_node_put(child);
> +			return ret;
> +		}
> +	}
> +
> +	/* Initialize the chip */
> +	ret = nct7363_init_chip(data);
> +	if (ret)
> +		return ret;
> +
> +	hwmon_dev =
> +		devm_hwmon_device_register_with_info(dev, client->name, data,
> +						     &nct7363_chip_info, NULL);

return devm_hwmon_device_register_with_info()?

> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}

...

CJ