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
>
