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

[Javier Carrasco]

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@xxxxxxxxx>
>> +L:	linux-hwmon@xxxxxxxxxxxxxxx
>> +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
>