drivers/hwmon/ina2xx.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
S32G2/S32G3 based boards which integrate the ina231 sensor do not have a
dedicated voltage regulator.
Co-developed-by: Florin Buica <florin.buica@nxp.com>
Signed-off-by: Florin Buica <florin.buica@nxp.com>
Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
---
drivers/hwmon/ina2xx.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 345fe7db9de9..ab4972f94a8c 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -959,8 +959,8 @@ static int ina2xx_probe(struct i2c_client *client)
return PTR_ERR(data->regmap);
}
- ret = devm_regulator_get_enable(dev, "vs");
- if (ret)
+ ret = devm_regulator_get_enable_optional(dev, "vs");
+ if (ret < 0 && ret != -ENODEV)
return dev_err_probe(dev, ret, "failed to enable vs regulator\n");
ret = ina2xx_init(dev, data);
--
2.45.2On 4/3/25 03:15, Ciprian Costea wrote: > From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> > > S32G2/S32G3 based boards which integrate the ina231 sensor do not have a > dedicated voltage regulator. > > Co-developed-by: Florin Buica <florin.buica@nxp.com> > Signed-off-by: Florin Buica <florin.buica@nxp.com> > Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> > --- > drivers/hwmon/ina2xx.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c > index 345fe7db9de9..ab4972f94a8c 100644 > --- a/drivers/hwmon/ina2xx.c > +++ b/drivers/hwmon/ina2xx.c > @@ -959,8 +959,8 @@ static int ina2xx_probe(struct i2c_client *client) > return PTR_ERR(data->regmap); > } > > - ret = (dev, "vs"); > - if (ret) > + ret = devm_regulator_get_enable_optional(dev, "vs"); devm_regulator_get_enable() should provide a dummy regulator if there is no explicit regulator. Why does this not work ? > + if (ret < 0 && ret != -ENODEV) Why this added check ? I know it used to be necessary if regulator support is disabled, but that is no longer the case. Guenter > return dev_err_probe(dev, ret, "failed to enable vs regulator\n"); > > ret = ina2xx_init(dev, data);
On 4/3/2025 3:15 PM, Guenter Roeck wrote: > On 4/3/25 03:15, Ciprian Costea wrote: >> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> >> >> S32G2/S32G3 based boards which integrate the ina231 sensor do not have a >> dedicated voltage regulator. >> >> Co-developed-by: Florin Buica <florin.buica@nxp.com> >> Signed-off-by: Florin Buica <florin.buica@nxp.com> >> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> >> --- >> drivers/hwmon/ina2xx.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c >> index 345fe7db9de9..ab4972f94a8c 100644 >> --- a/drivers/hwmon/ina2xx.c >> +++ b/drivers/hwmon/ina2xx.c >> @@ -959,8 +959,8 @@ static int ina2xx_probe(struct i2c_client *client) >> return PTR_ERR(data->regmap); >> } >> - ret = (dev, "vs"); >> - if (ret) >> + ret = devm_regulator_get_enable_optional(dev, "vs"); > > devm_regulator_get_enable() should provide a dummy regulator if there is > no explicit regulator. Why does this not work ? > >> + if (ret < 0 && ret != -ENODEV) > > Why this added check ? > > I know it used to be necessary if regulator support is disabled, > but that is no longer the case. > > Guenter > Hello Guenter, I've just tested and devm_regulator_get_enable() does work as you've described, providing a dummy regulator. But, according to the 'ti,ina2xx' binding [1] I see that the `vs-supply` property is not required. Hence wouldn't it be correct for `vs-supply` to be optional ? Using 'devm_regulator_get_enable_optional()' [1] https://elixir.bootlin.com/linux/v6.13.7/source/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml#L78-L80 Regards, Ciprian >> return dev_err_probe(dev, ret, "failed to enable vs >> regulator\n"); >> ret = ina2xx_init(dev, data); >
On Thu, Apr 03, 2025 at 05:29:26PM +0300, Ciprian Marian Costea wrote: > On 4/3/2025 3:15 PM, Guenter Roeck wrote: > > On 4/3/25 03:15, Ciprian Costea wrote: > > > From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> > > > > > > S32G2/S32G3 based boards which integrate the ina231 sensor do not have a > > > dedicated voltage regulator. > > > > > > Co-developed-by: Florin Buica <florin.buica@nxp.com> > > > Signed-off-by: Florin Buica <florin.buica@nxp.com> > > > Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> > > > --- > > > drivers/hwmon/ina2xx.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c > > > index 345fe7db9de9..ab4972f94a8c 100644 > > > --- a/drivers/hwmon/ina2xx.c > > > +++ b/drivers/hwmon/ina2xx.c > > > @@ -959,8 +959,8 @@ static int ina2xx_probe(struct i2c_client *client) > > > return PTR_ERR(data->regmap); > > > } > > > - ret = (dev, "vs"); > > > - if (ret) > > > + ret = devm_regulator_get_enable_optional(dev, "vs"); > > > > devm_regulator_get_enable() should provide a dummy regulator if there is > > no explicit regulator. Why does this not work ? > > > > > + if (ret < 0 && ret != -ENODEV) > > > > Why this added check ? > > > > I know it used to be necessary if regulator support is disabled, > > but that is no longer the case. > > > > Guenter > > > > Hello Guenter, > > I've just tested and devm_regulator_get_enable() does work as you've > described, providing a dummy regulator. > > But, according to the 'ti,ina2xx' binding [1] I see that the `vs-supply` > property is not required. Hence wouldn't it be correct for `vs-supply` to be > optional ? Using 'devm_regulator_get_enable_optional()' > Yes, but the reasoning you provided is different and suggested that the current code would not work. Since that is not the case, the change would be purely cosmetic. Also, I still don't see why the -ENODEV check would be necessary. Guenter
On 4/3/2025 7:06 PM, Guenter Roeck wrote: > On Thu, Apr 03, 2025 at 05:29:26PM +0300, Ciprian Marian Costea wrote: >> On 4/3/2025 3:15 PM, Guenter Roeck wrote: >>> On 4/3/25 03:15, Ciprian Costea wrote: >>>> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> >>>> >>>> S32G2/S32G3 based boards which integrate the ina231 sensor do not have a >>>> dedicated voltage regulator. >>>> >>>> Co-developed-by: Florin Buica <florin.buica@nxp.com> >>>> Signed-off-by: Florin Buica <florin.buica@nxp.com> >>>> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> >>>> --- >>>> drivers/hwmon/ina2xx.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c >>>> index 345fe7db9de9..ab4972f94a8c 100644 >>>> --- a/drivers/hwmon/ina2xx.c >>>> +++ b/drivers/hwmon/ina2xx.c >>>> @@ -959,8 +959,8 @@ static int ina2xx_probe(struct i2c_client *client) >>>> return PTR_ERR(data->regmap); >>>> } >>>> - ret = (dev, "vs"); >>>> - if (ret) >>>> + ret = devm_regulator_get_enable_optional(dev, "vs"); >>> >>> devm_regulator_get_enable() should provide a dummy regulator if there is >>> no explicit regulator. Why does this not work ? >>> >>>> + if (ret < 0 && ret != -ENODEV) >>> >>> Why this added check ? >>> >>> I know it used to be necessary if regulator support is disabled, >>> but that is no longer the case. >>> >>> Guenter >>> >> >> Hello Guenter, >> >> I've just tested and devm_regulator_get_enable() does work as you've >> described, providing a dummy regulator. >> >> But, according to the 'ti,ina2xx' binding [1] I see that the `vs-supply` >> property is not required. Hence wouldn't it be correct for `vs-supply` to be >> optional ? Using 'devm_regulator_get_enable_optional()' >> > Yes, but the reasoning you provided is different and suggested that the > current code would not work. Since that is not the case, the change would > be purely cosmetic. Also, I still don't see why the -ENODEV check would be > necessary. > > Guenter For boards such as S32G274A-EVB, S32G274A-RDB2 and S32G399A-RDB3 which do not have a voltage regulator, 'devm_regulator_get_enable_optional()' would return error value -19 (-ENODEV). Also, other usages from the Linux Kernel seem to perform the same error check when using 'devm_regulator_get_enable_optional()' [1], [2] and [3]. This patch would help in S32G2 and S32G3 to not print an unnecessary kernel log warning hinting usage of a dummy regulator when such a regulator is not required according to the binding. Would you like me to send a V2 with the commit title updated as follows ? " hwmon: (ina2xx) make regulator 'vs' support optional According to the 'ti,ina2xx' binding, the 'vs-supply' property is optional. Furthermore, S32G2/S32G3 based boards which integrate the ina231 sensor do not have a dedicated voltage regulator. Thus, making regulator support optional would help in avoiding any unnecessary kernel log warnings during boot. " [1] https://elixir.bootlin.com/linux/v6.13.7/source/drivers/iio/adc/ad7625.c#L524-L525 [2] https://elixir.bootlin.com/linux/v6.13.7/source/drivers/pci/controller/pcie-rcar-host.c#L982-L983 [3] https://elixir.bootlin.com/linux/v6.13.7/source/drivers/iio/adc/ad7944.c#L514-L515 Regards, Ciprian
On 4/4/25 01:36, Ciprian Marian Costea wrote: > On 4/3/2025 7:06 PM, Guenter Roeck wrote: >> On Thu, Apr 03, 2025 at 05:29:26PM +0300, Ciprian Marian Costea wrote: >>> On 4/3/2025 3:15 PM, Guenter Roeck wrote: >>>> On 4/3/25 03:15, Ciprian Costea wrote: >>>>> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> >>>>> >>>>> S32G2/S32G3 based boards which integrate the ina231 sensor do not have a >>>>> dedicated voltage regulator. >>>>> >>>>> Co-developed-by: Florin Buica <florin.buica@nxp.com> >>>>> Signed-off-by: Florin Buica <florin.buica@nxp.com> >>>>> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> >>>>> --- >>>>> drivers/hwmon/ina2xx.c | 4 ++-- >>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c >>>>> index 345fe7db9de9..ab4972f94a8c 100644 >>>>> --- a/drivers/hwmon/ina2xx.c >>>>> +++ b/drivers/hwmon/ina2xx.c >>>>> @@ -959,8 +959,8 @@ static int ina2xx_probe(struct i2c_client *client) >>>>> return PTR_ERR(data->regmap); >>>>> } >>>>> - ret = (dev, "vs"); >>>>> - if (ret) >>>>> + ret = devm_regulator_get_enable_optional(dev, "vs"); >>>> >>>> devm_regulator_get_enable() should provide a dummy regulator if there is >>>> no explicit regulator. Why does this not work ? >>>> >>>>> + if (ret < 0 && ret != -ENODEV) >>>> >>>> Why this added check ? >>>> >>>> I know it used to be necessary if regulator support is disabled, >>>> but that is no longer the case. >>>> >>>> Guenter >>>> >>> >>> Hello Guenter, >>> >>> I've just tested and devm_regulator_get_enable() does work as you've >>> described, providing a dummy regulator. >>> >>> But, according to the 'ti,ina2xx' binding [1] I see that the `vs-supply` >>> property is not required. Hence wouldn't it be correct for `vs-supply` to be >>> optional ? Using 'devm_regulator_get_enable_optional()' >>> >> Yes, but the reasoning you provided is different and suggested that the >> current code would not work. Since that is not the case, the change would >> be purely cosmetic. Also, I still don't see why the -ENODEV check would be >> necessary. >> >> Guenter > > For boards such as S32G274A-EVB, S32G274A-RDB2 and S32G399A-RDB3 which do not have a voltage regulator, 'devm_regulator_get_enable_optional()' would return error value -19 (-ENODEV). Also, other usages from the Linux Kernel seem to perform the same error check when using 'devm_regulator_get_enable_optional()' [1], [2] and [3]. > > This patch would help in S32G2 and S32G3 to not print an unnecessary kernel log warning hinting usage of a dummy regulator when such a regulator is not required according to the binding. > > Would you like me to send a V2 with the commit title updated as follows ? > > " > hwmon: (ina2xx) make regulator 'vs' support optional > > According to the 'ti,ina2xx' binding, the 'vs-supply' property is optional. Furthermore, S32G2/S32G3 based boards which integrate the ina231 sensor do not have a dedicated voltage regulator. Thus, making regulator support optional would help in avoiding any unnecessary kernel log warnings during boot. > " Make it: "According to the 'ti,ina2xx' binding, the 'vs-supply' property is optional. Use devm_regulator_get_enable_optional() to avoid a kernel warning message if the property is not provided. " Then add a note to the code explaining that the check for -ENODEV is necessary because the regulator core returns -ENODEV if the regulator is not available. Why it makes sense for this function to return -ENODEV if an _optional_ regulator is not available escapes me, but that is a different issue. Guenter
On 4/8/2025 9:07 PM, Guenter Roeck wrote: > On 4/4/25 01:36, Ciprian Marian Costea wrote: >> On 4/3/2025 7:06 PM, Guenter Roeck wrote: >>> On Thu, Apr 03, 2025 at 05:29:26PM +0300, Ciprian Marian Costea wrote: >>>> On 4/3/2025 3:15 PM, Guenter Roeck wrote: >>>>> On 4/3/25 03:15, Ciprian Costea wrote: >>>>>> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> >>>>>> >>>>>> S32G2/S32G3 based boards which integrate the ina231 sensor do not >>>>>> have a >>>>>> dedicated voltage regulator. >>>>>> >>>>>> Co-developed-by: Florin Buica <florin.buica@nxp.com> >>>>>> Signed-off-by: Florin Buica <florin.buica@nxp.com> >>>>>> Signed-off-by: Ciprian Marian Costea >>>>>> <ciprianmarian.costea@oss.nxp.com> >>>>>> --- >>>>>> drivers/hwmon/ina2xx.c | 4 ++-- >>>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c >>>>>> index 345fe7db9de9..ab4972f94a8c 100644 >>>>>> --- a/drivers/hwmon/ina2xx.c >>>>>> +++ b/drivers/hwmon/ina2xx.c >>>>>> @@ -959,8 +959,8 @@ static int ina2xx_probe(struct i2c_client >>>>>> *client) >>>>>> return PTR_ERR(data->regmap); >>>>>> } >>>>>> - ret = (dev, "vs"); >>>>>> - if (ret) >>>>>> + ret = devm_regulator_get_enable_optional(dev, "vs"); >>>>> >>>>> devm_regulator_get_enable() should provide a dummy regulator if >>>>> there is >>>>> no explicit regulator. Why does this not work ? >>>>> >>>>>> + if (ret < 0 && ret != -ENODEV) >>>>> >>>>> Why this added check ? >>>>> >>>>> I know it used to be necessary if regulator support is disabled, >>>>> but that is no longer the case. >>>>> >>>>> Guenter >>>>> >>>> >>>> Hello Guenter, >>>> >>>> I've just tested and devm_regulator_get_enable() does work as you've >>>> described, providing a dummy regulator. >>>> >>>> But, according to the 'ti,ina2xx' binding [1] I see that the >>>> `vs-supply` >>>> property is not required. Hence wouldn't it be correct for >>>> `vs-supply` to be >>>> optional ? Using 'devm_regulator_get_enable_optional()' >>>> >>> Yes, but the reasoning you provided is different and suggested that the >>> current code would not work. Since that is not the case, the change >>> would >>> be purely cosmetic. Also, I still don't see why the -ENODEV check >>> would be >>> necessary. >>> >>> Guenter >> >> For boards such as S32G274A-EVB, S32G274A-RDB2 and S32G399A-RDB3 which >> do not have a voltage regulator, >> 'devm_regulator_get_enable_optional()' would return error value -19 >> (-ENODEV). Also, other usages from the Linux Kernel seem to perform >> the same error check when using 'devm_regulator_get_enable_optional()' >> [1], [2] and [3]. >> >> This patch would help in S32G2 and S32G3 to not print an unnecessary >> kernel log warning hinting usage of a dummy regulator when such a >> regulator is not required according to the binding. >> >> Would you like me to send a V2 with the commit title updated as follows ? >> >> " >> hwmon: (ina2xx) make regulator 'vs' support optional >> >> According to the 'ti,ina2xx' binding, the 'vs-supply' property is >> optional. Furthermore, S32G2/S32G3 based boards which integrate the >> ina231 sensor do not have a dedicated voltage regulator. Thus, making >> regulator support optional would help in avoiding any unnecessary >> kernel log warnings during boot. >> " > > Make it: > > "According to the 'ti,ina2xx' binding, the 'vs-supply' property is > optional. > Use devm_regulator_get_enable_optional() to avoid a kernel warning > message > if the property is not provided. > " > > Then add a note to the code explaining that the check for -ENODEV is > necessary > because the regulator core returns -ENODEV if the regulator is not > available. > > Why it makes sense for this function to return -ENODEV if an _optional_ > regulator > is not available escapes me, but that is a different issue. > > Guenter > Hello Guenter, Thanks for your review & suggestions. I will send a V2 patch. Regards, Ciprian
© 2016 - 2026 Red Hat, Inc.