From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932537AbdA0OIj (ORCPT ); Fri, 27 Jan 2017 09:08:39 -0500 Received: from mail.linuxfoundation.org ([140.211.169.12]:53582 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932180AbdA0OHb (ORCPT ); Fri, 27 Jan 2017 09:07:31 -0500 Date: Fri, 27 Jan 2017 15:07:31 +0100 From: Greg KH To: =?utf-8?B?UmFmYcWCIE1pxYJlY2tp?= Cc: "Luis R. Rodriguez" , Ming Lei , Borislav Petkov , wagi@monom.org, Tom Gundersen , Mauro Carvalho Chehab , Linux Kernel Mailing List , Vikram Mulukutla , Stephen Boyd , Mark Brown , zohar@linux.vnet.ibm.com, Takashi Iwai , Johannes Berg , Christian Lamparter , Hauke Mehrtens , Josh Boyer , Dmitry Torokhov , David Woodhouse , jslaby@suse.com, Linus Torvalds , luto@amacapital.net, Fengguang Wu , Richard Purdie , Jacek Anaszewski , Abhay_Salunke@dell.com, Julia Lawall , Gilles.Muller@lip6.fr, nicolas.palix@imag.fr, dhowells@redhat.com, bjorn.andersson@linaro.org, Arend Van Spriel , Kalle Valo Subject: Re: [PATCH v4 3/3] p54: convert to sysdata API Message-ID: <20170127140731.GB8206@kroah.com> References: <20161216114632.22559-1-mcgrof@kernel.org> <20170112150244.12700-1-mcgrof@kernel.org> <20170112150244.12700-4-mcgrof@kernel.org> <20170119113857.GQ28024@kroah.com> <20170119162751.GJ13946@wotan.suse.de> <20170126215005.GU13946@wotan.suse.de> <20170127074706.GA31112@kroah.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.7.2 (2016-11-26) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jan 27, 2017 at 12:25:48PM +0100, Rafał Miłecki wrote: > On 27 January 2017 at 08:47, Greg KH 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