From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753602AbdASQ2U (ORCPT ); Thu, 19 Jan 2017 11:28:20 -0500 Received: from mx2.suse.de ([195.135.220.15]:33726 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753478AbdASQ16 (ORCPT ); Thu, 19 Jan 2017 11:27:58 -0500 Date: Thu, 19 Jan 2017 17:27:51 +0100 From: "Luis R. Rodriguez" To: Greg KH Cc: "Luis R. Rodriguez" , 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: <20170119162751.GJ13946@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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170119113857.GQ28024@kroah.com> 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 12:38:57PM +0100, Greg KH wrote: > On Thu, Jan 12, 2017 at 07:02:44AM -0800, Luis R. Rodriguez wrote: > > The Coccinelle sysdata patches were used to help with > > this transition. The changes have been carefully manually > > vetted for. With the conversion we modify the cases that do > > not need the firmware to be kept so that the sysdata API > > can release it for us. Using the new sysdata API also means > > we can get rid of our own completions. > > > > v2: was not present > > v3: initial release > > v4: small cosmetic fixes > > v5: bike shed changes > > v6: forgot to change one piece of code during the bikeshed name change > > > > Generated-by: Coccinelle SmPL > > What is this tag for? Every no wand then some tool scrapes for commit logs to see if Coccinelle was used to help with a kernel commit. There are different heuristics, this tag is to help make a more unique search more easily identifiable as I used Coccinelle to do the original port. > Also, meta-comment, put your vN: lines below the --- line like the > kernel documentation says to do. Oh, OK sounds good. > > Signed-off-by: Luis R. Rodriguez > > --- > > 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. > > --- a/drivers/net/wireless/intersil/p54/p54spi.c > > +++ b/drivers/net/wireless/intersil/p54/p54spi.c > > @@ -162,53 +162,73 @@ static int p54spi_spi_write_dma(struct p54s_priv *priv, __le32 base, > > return 0; > > } > > > > +static int p54spi_request_firmware_found_cb(void *context, > > + const struct drvdata *drvdata) > > +{ > > + int ret; > > + struct p54s_priv *priv = context; > > + > > + priv->firmware = drvdata; > > + ret = p54_parse_firmware(priv->hw, priv->firmware); > > + if (ret) > > + release_drvdata(priv->firmware); > > + > > + return ret; > > +} > > + > > static int p54spi_request_firmware(struct ieee80211_hw *dev) > > { > > struct p54s_priv *priv = dev->priv; > > + const struct drvdata_req_params req_params = { > > + DRVDATA_KEEP_SYNC(p54spi_request_firmware_found_cb, priv), > > + }; > > int ret; > > > > /* FIXME: should driver use it's own struct device? */ > > - ret = request_firmware(&priv->firmware, "3826.arm", &priv->spi->dev); > > - > > + ret = drvdata_request("3826.arm", &req_params, &priv->spi->dev); > > if (ret < 0) { > > - dev_err(&priv->spi->dev, "request_firmware() failed: %d", ret); > > - return ret; > > + dev_err(&priv->spi->dev, > > + "firmware request failed: %d", ret); > > shouldn't the call report this error to the kernel log? Why must each > user print it out themselves again? Great point. The API already has: static int _drvdata_request(const struct drvdata **drvdata_p, const char *name, const struct drvdata_req_params *params, struct device *device) { ... if (ret && !params->optional) pr_err("Direct driver data load for %s failed with error %d\n", name, ret); ... } So it is not needed for driver to moan about failures here. Luis