| Message ID | 20230710154932.68377-6-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | spi: Header and core clean up and refactoring | expand |
Hi, On Tue, Jul 11, 2023 at 02:06:20PM +0300, Andy Shevchenko wrote: > On Mon, Jul 10, 2023 at 06:16:22PM +0100, Mark Brown wrote: > > On Mon, Jul 10, 2023 at 06:49:22PM +0300, Andy Shevchenko wrote: > > > Seems by unknown reason, probably some kind of mis-rebase, > > > the commit 0c79378c0199 ("spi: add ancillary device support") > > > adds a dozen of duplicating lines of code. Drop them. > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > --- > > > drivers/spi/spi.c | 11 ----------- > > > 1 file changed, 11 deletions(-) > > > > > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > > > index c99ee4164f11..46cbda383228 100644 > > > --- a/drivers/spi/spi.c > > > +++ b/drivers/spi/spi.c > > > @@ -712,17 +712,6 @@ EXPORT_SYMBOL_GPL(spi_add_device); > > > static int spi_add_device_locked(struct spi_device *spi) > > > { > > > struct spi_controller *ctlr = spi->controller; > > > - struct device *dev = ctlr->dev.parent; > > > - > > > - /* Chipselects are numbered 0..max; validate. */ > > > - if (spi_get_chipselect(spi, 0) >= ctlr->num_chipselect) { > > > - dev_err(dev, "cs%d >= max %d\n", spi_get_chipselect(spi, 0), > > > - ctlr->num_chipselect); > > > - return -EINVAL; > > > - } > > > - > > > - /* Set the bus ID string */ > > > - spi_dev_set_name(spi); > > > > I see that this is duplicating spi_add_device() (and we really could do > > better with code sharing there I think) but I can't immediately see > > where the duplication that's intended to be elimiated is here - where > > else in the one call path that spi_add_device_locked() has would we do > > the above? Based on the changelog I was expecting to see some > > duplicated code in the function itself. > > Oh, by some reason Sebastian wasn't in this rather long Cc list. > Added him. > > Reading again I don't see any useful explanation why that piece of code has to > be duplicated among these two functions. It's 100% a copy. > > Sebastian, can you shed some light here? The patch in this thread is obviously wrong. It results in the checks never beeing called for spi_add_device_locked(). The copy is in spi_add_device() and those two are not calling into each other. But it should be fine to move the code to the start of __spi_add_device(), which allows removing the duplication. In that case the code will be run with the add_lock held, which is probably what I was worried about two years ago. Looking at it again, the lock is held anyways in case of spi_add_device_locked(). Greetings, -- Sebastian
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index c99ee4164f11..46cbda383228 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -712,17 +712,6 @@ EXPORT_SYMBOL_GPL(spi_add_device); static int spi_add_device_locked(struct spi_device *spi) { struct spi_controller *ctlr = spi->controller; - struct device *dev = ctlr->dev.parent; - - /* Chipselects are numbered 0..max; validate. */ - if (spi_get_chipselect(spi, 0) >= ctlr->num_chipselect) { - dev_err(dev, "cs%d >= max %d\n", spi_get_chipselect(spi, 0), - ctlr->num_chipselect); - return -EINVAL; - } - - /* Set the bus ID string */ - spi_dev_set_name(spi); WARN_ON(!mutex_is_locked(&ctlr->add_lock)); return __spi_add_device(spi);
Seems by unknown reason, probably some kind of mis-rebase, the commit 0c79378c0199 ("spi: add ancillary device support") adds a dozen of duplicating lines of code. Drop them. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/spi/spi.c | 11 ----------- 1 file changed, 11 deletions(-)