Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1567920

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

Show all headers | View raw

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.

greg k-h

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

Re: [PATCH v4 3/3] p54: convert to sysdata API Greg KH <gregkh@linuxfoundation.org> - 2017-01-19 12:50 +0100
  Re: [PATCH v4 3/3] p54: convert to sysdata API "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-01-19 17:30 +0100
    Re: [PATCH v4 3/3] p54: convert to sysdata API Linus Torvalds <torvalds@linux-foundation.org> - 2017-01-26 23:00 +0100
      Re: [PATCH v4 3/3] p54: convert to sysdata API "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-01-27 19:30 +0100
        Re: [PATCH v4 3/3] p54: convert to sysdata API Linus Torvalds <torvalds@linux-foundation.org> - 2017-01-27 22:00 +0100
          Re: [PATCH v4 3/3] p54: convert to sysdata API "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-01-27 22:40 +0100
    Re: [PATCH v4 3/3] p54: convert to sysdata API "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-01-26 23:00 +0100
      Re: [PATCH v4 3/3] p54: convert to sysdata API Greg KH <gregkh@linuxfoundation.org> - 2017-01-27 08:50 +0100
        Re: [PATCH v4 3/3] p54: convert to sysdata API Rafał Miłecki <zajec5@gmail.com> - 2017-01-27 12:30 +0100
          Re: [PATCH v4 3/3] p54: convert to sysdata API Greg KH <gregkh@linuxfoundation.org> - 2017-01-27 15:10 +0100
            Re: [PATCH v4 3/3] p54: convert to sysdata API Rafał Miłecki <zajec5@gmail.com> - 2017-01-27 15:20 +0100
              Re: [PATCH v4 3/3] p54: convert to sysdata API Greg KH <gregkh@linuxfoundation.org> - 2017-01-27 15:40 +0100
                Re: [PATCH v4 3/3] p54: convert to sysdata API Rafał Miłecki <zajec5@gmail.com> - 2017-01-27 15:50 +0100
                Re: [PATCH v4 3/3] p54: convert to sysdata API "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-01-27 22:40 +0100

csiph-web