| Message ID | 20140213152841.390de00f@endymion.delvare (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | aec35f4ee6eefba616065547e6882c084cc7f5cb |
| Headers | show |
On 2/13/2014 3:28 PM, Jean Delvare wrote: > While backporting 33cf00e5 ("spi: attach/detach SPI device to the ACPI > power domain"), I noticed that the code changes were suboptimal: > > * Why use &spi->dev when we have dev at hand? > > * After fixing the above, spi is used only once, so we don't really > need a local variable for it. > > This results in the following clean-up. > > Signed-off-by: Jean Delvare <jdelvare@suse.de> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com> > Cc: Mark Brown <broonie@kernel.org> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > Untested, no hardware. > > drivers/spi/spi.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > > --- linux-3.14-rc2.orig/drivers/spi/spi.c 2014-02-13 14:42:09.309512009 +0100 > +++ linux-3.14-rc2/drivers/spi/spi.c 2014-02-13 14:53:36.204366691 +0100 > @@ -255,13 +255,12 @@ EXPORT_SYMBOL_GPL(spi_bus_type); > static int spi_drv_probe(struct device *dev) > { > const struct spi_driver *sdrv = to_spi_driver(dev->driver); > - struct spi_device *spi = to_spi_device(dev); > int ret; > > - acpi_dev_pm_attach(&spi->dev, true); > - ret = sdrv->probe(spi); > + acpi_dev_pm_attach(dev, true); > + ret = sdrv->probe(to_spi_device(dev)); > if (ret) > - acpi_dev_pm_detach(&spi->dev, true); > + acpi_dev_pm_detach(dev, true); > > return ret; > } > @@ -269,11 +268,10 @@ static int spi_drv_probe(struct device * > static int spi_drv_remove(struct device *dev) > { > const struct spi_driver *sdrv = to_spi_driver(dev->driver); > - struct spi_device *spi = to_spi_device(dev); > int ret; > > - ret = sdrv->remove(spi); > - acpi_dev_pm_detach(&spi->dev, true); > + ret = sdrv->remove(to_spi_device(dev)); > + acpi_dev_pm_detach(dev, true); > > return ret; > } > > -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 13, 2014 at 03:28:41PM +0100, Jean Delvare wrote: > While backporting 33cf00e5 ("spi: attach/detach SPI device to the ACPI > power domain"), I noticed that the code changes were suboptimal: > > * Why use &spi->dev when we have dev at hand? > > * After fixing the above, spi is used only once, so we don't really > need a local variable for it. I've applied this but I have to say it's a bit marginal as a cleanup - for example while we do only use the SPI device once it's not entirely idiomatic to use to_spi_device() in the middle of the code rather than at the top of the function so it's a small speed bump to see that.
Hi Mark, Le Friday 14 February 2014 à 15:03 +0000, Mark Brown a écrit : > On Thu, Feb 13, 2014 at 03:28:41PM +0100, Jean Delvare wrote: > > While backporting 33cf00e5 ("spi: attach/detach SPI device to the ACPI > > power domain"), I noticed that the code changes were suboptimal: > > > > * Why use &spi->dev when we have dev at hand? > > > > * After fixing the above, spi is used only once, so we don't really > > need a local variable for it. > > I've applied this but I have to say it's a bit marginal as a cleanup - > for example while we do only use the SPI device once it's not entirely > idiomatic to use to_spi_device() in the middle of the code rather than > at the top of the function so it's a small speed bump to see that. I don't disagree. The rationale for the change is that I simply reverted to how the code was before 33cf00e5, assuming that the introduction of spi as a local variable was caused by it being used more than once. If you believe this was a good change on its own and would prefer to keep it that way, I could send a patch replacing this one and only changing &spi->dev to dev. Let me know. And yes, this is not the most groundbreaking patch either way, granted ;-)
On Fri, Feb 14, 2014 at 07:22:09PM +0100, Jean Delvare wrote: > I don't disagree. The rationale for the change is that I simply reverted > to how the code was before 33cf00e5, assuming that the introduction of > spi as a local variable was caused by it being used more than once. If > you believe this was a good change on its own and would prefer to keep > it that way, I could send a patch replacing this one and only changing > &spi->dev to dev. Let me know. I do have a small preference for it but I don't care enough to suggest you actually send that patch - if it annoyed me enough I'd just fix it myself.
--- linux-3.14-rc2.orig/drivers/spi/spi.c 2014-02-13 14:42:09.309512009 +0100 +++ linux-3.14-rc2/drivers/spi/spi.c 2014-02-13 14:53:36.204366691 +0100 @@ -255,13 +255,12 @@ EXPORT_SYMBOL_GPL(spi_bus_type); static int spi_drv_probe(struct device *dev) { const struct spi_driver *sdrv = to_spi_driver(dev->driver); - struct spi_device *spi = to_spi_device(dev); int ret; - acpi_dev_pm_attach(&spi->dev, true); - ret = sdrv->probe(spi); + acpi_dev_pm_attach(dev, true); + ret = sdrv->probe(to_spi_device(dev)); if (ret) - acpi_dev_pm_detach(&spi->dev, true); + acpi_dev_pm_detach(dev, true); return ret; } @@ -269,11 +268,10 @@ static int spi_drv_probe(struct device * static int spi_drv_remove(struct device *dev) { const struct spi_driver *sdrv = to_spi_driver(dev->driver); - struct spi_device *spi = to_spi_device(dev); int ret; - ret = sdrv->remove(spi); - acpi_dev_pm_detach(&spi->dev, true); + ret = sdrv->remove(to_spi_device(dev)); + acpi_dev_pm_detach(dev, true); return ret; }
While backporting 33cf00e5 ("spi: attach/detach SPI device to the ACPI power domain"), I noticed that the code changes were suboptimal: * Why use &spi->dev when we have dev at hand? * After fixing the above, spi is used only once, so we don't really need a local variable for it. This results in the following clean-up. Signed-off-by: Jean Delvare <jdelvare@suse.de> Cc: Mika Westerberg <mika.westerberg@linux.intel.com> Cc: Mark Brown <broonie@kernel.org> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> --- Untested, no hardware. drivers/spi/spi.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)