On 27 January 2017 at 15:07, Greg KH <gre...@linuxfoundation.org> wrote:
> On Fri, Jan 27, 2017 at 12:25:48PM +0100, Rafał Miłecki wrote:
>> On 27 January 2017 at 08:47, Greg KH <gre...@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?
>
> Does that patch really "simplify" anything? Anyway, resend it if the
> maintainer of the subsystem ignores it (you did cc: the correct people,
> right?)
According to the MAINTAINERS there isn't firmware API tree /
maintainer. Also this is just a cleanup so I don't know if I should
expect some random maintainer (e.g. wireless tree) to pick it.
--
Rafał
