diff mbox series

[v3,2/3] spi: Add a mechanism to use the fwnode name for the SPI device

Message ID 20240329114730.360313-3-ckeepax@opensource.cirrus.com
State Superseded
Headers show
Series Add bridged amplifiers to cs42l43 | expand

Commit Message

Charles Keepax March 29, 2024, 11:47 a.m. UTC
Add a mechanism to force the use of the fwnode name for the name of the
SPI device itself. This is useful when devices need to be manually added
within the kernel.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

No changes since v2.

Thanks,
Charles

 drivers/spi/spi.c       | 7 +++++++
 include/linux/spi/spi.h | 2 ++
 2 files changed, 9 insertions(+)

Comments

Andy Shevchenko April 3, 2024, 8:29 p.m. UTC | #1
Fri, Mar 29, 2024 at 11:47:29AM +0000, Charles Keepax kirjoitti:
> Add a mechanism to force the use of the fwnode name for the name of the
> SPI device itself. This is useful when devices need to be manually added
> within the kernel.

...

>  static void spi_dev_set_name(struct spi_device *spi)
>  {
>  	struct acpi_device *adev = ACPI_COMPANION(&spi->dev);
> +	struct fwnode_handle *fwnode = dev_fwnode(&spi->dev);
> +
> +	if (spi->use_fwnode_name && fwnode) {
> +		dev_set_name(&spi->dev, "spi-%s", fwnode_get_name(fwnode));
> +		return;
> +	}
>  
>  	if (adev) {
>  		dev_set_name(&spi->dev, "spi-%s", acpi_dev_name(adev));

This should be something like this

	struct device *dev = &spi->dev;
	struct fwnode_handle *fwnode = dev_fwnode(dev);

	if (is_acpi_device_node(fwnode)) {
		dev_set_name(dev, "spi-%s", acpi_dev_name(to_acpi_device_node(fwnode)));
		return;
	}

	if (is_software_node(fwnode)) {
		dev_set_name(dev, "spi-%s", fwnode_get_name(fwnode));
		return;
	}

i.o.w. we don't need to have two ways of checking fwnode type and you may get
rid of unneeded variable, and always use fwnode name for swnode.

...

> +	proxy->use_fwnode_name = chip->use_fwnode_name;

Unneeded variable. See above.
Charles Keepax April 8, 2024, 1:51 p.m. UTC | #2
On Wed, Apr 03, 2024 at 11:29:27PM +0300, Andy Shevchenko wrote:
> Fri, Mar 29, 2024 at 11:47:29AM +0000, Charles Keepax kirjoitti:
> >  	struct acpi_device *adev = ACPI_COMPANION(&spi->dev);
> > +	struct fwnode_handle *fwnode = dev_fwnode(&spi->dev);
> > +
> > +	if (spi->use_fwnode_name && fwnode) {
> > +		dev_set_name(&spi->dev, "spi-%s", fwnode_get_name(fwnode));
> > +		return;
> > +	}
> >  
> >  	if (adev) {
> >  		dev_set_name(&spi->dev, "spi-%s", acpi_dev_name(adev));
> 
> This should be something like this
> 
> 	struct device *dev = &spi->dev;
> 	struct fwnode_handle *fwnode = dev_fwnode(dev);
> 
> 	if (is_acpi_device_node(fwnode)) {
> 		dev_set_name(dev, "spi-%s", acpi_dev_name(to_acpi_device_node(fwnode)));
> 		return;
> 	}
> 
> 	if (is_software_node(fwnode)) {
> 		dev_set_name(dev, "spi-%s", fwnode_get_name(fwnode));
> 		return;
> 	}
> 
> i.o.w. we don't need to have two ways of checking fwnode type and you may get
> rid of unneeded variable, and always use fwnode name for swnode.
> 
> ...
> 
> > +	proxy->use_fwnode_name = chip->use_fwnode_name;
> 
> Unneeded variable. See above.
> 

Hmm... I guess I was viewing this feature more as something that
users would opt into. So other firmware mechanisms could use it
if required, and so most swnode based controllers would still get
caught by the standard naming at the bottom of the function.
Andy Shevchenko April 8, 2024, 4:11 p.m. UTC | #3
On Mon, Apr 8, 2024 at 4:52 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
> On Wed, Apr 03, 2024 at 11:29:27PM +0300, Andy Shevchenko wrote:
> > Fri, Mar 29, 2024 at 11:47:29AM +0000, Charles Keepax kirjoitti:

...

> > >     struct acpi_device *adev = ACPI_COMPANION(&spi->dev);
> > > +   struct fwnode_handle *fwnode = dev_fwnode(&spi->dev);
> > > +
> > > +   if (spi->use_fwnode_name && fwnode) {
> > > +           dev_set_name(&spi->dev, "spi-%s", fwnode_get_name(fwnode));
> > > +           return;
> > > +   }
> > >
> > >     if (adev) {
> > >             dev_set_name(&spi->dev, "spi-%s", acpi_dev_name(adev));
> >
> > This should be something like this
> >
> >       struct device *dev = &spi->dev;
> >       struct fwnode_handle *fwnode = dev_fwnode(dev);
> >
> >       if (is_acpi_device_node(fwnode)) {
> >               dev_set_name(dev, "spi-%s", acpi_dev_name(to_acpi_device_node(fwnode)));
> >               return;
> >       }
> >
> >       if (is_software_node(fwnode)) {
> >               dev_set_name(dev, "spi-%s", fwnode_get_name(fwnode));
> >               return;
> >       }
> >
> > i.o.w. we don't need to have two ways of checking fwnode type and you may get
> > rid of unneeded variable, and always use fwnode name for swnode.
> >
> > ...
> >
> > > +   proxy->use_fwnode_name = chip->use_fwnode_name;
> >
> > Unneeded variable. See above.
>
> Hmm... I guess I was viewing this feature more as something that
> users would opt into. So other firmware mechanisms could use it
> if required, and so most swnode based controllers would still get
> caught by the standard naming at the bottom of the function.

software nodes can be used as a trampoline for old board files (in
order to make the driver cleaner and prepared for DT conversion of the
board file in question) or for quirks. In either case when we use them
we really want to have their data to be propagated unconditionally
(just based on the type), the (per-subsystem) gate is a carefully
placed mine on the minefield.

> From my perspective it will do what I need either way, so happy
> to update it to always use this for software nodes if consensus
> goes that way.

Not a maintainer here, ask Mark.
diff mbox series

Patch

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index a2f01116ba092..a38a4960032b8 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -598,6 +598,12 @@  EXPORT_SYMBOL_GPL(spi_alloc_device);
 static void spi_dev_set_name(struct spi_device *spi)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&spi->dev);
+	struct fwnode_handle *fwnode = dev_fwnode(&spi->dev);
+
+	if (spi->use_fwnode_name && fwnode) {
+		dev_set_name(&spi->dev, "spi-%s", fwnode_get_name(fwnode));
+		return;
+	}
 
 	if (adev) {
 		dev_set_name(&spi->dev, "spi-%s", acpi_dev_name(adev));
@@ -830,6 +836,7 @@  struct spi_device *spi_new_device(struct spi_controller *ctlr,
 	 * spi->cs_index_mask as 0x01.
 	 */
 	proxy->cs_index_mask = 0x01;
+	proxy->use_fwnode_name = chip->use_fwnode_name;
 
 	if (chip->swnode) {
 		status = device_add_software_node(&proxy->dev, chip->swnode);
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index b589e25474393..265f5d0c15304 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -222,6 +222,7 @@  struct spi_device {
 	struct spi_delay	cs_setup;
 	struct spi_delay	cs_hold;
 	struct spi_delay	cs_inactive;
+	bool			use_fwnode_name;
 
 	/* The statistics */
 	struct spi_statistics __percpu	*pcpu_statistics;
@@ -1603,6 +1604,7 @@  struct spi_board_info {
 	char		modalias[SPI_NAME_SIZE];
 	const void	*platform_data;
 	const struct software_node *swnode;
+	bool		use_fwnode_name;
 	void		*controller_data;
 	int		irq;