This fixes bug introduced in 667a6b7a (regulator: max14577: Add missing
of_node_put). The DTS parsing function returned number of matched
regulators as success status which then was compared against 0 in probe.
Result was a probe fail after successful parsing the DTS:
max14577-regulator: probe of max14577-regulator failed with error 2
Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
drivers/regulator/max14577.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/regulator/max14577.c b/drivers/regulator/max14577.c
index 186df8785a91..6ba11a8dd23d 100644
--- a/drivers/regulator/max14577.c
+++ b/drivers/regulator/max14577.c
@@ -168,11 +168,13 @@ static int max14577_regulator_dt_parse_pdata(struct platform_device *pdev)
MAX14577_REG_MAX);
if (ret < 0) {
dev_err(&pdev->dev, "Error parsing regulator init data: %d\n", ret);
+ of_node_put(np);
+ return ret;
}
of_node_put(np);
- return ret;
+ return 0;
}
static inline struct regulator_init_data *match_init_data(int index)
--
1.7.9.5
Hi Krzysztof,
On 20 February 2014 20:49, Krzysztof Kozlowski <[email protected]> wrote:
> This fixes bug introduced in 667a6b7a (regulator: max14577: Add missing
> of_node_put). The DTS parsing function returned number of matched
> regulators as success status which then was compared against 0 in probe.
>
> Result was a probe fail after successful parsing the DTS:
> max14577-regulator: probe of max14577-regulator failed with error 2
>
> Signed-off-by: Krzysztof Kozlowski <[email protected]>
> ---
> drivers/regulator/max14577.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/regulator/max14577.c b/drivers/regulator/max14577.c
> index 186df8785a91..6ba11a8dd23d 100644
> --- a/drivers/regulator/max14577.c
> +++ b/drivers/regulator/max14577.c
> @@ -168,11 +168,13 @@ static int max14577_regulator_dt_parse_pdata(struct platform_device *pdev)
> MAX14577_REG_MAX);
> if (ret < 0) {
> dev_err(&pdev->dev, "Error parsing regulator init data: %d\n", ret);
> + of_node_put(np);
> + return ret;
> }
>
> of_node_put(np);
>
> - return ret;
> + return 0;
> }
Instead of so many changes here, you could simply make the following change
in the max14577_regulator_probe function.
ret = max14577_regulator_dt_parse_pdata(pdev);
if (ret < 0)
return ret;
--
With warm regards,
Sachin
On Fri, 2014-02-21 at 15:54 +0530, Sachin Kamat wrote:
> Hi Krzysztof,
>
> On 20 February 2014 20:49, Krzysztof Kozlowski <[email protected]> wrote:
> > This fixes bug introduced in 667a6b7a (regulator: max14577: Add missing
> > of_node_put). The DTS parsing function returned number of matched
> > regulators as success status which then was compared against 0 in probe.
> >
> > Result was a probe fail after successful parsing the DTS:
> > max14577-regulator: probe of max14577-regulator failed with error 2
> >
> > Signed-off-by: Krzysztof Kozlowski <[email protected]>
> > ---
> > drivers/regulator/max14577.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/regulator/max14577.c b/drivers/regulator/max14577.c
> > index 186df8785a91..6ba11a8dd23d 100644
> > --- a/drivers/regulator/max14577.c
> > +++ b/drivers/regulator/max14577.c
> > @@ -168,11 +168,13 @@ static int max14577_regulator_dt_parse_pdata(struct platform_device *pdev)
> > MAX14577_REG_MAX);
> > if (ret < 0) {
> > dev_err(&pdev->dev, "Error parsing regulator init data: %d\n", ret);
> > + of_node_put(np);
> > + return ret;
> > }
> >
> > of_node_put(np);
> >
> > - return ret;
> > + return 0;
> > }
>
> Instead of so many changes here, you could simply make the following change
> in the max14577_regulator_probe function.
>
> ret = max14577_regulator_dt_parse_pdata(pdev);
> if (ret < 0)
> return ret;
That still wouldn't be sufficient because later in probe() the 'ret' is
not reassigned (if devm_regulator_register succeeds) and it is directly
returned at the probe() end. So still the number of matched regulators
would be returned as probe result.
Best regards,
Krzysztof
On 21 February 2014 16:12, Krzysztof Kozlowski <[email protected]> wrote:
> On Fri, 2014-02-21 at 15:54 +0530, Sachin Kamat wrote:
>> Hi Krzysztof,
>>
>> On 20 February 2014 20:49, Krzysztof Kozlowski <[email protected]> wrote:
>> > This fixes bug introduced in 667a6b7a (regulator: max14577: Add missing
>> > of_node_put). The DTS parsing function returned number of matched
>> > regulators as success status which then was compared against 0 in probe.
>> >
>> > Result was a probe fail after successful parsing the DTS:
>> > max14577-regulator: probe of max14577-regulator failed with error 2
>> >
>> > Signed-off-by: Krzysztof Kozlowski <[email protected]>
>> > ---
>> > drivers/regulator/max14577.c | 4 +++-
>> > 1 file changed, 3 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/regulator/max14577.c b/drivers/regulator/max14577.c
>> > index 186df8785a91..6ba11a8dd23d 100644
>> > --- a/drivers/regulator/max14577.c
>> > +++ b/drivers/regulator/max14577.c
>> > @@ -168,11 +168,13 @@ static int max14577_regulator_dt_parse_pdata(struct platform_device *pdev)
>> > MAX14577_REG_MAX);
>> > if (ret < 0) {
>> > dev_err(&pdev->dev, "Error parsing regulator init data: %d\n", ret);
>> > + of_node_put(np);
>> > + return ret;
>> > }
>> >
>> > of_node_put(np);
>> >
>> > - return ret;
>> > + return 0;
>> > }
>>
>> Instead of so many changes here, you could simply make the following change
>> in the max14577_regulator_probe function.
>>
>> ret = max14577_regulator_dt_parse_pdata(pdev);
>> if (ret < 0)
>> return ret;
>
> That still wouldn't be sufficient because later in probe() the 'ret' is
> not reassigned (if devm_regulator_register succeeds) and it is directly
> returned at the probe() end. So still the number of matched regulators
> would be returned as probe result.
Ah, you are right. In that case how about the following:
- if (ret < 0) {
+ if (ret < 0)
dev_err(&pdev->dev, "Error parsing regulator init data: %d\n", ret);
- }
+ else
+ ret = 0;
I am just trying to see if we can avoid any code duplication.
--
With warm regards,
Sachin
On Fri, 2014-02-21 at 16:25 +0530, Sachin Kamat wrote:
> On 21 February 2014 16:12, Krzysztof Kozlowski <[email protected]> wrote:
> > On Fri, 2014-02-21 at 15:54 +0530, Sachin Kamat wrote:
> >> Hi Krzysztof,
> >>
> >> On 20 February 2014 20:49, Krzysztof Kozlowski <[email protected]> wrote:
> >> > This fixes bug introduced in 667a6b7a (regulator: max14577: Add missing
> >> > of_node_put). The DTS parsing function returned number of matched
> >> > regulators as success status which then was compared against 0 in probe.
> >> >
> >> > Result was a probe fail after successful parsing the DTS:
> >> > max14577-regulator: probe of max14577-regulator failed with error 2
> >> >
> >> > Signed-off-by: Krzysztof Kozlowski <[email protected]>
> >> > ---
> >> > drivers/regulator/max14577.c | 4 +++-
> >> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/regulator/max14577.c b/drivers/regulator/max14577.c
> >> > index 186df8785a91..6ba11a8dd23d 100644
> >> > --- a/drivers/regulator/max14577.c
> >> > +++ b/drivers/regulator/max14577.c
> >> > @@ -168,11 +168,13 @@ static int max14577_regulator_dt_parse_pdata(struct platform_device *pdev)
> >> > MAX14577_REG_MAX);
> >> > if (ret < 0) {
> >> > dev_err(&pdev->dev, "Error parsing regulator init data: %d\n", ret);
> >> > + of_node_put(np);
> >> > + return ret;
> >> > }
> >> >
> >> > of_node_put(np);
> >> >
> >> > - return ret;
> >> > + return 0;
> >> > }
> >>
> >> Instead of so many changes here, you could simply make the following change
> >> in the max14577_regulator_probe function.
> >>
> >> ret = max14577_regulator_dt_parse_pdata(pdev);
> >> if (ret < 0)
> >> return ret;
> >
> > That still wouldn't be sufficient because later in probe() the 'ret' is
> > not reassigned (if devm_regulator_register succeeds) and it is directly
> > returned at the probe() end. So still the number of matched regulators
> > would be returned as probe result.
>
> Ah, you are right. In that case how about the following:
>
> - if (ret < 0) {
> + if (ret < 0)
> dev_err(&pdev->dev, "Error parsing regulator init data: %d\n", ret);
> - }
> + else
> + ret = 0;
>
> I am just trying to see if we can avoid any code duplication.
Looks good. Thanks for idea, I'll send v2.
Best regards,
Krzysztof