mbox series

[0/4] hwmon: Add support for Amphenol ChipCap 2

Message ID 20231020-topic-chipcap2-v1-0-087e21d4b1ed@gmail.com
Headers show
Series hwmon: Add support for Amphenol ChipCap 2 | expand

Message

Javier Carrasco Nov. 8, 2023, 12:29 p.m. UTC 
This series adds support and documentation for the Amphenol ChipCap 2
humidity and temperature sensor in its digital version.

This I2C device provides 14-bit humidity and temperature measurements as
well as low (minimum) and high (maximum) humidity alarms. A ready signal
is also available to reduce delays while fetching data.

The proposed driver implements the logic to perform measurements with
and without the ready signal, EEPROM configuration and alarm signaling.

The features this driver does not support (I2C address and command
window length modification) have been documented in the "Known Issues"
section.

The complete supported functionality has been tested with a CC2D33S
sensor (a 'sleep' device) connected to a Raspberry Pi Zero 2 w.
Different device tree node definitions (with and without regulator,
ready and/or alarm signals) have been positively tested.

The non-sleep measurement mechanism has been inferred from the first
measurement, which is carried out automatically and it is common for all
part numbers. Any testing or improvements with a non-sleep device is
more than welcome.

The tests have also covered the properties added to the hwmon core to
account for minimum and maximum humidity alarms.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
Javier Carrasco (4):
      dt-bindings: vendor-prefixes: add Amphenol
      hwmon: (core) Add support for humidity min/max alarm
      hwmon: Add support for Amphenol ChipCap 2
      dt-bindings: hwmon: Add Amphenol ChipCap 2

 .../bindings/hwmon/amphenol,chipcap2.yaml          |   72 ++
 .../devicetree/bindings/vendor-prefixes.yaml       |    2 +
 Documentation/hwmon/chipcap2.rst                   |   73 ++
 Documentation/hwmon/index.rst                      |    1 +
 MAINTAINERS                                        |    8 +
 drivers/hwmon/Kconfig                              |   10 +
 drivers/hwmon/Makefile                             |    1 +
 drivers/hwmon/chipcap2.c                           | 1009 ++++++++++++++++++++
 drivers/hwmon/hwmon.c                              |    2 +
 include/linux/hwmon.h                              |    4 +
 10 files changed, 1182 insertions(+)
---
base-commit: ffc253263a1375a65fa6c9f62a893e9767fbebfa
change-id: 20231020-topic-chipcap2-e2d8985430c2

Best regards,

Comments

Krzysztof Kozlowski Nov. 8, 2023, 12:41 p.m. UTC | #1 
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@gmail.com>
> +L:	linux-hwmon@vger.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
Javier Carrasco Nov. 8, 2023, 4:35 p.m. UTC | #2 
On 08.11.23 13:41, Krzysztof Kozlowski wrote:
> 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@gmail.com>
>> +L:	linux-hwmon@vger.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.
> 
The -EPROBE_DEFER is propagated to the probe function and it is the
returned value. I clarified the error path in v2 so no error messages
are displayed in that case, going directly to the dev_err_probe in the
probe cleanup.
When the EPROBE_DEFER error is returned, the probe function is deferred
and called again later on, which is the desired behavior.

>> +		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
>
Krzysztof Kozlowski Nov. 9, 2023, 8:40 a.m. UTC | #3 
On 08/11/2023 17:35, Javier Carrasco wrote:
>>> +
>>> +	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.
>>
> The -EPROBE_DEFER is propagated to the probe function and it is the
> returned value. I clarified the error path in v2 so no error messages

Really?

I see:
if (!IS_ERR(data->regulator)) {
	// so you do not go here
} else {
	goto dev_register;
}
dev_register is not error path. So how do you return EPROBE_DEFER?

Which line of code does it?

> are displayed in that case, going directly to the dev_err_probe in the
> probe cleanup.
> When the EPROBE_DEFER error is returned, the probe function is deferred
> and called again later on, which is the desired behavior.
> 


Best regards,
Krzysztof
Javier Carrasco Nov. 9, 2023, 8:59 a.m. UTC | #4 
On 09.11.23 09:40, Krzysztof Kozlowski wrote:
> On 08/11/2023 17:35, Javier Carrasco wrote:
>>>> +
>>>> +	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.
>>>
>> The -EPROBE_DEFER is propagated to the probe function and it is the
>> returned value. I clarified the error path in v2 so no error messages
> 
> Really?
> 
> I see:
> if (!IS_ERR(data->regulator)) {
> 	// so you do not go here
> } else {
> 	goto dev_register;
> }
> dev_register is not error path. So how do you return EPROBE_DEFER?
> 
> Which line of code does it?
> 
EPROBE_DEFER is returned if the command window was missed, which is
checked in cc2_retrieve_alarm_config() (there is a typo I just corrected
-> cc2_retrive_alarm_config() in the current version). It could then
happen where you added a comment, but not because
devm_regulator_get_optional() failed.

