linux-kernel - Re: [PATCH v1 2/2] hwmon: Driver for Nuvoton NCT736X

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <eff4defd-dfd5-448b-9056-d2f711f14018@linaro.org>
Date:   Mon, 4 Dec 2023 09:06:45 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     baneric926@...il.com, jdelvare@...e.com, linux@...ck-us.net,
        robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        conor+dt@...nel.org, corbet@....net
Cc:     linux-hwmon@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
        openbmc@...ts.ozlabs.org, kwliu@...oton.com, kcfeng0@...oton.com,
        DELPHINE_CHIU@...ynn.com, Bonnie_Lo@...ynn.com
Subject: Re: [PATCH v1 2/2] hwmon: Driver for Nuvoton NCT736X

On 04/12/2023 06:56, baneric926@...il.com wrote:
> From: Ban Feng <kcfeng0@...oton.com>
> 
> NCT736X is an I2C based hardware monitoring chip from Nuvoton.
> 
> Signed-off-by: Ban Feng <kcfeng0@...oton.com>
> ---


> +
> +static const struct i2c_device_id nct736x_id[] = {
> +	{"nct7362", nct7362},
> +	{"nct7363", nct7363},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, nct736x_id);
> +

All ID tables are next to each other. Move it down. Why does it not
match of_device_id?

...

> +
> +static int nct736x_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct nct736x_data *data;
> +	struct device *hwmon_dev;
> +	u32 pwm_mask, fanin_mask, val, wdt_cfg;
> +	int ret;
> +
> +	data = devm_kzalloc(dev, sizeof(struct nct736x_data), GFP_KERNEL);

sizeof(*)

> +	if (!data)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, data);
> +	mutex_init(&data->update_lock);
> +
> +	data->client = client;
> +
> +	if (of_property_read_u32(dev->of_node, "nuvoton,pwm-mask", &pwm_mask))
> +		pwm_mask = 0;
> +	if (of_property_read_u32(dev->of_node,
> +				 "nuvoton,fanin-mask", &fanin_mask))
> +		fanin_mask = 0;
> +	if (of_property_read_u32(dev->of_node, "nuvoton,wdt-timeout", &val))
> +		wdt_cfg = 0xff;
> +	else
> +		wdt_cfg = WDT_CFG(val) | EN_WDT;
> +
> +	/* Initialize the chip */
> +	ret = nct736x_init_chip(client, pwm_mask, fanin_mask, wdt_cfg);
> +	if (ret)
> +		return ret;
> +
> +	data->fan_mask = (u16)fanin_mask;
> +	data->pwm_mask = (u16)pwm_mask;
> +
> +	data = nct736x_update_device(dev);
> +
> +	data->groups[0] = &nct736x_group_fan;
> +	data->groups[1] = &nct736x_group_pwm;
> +	data->groups[2] = NULL;
> +
> +	hwmon_dev = devm_hwmon_device_register_with_groups(dev,
> +							   client->name,
> +							   data, data->groups);
> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct of_device_id nct736x_of_match[] = {
> +	{ .compatible = "nuvoton,nct7362" },
> +	{ .compatible = "nuvoton,nct7363" },

This means your devices are compatible. Express compatibility in your
bindings (specific compatible followed by fallback). But then your
i2c_device_id is not matching this one here... confusing and clearly wrong.

Best regards,
Krzysztof