diff mbox series

[v1,2/2] hwmon: ina2xx: add optional regulator support

Message ID	20230308094024.14115-3-clamor95@gmail.com
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; From: Svyatoslav Ryhel <clamor95@gmail.com> To: Guenter Roeck <linux@roeck-us.net>, Jean Delvare <jdelvare@suse.com>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Liam Girdwood <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org> Cc: linux-hwmon@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH v1 2/2] hwmon: ina2xx: add optional regulator support Date: Wed, 8 Mar 2023 11:40:24 +0200 Message-Id: <20230308094024.14115-3-clamor95@gmail.com> In-Reply-To: <20230308094024.14115-1-clamor95@gmail.com> References: <20230308094024.14115-1-clamor95@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	Add optional power supply for INA2XX \| [v1,0/2] Add optional power supply for INA2XX [v1,1/2] dt-bindings: hwmon: ina2xx: add supply property [v1,2/2] hwmon: ina2xx: add optional regulator support

Commit Message

Svyatoslav Ryhel March 8, 2023, 9:40 a.m. UTC

  Some devices may need a specific supply provided
for this sensor to work properly, like p895 does.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 drivers/hwmon/ina2xx.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Krzysztof Kozlowski March 8, 2023, 10:25 a.m. UTC | #1

On 08/03/2023 10:40, Svyatoslav Ryhel wrote:
> Some devices may need a specific supply provided
> for this sensor to work properly, like p895 does.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>  drivers/hwmon/ina2xx.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index 00fc70305a89..4a3e2b1bbe8b 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -119,6 +119,7 @@ struct ina2xx_data {
>  	long power_lsb_uW;
>  	struct mutex config_lock;
>  	struct regmap *regmap;
> +	struct regulator *vdd_supply;
>  
>  	const struct attribute_group *groups[INA2XX_MAX_ATTRIBUTE_GROUPS];
>  };
> @@ -656,6 +657,17 @@ static int ina2xx_probe(struct i2c_client *client)
>  		return PTR_ERR(data->regmap);
>  	}
>  
> +	data->vdd_supply = devm_regulator_get_optional(dev, "vdd");
> +	if (IS_ERR(data->vdd_supply))
> +		return dev_err_probe(dev, PTR_ERR(data->vdd_supply),
> +				     "failed to get vdd regulator\n");
> +
> +	ret = regulator_enable(data->vdd_supply);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to enable vdd power supply\n");
> +		return ret;

And where is disable? On each error path, removal etc.

Best regards,
Krzysztof

Svyatoslav Ryhel March 8, 2023, 10:35 a.m. UTC | #2

