[PATCH 0/2] hwmon: chipcap2: small improvements in probe function

[PATCH 0/2] hwmon: chipcap2: small improvements in probe function

[Javier Carrasco]

These modifications only affect error paths, simplifying a case where
dev_err_probe() could be returned, and disabling the sensor if getting
the ready interrupt fails.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
Javier Carrasco (2):
      hwmon: chipcap2: return dev_err_probe if get regulator fails
      hwmon: chipcap2: disable sensor if request ready irq fails

 drivers/hwmon/chipcap2.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)
---
base-commit: 9e6869691724b12e1f43655eeedc35fade38120c
change-id: 20240812-chipcap2-probe-improvements-c94d1431f1ef

Best regards,
-- 
Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH 1/2] hwmon: chipcap2: return dev_err_probe if get regulator fails

[Javier Carrasco]

The error value can be directly returned via dev_err_probe(), and there
is no need to split that into two instructions.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/hwmon/chipcap2.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
index 6ccceae21f70..88689f4eb598 100644
--- a/drivers/hwmon/chipcap2.c
+++ b/drivers/hwmon/chipcap2.c
@@ -740,11 +740,9 @@ static int cc2_probe(struct i2c_client *client)
 	data->client = client;
 
 	data->regulator = devm_regulator_get_exclusive(dev, "vdd");
