Re: [PATCH v4 3/3] p54: convert to sysdata API

From: "Rafał Miłecki" <zajec5@gmail.com>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: "Luis R. Rodriguez" <mcgrof@kernel.org>,
	Ming Lei <ming.lei@canonical.com>, Borislav Petkov <bp@alien8.de>,
	wagi@monom.org, Tom Gundersen <teg@jklm.no>,
	Mauro Carvalho Chehab <mchehab@osg.samsung.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Vikram Mulukutla <markivx@codeaurora.org>,
	Stephen Boyd <stephen.boyd@linaro.org>,
	Mark Brown <broonie@kernel.org>,
	zohar@linux.vnet.ibm.com, Takashi Iwai <tiwai@suse.de>,
	Johannes Berg <johannes@sipsolutions.net>,
	Christian Lamparter <chunkeey@googlemail.com>,
	Hauke Mehrtens <hauke@hauke-m.de>,
	Josh Boyer <jwboyer@fedoraproject.org>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	David Woodhouse <dwmw2@infradead.org>,
	jslaby@suse.com, Linus Torvalds <torvalds@linux-foundation.org>,
	luto@amacapital.net, Fengguang Wu <fengguang.wu@intel.com>,
	Richard Purdie <rpurdie@rpsys.net>,
	Jacek Anaszewski <j.anaszewski@samsung.com>,
	Abhay_Salunke@dell.com, Julia Lawall <Julia.Lawall@lip6.fr>,
	Gilles.Muller@lip6.fr, nicolas.palix@imag.fr,
	dhowells@redhat.com, bjorn.andersson@linaro.org,
	Arend Van Spriel <arend.vanspriel@broadcom.com>,
	Kalle Valo <kvalo@codeaurora.org>
Subject: Re: [PATCH v4 3/3] p54: convert to sysdata API
Date: Fri, 27 Jan 2017 12:25:48 +0100	[thread overview]
Message-ID: <CACna6ryHxFqGwY1d8yDWzmJwQYKZBzN7EwTcPyHRj-h-=sVaCw@mail.gmail.com> (raw)
In-Reply-To: <20170127074706.GA31112@kroah.com>

On 27 January 2017 at 08:47, Greg KH <gregkh@linuxfoundation.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?

-- 
Rafał