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

[Javier Carrasco]

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