[PATCH v3 1/5] spi: allow ancillary devices to share parent's chip selects

Antoniu Miclaus posted 5 patches 1 month, 2 weeks ago
There is a newer version of this series
[PATCH v3 1/5] spi: allow ancillary devices to share parent's chip selects
Posted by Antoniu Miclaus 1 month, 2 weeks ago
When registering an ancillary SPI device, the current code flags a chip
select conflict with the parent device. This happens because the
ancillary device intentionally uses one of the parent's chip selects,
but __spi_add_device() checks against all existing devices including
the parent.

Allow this by passing the parent device pointer to __spi_add_device()
and skipping the conflict check when the existing device is the parent.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
Reviewed-by: David Lechner <dlechner@baylibre.com>
---
Changes in v3:
  - Drop redundant NULL check for info->parent in spi_dev_check()

 drivers/spi/spi.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index e25df9990f82..693bdcc5a12a 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -641,12 +641,26 @@ static inline int spi_dev_check_cs(struct device *dev,
 	return 0;
 }
 
+struct spi_dev_check_info {
+	struct spi_device *new_spi;
+	struct spi_device *parent;	/* set for ancillary devices */
+};
+
 static int spi_dev_check(struct device *dev, void *data)
 {
 	struct spi_device *spi = to_spi_device(dev);
-	struct spi_device *new_spi = data;
+	struct spi_dev_check_info *info = data;
+	struct spi_device *new_spi = info->new_spi;
 	int status, idx;
 
+	/*
+	 * When registering an ancillary device, skip checking against the
+	 * parent device since the ancillary is intentionally using one of
+	 * the parent's chip selects.
+	 */
+	if (spi == info->parent)
+		return 0;
+
 	if (spi->controller == new_spi->controller) {
 		for (idx = 0; idx < spi->num_chipselect; idx++) {
 			status = spi_dev_check_cs(dev, spi, idx, new_spi, 0);
@@ -663,10 +677,11 @@ static void spi_cleanup(struct spi_device *spi)
 		spi->controller->cleanup(spi);
 }
 
-static int __spi_add_device(struct spi_device *spi)
+static int __spi_add_device(struct spi_device *spi, struct spi_device *parent)
 {
 	struct spi_controller *ctlr = spi->controller;
 	struct device *dev = ctlr->dev.parent;
+	struct spi_dev_check_info check_info;
 	int status, idx;
 	u8 cs;
 
@@ -710,7 +725,9 @@ static int __spi_add_device(struct spi_device *spi)
 	 * chipselect **BEFORE** we call setup(), else we'll trash
 	 * its configuration.
 	 */
-	status = bus_for_each_dev(&spi_bus_type, NULL, spi, spi_dev_check);
+	check_info.new_spi = spi;
+	check_info.parent = parent;
+	status = bus_for_each_dev(&spi_bus_type, NULL, &check_info, spi_dev_check);
 	if (status)
 		return status;
 
@@ -772,7 +789,7 @@ int spi_add_device(struct spi_device *spi)
 	spi_dev_set_name(spi);
 
 	mutex_lock(&ctlr->add_lock);
-	status = __spi_add_device(spi);
+	status = __spi_add_device(spi, NULL);
 	mutex_unlock(&ctlr->add_lock);
 	return status;
 }
@@ -2580,8 +2597,8 @@ struct spi_device *spi_new_ancillary_device(struct spi_device *spi,
 
 	WARN_ON(!mutex_is_locked(&ctlr->add_lock));
 
-	/* Register the new device */
-	rc = __spi_add_device(ancillary);
+	/* Register the new device, passing the parent to skip CS conflict check */
+	rc = __spi_add_device(ancillary, spi);
 	if (rc) {
 		dev_err(&spi->dev, "failed to register ancillary device\n");
 		goto err_out;
-- 
2.43.0
Re: [PATCH v3 1/5] spi: allow ancillary devices to share parent's chip selects
Posted by Jonathan Cameron 1 month, 2 weeks ago
On Fri, 13 Feb 2026 16:47:33 +0200
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:

> When registering an ancillary SPI device, the current code flags a chip
> select conflict with the parent device. This happens because the
> ancillary device intentionally uses one of the parent's chip selects,
> but __spi_add_device() checks against all existing devices including
> the parent.
> 
> Allow this by passing the parent device pointer to __spi_add_device()
> and skipping the conflict check when the existing device is the parent.

Smells like a fix. If so why don't we have a fixes tag?

This will need review from Mark. I haven't really understood how this
is intended to work.

Jonathan


> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> Reviewed-by: David Lechner <dlechner@baylibre.com>
> ---
> Changes in v3:
>   - Drop redundant NULL check for info->parent in spi_dev_check()
> 
>  drivers/spi/spi.c | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index e25df9990f82..693bdcc5a12a 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -641,12 +641,26 @@ static inline int spi_dev_check_cs(struct device *dev,
>  	return 0;
>  }
>  
> +struct spi_dev_check_info {
> +	struct spi_device *new_spi;
> +	struct spi_device *parent;	/* set for ancillary devices */
> +};
> +
>  static int spi_dev_check(struct device *dev, void *data)
>  {
>  	struct spi_device *spi = to_spi_device(dev);
> -	struct spi_device *new_spi = data;
> +	struct spi_dev_check_info *info = data;
> +	struct spi_device *new_spi = info->new_spi;
>  	int status, idx;
>  
> +	/*
> +	 * When registering an ancillary device, skip checking against the
> +	 * parent device since the ancillary is intentionally using one of
> +	 * the parent's chip selects.
> +	 */
> +	if (spi == info->parent)
> +		return 0;
> +
>  	if (spi->controller == new_spi->controller) {
>  		for (idx = 0; idx < spi->num_chipselect; idx++) {
>  			status = spi_dev_check_cs(dev, spi, idx, new_spi, 0);
> @@ -663,10 +677,11 @@ static void spi_cleanup(struct spi_device *spi)
>  		spi->controller->cleanup(spi);
>  }
>  
> -static int __spi_add_device(struct spi_device *spi)
> +static int __spi_add_device(struct spi_device *spi, struct spi_device *parent)
>  {
>  	struct spi_controller *ctlr = spi->controller;
>  	struct device *dev = ctlr->dev.parent;
> +	struct spi_dev_check_info check_info;
>  	int status, idx;
>  	u8 cs;
>  
> @@ -710,7 +725,9 @@ static int __spi_add_device(struct spi_device *spi)
>  	 * chipselect **BEFORE** we call setup(), else we'll trash
>  	 * its configuration.
>  	 */
> -	status = bus_for_each_dev(&spi_bus_type, NULL, spi, spi_dev_check);
> +	check_info.new_spi = spi;
> +	check_info.parent = parent;
> +	status = bus_for_each_dev(&spi_bus_type, NULL, &check_info, spi_dev_check);
>  	if (status)
>  		return status;
>  
> @@ -772,7 +789,7 @@ int spi_add_device(struct spi_device *spi)
>  	spi_dev_set_name(spi);
>  
>  	mutex_lock(&ctlr->add_lock);
> -	status = __spi_add_device(spi);
> +	status = __spi_add_device(spi, NULL);
>  	mutex_unlock(&ctlr->add_lock);
>  	return status;
>  }
> @@ -2580,8 +2597,8 @@ struct spi_device *spi_new_ancillary_device(struct spi_device *spi,
>  
>  	WARN_ON(!mutex_is_locked(&ctlr->add_lock));
>  
> -	/* Register the new device */
> -	rc = __spi_add_device(ancillary);
> +	/* Register the new device, passing the parent to skip CS conflict check */
> +	rc = __spi_add_device(ancillary, spi);
>  	if (rc) {
>  		dev_err(&spi->dev, "failed to register ancillary device\n");
>  		goto err_out;
Re: [PATCH v3 1/5] spi: allow ancillary devices to share parent's chip selects
Posted by Nuno Sá 1 month, 2 weeks ago
On Fri, 2026-02-13 at 16:47 +0200, Antoniu Miclaus wrote:
> When registering an ancillary SPI device, the current code flags a chip
> select conflict with the parent device. This happens because the
> ancillary device intentionally uses one of the parent's chip selects,
> but __spi_add_device() checks against all existing devices including
> the parent.
> 
> Allow this by passing the parent device pointer to __spi_add_device()
> and skipping the conflict check when the existing device is the parent.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> Reviewed-by: David Lechner <dlechner@baylibre.com>
> ---

Acked-by: Nuno Sá <nuno.sa@analog.com>

> Changes in v3:
>   - Drop redundant NULL check for info->parent in spi_dev_check()
> 
>  drivers/spi/spi.c | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index e25df9990f82..693bdcc5a12a 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -641,12 +641,26 @@ static inline int spi_dev_check_cs(struct device *dev,
>  	return 0;
>  }
>  
> +struct spi_dev_check_info {
> +	struct spi_device *new_spi;
> +	struct spi_device *parent;	/* set for ancillary devices */
> +};
> +
>  static int spi_dev_check(struct device *dev, void *data)
>  {
>  	struct spi_device *spi = to_spi_device(dev);
> -	struct spi_device *new_spi = data;
> +	struct spi_dev_check_info *info = data;
> +	struct spi_device *new_spi = info->new_spi;
>  	int status, idx;
>  
> +	/*
> +	 * When registering an ancillary device, skip checking against the
> +	 * parent device since the ancillary is intentionally using one of
> +	 * the parent's chip selects.
> +	 */
> +	if (spi == info->parent)
> +		return 0;
> +
>  	if (spi->controller == new_spi->controller) {
>  		for (idx = 0; idx < spi->num_chipselect; idx++) {
>  			status = spi_dev_check_cs(dev, spi, idx, new_spi, 0);
> @@ -663,10 +677,11 @@ static void spi_cleanup(struct spi_device *spi)
>  		spi->controller->cleanup(spi);
>  }
>  
> -static int __spi_add_device(struct spi_device *spi)
> +static int __spi_add_device(struct spi_device *spi, struct spi_device *parent)
>  {
>  	struct spi_controller *ctlr = spi->controller;
>  	struct device *dev = ctlr->dev.parent;
> +	struct spi_dev_check_info check_info;
>  	int status, idx;
>  	u8 cs;
>  
> @@ -710,7 +725,9 @@ static int __spi_add_device(struct spi_device *spi)
>  	 * chipselect **BEFORE** we call setup(), else we'll trash
>  	 * its configuration.
>  	 */
> -	status = bus_for_each_dev(&spi_bus_type, NULL, spi, spi_dev_check);
> +	check_info.new_spi = spi;
> +	check_info.parent = parent;
> +	status = bus_for_each_dev(&spi_bus_type, NULL, &check_info, spi_dev_check);
>  	if (status)
>  		return status;
>  
> @@ -772,7 +789,7 @@ int spi_add_device(struct spi_device *spi)
>  	spi_dev_set_name(spi);
>  
>  	mutex_lock(&ctlr->add_lock);
> -	status = __spi_add_device(spi);
> +	status = __spi_add_device(spi, NULL);
>  	mutex_unlock(&ctlr->add_lock);
>  	return status;
>  }
> @@ -2580,8 +2597,8 @@ struct spi_device *spi_new_ancillary_device(struct spi_device *spi,
>  
>  	WARN_ON(!mutex_is_locked(&ctlr->add_lock));
>  
> -	/* Register the new device */
> -	rc = __spi_add_device(ancillary);
> +	/* Register the new device, passing the parent to skip CS conflict check */
> +	rc = __spi_add_device(ancillary, spi);
>  	if (rc) {
>  		dev_err(&spi->dev, "failed to register ancillary device\n");
>  		goto err_out;