Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <cover.1621577204.git.matti.vaittinen@fi.rohmeurope.com>
 <e3d3e704804668d1403f3630c181010b34409c8f.1621577204.git.matti.vaittinen@fi.rohmeurope.com>
 <12bb40f022be0378ed493e7ad33122b0@walle.cc> <87a6ooh46s.fsf@miraculix.mork.no>
 <d6bb1e458d5aa6a32f31f7731e1a6097a225d634.camel@fi.rohmeurope.com>
In-Reply-To: <d6bb1e458d5aa6a32f31f7731e1a6097a225d634.camel@fi.rohmeurope.com>
From:   Bartosz Golaszewski <bgolaszewski@baylibre.com>
Date:   Fri, 21 May 2021 18:35:50 +0200
Message-ID: <CAMpxmJXkYZ7mZA426Jgm_zL+L1ZFB1ToRf2L8oGmyBuOHQo=UQ@mail.gmail.com>
Subject: Re: [PATCH v2 2/3] gpio: gpio-regmap: Use devm_add_action()
To:     "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>
Cc:     "michael@walle.cc" <michael@walle.cc>,
        "bjorn@mork.no" <bjorn@mork.no>,
        linux-power <linux-power@fi.rohmeurope.com>,
        "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linus.walleij@linaro.org" <linus.walleij@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, May 21, 2021 at 12:31 PM Vaittinen, Matti
<Matti.Vaittinen@fi.rohmeurope.com> wrote:
>
>
> On Fri, 2021-05-21 at 10:38 +0200, Bj=C3=B8rn Mork wrote:
> > Michael Walle <michael@walle.cc> writes:
> >
> > > Am 2021-05-21 08:28, schrieb Matti Vaittinen:
> > > > Slightly simplify the devm_gpio_regmap_register() by using the
> > > > devm_add_action().
> > >
> > > Hm, nice, but what bothers me a bit is that no other subsystem
> > > does it that way, eg. hwmon/hwmon.c or watchdog/watchdog_core.c.
> > > They also store just one pointer, thus could be simplified in the
> > > same way. What I don't know is if devm_add_action() was intended
> > > to be used this way. So I can't say much for this patch ;)
> >
> > There are some examples.  Like:
> >
> > int devm_i2c_add_adapter(struct device *dev, struct i2c_adapter
> > *adapter)
> > {
> >         int ret;
> >
> >         ret =3D i2c_add_adapter(adapter);
> >         if (ret)
> >                 return ret;
> >
> >         return devm_add_action_or_reset(dev, devm_i2c_del_adapter,
> > adapter);
> > }
> >
> >
> > You should probably use the devm_add_action_or_reset() wrapper here
> > too,
> > catching the unlikely devm_add_action() alloc failure.
> >
>
> I was thinking of it but as the gpio registration succeeded I was
> thinking that we could go on with it - (which means we can proceed but
> the gpio is never released.)
>
> I am not sure how much difference it makes in the case of small alloc
> failure ;)
>
> But as it seems I am in any case re-spinning this I can change this to
> the devm_add_action_or_reset() and fail the gpio_regmap registration if
> alloc fails.
>
> Best Regards
>         Matti Vaittinen

Hi Matti,

Please use the reset variant. We always want to roll-back the changes
done in a function before the failure and propagate the error code.

Bart