| Message ID | 20240329114730.360313-3-ckeepax@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add bridged amplifiers to cs42l43 | expand |
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.
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. 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. Thanks, Charles
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 --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;
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(+)