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

[Javier Carrasco <javier.carrasco.cruz@gmail.com>]

On 09.11.23 15:55, Guenter Roeck wrote:
> On 11/9/23 00:52, Krzysztof Kozlowski wrote:
>> On 08/11/2023 16:37, Javier Carrasco wrote:
>>> The Amphenol 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
>>>
>>> This driver adds support for the digital variant (CC2D part numbers),
>>> which is also divided into two subfamilies [1]:
>>> - CC2DXX: non-sleep measurement mode
>>> - CC2DXXS: sleep measurement mode
>>
>> ...
>>
>>> +
>>> +static int cc2_probe(struct i2c_client *client)
>>> +{
>>> +    struct cc2_data *data;
>>> +    struct device *dev = &client->dev;
>>> +    enum cc2_ids chip;
>>> +    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;
>>> +
>>> +    if (client->dev.of_node)
>>> +        chip = (uintptr_t)of_device_get_match_data(&client->dev);
>>> +    else
>>> +        chip = i2c_match_id(cc2_id, client)->driver_data;
>>> +
>>> +    data->config = &cc2_config[chip];
>>> +
>>> +    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);
> 
> fwiw, s/retrive/retrieve/g
ack.
> 
>>> +        if (ret)
>>> +            goto cleanup;
>>> +    } else {
>>> +        /* No access to EEPROM without regulator: no alarm control */
>>> +        goto dev_register;
>>
>> Nothing improved here.
>>
>> Do not send new version of patchset before discussion finishes.
>>
> 
> This driver will take a while to review due to its complexity.
That is absolutely ok. I will wait for a few days to gather more
feedback and hopefully send less versions.
> 
> As for the code above: Error handling goes first. Something like
> the above, where the error case is just a goto, is unacceptable and
> just increases indentation level for the other code and makes it
> more difficult to read. Also, the above code _will_ have to handle
> error cases other than -ENODEV. Besides deferred probe, it is
> completely inappropriate to ignore -EINVAL or -ENOMEM or any other
> error codes other than -ENODEV.
> 
The probe function will return from errors other than -ENODEV directly
with no goto and checking them first.
I just need to skip the alarm registration for -ENODEV and do the
cleanup if an error occurs after enabling the regulator to keep
enable/disable parity.
>>> +    }
>>> +
>>> +    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)) {
>>> +        ret = PTR_ERR(data->hwmon);
>>> +        goto cleanup;
>>> +    }
>>> +
>>> +    return 0;
>>> +
>>> +cleanup:
>>> +    if (cc2_disable(data))
>>> +        dev_dbg(dev, "Failed to disable device");
>>> +
>>> +    return dev_err_probe(dev, ret,
>>> +                 "Unable to register hwmon device\n");
>>
>> Drop or move to each error path.
>>
> This actually follows Documentation/process/coding-style.rst, chapter 7
> (Centralized exiting of functions).
> 
> Guenter
> 
Thanks for your comments.

Best regards,
Javier Carrasco