| Message ID | 20210609151235.48964-2-sebastian.reichel@collabora.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | GE Healthcare PPD firmware upgrade driver for ACHC | expand |
On Wed, Jun 09, 2021 at 05:12:30PM +0200, Sebastian Reichel wrote: > Introduce support for ancillary devices, similar to existing > implementation for I2C. This is useful for devices having > multiple chip-selects, for example some microcontrollers > provide a normal SPI interface and a flashing SPI interface. > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > --- > drivers/spi/spi.c | 139 +++++++++++++++++++++++++++++++--------- > include/linux/spi/spi.h | 2 + > 2 files changed, 109 insertions(+), 32 deletions(-) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index ba425b9c7700..7fdf224262b1 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -558,49 +558,23 @@ static int spi_dev_check(struct device *dev, void *data) > return 0; > } > > -/** > - * spi_add_device - Add spi_device allocated with spi_alloc_device > - * @spi: spi_device to register > - * > - * Companion function to spi_alloc_device. Devices allocated with > - * spi_alloc_device can be added onto the spi bus with this function. > - * > - * Return: 0 on success; negative errno on failure > - */ > -int spi_add_device(struct spi_device *spi) > +static int __spi_add_device(struct spi_device *spi) > { > struct spi_controller *ctlr = spi->controller; > struct device *dev = ctlr->dev.parent; > int status; > > - /* Chipselects are numbered 0..max; validate. */ > - if (spi->chip_select >= ctlr->num_chipselect) { > - dev_err(dev, "cs%d >= max %d\n", spi->chip_select, > - ctlr->num_chipselect); > - return -EINVAL; > - } > - > - /* Set the bus ID string */ > - spi_dev_set_name(spi); > - > - /* We need to make sure there's no other device with this > - * chipselect **BEFORE** we call setup(), else we'll trash > - * its configuration. Lock against concurrent add() calls. > - */ > - mutex_lock(&spi_add_lock); > - > status = bus_for_each_dev(&spi_bus_type, NULL, spi, spi_dev_check); > if (status) { > dev_err(dev, "chipselect %d already in use\n", > spi->chip_select); > - goto done; > + return status; > } > > /* Controller may unregister concurrently */ > if (IS_ENABLED(CONFIG_SPI_DYNAMIC) && > !device_is_registered(&ctlr->dev)) { > - status = -ENODEV; > - goto done; > + return -ENODEV; > } > > /* Descriptors take precedence */ > @@ -617,7 +591,7 @@ int spi_add_device(struct spi_device *spi) > if (status < 0) { > dev_err(dev, "can't setup %s, status %d\n", > dev_name(&spi->dev), status); > - goto done; > + return status; > } > > /* Device may be bound to an active driver when this returns */ > @@ -628,12 +602,64 @@ int spi_add_device(struct spi_device *spi) > else > dev_dbg(dev, "registered child %s\n", dev_name(&spi->dev)); > > -done: > + return status; > +} > + > +/** > + * spi_add_device - Add spi_device allocated with spi_alloc_device > + * @spi: spi_device to register > + * > + * Companion function to spi_alloc_device. Devices allocated with > + * spi_alloc_device can be added onto the spi bus with this function. > + * > + * Return: 0 on success; negative errno on failure > + */ > +int spi_add_device(struct spi_device *spi) > +{ > + struct spi_controller *ctlr = spi->controller; > + struct device *dev = ctlr->dev.parent; > + int status; > + > + /* Chipselects are numbered 0..max; validate. */ > + if (spi->chip_select >= ctlr->num_chipselect) { > + dev_err(dev, "cs%d >= max %d\n", spi->chip_select, > + ctlr->num_chipselect); > + return -EINVAL; > + } > + > + /* Set the bus ID string */ > + spi_dev_set_name(spi); > + > + /* We need to make sure there's no other device with this > + * chipselect **BEFORE** we call setup(), else we'll trash > + * its configuration. Lock against concurrent add() calls. > + */ > + mutex_lock(&spi_add_lock); > + status = __spi_add_device(spi); > mutex_unlock(&spi_add_lock); > return status; > } > 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->chip_select >= ctlr->num_chipselect) { > + dev_err(dev, "cs%d >= max %d\n", spi->chip_select, > + ctlr->num_chipselect); > + return -EINVAL; > + } > + > + /* Set the bus ID string */ > + spi_dev_set_name(spi); > + > + WARN_ON(!mutex_is_locked(&spi_add_lock)); So you just rebooted a machine that has panic-on-warn set. Not nice. If this really can happen, test for it and recover, do not reboot devices. If this really can never happen, why are you testing for it? thanks, greg k-h
Hi Greg, On Fri, Jun 11, 2021 at 11:06:53AM +0200, Greg Kroah-Hartman wrote: > On Wed, Jun 09, 2021 at 05:12:30PM +0200, Sebastian Reichel wrote: > > Introduce support for ancillary devices, similar to existing > > implementation for I2C. This is useful for devices having > > multiple chip-selects, for example some microcontrollers > > provide a normal SPI interface and a flashing SPI interface. > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > > --- > > [...] > > +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->chip_select >= ctlr->num_chipselect) { > > + dev_err(dev, "cs%d >= max %d\n", spi->chip_select, > > + ctlr->num_chipselect); > > + return -EINVAL; > > + } > > + > > + /* Set the bus ID string */ > > + spi_dev_set_name(spi); > > + > > + WARN_ON(!mutex_is_locked(&spi_add_lock)); > > So you just rebooted a machine that has panic-on-warn set. Not > nice. > > If this really can happen, test for it and recover, do not reboot > devices. > > If this really can never happen, why are you testing for it? This is reached when ancillary device is not registered in the main SPI device's probe routine, which would be a driver bug. The gehc-achc driver calls it in the right place, so this is not reached with this patchset, but the function to register ancillary devices is generic and is expected to be also used by others. -- Sebastian
On Wed, Jun 9, 2021 at 7:07 PM Sebastian Reichel <sebastian.reichel@collabora.com> wrote: > > Introduce support for ancillary devices, similar to existing > implementation for I2C. This is useful for devices having > multiple chip-selects, for example some microcontrollers > provide a normal SPI interface and a flashing SPI interface. ... > @@ -1993,7 +2019,7 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi, > } > > /* Device address */ > - rc = of_property_read_u32(nc, "reg", &value); > + rc = of_property_read_u32_index(nc, "reg", 0, &value); > if (rc) { > dev_err(&ctlr->dev, "%pOF has no valid 'reg' property (%d)\n", > nc, rc); Unrelated change. -- With Best Regards, Andy Shevchenko
On Wed, Jun 09, 2021 at 05:12:30PM +0200, Sebastian Reichel wrote: > Introduce support for ancillary devices, similar to existing > implementation for I2C. This is useful for devices having > multiple chip-selects, for example some microcontrollers > provide a normal SPI interface and a flashing SPI interface. This doesn't apply against current code, please check and resend.
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index ba425b9c7700..7fdf224262b1 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -558,49 +558,23 @@ static int spi_dev_check(struct device *dev, void *data) return 0; } -/** - * spi_add_device - Add spi_device allocated with spi_alloc_device - * @spi: spi_device to register - * - * Companion function to spi_alloc_device. Devices allocated with - * spi_alloc_device can be added onto the spi bus with this function. - * - * Return: 0 on success; negative errno on failure - */ -int spi_add_device(struct spi_device *spi) +static int __spi_add_device(struct spi_device *spi) { struct spi_controller *ctlr = spi->controller; struct device *dev = ctlr->dev.parent; int status; - /* Chipselects are numbered 0..max; validate. */ - if (spi->chip_select >= ctlr->num_chipselect) { - dev_err(dev, "cs%d >= max %d\n", spi->chip_select, - ctlr->num_chipselect); - return -EINVAL; - } - - /* Set the bus ID string */ - spi_dev_set_name(spi); - - /* We need to make sure there's no other device with this - * chipselect **BEFORE** we call setup(), else we'll trash - * its configuration. Lock against concurrent add() calls. - */ - mutex_lock(&spi_add_lock); - status = bus_for_each_dev(&spi_bus_type, NULL, spi, spi_dev_check); if (status) { dev_err(dev, "chipselect %d already in use\n", spi->chip_select); - goto done; + return status; } /* Controller may unregister concurrently */ if (IS_ENABLED(CONFIG_SPI_DYNAMIC) && !device_is_registered(&ctlr->dev)) { - status = -ENODEV; - goto done; + return -ENODEV; } /* Descriptors take precedence */ @@ -617,7 +591,7 @@ int spi_add_device(struct spi_device *spi) if (status < 0) { dev_err(dev, "can't setup %s, status %d\n", dev_name(&spi->dev), status); - goto done; + return status; } /* Device may be bound to an active driver when this returns */ @@ -628,12 +602,64 @@ int spi_add_device(struct spi_device *spi) else dev_dbg(dev, "registered child %s\n", dev_name(&spi->dev)); -done: + return status; +} + +/** + * spi_add_device - Add spi_device allocated with spi_alloc_device + * @spi: spi_device to register + * + * Companion function to spi_alloc_device. Devices allocated with + * spi_alloc_device can be added onto the spi bus with this function. + * + * Return: 0 on success; negative errno on failure + */ +int spi_add_device(struct spi_device *spi) +{ + struct spi_controller *ctlr = spi->controller; + struct device *dev = ctlr->dev.parent; + int status; + + /* Chipselects are numbered 0..max; validate. */ + if (spi->chip_select >= ctlr->num_chipselect) { + dev_err(dev, "cs%d >= max %d\n", spi->chip_select, + ctlr->num_chipselect); + return -EINVAL; + } + + /* Set the bus ID string */ + spi_dev_set_name(spi); + + /* We need to make sure there's no other device with this + * chipselect **BEFORE** we call setup(), else we'll trash + * its configuration. Lock against concurrent add() calls. + */ + mutex_lock(&spi_add_lock); + status = __spi_add_device(spi); mutex_unlock(&spi_add_lock); return status; } 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->chip_select >= ctlr->num_chipselect) { + dev_err(dev, "cs%d >= max %d\n", spi->chip_select, + ctlr->num_chipselect); + return -EINVAL; + } + + /* Set the bus ID string */ + spi_dev_set_name(spi); + + WARN_ON(!mutex_is_locked(&spi_add_lock)); + return __spi_add_device(spi); +} + /** * spi_new_device - instantiate one new SPI device * @ctlr: Controller to which device is connected @@ -1993,7 +2019,7 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi, } /* Device address */ - rc = of_property_read_u32(nc, "reg", &value); + rc = of_property_read_u32_index(nc, "reg", 0, &value); if (rc) { dev_err(&ctlr->dev, "%pOF has no valid 'reg' property (%d)\n", nc, rc); @@ -2084,6 +2110,55 @@ static void of_register_spi_devices(struct spi_controller *ctlr) static void of_register_spi_devices(struct spi_controller *ctlr) { } #endif +/** + * spi_new_ancillary_device() - Register ancillary SPI device + * @spi: Pointer to the main SPI device registering the ancillary device + * @chip_select: Chip Select of the ancillary device + * + * Register an ancillary SPI device; for example some chips have a chip-select + * for normal device usage and another one for setup/firmware upload. + * + * This may only be called from main SPI device's probe routine. + * + * Return: 0 on success; negative errno on failure + */ +struct spi_device *spi_new_ancillary_device(struct spi_device *spi, + u8 chip_select) +{ + struct spi_device *ancillary; + int rc = 0; + + /* Alloc an spi_device */ + ancillary = spi_alloc_device(spi->controller); + if (!ancillary) { + rc = -ENOMEM; + goto err_out; + } + + strlcpy(ancillary->modalias, "dummy", sizeof(ancillary->modalias)); + + /* Use provided chip-select for ancillary device */ + ancillary->chip_select = chip_select; + + /* Take over SPI mode/speed from SPI main device */ + ancillary->max_speed_hz = spi->max_speed_hz; + ancillary->mode = ancillary->mode; + + /* Register the new device */ + rc = spi_add_device_locked(ancillary); + if (rc) { + dev_err(&spi->dev, "failed to register ancillary device\n"); + goto err_out; + } + + return ancillary; + +err_out: + spi_dev_put(ancillary); + return ERR_PTR(rc); +} +EXPORT_SYMBOL_GPL(spi_new_ancillary_device); + #ifdef CONFIG_ACPI struct acpi_spi_lookup { struct spi_controller *ctlr; diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index 360a3bc767ca..1949230ccd3a 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -299,6 +299,8 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv) driver_unregister(&sdrv->driver); } +extern struct spi_device *spi_new_ancillary_device(struct spi_device *spi, u8 chip_select); + /* use a define to avoid include chaining to get THIS_MODULE */ #define spi_register_driver(driver) \ __spi_register_driver(THIS_MODULE, driver)
Introduce support for ancillary devices, similar to existing implementation for I2C. This is useful for devices having multiple chip-selects, for example some microcontrollers provide a normal SPI interface and a flashing SPI interface. Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> --- drivers/spi/spi.c | 139 +++++++++++++++++++++++++++++++--------- include/linux/spi/spi.h | 2 + 2 files changed, 109 insertions(+), 32 deletions(-)