-	if (IS_ERR(data->regulator)) {
-		dev_err_probe(dev, PTR_ERR(data->regulator),
-			      "Failed to get regulator\n");
-		return PTR_ERR(data->regulator);
-	}
+	if (IS_ERR(data->regulator))
+		return dev_err_probe(dev, PTR_ERR(data->regulator),
+				     "Failed to get regulator\n");
 
 	ret = cc2_request_ready_irq(data, dev);
 	if (ret) {

-- 
2.43.0

[PATCH 2/2] hwmon: chipcap2: disable sensor if request ready irq fails

[Javier Carrasco]

This check is carried out after getting the regulator, and the device
can be disabled if an error occurs.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/hwmon/chipcap2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
index 88689f4eb598..02764689ed21 100644
--- a/drivers/hwmon/chipcap2.c
+++ b/drivers/hwmon/chipcap2.c
@@ -747,7 +747,7 @@ static int cc2_probe(struct i2c_client *client)
 	ret = cc2_request_ready_irq(data, dev);
 	if (ret) {
 		dev_err_probe(dev, ret, "Failed to request ready irq\n");
-		return ret;
+		goto disable;
 	}
 
 	ret = cc2_request_alarm_irqs(data, dev);

-- 
2.43.0

Re: [PATCH 2/2] hwmon: chipcap2: disable sensor if request ready irq fails

[Guenter Roeck]

On 8/12/24 08:43, Javier Carrasco wrote:
> This check is carried out after getting the regulator, and the device
> can be disabled if an error occurs.
> 

I do not see a possible path for a call to cc2_enable() at this point,
meaning the regulator won't ever be enabled. Please provide a better
explanation why this patch would be necessary.

Guenter

> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
>   drivers/hwmon/chipcap2.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
> index 88689f4eb598..02764689ed21 100644
> --- a/drivers/hwmon/chipcap2.c
> +++ b/drivers/hwmon/chipcap2.c
> @@ -747,7 +747,7 @@ static int cc2_probe(struct i2c_client *client)
>   	ret = cc2_request_ready_irq(data, dev);
>   	if (ret) {
>   		dev_err_probe(dev, ret, "Failed to request ready irq\n");
> -		return ret;
> +		goto disable;
>   	}
>   
>   	ret = cc2_request_alarm_irqs(data, dev);
> 

Re: [PATCH 2/2] hwmon: chipcap2: disable sensor if request ready irq fails

[Javier Carrasco]

On 12/08/2024 18:49, Guenter Roeck wrote:
> On 8/12/24 08:43, Javier Carrasco wrote:
>> This check is carried out after getting the regulator, and the device
>> can be disabled if an error occurs.
>>
> 
> I do not see a possible path for a call to cc2_enable() at this point,
> meaning the regulator won't ever be enabled. Please provide a better
> explanation why this patch would be necessary.
> 
> Guenter
> 

Hi Guenter,

this patch enforces the state where the dedicated regulator is disabled,
no matter what the history of the regulator was. If a previous
regulator_disable() failed, it would still be desirable that the
regulator gets disabled the next time the driver is probed (i.e. a new
attempt to disable it on failure).
cc2_disable() checks first if the regulator is enabled to avoid any
imbalance.

Best regards,
Javier Carrasco

Re: [PATCH 2/2] hwmon: chipcap2: disable sensor if request ready irq fails

[Guenter Roeck]

On 8/12/24 12:59, Javier Carrasco wrote:
> On 12/08/2024 18:49, Guenter Roeck wrote:
>> On 8/12/24 08:43, Javier Carrasco wrote:
>>> This check is carried out after getting the regulator, and the device
>>> can be disabled if an error occurs.
>>>
>>
>> I do not see a possible path for a call to cc2_enable() at this point,
>> meaning the regulator won't ever be enabled. Please provide a better
>> explanation why this patch would be necessary.
>>
>> Guenter
>>
> 
> Hi Guenter,
> 
> this patch enforces the state where the dedicated regulator is disabled,
> no matter what the history of the regulator was. If a previous
> regulator_disable() failed, it would still be desirable that the
> regulator gets disabled the next time the driver is probed (i.e. a new
> attempt to disable it on failure).
> cc2_disable() checks first if the regulator is enabled to avoid any
> imbalance.
> 

That is very theoretic. Sorry, I am not going to accept this patch.

Guenter

Re: [PATCH 2/2] hwmon: chipcap2: disable sensor if request ready irq fails

[Javier Carrasco]

On 12/08/2024 22:08, Guenter Roeck wrote:
> On 8/12/24 12:59, Javier Carrasco wrote:
>> On 12/08/2024 18:49, Guenter Roeck wrote:
>>> On 8/12/24 08:43, Javier Carrasco wrote:
>>>> This check is carried out after getting the regulator, and the device
>>>> can be disabled if an error occurs.
>>>>
>>>
>>> I do not see a possible path for a call to cc2_enable() at this point,
>>> meaning the regulator won't ever be enabled. Please provide a better
>>> explanation why this patch would be necessary.
>>>
>>> Guenter
>>>
>>
>> Hi Guenter,
>>
>> this patch enforces the state where the dedicated regulator is disabled,
>> no matter what the history of the regulator was. If a previous
>> regulator_disable() failed, it would still be desirable that the
>> regulator gets disabled the next time the driver is probed (i.e. a new
>> attempt to disable it on failure).
>> cc2_disable() checks first if the regulator is enabled to avoid any
>> imbalance.
>>
> 
> That is very theoretic. Sorry, I am not going to accept this patch.
> 
> Guenter
> 

I get your point, but given that this device requires a dedicated
regulator, I believe it makes sense that it tries to disable it whenever
possible if it's not going to be used. I think that makes more sense
that just returning an error value without even making sure that de
regulator was disabled, doesn't it?

Of course this is not a killer feature, and I don't want to make you
waste much time with it. But I think the dedicated regulator should be
shut down in all error paths, whatever status it had before.

If that does not sound convincing, then I won't argue any longer. Please
take a look at the first patch of the series in any case, which is not a
killer feature either, but cleaner than the current implementation.

Thanks and best regards,
Javier Carrasco

Re: [PATCH 2/2] hwmon: chipcap2: disable sensor if request ready irq fails

[Guenter Roeck]

On 8/12/24 13:48, Javier Carrasco wrote:
> On 12/08/2024 22:08, Guenter Roeck wrote:
>> On 8/12/24 12:59, Javier Carrasco wrote:
>>> On 12/08/2024 18:49, Guenter Roeck wrote:
>>>> On 8/12/24 08:43, Javier Carrasco wrote:
>>>>> This check is carried out after getting the regulator, and the device
>>>>> can be disabled if an error occurs.
>>>>>
>>>>
>>>> I do not see a possible path for a call to cc2_enable() at this point,
>>>> meaning the regulator won't ever be enabled. Please provide a better
>>>> explanation why this patch would be necessary.
>>>>
>>>> Guenter
>>>>
>>>
>>> Hi Guenter,
>>>
>>> this patch enforces the state where the dedicated regulator is disabled,
>>> no matter what the history of the regulator was. If a previous
>>> regulator_disable() failed, it would still be desirable that the
>>> regulator gets disabled the next time the driver is probed (i.e. a new
>>> attempt to disable it on failure).
>>> cc2_disable() checks first if the regulator is enabled to avoid any
>>> imbalance.
>>>
>>
>> That is very theoretic. Sorry, I am not going to accept this patch.
>>
>> Guenter
>>
> 
> I get your point, but given that this device requires a dedicated
> regulator, I believe it makes sense that it tries to disable it whenever
> possible if it's not going to be used. I think that makes more sense
> that just returning an error value without even making sure that de
> regulator was disabled, doesn't it?
> 

No, it doesn't make any sense whatsoever. What are you planning to do,
clutter the kernel with code to disable regulators if instantiating a device
fails for whatever reason and it turns out that a regulator which should
not have been enabled to start with turns out to be enabled anyway ?

> Of course this is not a killer feature, and I don't want to make you
> waste much time with it. But I think the dedicated regulator should be
> shut down in all error paths, whatever status it had before.
> 

I strongly disagree. This can only mess up the kernel all over the place.
Maybe you can convince other maintainers to accept such code, but please
refrain from doing that in my scope of responsibility. If the regulator
subsystem has the habit of leaving regulators enabled even after they
have been released, that problem should be fixed in the regulator subsystem
and not be worked around in individual drivers.

Guenter

Re: [PATCH 2/2] hwmon: chipcap2: disable sensor if request ready irq fails

[Javier Carrasco]

On 12/08/2024 23:26, Guenter Roeck wrote:
> On 8/12/24 13:48, Javier Carrasco wrote:
>> On 12/08/2024 22:08, Guenter Roeck wrote:
>>> On 8/12/24 12:59, Javier Carrasco wrote:
>>>> On 12/08/2024 18:49, Guenter Roeck wrote:
>>>>> On 8/12/24 08:43, Javier Carrasco wrote:
>>>>>> This check is carried out after getting the regulator, and the device
>>>>>> can be disabled if an error occurs.
>>>>>>
>>>>>
>>>>> I do not see a possible path for a call to cc2_enable() at this point,
>>>>> meaning the regulator won't ever be enabled. Please provide a better
>>>>> explanation why this patch would be necessary.
>>>>>
>>>>> Guenter
>>>>>
>>>>
>>>> Hi Guenter,
>>>>
>>>> this patch enforces the state where the dedicated regulator is
>>>> disabled,
>>>> no matter what the history of the regulator was. If a previous
>>>> regulator_disable() failed, it would still be desirable that the
>>>> regulator gets disabled the next time the driver is probed (i.e. a new
>>>> attempt to disable it on failure).
>>>> cc2_disable() checks first if the regulator is enabled to avoid any
>>>> imbalance.
>>>>
>>>
>>> That is very theoretic. Sorry, I am not going to accept this patch.
>>>
>>> Guenter
>>>
>>
>> I get your point, but given that this device requires a dedicated
>> regulator, I believe it makes sense that it tries to disable it whenever
>> possible if it's not going to be used. I think that makes more sense
>> that just returning an error value without even making sure that de
>> regulator was disabled, doesn't it?
>>
> 
> No, it doesn't make any sense whatsoever. What are you planning to do,
> clutter the kernel with code to disable regulators if instantiating a
> device
> fails for whatever reason and it turns out that a regulator which should
> not have been enabled to start with turns out to be enabled anyway ?
> 
>> Of course this is not a killer feature, and I don't want to make you
>> waste much time with it. But I think the dedicated regulator should be
>> shut down in all error paths, whatever status it had before.
>>
> 
> I strongly disagree. This can only mess up the kernel all over the place.
> Maybe you can convince other maintainers to accept such code, but please
> refrain from doing that in my scope of responsibility. If the regulator
> subsystem has the habit of leaving regulators enabled even after they
> have been released, that problem should be fixed in the regulator subsystem
> and not be worked around in individual drivers.
> 
> Guenter
> 

In that case the current behavior is wrong in the opposite direction,
and disabling the regulator before any call to cc2_enable() follows the
same invalid assumption.

I will remove the call to cc2_disable() in the probe function, which was
there exactly to ensure that the regulator gets disabled under all
circumstances. All error paths will just return, letting the regulator
untouched if it was not actively enabled.

Best regards,
Javier Carrasco