diff mbox series

[v8,03/13] squash! max9286: Fix cleanup path from GPIO powerdown

Message ID 20200409121202.11130-4-kieran.bingham+renesas@ideasonboard.com (mailing list archive)
State Accepted
Delegated to: Kieran Bingham
Headers show
Series max9286 v8 - modifications | expand

Commit Message

Kieran Bingham April 9, 2020, 12:11 p.m. UTC 
- Fix up cleanup path from GPIO PowerDown registration
---
 drivers/media/i2c/max9286.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Comments

Jacopo Mondi April 9, 2020, 4:22 p.m. UTC | #1 
Hi Kieran,
  slightly unrelated on this patch but

On Thu, Apr 09, 2020 at 01:11:52PM +0100, Kieran Bingham wrote:
>  - Fix up cleanup path from GPIO PowerDown registration
> ---
>  drivers/media/i2c/max9286.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 0a43137b8112..cc99740b34c5 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -1171,8 +1171,10 @@ static int max9286_probe(struct i2c_client *client)
>
>  	priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable",
>  						   GPIOD_OUT_HIGH);
> -	if (IS_ERR(priv->gpiod_pwdn))
> -		return PTR_ERR(priv->gpiod_pwdn);
> +	if (IS_ERR(priv->gpiod_pwdn)) {
> +		ret = PTR_ERR(priv->gpiod_pwdn);
> +		goto err_cleanup_dt;
> +	}
>
>  	gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn");
>  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 1);

As we get_optional(), shouldn't this be protected by an
        if (priv->gpiod_pwdn)

 ?


