Date:   Fri, 27 Jan 2017 15:07:31 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Rafał Miłecki <zajec5@...il.com>
Cc:     "Luis R. Rodriguez" <mcgrof@...nel.org>,
        Ming Lei <ming.lei@...onical.com>,
        Borislav Petkov <bp@...en8.de>, wagi@...om.org,
        Tom Gundersen <teg@...m.no>,
        Mauro Carvalho Chehab <mchehab@....samsung.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Vikram Mulukutla <markivx@...eaurora.org>,
        Stephen Boyd <stephen.boyd@...aro.org>,
        Mark Brown <broonie@...nel.org>, zohar@...ux.vnet.ibm.com,
        Takashi Iwai <tiwai@...e.de>,
        Johannes Berg <johannes@...solutions.net>,
        Christian Lamparter <chunkeey@...glemail.com>,
        Hauke Mehrtens <hauke@...ke-m.de>,
        Josh Boyer <jwboyer@...oraproject.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        David Woodhouse <dwmw2@...radead.org>, jslaby@...e.com,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        luto@...capital.net, Fengguang Wu <fengguang.wu@...el.com>,
        Richard Purdie <rpurdie@...ys.net>,
        Jacek Anaszewski <j.anaszewski@...sung.com>,
        Abhay_Salunke@...l.com, Julia Lawall <Julia.Lawall@...6.fr>,
        Gilles.Muller@...6.fr, nicolas.palix@...g.fr, dhowells@...hat.com,
        bjorn.andersson@...aro.org,
        Arend Van Spriel <arend.vanspriel@...adcom.com>,
        Kalle Valo <kvalo@...eaurora.org>
Subject: Re: [PATCH v4 3/3] p54: convert to sysdata API

On Fri, Jan 27, 2017 at 12:25:48PM +0100, Rafał Miłecki wrote:
> On 27 January 2017 at 08:47, Greg KH <gregkh@...uxfoundation.org> wrote:
> > On Thu, Jan 26, 2017 at 10:50:05PM +0100, Luis R. Rodriguez wrote:
> >> On Thu, Jan 19, 2017 at 05:27:51PM +0100, Luis R. Rodriguez wrote:
> >> > On Thu, Jan 19, 2017 at 12:38:57PM +0100, Greg KH wrote:
> >> > > On Thu, Jan 12, 2017 at 07:02:44AM -0800, Luis R. Rodriguez wrote:
> >> > > > ---
> >> > > >  drivers/net/wireless/intersil/p54/eeprom.c |  2 +-
> >> > > >  drivers/net/wireless/intersil/p54/fwio.c   |  5 +-
> >> > > >  drivers/net/wireless/intersil/p54/led.c    |  2 +-
> >> > > >  drivers/net/wireless/intersil/p54/main.c   |  2 +-
> >> > > >  drivers/net/wireless/intersil/p54/p54.h    |  3 +-
> >> > > >  drivers/net/wireless/intersil/p54/p54pci.c | 26 ++++++----
> >> > > >  drivers/net/wireless/intersil/p54/p54pci.h |  4 +-
> >> > > >  drivers/net/wireless/intersil/p54/p54spi.c | 80 +++++++++++++++++++-----------
> >> > > >  drivers/net/wireless/intersil/p54/p54spi.h |  2 +-
> >> > > >  drivers/net/wireless/intersil/p54/p54usb.c | 18 +++----
> >> > > >  drivers/net/wireless/intersil/p54/p54usb.h |  4 +-
> >> > > >  drivers/net/wireless/intersil/p54/txrx.c   |  2 +-
> >> > > >  12 files changed, 89 insertions(+), 61 deletions(-)
> >> > >
> >> > > why does the "new" api require more lines?
> >> >
> >> > This is a bare bones flexible API with only a few new tiny features to start
> >> > with, one of them was to enable the API do the freeing of the driver data for
> >> > you. In the kernel we have devres to help with this but devres only helps if
> >> > you would use the API call on probe. We want to support the ability to let the
> >> > API free the driver data for you even if your call is outside of probe, for this
> >> > to work we need a callback. For async calls this is rather trivial given we
> >> > already have a callback, for sync calls this means a new routine is needed.
> >> > Freeing the data for you is an option, but I decided to keep the callback
> >> > requirement even if you didn't want the free'ing to be done for you. The
> >> > addition of a callback is what accounts for the slight increase on this driver.
> >> >
> >> > I could try avoiding the callback if no freeing is needed.
> >>
> >> OK I've added a respective helper call which would map 1-1 with the
> >> old sync mechanism to enable a 1-1 change, this will be called
> >> driver_data_request_simple(), but let me know if there is a preference
> >> for something else.
> >>
> >> With this the only visible delta now is from taking advantage of new
> >> features. In p54's case this would re-organize the mess in
> >> drivers/net/wireless/intersil/p54/p54spi.c, the diff stat is a bit
> >> larger for that file just because of this but I think in this case
> >> its very much worth the small additions. In this case two routines are
> >> added for handling the work through callbacks on a sync call.
> >>
> >>  1 file changed, 38 insertions(+), 30 deletions(-)
> >
> > I agree with Linus, as well as, look, it's still bigger, so you are
> > making driver developers do more work :(
> >
> >>       /* FIXME: should driver use it's own struct device? */
> >> -     ret = request_firmware(&priv->firmware, "3826.arm", &priv->spi->dev);
> >> -
> >> -     if (ret < 0) {
> >> -             dev_err(&priv->spi->dev, "request_firmware() failed: %d", ret);
> >> +     ret = driver_data_request_simple("3826.arm", &priv->spi->dev,
> >> +                                      &priv->firmware);
> >> +     if (ret < 0)
> >>               return ret;
> >> -     }
> >
> > Hm, a FIXME that you aren't fixing :(
> >
> > I still fail to see why this new api is worth it at all, sorry.
> 
> Maybe we could try cleaning up existing firmware API and see if we
> really hit something that can't be solved in any sane way? What do you
> think?
> 
> I'd love to help with that, I started with a trivial cleaning patch:
> [PATCH V2] firmware: simplify defining and handling FW_OPT_FALLBACK
> https://patchwork.kernel.org/patch/9469875/
> 
> It didn't receive any real negative comments but I also have no idea
> how could pick it up for me and send in some pull request. Any
> suggestions?

Does that patch really "simplify" anything?  Anyway, resend it if the
maintainer of the subsystem ignores it (you did cc: the correct people,
right?)

And yes, I always like seeing things that simplify apis, and the
firmware interface could really use that, which I thought is what Luis
was trying to do here, which is why I keep pushing back on this
patchset as it doesn't seem to be happening.

thanks,

greg k-h

Powered by blists - more mailing lists