Messages in this thread | ![/](/images/icornerl.gif) | | Date | Tue, 11 Jul 2023 15:47:57 +0300 | From | Andy Shevchenko <> | Subject | Re: [PATCH v2 05/15] spi: Remove code duplication in spi_add_device_locked() |
| |
On Tue, Jul 11, 2023 at 02:01:33PM +0200, Sebastian Reichel wrote: > 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:
...
> > > > - 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.
Ah, now I see, I missed __ in the name. Thank you for opening my eyes!
> 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().
Right, I will re-do that.
-- With Best Regards, Andy Shevchenko
| ![\](/images/icornerr.gif) |