Are you expecting a probe deferring if devm_regulator_get_optional()
fails as well? Like if the regulator is still not ready when the
function is called.
>> are displayed in that case, going directly to the dev_err_probe in the
>> probe cleanup.
>> When the EPROBE_DEFER error is returned, the probe function is deferred
>> and called again later on, which is the desired behavior.
>>
> 
> 
> Best regards,
> Krzysztof
> 
Best regards,
Javier Carrasco
Krzysztof Kozlowski Nov. 9, 2023, 9:35 a.m. UTC | #5 
On 09/11/2023 09:59, Javier Carrasco wrote:
> 
> 
> On 09.11.23 09:40, Krzysztof Kozlowski wrote:
>> On 08/11/2023 17:35, Javier Carrasco wrote:
>>>>> +
>>>>> +	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.
>>>>
>>> The -EPROBE_DEFER is propagated to the probe function and it is the
>>> returned value. I clarified the error path in v2 so no error messages
>>
>> Really?
>>
>> I see:
>> if (!IS_ERR(data->regulator)) {
>> 	// so you do not go here
>> } else {
>> 	goto dev_register;
>> }
>> dev_register is not error path. So how do you return EPROBE_DEFER?
>>
>> Which line of code does it?
>>
> EPROBE_DEFER is returned if the command window was missed, which is

How "command window was missed" is related to the place I commented?

> checked in cc2_retrieve_alarm_config() (there is a typo I just corrected
> -> cc2_retrive_alarm_config() in the current version). It could then
> happen where you added a comment, but not because
> devm_regulator_get_optional() failed.
> 
> Are you expecting a probe deferring if devm_regulator_get_optional()
> fails as well? Like if the regulator is still not ready when the
> function is called.

We talk only about this place. Not others.


Best regards,
Krzysztof
Javier Carrasco Nov. 9, 2023, 9:52 a.m. UTC | #6 
On 09.11.23 10:35, Krzysztof Kozlowski wrote:
> On 09/11/2023 09:59, Javier Carrasco wrote:
>>
>>
>> On 09.11.23 09:40, Krzysztof Kozlowski wrote:
>>> On 08/11/2023 17:35, Javier Carrasco wrote:
>>>>>> +
>>>>>> +	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.
>>>>>
>>>> The -EPROBE_DEFER is propagated to the probe function and it is the
>>>> returned value. I clarified the error path in v2 so no error messages
>>>
>>> Really?
>>>
>>> I see:
>>> if (!IS_ERR(data->regulator)) {
>>> 	// so you do not go here
>>> } else {
>>> 	goto dev_register;
>>> }
>>> dev_register is not error path. So how do you return EPROBE_DEFER?
>>>
>>> Which line of code does it?
>>>
>> EPROBE_DEFER is returned if the command window was missed, which is
> 
> How "command window was missed" is related to the place I commented?
> 
it is right below the comment you added and hence the misunderstanding.
But focusing on the line where your comment is, there is no probe
deferring in that case. This is why I asked if you were talking about
devm_regulator_get_optional() failing, which is not covered by the
deferring mechanism in the current form.

I have never experienced the case where the regulator was still not
available, but I suppose there is no reason why that should never happen.
The regulator is not mandatory and there is no reason to retry if it is
not defined. But in case it is defined and not available, the deferring
would make sense. I could consider that case as well.
>> checked in cc2_retrieve_alarm_config() (there is a typo I just corrected
>> -> cc2_retrive_alarm_config() in the current version). It could then
>> happen where you added a comment, but not because
>> devm_regulator_get_optional() failed.
>>
>> Are you expecting a probe deferring if devm_regulator_get_optional()
>> fails as well? Like if the regulator is still not ready when the
>> function is called.
> 
> We talk only about this place. Not others.
> 
> 
> Best regards,
> Krzysztof
> 
Best regards,
Javier Carrasco
Krzysztof Kozlowski Nov. 9, 2023, 10:23 a.m. UTC | #7 
On 09/11/2023 10:52, Javier Carrasco wrote:
> On 09.11.23 10:35, Krzysztof Kozlowski wrote:
>> On 09/11/2023 09:59, Javier Carrasco wrote:
>>>
>>>
>>> On 09.11.23 09:40, Krzysztof Kozlowski wrote:
>>>> On 08/11/2023 17:35, Javier Carrasco wrote:
>>>>>>> +
>>>>>>> +	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.
>>>>>>
>>>>> The -EPROBE_DEFER is propagated to the probe function and it is the
>>>>> returned value. I clarified the error path in v2 so no error messages
>>>>
>>>> Really?
>>>>
>>>> I see:
>>>> if (!IS_ERR(data->regulator)) {
>>>> 	// so you do not go here
>>>> } else {
>>>> 	goto dev_register;
>>>> }
>>>> dev_register is not error path. So how do you return EPROBE_DEFER?
>>>>
>>>> Which line of code does it?
>>>>
>>> EPROBE_DEFER is returned if the command window was missed, which is
>>
>> How "command window was missed" is related to the place I commented?
>>
> it is right below the comment you added and hence the misunderstanding.
> But focusing on the line where your comment is, there is no probe
> deferring in that case. This is why I asked if you were talking about
> devm_regulator_get_optional() failing, which is not covered by the
> deferring mechanism in the current form.
> 
> I have never experienced the case where the regulator was still not
> available, but I suppose there is no reason why that should never happen.

Defer on regulators, just like several other resources, is quite likely,
so all code must be ready for this.

> The regulator is not mandatory and there is no reason to retry if it is
> not defined. But in case it is defined and not available, the deferring
> would make sense. I could consider that case as well.

Your code should consider it always.


Best regards,
Krzysztof