ср, 8 бер. 2023 р. о 12:25 Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> пише:
>
> On 08/03/2023 10:40, Svyatoslav Ryhel wrote:
> > Some devices may need a specific supply provided
> > for this sensor to work properly, like p895 does.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > ---
> >  drivers/hwmon/ina2xx.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> > index 00fc70305a89..4a3e2b1bbe8b 100644
> > --- a/drivers/hwmon/ina2xx.c
> > +++ b/drivers/hwmon/ina2xx.c
> > @@ -119,6 +119,7 @@ struct ina2xx_data {
> >       long power_lsb_uW;
> >       struct mutex config_lock;
> >       struct regmap *regmap;
> > +     struct regulator *vdd_supply;
> >
> >       const struct attribute_group *groups[INA2XX_MAX_ATTRIBUTE_GROUPS];
> >  };
> > @@ -656,6 +657,17 @@ static int ina2xx_probe(struct i2c_client *client)
> >               return PTR_ERR(data->regmap);
> >       }
> >
> > +     data->vdd_supply = devm_regulator_get_optional(dev, "vdd");
> > +     if (IS_ERR(data->vdd_supply))
> > +             return dev_err_probe(dev, PTR_ERR(data->vdd_supply),
> > +                                  "failed to get vdd regulator\n");
> > +
> > +     ret = regulator_enable(data->vdd_supply);
> > +     if (ret < 0) {
> > +             dev_err(dev, "failed to enable vdd power supply\n");
> > +             return ret;
>
> And where is disable? On each error path, removal etc.
>

error path ok, should I create a remove function just to disable the regulator?

> Best regards,
> Krzysztof
>

Krzysztof Kozlowski March 8, 2023, 10:37 a.m. UTC | #3

On 08/03/2023 11:35, Svyatoslav Ryhel wrote:
> ср, 8 бер. 2023 р. о 12:25 Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> пише:
>>
>> On 08/03/2023 10:40, Svyatoslav Ryhel wrote:
>>> Some devices may need a specific supply provided
>>> for this sensor to work properly, like p895 does.
>>>
>>> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
>>> ---
>>>  drivers/hwmon/ina2xx.c | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
>>> index 00fc70305a89..4a3e2b1bbe8b 100644
>>> --- a/drivers/hwmon/ina2xx.c
>>> +++ b/drivers/hwmon/ina2xx.c
>>> @@ -119,6 +119,7 @@ struct ina2xx_data {
>>>       long power_lsb_uW;
>>>       struct mutex config_lock;
>>>       struct regmap *regmap;
>>> +     struct regulator *vdd_supply;
>>>
>>>       const struct attribute_group *groups[INA2XX_MAX_ATTRIBUTE_GROUPS];
>>>  };
>>> @@ -656,6 +657,17 @@ static int ina2xx_probe(struct i2c_client *client)
>>>               return PTR_ERR(data->regmap);
>>>       }
>>>
>>> +     data->vdd_supply = devm_regulator_get_optional(dev, "vdd");
>>> +     if (IS_ERR(data->vdd_supply))
>>> +             return dev_err_probe(dev, PTR_ERR(data->vdd_supply),
>>> +                                  "failed to get vdd regulator\n");
>>> +
>>> +     ret = regulator_enable(data->vdd_supply);
>>> +     if (ret < 0) {
>>> +             dev_err(dev, "failed to enable vdd power supply\n");
>>> +             return ret;
>>
>> And where is disable? On each error path, removal etc.
>>
> 
> error path ok, should I create a remove function just to disable the regulator?

Unbind device. Then bind. Then unbind. Then check regulator status
(/sys/kernel/debug). Do you have the answer now?

Best regards,
Krzysztof

Guenter Roeck March 8, 2023, 11:13 a.m. UTC | #4

On 3/8/23 02:35, Svyatoslav Ryhel wrote:
> ср, 8 бер. 2023 р. о 12:25 Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> пише:
>>
>> On 08/03/2023 10:40, Svyatoslav Ryhel wrote:
>>> Some devices may need a specific supply provided
>>> for this sensor to work properly, like p895 does.
>>>
>>> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
>>> ---
>>>   drivers/hwmon/ina2xx.c | 12 ++++++++++++
>>>   1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
>>> index 00fc70305a89..4a3e2b1bbe8b 100644
>>> --- a/drivers/hwmon/ina2xx.c
>>> +++ b/drivers/hwmon/ina2xx.c
>>> @@ -119,6 +119,7 @@ struct ina2xx_data {
>>>        long power_lsb_uW;
>>>        struct mutex config_lock;
>>>        struct regmap *regmap;
>>> +     struct regulator *vdd_supply;
>>>
>>>        const struct attribute_group *groups[INA2XX_MAX_ATTRIBUTE_GROUPS];
>>>   };
>>> @@ -656,6 +657,17 @@ static int ina2xx_probe(struct i2c_client *client)
>>>                return PTR_ERR(data->regmap);
>>>        }
>>>
>>> +     data->vdd_supply = devm_regulator_get_optional(dev, "vdd");
>>> +     if (IS_ERR(data->vdd_supply))
>>> +             return dev_err_probe(dev, PTR_ERR(data->vdd_supply),
>>> +                                  "failed to get vdd regulator\n");
>>> +
>>> +     ret = regulator_enable(data->vdd_supply);
>>> +     if (ret < 0) {
>>> +             dev_err(dev, "failed to enable vdd power supply\n");
>>> +             return ret;
>>
>> And where is disable? On each error path, removal etc.
>>
> 
> error path ok, should I create a remove function just to disable the regulator?
> 
Use devm_add_action_or_reset().

Guenter

>> Best regards,
>> Krzysztof
>>

Svyatoslav Ryhel March 8, 2023, 11:19 a.m. UTC | #5

8 березня 2023 р. 13:13:18 GMT+02:00, Guenter Roeck <linux@roeck-us.net> написав(-ла):
>On 3/8/23 02:35, Svyatoslav Ryhel wrote:
>> ср, 8 бер. 2023 р. о 12:25 Krzysztof Kozlowski
>> <krzysztof.kozlowski@linaro.org> пише:
>>> 
>>> On 08/03/2023 10:40, Svyatoslav Ryhel wrote:
>>>> Some devices may need a specific supply provided
>>>> for this sensor to work properly, like p895 does.
>>>> 
>>>> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
>>>> ---
>>>>   drivers/hwmon/ina2xx.c | 12 ++++++++++++
>>>>   1 file changed, 12 insertions(+)
>>>> 
>>>> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
>>>> index 00fc70305a89..4a3e2b1bbe8b 100644
>>>> --- a/drivers/hwmon/ina2xx.c
>>>> +++ b/drivers/hwmon/ina2xx.c
>>>> @@ -119,6 +119,7 @@ struct ina2xx_data {
>>>>        long power_lsb_uW;
>>>>        struct mutex config_lock;
>>>>        struct regmap *regmap;
>>>> +     struct regulator *vdd_supply;
>>>> 
>>>>        const struct attribute_group *groups[INA2XX_MAX_ATTRIBUTE_GROUPS];
>>>>   };
>>>> @@ -656,6 +657,17 @@ static int ina2xx_probe(struct i2c_client *client)
>>>>                return PTR_ERR(data->regmap);
>>>>        }
>>>> 
>>>> +     data->vdd_supply = devm_regulator_get_optional(dev, "vdd");
>>>> +     if (IS_ERR(data->vdd_supply))
>>>> +             return dev_err_probe(dev, PTR_ERR(data->vdd_supply),
>>>> +                                  "failed to get vdd regulator\n");
>>>> +
>>>> +     ret = regulator_enable(data->vdd_supply);
>>>> +     if (ret < 0) {
>>>> +             dev_err(dev, "failed to enable vdd power supply\n");
>>>> +             return ret;
>>> 
>>> And where is disable? On each error path, removal etc.
>>> 
>> 
>> error path ok, should I create a remove function just to disable the regulator?
>> 
>Use devm_add_action_or_reset().
>
>Guenter
>

That is good suggestion. Thanks!

Best regards,
Svyatoslav R.

>>> Best regards,
>>> Krzysztof
>>> 
>

Lars-Peter Clausen March 8, 2023, 11:32 a.m. UTC | #6

On 3/8/23 03:19, Svyatoslav Ryhel wrote:
>
> 8 березня 2023 р. 13:13:18 GMT+02:00, Guenter Roeck <linux@roeck-us.net> написав(-ла):
>> On 3/8/23 02:35, Svyatoslav Ryhel wrote:
>>> ср, 8 бер. 2023 р. о 12:25 Krzysztof Kozlowski
>>> <krzysztof.kozlowski@linaro.org> пише:
>>>> On 08/03/2023 10:40, Svyatoslav Ryhel wrote:
>>>>> Some devices may need a specific supply provided
>>>>> for this sensor to work properly, like p895 does.
>>>>>
>>>>> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
>>>>> ---
>>>>>    drivers/hwmon/ina2xx.c | 12 ++++++++++++
>>>>>    1 file changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
>>>>> index 00fc70305a89..4a3e2b1bbe8b 100644
>>>>> --- a/drivers/hwmon/ina2xx.c
>>>>> +++ b/drivers/hwmon/ina2xx.c
>>>>> @@ -119,6 +119,7 @@ struct ina2xx_data {
>>>>>         long power_lsb_uW;
>>>>>         struct mutex config_lock;
>>>>>         struct regmap *regmap;
>>>>> +     struct regulator *vdd_supply;
>>>>>
>>>>>         const struct attribute_group *groups[INA2XX_MAX_ATTRIBUTE_GROUPS];
>>>>>    };
>>>>> @@ -656,6 +657,17 @@ static int ina2xx_probe(struct i2c_client *client)
>>>>>                 return PTR_ERR(data->regmap);
>>>>>         }
>>>>>
>>>>> +     data->vdd_supply = devm_regulator_get_optional(dev, "vdd");
>>>>> +     if (IS_ERR(data->vdd_supply))
>>>>> +             return dev_err_probe(dev, PTR_ERR(data->vdd_supply),
>>>>> +                                  "failed to get vdd regulator\n");
>>>>> +
>>>>> +     ret = regulator_enable(data->vdd_supply);
>>>>> +     if (ret < 0) {
>>>>> +             dev_err(dev, "failed to enable vdd power supply\n");
>>>>> +             return ret;
>>>> And where is disable? On each error path, removal etc.
>>>>
>>> error path ok, should I create a remove function just to disable the regulator?
>>>
>> Use devm_add_action_or_reset().
>>
>> Guenter
>>
> That is good suggestion. Thanks!

There is a new function devm_regulator_get_enable(). It will both 
request the regulator and enable in and then automatically disable and 
free it when the device is removed.

The API can be slightly confusing in this regard. In your case you also 
want to use the non-optional API. The optional API is for cases where 
you need to know whether a regulator is connected or not. If you do not 
need to know use the non-optional API, if no regulator is specified the 
regulator framework will just a return a noop regulator as a 
placeholder. The optional version will actually return an error if no 
regulator is specified, so with your patch existing users of this driver 
will break.

diff mbox series

Patch

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 00fc70305a89..4a3e2b1bbe8b 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -119,6 +119,7 @@  struct ina2xx_data {
 	long power_lsb_uW;
 	struct mutex config_lock;
 	struct regmap *regmap;
+	struct regulator *vdd_supply;
 
 	const struct attribute_group *groups[INA2XX_MAX_ATTRIBUTE_GROUPS];
 };
@@ -656,6 +657,17 @@  static int ina2xx_probe(struct i2c_client *client)
 		return PTR_ERR(data->regmap);
 	}
 
+	data->vdd_supply = devm_regulator_get_optional(dev, "vdd");
+	if (IS_ERR(data->vdd_supply))
+		return dev_err_probe(dev, PTR_ERR(data->vdd_supply),
+				     "failed to get vdd regulator\n");
+
+	ret = regulator_enable(data->vdd_supply);
+	if (ret < 0) {
+		dev_err(dev, "failed to enable vdd power supply\n");
+		return ret;
+	}
+
 	ret = ina2xx_init(data);
 	if (ret < 0) {
 		dev_err(dev, "error configuring the device: %d\n", ret);