From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754154AbdAZVuN (ORCPT ); Thu, 26 Jan 2017 16:50:13 -0500 Received: from mx2.suse.de ([195.135.220.15]:55471 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753500AbdAZVuL (ORCPT ); Thu, 26 Jan 2017 16:50:11 -0500 Date: Thu, 26 Jan 2017 22:50:05 +0100 From: "Luis R. Rodriguez" To: "Luis R. Rodriguez" Cc: Greg KH , ming.lei@canonical.com, bp@alien8.de, wagi@monom.org, teg@jklm.no, mchehab@osg.samsung.com, zajec5@gmail.com, linux-kernel@vger.kernel.org, markivx@codeaurora.org, stephen.boyd@linaro.org, broonie@kernel.org, zohar@linux.vnet.ibm.com, tiwai@suse.de, johannes@sipsolutions.net, chunkeey@googlemail.com, hauke@hauke-m.de, jwboyer@fedoraproject.org, dmitry.torokhov@gmail.com, dwmw2@infradead.org, jslaby@suse.com, torvalds@linux-foundation.org, luto@amacapital.net, fengguang.wu@intel.com, rpurdie@rpsys.net, j.anaszewski@samsung.com, Abhay_Salunke@dell.com, Julia.Lawall@lip6.fr, Gilles.Muller@lip6.fr, nicolas.palix@imag.fr, dhowells@redhat.com, bjorn.andersson@linaro.org, arend.vanspriel@broadcom.com, kvalo@codeaurora.org Subject: Re: [PATCH v4 3/3] p54: convert to sysdata API Message-ID: <20170126215005.GU13946@wotan.suse.de> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170119162751.GJ13946@wotan.suse.de> User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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(-) diff --git a/drivers/net/wireless/intersil/p54/p54spi.c b/drivers/net/wireless/intersil/p54/p54spi.c index 7ab2f43ab425..6183a8bfa149 100644 --- a/drivers/net/wireless/intersil/p54/p54spi.c +++ b/drivers/net/wireless/intersil/p54/p54spi.c @@ -23,7 +23,7 @@ #include #include #include -#include +#include #include #include #include @@ -168,47 +168,55 @@ static int p54spi_request_firmware(struct ieee80211_hw *dev) int ret; /* 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; - } ret = p54_parse_firmware(dev, priv->firmware); if (ret) { - release_firmware(priv->firmware); + release_driver_data(priv->firmware); return ret; } - return 0; } -static int p54spi_request_eeprom(struct ieee80211_hw *dev) +#ifdef CONFIG_P54_SPI_DEFAULT_EEPROM +static int p54spi_load_eeprom_default(void *context) { - struct p54s_priv *priv = dev->priv; - const struct firmware *eeprom; - int ret; + struct p54s_priv *priv = context; + struct ieee80211_hw *dev = priv->hw; - /* allow users to customize their eeprom. - */ + dev_info(&priv->spi->dev, "loading default eeprom...\n"); + return p54_parse_eeprom(dev, (void *) p54spi_eeprom, + sizeof(p54spi_eeprom)); +} +#endif - ret = request_firmware_direct(&eeprom, "3826.eeprom", &priv->spi->dev); - if (ret < 0) { +static int p54spi_load_eeprom_cb(void *context, + const struct driver_data *driver_data) +{ + struct p54s_priv *priv = context; + struct ieee80211_hw *dev = priv->hw; + + dev_info(&priv->spi->dev, "loading user eeprom...\n"); + return p54_parse_eeprom(dev, (void *) driver_data->data, + (int)driver_data->size); +} +static int p54spi_request_eeprom(struct ieee80211_hw *dev) +{ + struct p54s_priv *priv = dev->priv; + const struct driver_data_req_params req_params = { + DRIVER_DATA_DEFAULT_SYNC(p54spi_load_eeprom_cb, priv), #ifdef CONFIG_P54_SPI_DEFAULT_EEPROM - dev_info(&priv->spi->dev, "loading default eeprom...\n"); - ret = p54_parse_eeprom(dev, (void *) p54spi_eeprom, - sizeof(p54spi_eeprom)); -#else - dev_err(&priv->spi->dev, "Failed to request user eeprom\n"); -#endif /* CONFIG_P54_SPI_DEFAULT_EEPROM */ - } else { - dev_info(&priv->spi->dev, "loading user eeprom...\n"); - ret = p54_parse_eeprom(dev, (void *) eeprom->data, - (int)eeprom->size); - release_firmware(eeprom); - } - return ret; + DRIVER_DATA_SYNC_OPT_CB(p54spi_load_eeprom_default, priv), +#endif + }; + /* + * allow users to customize their eeprom. + */ + return driver_data_request("3826.eeprom", &req_params, + &priv->spi->dev); } static int p54spi_upload_firmware(struct ieee80211_hw *dev) @@ -692,7 +700,7 @@ static int p54spi_remove(struct spi_device *spi) gpio_free(p54spi_gpio_power); gpio_free(p54spi_gpio_irq); - release_firmware(priv->firmware); + release_driver_data(priv->firmware); mutex_destroy(&priv->mutex);