[PATCH v5 21/21] regulator: s2mps11: enable-gpios is optional on s2mpg1x

André Draszik posted 21 patches 1 month, 2 weeks ago
There is a newer version of this series
[PATCH v5 21/21] regulator: s2mps11: enable-gpios is optional on s2mpg1x
Posted by André Draszik 1 month, 2 weeks ago
For s2mpg1x, enable-gpios is optional, but when not given, the driver
is complaining quite verbosely about the missing property.

Refactor the code slightly to avoid printing those messages to the
kernel log in that case.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/regulator/s2mps11.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/regulator/s2mps11.c b/drivers/regulator/s2mps11.c
index 178a032c0dc192874118906aee45441a1bbd8515..2d5510acd0780ab6f9296c48ddcde5efe15ff488 100644
--- a/drivers/regulator/s2mps11.c
+++ b/drivers/regulator/s2mps11.c
@@ -352,7 +352,7 @@ static int s2mps11_regulator_set_suspend_disable(struct regulator_dev *rdev)
 }
 
 static int s2mps11_of_parse_gpiod(struct device_node *np,
-				  const char *con_id,
+				  const char *con_id, bool optional,
 				  const struct regulator_desc *desc,
 				  struct regulator_config *config)
 {
@@ -371,14 +371,19 @@ static int s2mps11_of_parse_gpiod(struct device_node *np,
 		if (ret == -EPROBE_DEFER)
 			return ret;
 
-		if (ret == -ENOENT)
+		if (ret == -ENOENT) {
+			if (optional)
+				return 0;
+
 			dev_info(config->dev,
 				 "No entry for control GPIO for %d/%s in node %pOF\n",
 				 desc->id, desc->name, np);
-		else
+		} else {
 			dev_warn_probe(config->dev, ret,
 				       "Failed to get control GPIO for %d/%s in node %pOF\n",
 				       desc->id, desc->name, np);
+		}
+
 		return 0;
 	}
 
@@ -409,7 +414,8 @@ static int s2mps11_of_parse_cb(struct device_node *np,
 	else
 		return 0;
 
-	return s2mps11_of_parse_gpiod(np, "samsung,ext-control", desc, config);
+	return s2mps11_of_parse_gpiod(np, "samsung,ext-control", false, desc,
+				      config);
 }
 
 static int s2mpg10_of_parse_cb(struct device_node *np,
@@ -528,7 +534,7 @@ static int s2mpg10_of_parse_cb(struct device_node *np,
 
 	++s2mpg10_desc->desc.ops;
 
-	return s2mps11_of_parse_gpiod(np, "enable", desc, config);
+	return s2mps11_of_parse_gpiod(np, "enable", true, desc, config);
 }
 
 static int s2mpg10_enable_ext_control(struct s2mps11_info *s2mps11,

-- 
2.52.0.351.gbe84eed79e-goog

Re: [PATCH v5 21/21] regulator: s2mps11: enable-gpios is optional on s2mpg1x
Posted by Bartosz Golaszewski 1 month, 1 week ago
On Sat, Dec 27, 2025 at 1:24 PM André Draszik <andre.draszik@linaro.org> wrote:
>
> For s2mpg1x, enable-gpios is optional, but when not given, the driver
> is complaining quite verbosely about the missing property.
>
> Refactor the code slightly to avoid printing those messages to the
> kernel log in that case.
>

I don't get the point of this - you added this function in the same
series, why can't it be done right the first time it's implemented?

Bart
Re: [PATCH v5 21/21] regulator: s2mps11: enable-gpios is optional on s2mpg1x
Posted by André Draszik 1 month, 1 week ago
On Fri, 2026-01-02 at 11:19 +0100, Bartosz Golaszewski wrote:
> On Sat, Dec 27, 2025 at 1:24 PM André Draszik <andre.draszik@linaro.org> wrote:
> > 
> > For s2mpg1x, enable-gpios is optional, but when not given, the driver
> > is complaining quite verbosely about the missing property.
> > 
> > Refactor the code slightly to avoid printing those messages to the
> > kernel log in that case.
> > 
> 
> I don't get the point of this - you added this function in the same
> series, why can't it be done right the first time it's implemented?

Sure, I can merge this patch into the refactoring patch 15 - the intention
was to have incremental changes to simplify review.

Cheers,
Andre
Re: [PATCH v5 21/21] regulator: s2mps11: enable-gpios is optional on s2mpg1x
Posted by Krzysztof Kozlowski 1 month, 1 week ago
On 02/01/2026 12:26, André Draszik wrote:
> On Fri, 2026-01-02 at 11:19 +0100, Bartosz Golaszewski wrote:
>> On Sat, Dec 27, 2025 at 1:24 PM André Draszik <andre.draszik@linaro.org> wrote:
>>>
>>> For s2mpg1x, enable-gpios is optional, but when not given, the driver
>>> is complaining quite verbosely about the missing property.
>>>
>>> Refactor the code slightly to avoid printing those messages to the
>>> kernel log in that case.
>>>
>>
>> I don't get the point of this - you added this function in the same
>> series, why can't it be done right the first time it's implemented?
> 
> Sure, I can merge this patch into the refactoring patch 15 - the intention
> was to have incremental changes to simplify review.

When you add new code which is already wrong and you need to fix it in
patch 21, it is not easier to review. Adding undesired code, which you
immediately change, is making things difficult to review.

Best regards,
Krzysztof
Re: [PATCH v5 21/21] regulator: s2mps11: enable-gpios is optional on s2mpg1x
Posted by Bartosz Golaszewski 1 month, 1 week ago
On Fri, Jan 2, 2026 at 12:26 PM André Draszik <andre.draszik@linaro.org> wrote:
>
> On Fri, 2026-01-02 at 11:19 +0100, Bartosz Golaszewski wrote:
> > On Sat, Dec 27, 2025 at 1:24 PM André Draszik <andre.draszik@linaro.org> wrote:
> > >
> > > For s2mpg1x, enable-gpios is optional, but when not given, the driver
> > > is complaining quite verbosely about the missing property.
> > >
> > > Refactor the code slightly to avoid printing those messages to the
> > > kernel log in that case.
> > >
> >
> > I don't get the point of this - you added this function in the same
> > series, why can't it be done right the first time it's implemented?
>
> Sure, I can merge this patch into the refactoring patch 15 - the intention
> was to have incremental changes to simplify review.
>

I'm all for it but introducing issues in one patch and fixing it in
another is a bit too much. :)

Bartosz