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
