| Message ID | 20230308094024.14115-3-clamor95@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add optional power supply for INA2XX | expand |
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
ср, 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 >
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
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 >>
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 >>> >
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 --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);
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(+)