Another point (sorry, I'm looking at gpio handling only now) we have
        ret = max9286_gpio(priv);
        if (ret)
                return ret;

That's not really a descriptive function name.. Could this be
        max9286_register_gpiochip()
?

One last point and then I'll stop.

We currently do

probe() {
        parse_dt()

        pwdn = devm_get_gpio_optional()
        if (err)
                goto err_cleanup_dt()
        set(pwdn, 1);

        register_gpiochip(); //uses devm
                goto err_cleanup_dt()

        get_regulator()
                goto err_cleanup_dt()

        ret = init()
        if (ret)
                goto err_regulator();

        return 0

err_regulator:
        regulator_put()
        mux_close()
        i2c_ack_disable()
err_cleanup_dt:
       cleanup_dt()

}

With patch 5 of this series this becomes

probe() {
        parse_dt()

        pwdn = devm_get_gpio_optional()
        if (err)
                goto err_cleanup_dt()
        set(pwdn, 1);

        register_gpiochip(); //uses devm
                goto err_cleanup_dt()

        devm_get_regulator()
                goto err_cleanup_dt()

        ret = init()
        if (ret)
                goto err_regulator();

        return 0

err_regulator:
        mux_close()
        i2c_ack_disable()
err_cleanup_dt:
       cleanup_dt()
}

as the i2c_mux is already closed at the end of init() (or never open
if we fail earlier) and i2c_ack can be disabled at the end of
max9286_setup() and in the error path there (as there are no more i2c
writes after that function returns), I think we could simplify all of
thise even more to:

probe() {
        pwdn = devm_get_gpio_optional()
        if (err)
                return;

        set(pwdn, 1);

        register_gpiochip(); //uses devm
                return;

        devm_get_regulator()
                return;

        parse_dt()

        ret = init()
        if (ret)
                goto cleanup_dt();

        return 0

err_cleanup_dt:
       cleanup_dt()
}

This could be done after 5/5 in this series if you want to keep fixups
separate for another review round.

What do you think ?

Thanks
   j



> @@ -1193,7 +1195,7 @@ static int max9286_probe(struct i2c_client *client)
>  				PTR_ERR(priv->regulator));
>  		ret = PTR_ERR(priv->regulator);
>  		priv->regulator = NULL;
> -		goto err_free;
> +		goto err_cleanup_dt;
>  	}
>
>  	/*
> @@ -1230,7 +1232,7 @@ static int max9286_probe(struct i2c_client *client)
>  	regulator_put(priv->regulator);
>  	max9286_i2c_mux_close(priv);
>  	max9286_configure_i2c(priv, false);
> -err_free:
> +err_cleanup_dt:
>  	max9286_cleanup_dt(priv);
>
>  	return ret;
> @@ -1248,10 +1250,10 @@ static int max9286_remove(struct i2c_client *client)
>  		regulator_disable(priv->regulator);
>  	regulator_put(priv->regulator);
>
> -	max9286_cleanup_dt(priv);
> -
>  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
>
> +	max9286_cleanup_dt(priv);
> +
>  	return 0;
>  }
>
> --
> 2.20.1
>
Geert Uytterhoeven April 10, 2020, 7:34 a.m. UTC | #2 
Hi Jacopo,

On Thu, Apr 9, 2020 at 7:07 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
> On Thu, Apr 09, 2020 at 01:11:52PM +0100, Kieran Bingham wrote:
> >  - Fix up cleanup path from GPIO PowerDown registration
> > ---
> >  drivers/media/i2c/max9286.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index 0a43137b8112..cc99740b34c5 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -1171,8 +1171,10 @@ static int max9286_probe(struct i2c_client *client)
> >
> >       priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable",
> >                                                  GPIOD_OUT_HIGH);
> > -     if (IS_ERR(priv->gpiod_pwdn))
> > -             return PTR_ERR(priv->gpiod_pwdn);
> > +     if (IS_ERR(priv->gpiod_pwdn)) {
> > +             ret = PTR_ERR(priv->gpiod_pwdn);
> > +             goto err_cleanup_dt;
> > +     }
> >
> >       gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn");
> >       gpiod_set_value_cansleep(priv->gpiod_pwdn, 1);
>
> As we get_optional(), shouldn't this be protected by an
>         if (priv->gpiod_pwdn)
>
>  ?

When passed a NULL desc, validate_desc() just returns 0, without printing
an error message, so both functions become no-ops.

Gr{oetje,eeting}s,

                        Geert
Kieran Bingham April 10, 2020, 7:38 a.m. UTC | #3 
Hi Jacopo,

On 09/04/2020 17:22, Jacopo Mondi wrote:
> Hi Kieran,
>   slightly unrelated on this patch but

Everything's related :-) All comments welcome!

> 
> On Thu, Apr 09, 2020 at 01:11:52PM +0100, Kieran Bingham wrote:
>>  - Fix up cleanup path from GPIO PowerDown registration
>> ---
>>  drivers/media/i2c/max9286.c | 14 ++++++++------
>>  1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
>> index 0a43137b8112..cc99740b34c5 100644
>> --- a/drivers/media/i2c/max9286.c
>> +++ b/drivers/media/i2c/max9286.c
>> @@ -1171,8 +1171,10 @@ static int max9286_probe(struct i2c_client *client)
>>
>>  	priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable",
>>  						   GPIOD_OUT_HIGH);
>> -	if (IS_ERR(priv->gpiod_pwdn))
>> -		return PTR_ERR(priv->gpiod_pwdn);
>> +	if (IS_ERR(priv->gpiod_pwdn)) {
>> +		ret = PTR_ERR(priv->gpiod_pwdn);
>> +		goto err_cleanup_dt;
>> +	}
>>
>>  	gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn");
>>  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 1);
> 
> As we get_optional(), shouldn't this be protected by an
>         if (priv->gpiod_pwdn)
> 
>  ?
> 

Err - yes! That's odd - I was sure I had tested this without a gpio
specifier, and I thought those functions were null-safe, and were a
no-op if there was no desc. Clearly some wire got crossed in my head -
because I see no such null-checks now!


I've added a new squash patch on top to correct for this (including
checking priv->gpiod_pwdn on all uses).


> Another point (sorry, I'm looking at gpio handling only now) we have
>         ret = max9286_gpio(priv);
>         if (ret)
>                 return ret;
> 
> That's not really a descriptive function name.. Could this be
>         max9286_register_gpiochip()

Yup, I'm happy enough to rename that.

Patch added on top.



> ?
> 
> One last point and then I'll stop.
> 
> We currently do
> 
> probe() {
>         parse_dt()
> 
>         pwdn = devm_get_gpio_optional()
>         if (err)
>                 goto err_cleanup_dt()
>         set(pwdn, 1);
> 
>         register_gpiochip(); //uses devm
>                 goto err_cleanup_dt()
> 
>         get_regulator()
>                 goto err_cleanup_dt()
> 
>         ret = init()
>         if (ret)
>                 goto err_regulator();
> 
>         return 0
> 
> err_regulator:
>         regulator_put()
>         mux_close()
>         i2c_ack_disable()
> err_cleanup_dt:
>        cleanup_dt()
> 
> }
> 
> With patch 5 of this series this becomes
> 
> probe() {
>         parse_dt()
> 
>         pwdn = devm_get_gpio_optional()
>         if (err)
>                 goto err_cleanup_dt()
>         set(pwdn, 1);
> 
>         register_gpiochip(); //uses devm
>                 goto err_cleanup_dt()
> 
>         devm_get_regulator()
>                 goto err_cleanup_dt()
> 
>         ret = init()
>         if (ret)
>                 goto err_regulator();
> 
>         return 0
> 
> err_regulator:
>         mux_close()
>         i2c_ack_disable()
> err_cleanup_dt:
>        cleanup_dt()
> }
> 
> as the i2c_mux is already closed at the end of init() (or never open
> if we fail earlier) and i2c_ack can be disabled at the end of
> max9286_setup() and in the error path there (as there are no more i2c
> writes after that function returns), I think we could simplify all of
> thise even more to:
> 
> probe() {
>         pwdn = devm_get_gpio_optional()
>         if (err)
>                 return;
> 
>         set(pwdn, 1);
> 
>         register_gpiochip(); //uses devm
>                 return;
> 
>         devm_get_regulator()
>                 return;
> 
>         parse_dt()
> 
>         ret = init()
>         if (ret)
>                 goto cleanup_dt();
> 
>         return 0
> 
> err_cleanup_dt:
>        cleanup_dt()
> }
> 
> This could be done after 5/5 in this series if you want to keep fixups
> separate for another review round.
> 

Yes, indeed - it would make sense where possible to have the devm_
handled constructions before the manually managed ones.

I'll add a patch on top.


> What do you think ?
> 
> Thanks
>    j
> 
> 
> 
>> @@ -1193,7 +1195,7 @@ static int max9286_probe(struct i2c_client *client)
>>  				PTR_ERR(priv->regulator));
>>  		ret = PTR_ERR(priv->regulator);
>>  		priv->regulator = NULL;
>> -		goto err_free;
>> +		goto err_cleanup_dt;
>>  	}
>>
>>  	/*
>> @@ -1230,7 +1232,7 @@ static int max9286_probe(struct i2c_client *client)
>>  	regulator_put(priv->regulator);
>>  	max9286_i2c_mux_close(priv);
>>  	max9286_configure_i2c(priv, false);
>> -err_free:
>> +err_cleanup_dt:
>>  	max9286_cleanup_dt(priv);
>>
>>  	return ret;
>> @@ -1248,10 +1250,10 @@ static int max9286_remove(struct i2c_client *client)
>>  		regulator_disable(priv->regulator);
>>  	regulator_put(priv->regulator);
>>
>> -	max9286_cleanup_dt(priv);
>> -
>>  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
>>
>> +	max9286_cleanup_dt(priv);
>> +
>>  	return 0;
>>  }
>>
>> --
>> 2.20.1
>>
Kieran Bingham April 10, 2020, 7:41 a.m. UTC | #4 
On 10/04/2020 08:34, Geert Uytterhoeven wrote:
> Hi Jacopo,
> 
> On Thu, Apr 9, 2020 at 7:07 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>> On Thu, Apr 09, 2020 at 01:11:52PM +0100, Kieran Bingham wrote:
>>>  - Fix up cleanup path from GPIO PowerDown registration
>>> ---
>>>  drivers/media/i2c/max9286.c | 14 ++++++++------
>>>  1 file changed, 8 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
>>> index 0a43137b8112..cc99740b34c5 100644
>>> --- a/drivers/media/i2c/max9286.c
>>> +++ b/drivers/media/i2c/max9286.c
>>> @@ -1171,8 +1171,10 @@ static int max9286_probe(struct i2c_client *client)
>>>
>>>       priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable",
>>>                                                  GPIOD_OUT_HIGH);
>>> -     if (IS_ERR(priv->gpiod_pwdn))
>>> -             return PTR_ERR(priv->gpiod_pwdn);
>>> +     if (IS_ERR(priv->gpiod_pwdn)) {
>>> +             ret = PTR_ERR(priv->gpiod_pwdn);
>>> +             goto err_cleanup_dt;
>>> +     }
>>>
>>>       gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn");
>>>       gpiod_set_value_cansleep(priv->gpiod_pwdn, 1);
>>
>> As we get_optional(), shouldn't this be protected by an
>>         if (priv->gpiod_pwdn)
>>
>>  ?
> 
> When passed a NULL desc, validate_desc() just returns 0, without printing
> an error message, so both functions become no-ops.

Aha! I knew I'd tested that code path :-)

Thanks for highlighting that, I missed it just 10 minutes ago when I
rechecked because:

 	VALIDATE_DESC(desc);

has a *HIDDEN RETURN* dammit:

	if (__valid <= 0) \
		return __valid; \
	} while (0)

Honestly - I thought we didn't do that in the kernel for exactly this
reason. Grrrrrrrrrr ... oh and grrrrr again !

They could have at least named it:

 VALIDATE_DESC_OR_RETURN(desc)



> 
> Gr{oetje,eeting}s,
> 
>                         Geert
>
Kieran Bingham April 10, 2020, 7:42 a.m. UTC | #5 
On 10/04/2020 08:38, Kieran Bingham wrote:
> Hi Jacopo,
>>>
>>>  	priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable",
>>>  						   GPIOD_OUT_HIGH);
>>> -	if (IS_ERR(priv->gpiod_pwdn))
>>> -		return PTR_ERR(priv->gpiod_pwdn);
>>> +	if (IS_ERR(priv->gpiod_pwdn)) {
>>> +		ret = PTR_ERR(priv->gpiod_pwdn);
>>> +		goto err_cleanup_dt;
>>> +	}
>>>
>>>  	gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn");
>>>  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 1);
>>
>> As we get_optional(), shouldn't this be protected by an
>>         if (priv->gpiod_pwdn)
>>
>>  ?
>>
> 
> Err - yes! That's odd - I was sure I had tested this without a gpio
> specifier, and I thought those functions were null-safe, and were a
> no-op if there was no desc. Clearly some wire got crossed in my head -
> because I see no such null-checks now!


Ahem, ok - so as highlighted by Geert, - these *are* NULL safe...

> 
> 
> I've added a new squash patch on top to correct for this (including
> checking priv->gpiod_pwdn on all uses).

now dropped :-)
Jacopo Mondi April 10, 2020, 7:52 a.m. UTC | #6 
Hi Geert,

On Fri, Apr 10, 2020 at 09:34:28AM +0200, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Thu, Apr 9, 2020 at 7:07 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
> > On Thu, Apr 09, 2020 at 01:11:52PM +0100, Kieran Bingham wrote:
> > >  - Fix up cleanup path from GPIO PowerDown registration
> > > ---
> > >  drivers/media/i2c/max9286.c | 14 ++++++++------
> > >  1 file changed, 8 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > > index 0a43137b8112..cc99740b34c5 100644
> > > --- a/drivers/media/i2c/max9286.c
> > > +++ b/drivers/media/i2c/max9286.c
> > > @@ -1171,8 +1171,10 @@ static int max9286_probe(struct i2c_client *client)
> > >
> > >       priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable",
> > >                                                  GPIOD_OUT_HIGH);
> > > -     if (IS_ERR(priv->gpiod_pwdn))
> > > -             return PTR_ERR(priv->gpiod_pwdn);
> > > +     if (IS_ERR(priv->gpiod_pwdn)) {
> > > +             ret = PTR_ERR(priv->gpiod_pwdn);
> > > +             goto err_cleanup_dt;
> > > +     }
> > >
> > >       gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn");
> > >       gpiod_set_value_cansleep(priv->gpiod_pwdn, 1);
> >
> > As we get_optional(), shouldn't this be protected by an
> >         if (priv->gpiod_pwdn)
> >
> >  ?
>
> When passed a NULL desc, validate_desc() just returns 0, without printing
> an error message, so both functions become no-ops.
>

Ah indeed, sorry I have overlooked this!

Thanks for pointing this out and sorry Kieran for the noise!

> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
Jacopo Mondi April 11, 2020, 12:33 p.m. UTC | #7 
HI Kieran,

On Thu, Apr 09, 2020 at 06:22:12PM +0200, Jacopo Mondi wrote:
> Hi Kieran,
>   slightly unrelated on this patch but
>
> On Thu, Apr 09, 2020 at 01:11:52PM +0100, Kieran Bingham wrote:
> >  - Fix up cleanup path from GPIO PowerDown registration
> > ---
> >  drivers/media/i2c/max9286.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index 0a43137b8112..cc99740b34c5 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -1171,8 +1171,10 @@ static int max9286_probe(struct i2c_client *client)
> >
> >  	priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable",
> >  						   GPIOD_OUT_HIGH);
> > -	if (IS_ERR(priv->gpiod_pwdn))
> > -		return PTR_ERR(priv->gpiod_pwdn);
> > +	if (IS_ERR(priv->gpiod_pwdn)) {
> > +		ret = PTR_ERR(priv->gpiod_pwdn);
> > +		goto err_cleanup_dt;
> > +	}
> >
> >  	gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn");
> >  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 1);
>
>

[snip]

> as the i2c_mux is already closed at the end of init() (or never open
> if we fail earlier) and i2c_ack can be disabled at the end of
> max9286_setup() and in the error path there (as there are no more i2c
> writes after that function returns), I think we could simplify all of

Knowing you're working on a new squash! series, I've just noticed I've
said somthing not correct here.

> i2c_ack can be disabled at the end of
> max9286_setup() and in the error path there (as there are no more i2c
> writes after that function returns)

That's not true, i2c auto ack should be disabled after registering the
i2c adapters for the remote ends. i2c_add_adapter probes the remote
ends, and so -a lot- of i2c writes take place.

Sorry for the confusion, I think i2c auto ack could be disabled at the
end of _init() not _setup().

Thanks
   j

> probe() {
>         pwdn = devm_get_gpio_optional()
>         if (err)
>                 return;
>
>         set(pwdn, 1);
>
>         register_gpiochip(); //uses devm
>                 return;
>
>         devm_get_regulator()
>                 return;
>
>         parse_dt()
>
>         ret = init()
>         if (ret)
>                 goto cleanup_dt();
>
>         return 0
>
> err_cleanup_dt:
>        cleanup_dt()
> }
>
> This could be done after 5/5 in this series if you want to keep fixups
> separate for another review round.
>
> What do you think ?
>
> Thanks
>    j
>
>
>
> > @@ -1193,7 +1195,7 @@ static int max9286_probe(struct i2c_client *client)
> >  				PTR_ERR(priv->regulator));
> >  		ret = PTR_ERR(priv->regulator);
> >  		priv->regulator = NULL;
> > -		goto err_free;
> > +		goto err_cleanup_dt;
> >  	}
> >
> >  	/*
> > @@ -1230,7 +1232,7 @@ static int max9286_probe(struct i2c_client *client)
> >  	regulator_put(priv->regulator);
> >  	max9286_i2c_mux_close(priv);
> >  	max9286_configure_i2c(priv, false);
> > -err_free:
> > +err_cleanup_dt:
> >  	max9286_cleanup_dt(priv);
> >
> >  	return ret;
> > @@ -1248,10 +1250,10 @@ static int max9286_remove(struct i2c_client *client)
> >  		regulator_disable(priv->regulator);
> >  	regulator_put(priv->regulator);
> >
> > -	max9286_cleanup_dt(priv);
> > -
> >  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
> >
> > +	max9286_cleanup_dt(priv);
> > +
> >  	return 0;
> >  }
> >
> > --
> > 2.20.1
> >
diff mbox series

Patch

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 0a43137b8112..cc99740b34c5 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -1171,8 +1171,10 @@  static int max9286_probe(struct i2c_client *client)
 
 	priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable",
 						   GPIOD_OUT_HIGH);
-	if (IS_ERR(priv->gpiod_pwdn))
-		return PTR_ERR(priv->gpiod_pwdn);
+	if (IS_ERR(priv->gpiod_pwdn)) {
+		ret = PTR_ERR(priv->gpiod_pwdn);
+		goto err_cleanup_dt;
+	}
 
 	gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn");
 	gpiod_set_value_cansleep(priv->gpiod_pwdn, 1);
@@ -1193,7 +1195,7 @@  static int max9286_probe(struct i2c_client *client)
 				PTR_ERR(priv->regulator));
 		ret = PTR_ERR(priv->regulator);
 		priv->regulator = NULL;
-		goto err_free;
+		goto err_cleanup_dt;
 	}
 
 	/*
@@ -1230,7 +1232,7 @@  static int max9286_probe(struct i2c_client *client)
 	regulator_put(priv->regulator);
 	max9286_i2c_mux_close(priv);
 	max9286_configure_i2c(priv, false);
-err_free:
+err_cleanup_dt:
 	max9286_cleanup_dt(priv);
 
 	return ret;
@@ -1248,10 +1250,10 @@  static int max9286_remove(struct i2c_client *client)
 		regulator_disable(priv->regulator);
 	regulator_put(priv->regulator);
 
-	max9286_cleanup_dt(priv);
-
 	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
 
+	max9286_cleanup_dt(priv);
+
 	return 0;
 }