Message-ID: <20210115121601.4xw2kchnkcowxlga@pengutronix.de>
Date:   Fri, 15 Jan 2021 13:16:01 +0100
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Lee Jones <lee.jones@...aro.org>, Chen-Yu Tsai <wens@...e.org>,
        Maxime Ripard <mripard@...nel.org>
Cc:     Jernej Skrabec <jernej.skrabec@...l.net>, kernel@...gutronix.de,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: [PATCH v2] bus: sunxi-rsb: make remove callback return void

The driver core ignores the return value of struct device_driver::remove
because there is only little that can be done. To simplify the quest to
make this function return void, let struct sunxi_rsb_driver::remove
return void, too.

axp20x_device_remove() always returns 0, so there is no information
lost in axp20x_rsb_remove(). The only other sunxi-rsb driver doesn't
have a remove callback and so doesn't require adaption.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
---

Hello Lee,

On Fri, Jan 15, 2021 at 11:05:43AM +0000, Lee Jones wrote:
> On Fri, 15 Jan 2021, Uwe Kleine-König wrote:
> > On Fri, Jan 15, 2021 at 08:11:22AM +0000, Lee Jones wrote:
> > > There are no dependencies between the MFD and Bus changes as far as I
> > > can tell.
> > 
> > There are dependencies, because
> > 
> > -static int axp20x_rsb_remove(struct sunxi_rsb_device *rdev)
> > +static void axp20x_rsb_remove(struct sunxi_rsb_device *rdev)
> > 
> > in drivers/mfd/axp20x-rsb.c must be done together with
> > 
> > --- a/include/linux/sunxi-rsb.h
> > +++ b/include/linux/sunxi-rsb.h
> > @@ -59,7 +59,7 @@ static inline void sunxi_rsb_device_set_drvdata(struct sunxi_rsb_device *rdev,
> >  struct sunxi_rsb_driver {
> >  	struct device_driver driver;
> >  	int (*probe)(struct sunxi_rsb_device *rdev);
> > -	int (*remove)(struct sunxi_rsb_device *rdev);
> > +	void (*remove)(struct sunxi_rsb_device *rdev);
> >  };
> >  [...]
> 
> Yes, this will need to be taken in with the MFD patch.
> 
> > > For the sake of simplicity i.e. to avoid the requirement of
> > > immutable branch maintenance and an associated pull-request, it would
> > > be better to split this out into 2 separate patches.
> > 
> > So the base for this statement is gone
> 
> It still stands.

I don't understand this. Now I dropped the simplification and just kept
the part implementing the change of struct sunxi_rsb_driver::remove to
return void.

Is the need for an immutable branch in your eyes gone now? (If yes, I
don't understand what is the relevant difference compared to the
previous patch; and if not I don't understand why you wrote "For the
sake of simplicity [...] it would be better to split this out into 2
separate patches." if even only one of the two patches you requested
still needs coordination.)

> > and the following questions remain:
> 
> >  - Do you insist on splitting out the change to axp20x_device_remove()?
> 
> [0] Unless you gave give me a compelling reason why it shouldn't, yes.
> 
> >  - Do you prefer to ack the mfd part to let the patch (or patches if
> >    they get split) go via the sunxi people or do you want to take the
> >    it (them) via mfd?
> 
> I'd prefer the MFD (and header only affecting MFD) to go in via MFD.

ok.

> The Bus patch can do in via it's own tree.

I'm not sure what you mean saying "the Bus patch". This v2 that is
still touching drivers/mfd? Probably not, because above you wrote that
the prototype change "will need to be taken in with the MFD patch". /me
is confused.

> > Looking at next there are four patches touching drivers/bus/sunxi-rsb.c
> > and none touching drivers/mfd/axp20x* or include/linux/mfd/axp20x.h
> > which suggests that letting it go via sunxi might be more sensible. IMHO
> > an immutable branch is not necessary?!
> 
> It's only -rc3 and you cannot tell the future.
> 
> If you manage to satisfy [0] and they do end up going in together, I
> will insist on an immutable branch.

I look forward to your position regarding this patch.

If this patch is simple enough to not need coordination and if adding
the simplifcation (as a separate patch) brings back this need, I'd just
go with this patch only.

Best regards
Uwe

 drivers/bus/sunxi-rsb.c   | 4 +++-
 drivers/mfd/axp20x-rsb.c  | 4 ++--
 include/linux/sunxi-rsb.h | 2 +-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
index 1bb00a959c67..117716e23ffb 100644
--- a/drivers/bus/sunxi-rsb.c
+++ b/drivers/bus/sunxi-rsb.c
@@ -170,7 +170,9 @@ static int sunxi_rsb_device_remove(struct device *dev)
 {
 	const struct sunxi_rsb_driver *drv = to_sunxi_rsb_driver(dev->driver);
 
-	return drv->remove(to_sunxi_rsb_device(dev));
+	drv->remove(to_sunxi_rsb_device(dev));
+
+	return 0;
 }
 
 static struct bus_type sunxi_rsb_bus = {
diff --git a/drivers/mfd/axp20x-rsb.c b/drivers/mfd/axp20x-rsb.c
index 4cdc79f5cc48..214bc0d84d44 100644
--- a/drivers/mfd/axp20x-rsb.c
+++ b/drivers/mfd/axp20x-rsb.c
@@ -49,11 +49,11 @@ static int axp20x_rsb_probe(struct sunxi_rsb_device *rdev)
 	return axp20x_device_probe(axp20x);
 }
 
-static int axp20x_rsb_remove(struct sunxi_rsb_device *rdev)
+static void axp20x_rsb_remove(struct sunxi_rsb_device *rdev)
 {
 	struct axp20x_dev *axp20x = sunxi_rsb_device_get_drvdata(rdev);
 
-	return axp20x_device_remove(axp20x);
+	axp20x_device_remove(axp20x);
 }
 
 static const struct of_device_id axp20x_rsb_of_match[] = {
diff --git a/include/linux/sunxi-rsb.h b/include/linux/sunxi-rsb.h
index 7e75bb0346d0..bf0d365f471c 100644
--- a/include/linux/sunxi-rsb.h
+++ b/include/linux/sunxi-rsb.h
@@ -59,7 +59,7 @@ static inline void sunxi_rsb_device_set_drvdata(struct sunxi_rsb_device *rdev,
 struct sunxi_rsb_driver {
 	struct device_driver driver;
 	int (*probe)(struct sunxi_rsb_device *rdev);
-	int (*remove)(struct sunxi_rsb_device *rdev);
+	void (*remove)(struct sunxi_rsb_device *rdev);
 };
 
 static inline struct sunxi_rsb_driver *to_sunxi_rsb_driver(struct device_driver *d)
-- 
2.29.2


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists