linux-kernel - Re: [PATCH 3/4] hwmon: Add support for Amphenol ChipCap 2

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: <e58cdedb-1825-4713-9d3f-5239bb182230@linaro.org>
Date:   Wed, 8 Nov 2023 13:41:46 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Javier Carrasco <javier.carrasco.cruz@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Jean Delvare <jdelvare@...e.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Jonathan Corbet <corbet@....net>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>
Cc:     Rob Herring <robh@...nel.org>, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org,
        linux-doc@...r.kernel.org
Subject: Re: [PATCH 3/4] hwmon: Add support for Amphenol ChipCap 2

On 08/11/2023 13:29, Javier Carrasco wrote:
> The Telaire ChipCap 2 is a capacitive polymer humidity and temperature
> sensor with an integrated EEPROM and minimum/maximum humidity alarms.
> 
> All device variants offer an I2C interface and depending on the part
> number, two different output modes:
> - CC2D: digital output
> - CC2A: analog (PDM) output

Thank you for your patch. There is something to discuss/improve.


> diff --git a/MAINTAINERS b/MAINTAINERS
> index dd5de540ec0b..63361104469f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21572,6 +21572,14 @@ F:	Documentation/devicetree/bindings/media/i2c/ti,ds90*
>  F:	drivers/media/i2c/ds90*
>  F:	include/media/i2c/ds90*
>  
> +TI CHIPCAP 2 HUMIDITY-TEMPERATURE IIO DRIVER

Why this is TI?

Bindings say Amphenol. Subject as well. Commit msg says Telaire. Here
you write Texas Instruments.

Three different companies used. How possibly we could understand this?


> +M:	Javier Carrasco <javier.carrasco.cruz@...il.com>
> +L:	linux-hwmon@...r.kernel.org
> +S:	Maintained

...

> +
> +/* Command mode is only accessible in the first 10 ms after power-up, but the
> + * device does not provide any kind of reset. In order to access the command
> + * mode during normal operation, a power cycle must be triggered.
> + */


Please use full comment style, as described in Coding Style document.

...

> +
> +static const struct hwmon_ops cc2_hwmon_ops = {
> +	.is_visible = cc2_is_visible,
> +	.read = cc2_read,
> +	.write = cc2_write,
> +};
> +
> +static const struct hwmon_chip_info cc2_chip_info = {
> +	.ops = &cc2_hwmon_ops,
> +	.info = cc2_info,
> +};
> +
> +static const struct cc2_config cc2dxx_config = {
> +	.measurement = cc2dxx_meas,
> +};
> +
> +static const struct cc2_config cc2dxxs_config = {
> +	.measurement = cc2dxxs_meas,
> +};
> +
> +static const struct of_device_id cc2_of_match[] = {
> +	{ .compatible = "amphenol,cc2dxx",
> +	  .data = &cc2dxx_config,
> +	},
> +	{ .compatible = "amphenol,cc2dxxs",

Format it as in other sources. Don't introduce your own codings style.

> +	  .data = &cc2dxxs_config,
> +	},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, cc2_of_match);

Keep ID tables together.

> +
> +static int cc2_probe(struct i2c_client *client)
> +{
> +	struct cc2_data *data;
> +	struct device *dev = &client->dev;
> +	const struct of_device_id *match;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> +		return -EOPNOTSUPP;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, data);
> +
> +	mutex_init(&data->i2c_lock);
> +	mutex_init(&data->alarm_lock);
> +
> +	data->client = client;
> +
> +	match = i2c_of_match_device(cc2_of_match, client);
> +	if (!match)
> +		return -ENODEV;
> +
> +	data->config = match->data;
> +
> +	ret = cc2_request_ready_irq(data, dev);
> +	if (ret)
> +		return ret;
> +
> +	data->regulator = devm_regulator_get_optional(dev, "vdd");
> +	if (!IS_ERR(data->regulator)) {
> +		ret = cc2_retrive_alarm_config(data);
> +		if (ret)
> +			goto cleanup;
> +	} else {
> +		/* No access to EEPROM without regulator: no alarm control */

Test your code with deferred probe. Are you sure you handle it
correctly? To me, it looks like you handle deferred probe the same as
any error.

> +		goto dev_register;
> +	}
> +
> +	ret = cc2_request_alarm_irqs(data, dev);
> +	if (ret)
> +		goto cleanup;
> +
> +dev_register:
> +	data->hwmon = devm_hwmon_device_register_with_info(dev, client->name,
> +							   data, &cc2_chip_info,
> +							   NULL);
> +	if (IS_ERR(data->hwmon))
> +		return dev_err_probe(dev, PTR_ERR(data->hwmon),
> +				     "Unable to register hwmon device\n");
> +
> +	return 0;
> +
> +cleanup:
> +	if (cc2_disable(data))
> +		dev_dbg(dev, "Failed to disable device");
> +
> +	return ret;
> +}
> +
> +static void cc2_remove(struct i2c_client *client)
> +{
> +	struct cc2_data *data = i2c_get_clientdata(client);
> +	int ret = cc2_disable(data);
> +
> +	if (ret)
> +		dev_dbg(&client->dev, "Failed to disable device");
> +}
> +
> +static const struct i2c_device_id cc2_id[] = { { "chipcap2", 0 }, {} };

Use style like in other files.
git grep i2c_device_id

BTW, having mismatched entries looks error-prone. Why do you even need
i2c_device_id if it is not matching of_device_id?

Best regards,
Krzysztof