| Message ID | 20240411090628.2436389-5-ckeepax@opensource.cirrus.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add bridged amplifiers to cs42l43 | expand |
On Thu, Apr 11, 2024 at 8:13 PM Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > On Thu, Apr 11, 2024 at 05:04:33PM +0300, Andy Shevchenko wrote: > > On Thu, Apr 11, 2024 at 12:06 PM Charles Keepax > > <ckeepax@opensource.cirrus.com> wrote: ... > > > +static const struct software_node ampl = { > > > + .name = "cs35l56-left", > > > +}; > > > + > > > +static const struct software_node ampr = { > > > + .name = "cs35l56-right", > > > +}; > > > > Still I fail to see how these are used. They provide just a static > > swnode with name and no properties. How is that different from the > > no-fwnode case? Can you test without these being defined? > > The code in the last patch will pick up the name and use it to > name the amps that are registered. This means when those amps are > referred to by the audio machine driver code we will know what > they are called. Admittedly that audio machine driver change > isn't in this series as it is a bit of a work in progress and not > technically necessary for just registering the amps as this > series does. Thank you for the patience, now I realise how it's connected. But wouldn't the simple spi-<SPI ID>-<bus number>.<chip select> work? Thinking loudly: Probably not as bus number is dynamic nowadays, ... > > > +static const struct software_node cs42l43_gpiochip_swnode = { > > > + .name = "cs42l43-pinctrl", > > > +}; > > > > On the contrary I understand this one (however, using that generic > > name prevents more than one or two drivers from being instantiated). > > Yeah that might need to change in the future, however there is no > obvious use-cases for using multiple cs42l43's in a single system > so at the moment we are not doing the work to support that case. > There are plenty other things that would need fixed to support > that as well. I see, just be aware that names for "root" swnodes must be globally unique. Otherwise they will clash over sysfs folder namings. ... > > > + SOFTWARE_NODE_REFERENCE(&swnode_gpio_undefined), > > > > gpio/property.h for this one. > > Sorry, still not quite following this one, are you just reminding > me to include the header when I move the swnode_gpio_undefined > definition or are you asking for something more? Yes, when you have moved the newly added exported variable there, include itt in addition to gpio/consumer.h. ... > > > + acpi_handle handle = ACPI_HANDLE_FWNODE(child_fwnode); > > > > > + if (!handle) > > > + continue; > > > > This is _almost_ redundant check. handle == NULL is for the global > > root object which quite unlikely will have the _ADR method. The > > specification is clear about this: "The _ADR object is valid only > > within an Augmented Device Descriptor." That said, the check makes > > sense against the (very) ill-formed DSDT. > > I am willing to be guided here, but given it would result in a > null pointer dereference I am inclined to keep the check > personally. There is no NULL pointer dereference. That's the point. And I explained how ACPICA treats this. ... > > > + if (has_sidecar) { > > > + ret = software_node_register(&cs42l43_gpiochip_swnode); > > > + if (ret) { > > > + return dev_err_probe(priv->dev, ret, > > > + "Failed to register gpio swnode\n"); > > > + } > > > + > > > + ret = device_create_managed_software_node(&priv->ctlr->dev, > > > + cs42l43_cs_props, NULL); > > > + if (ret) { > > > + dev_err_probe(priv->dev, ret, "Failed to add swnode\n"); > > > + goto err; > > > + } > > > > Wouldn't it miss the parent fwnode? I mean that you might probably > > need to call... > > > > > + } else { > > > + device_set_node(&priv->ctlr->dev, fwnode); > > > > ...this one always. Have you checked it? How does sysfs look like > > before and after this change on the device in question? > > I will check this. Basically in the expected case there should be two symlinks: to physical node and to swnode.
On Thu, Apr 11, 2024 at 09:17:53PM +0300, Andy Shevchenko wrote: > On Thu, Apr 11, 2024 at 8:13 PM Charles Keepax > <ckeepax@opensource.cirrus.com> wrote: > > On Thu, Apr 11, 2024 at 05:04:33PM +0300, Andy Shevchenko wrote: > > > On Thu, Apr 11, 2024 at 12:06 PM Charles Keepax > > > <ckeepax@opensource.cirrus.com> wrote: > > > > + if (has_sidecar) { > > > > + ret = software_node_register(&cs42l43_gpiochip_swnode); > > > > + if (ret) { > > > > + return dev_err_probe(priv->dev, ret, > > > > + "Failed to register gpio swnode\n"); > > > > + } > > > > + > > > > + ret = device_create_managed_software_node(&priv->ctlr->dev, > > > > + cs42l43_cs_props, NULL); > > > > + if (ret) { > > > > + dev_err_probe(priv->dev, ret, "Failed to add swnode\n"); > > > > + goto err; > > > > + } > > > > > > Wouldn't it miss the parent fwnode? I mean that you might probably > > > need to call... > > > Ok I am pretty sure this is all fine, I don't think we can pass a parent into device_create_managed_software_node since it requires a parent software node, but in this case there isn't one. This is the root node here, since the "parent" would be ACPI stuff here. > > > > + } else { > > > > + device_set_node(&priv->ctlr->dev, fwnode); > > > > > > ...this one always. Have you checked it? How does sysfs look like > > > before and after this change on the device in question? > > > > I will check this. > We can't always call device_set_node. Firstly, we would need to set it to the software node, however that is never returned from device_create_managed_software_node. Secondly, the set_secondary_node called in device_create_managed_software_node will set the primary node anyway since there isn't a valid primary node on the device. Finally, we don't want the primary node set to the ACPI node anyway since we want to override those settings here with our bridged amp settings. > Basically in the expected case there should be two symlinks: to > physical node and to swnode. > I think the sysfs all looks reasonable to me, I can see the SPI devices in /sys/bus/spi/devices, under those devices I can see a symlink to the software node. Thanks, Charles
On Mon, Apr 15, 2024 at 4:39 PM Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > On Thu, Apr 11, 2024 at 09:17:53PM +0300, Andy Shevchenko wrote: > > On Thu, Apr 11, 2024 at 8:13 PM Charles Keepax > > <ckeepax@opensource.cirrus.com> wrote: > > > On Thu, Apr 11, 2024 at 05:04:33PM +0300, Andy Shevchenko wrote: > > > > On Thu, Apr 11, 2024 at 12:06 PM Charles Keepax > > > > <ckeepax@opensource.cirrus.com> wrote: ... > > > > > + ret = software_node_register(&cs42l43_gpiochip_swnode); > > > > > + if (ret) { > > > > > + return dev_err_probe(priv->dev, ret, > > > > > + "Failed to register gpio swnode\n"); > > > > > + } > > > > > + > > > > > + ret = device_create_managed_software_node(&priv->ctlr->dev, > > > > > + cs42l43_cs_props, NULL); > > > > > + if (ret) { > > > > > + dev_err_probe(priv->dev, ret, "Failed to add swnode\n"); > > > > > + goto err; > > > > > + } > > > > > > > > Wouldn't it miss the parent fwnode? I mean that you might probably > > > > need to call... > > Ok I am pretty sure this is all fine, But have you checked this? > I don't think we can pass a > parent into device_create_managed_software_node since it requires > a parent software node, but in this case there isn't one. This is > the root node here, since the "parent" would be ACPI stuff here. No, this is done implicitly by so called primary and secondary fwnode. If you have no fwnode is added to the device (via let's say device_set_node() call), it most likely has no "primary" fwnode which is usually points to the "physical" one (from ACPI or DT), while secondary one will be pointing to swnode: Ex. # ls -ld /sys/devices/pci0000:00/0000:00:11.0/dwc3.0.auto/*_node lrwxrwxrwx 1 root root 0 Jan 1 00:01 /sys/devices/pci0000:00/0000:00:11.0/dwc3.0.auto/firmwar e_node -> ../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:08 lrwxrwxrwx 1 root root 0 Jan 1 00:01 /sys/devices/pci0000:00/0000:00:11.0/dwc3.0.auto/softwar e_node -> ../../../../kernel/software_nodes/node0 > > > > > + } else { > > > > > + device_set_node(&priv->ctlr->dev, fwnode); > > > > > > > > ...this one always. Have you checked it? How does sysfs look like > > > > before and after this change on the device in question? > > > > > > I will check this. > > We can't always call device_set_node. Firstly, we would need to > set it to the software node, however that is never returned from > device_create_managed_software_node. Secondly, the set_secondary_node > called in device_create_managed_software_node will set the primary > node anyway since there isn't a valid primary node on the device. That was basically my question above. If the device has a primary fwnode (or one shared with a parent) it would be nice to propagate it. OTOH it might have a side effect of using properties from that in the code. > Finally, we don't want the primary node set to the ACPI node anyway > since we want to override those settings here with our bridged amp > settings. > > > Basically in the expected case there should be two symlinks: to > > physical node and to swnode. > I think the sysfs all looks reasonable to me, I can see the SPI > devices in /sys/bus/spi/devices, under those devices I can see a > symlink to the software node. OK. -- With Best Regards, Andy Shevchenko
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index 554664efda86..17325e0b7bd5 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -284,6 +284,7 @@ config SPI_COLDFIRE_QSPI config SPI_CS42L43 tristate "Cirrus Logic CS42L43 SPI controller" depends on MFD_CS42L43 && PINCTRL_CS42L43 + select GPIO_SWNODE_UNDEFINED help This enables support for the SPI controller inside the Cirrus Logic CS42L43 audio codec. diff --git a/drivers/spi/spi-cs42l43.c b/drivers/spi/spi-cs42l43.c index aabef9fc84bd..29049f3f1f64 100644 --- a/drivers/spi/spi-cs42l43.c +++ b/drivers/spi/spi-cs42l43.c @@ -5,10 +5,14 @@ // Copyright (C) 2022-2023 Cirrus Logic, Inc. and // Cirrus Logic International Semiconductor Ltd. +#include <linux/acpi.h> +#include <linux/array_size.h> #include <linux/bits.h> #include <linux/bitfield.h> #include <linux/device.h> #include <linux/errno.h> +#include <linux/fwnode.h> +#include <linux/gpio/machine.h> #include <linux/mfd/cs42l43.h> #include <linux/mfd/cs42l43-regs.h> #include <linux/mod_devicetable.h> @@ -16,6 +20,7 @@ #include <linux/of.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> +#include <linux/property.h> #include <linux/regmap.h> #include <linux/spi/spi.h> #include <linux/units.h> @@ -39,6 +44,44 @@ static const unsigned int cs42l43_clock_divs[] = { 2, 2, 4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24, 26, 28, 30 }; +static const struct software_node ampl = { + .name = "cs35l56-left", +}; + +static const struct software_node ampr = { + .name = "cs35l56-right", +}; + +static struct spi_board_info ampl_info = { + .modalias = "cs35l56", + .max_speed_hz = 20 * HZ_PER_MHZ, + .chip_select = 0, + .mode = SPI_MODE_0, + .swnode = &l, +}; + +static struct spi_board_info ampr_info = { + .modalias = "cs35l56", + .max_speed_hz = 20 * HZ_PER_MHZ, + .chip_select = 1, + .mode = SPI_MODE_0, + .swnode = &r, +}; + +static const struct software_node cs42l43_gpiochip_swnode = { + .name = "cs42l43-pinctrl", +}; + +static const struct software_node_ref_args cs42l43_cs_refs[] = { + SOFTWARE_NODE_REFERENCE(&cs42l43_gpiochip_swnode, 0, GPIO_ACTIVE_LOW), + SOFTWARE_NODE_REFERENCE(&swnode_gpio_undefined), +}; + +static const struct property_entry cs42l43_cs_props[] = { + PROPERTY_ENTRY_REF_ARRAY("cs-gpios", cs42l43_cs_refs), + {} +}; + static int cs42l43_spi_tx(struct regmap *regmap, const u8 *buf, unsigned int len) { const u8 *end = buf + len; @@ -203,6 +246,43 @@ static size_t cs42l43_spi_max_length(struct spi_device *spi) return CS42L43_SPI_MAX_LENGTH; } +static bool cs42l43_has_sidecar(struct fwnode_handle *fwnode) +{ + static const u32 func_smart_amp = 0x1; + struct fwnode_handle *child_fwnode, *ext_fwnode; + unsigned int val; + u32 function; + int ret; + + fwnode_for_each_child_node(fwnode, child_fwnode) { + acpi_handle handle = ACPI_HANDLE_FWNODE(child_fwnode); + + if (!handle) + continue; + + ret = acpi_get_local_address(handle, &function); + if (ret || function != func_smart_amp) + continue; + + ext_fwnode = fwnode_get_named_child_node(child_fwnode, + "mipi-sdca-function-expansion-subproperties"); + if (!ext_fwnode) + continue; + + ret = fwnode_property_read_u32(ext_fwnode, + "01fa-cirrus-sidecar-instances", + &val); + + fwnode_handle_put(ext_fwnode); + fwnode_handle_put(child_fwnode); + + if (!ret) + return !!val; + } + + return false; +} + static void cs42l43_release_of_node(void *data) { fwnode_handle_put(data); @@ -213,6 +293,7 @@ static int cs42l43_spi_probe(struct platform_device *pdev) struct cs42l43 *cs42l43 = dev_get_drvdata(pdev->dev.parent); struct cs42l43_spi *priv; struct fwnode_handle *fwnode = dev_fwnode(cs42l43->dev); + bool has_sidecar = cs42l43_has_sidecar(fwnode); int ret; priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); @@ -266,16 +347,64 @@ static int cs42l43_spi_probe(struct platform_device *pdev) } } - device_set_node(&priv->ctlr->dev, fwnode); + if (has_sidecar) { + ret = software_node_register(&cs42l43_gpiochip_swnode); + if (ret) { + return dev_err_probe(priv->dev, ret, + "Failed to register gpio swnode\n"); + } + + ret = device_create_managed_software_node(&priv->ctlr->dev, + cs42l43_cs_props, NULL); + if (ret) { + dev_err_probe(priv->dev, ret, "Failed to add swnode\n"); + goto err; + } + } else { + device_set_node(&priv->ctlr->dev, fwnode); + } ret = devm_spi_register_controller(priv->dev, priv->ctlr); if (ret) { - dev_err(priv->dev, "Failed to register SPI controller: %d\n", ret); + dev_err_probe(priv->dev, ret, "Failed to register SPI controller\n"); + goto err; } + if (has_sidecar) { + if (!spi_new_device(priv->ctlr, &l_info)) { + ret = dev_err_probe(priv->dev, -ENODEV, + "Failed to create left amp slave\n"); + goto err; + } + + if (!spi_new_device(priv->ctlr, &r_info)) { + ret = dev_err_probe(priv->dev, -ENODEV, + "Failed to create right amp slave\n"); + goto err; + } + } + + return 0; + +err: + if (has_sidecar) + software_node_unregister(&cs42l43_gpiochip_swnode); + return ret; } +static int cs42l43_spi_remove(struct platform_device *pdev) +{ + struct cs42l43 *cs42l43 = dev_get_drvdata(pdev->dev.parent); + struct fwnode_handle *fwnode = dev_fwnode(cs42l43->dev); + bool has_sidecar = cs42l43_has_sidecar(fwnode); + + if (has_sidecar) + software_node_unregister(&cs42l43_gpiochip_swnode); + + return 0; +}; + static const struct platform_device_id cs42l43_spi_id_table[] = { { "cs42l43-spi", }, {} @@ -288,9 +417,11 @@ static struct platform_driver cs42l43_spi_driver = { }, .probe = cs42l43_spi_probe, .id_table = cs42l43_spi_id_table, + .remove = cs42l43_spi_remove, }; module_platform_driver(cs42l43_spi_driver); +MODULE_IMPORT_NS(GPIO_SWNODE); MODULE_DESCRIPTION("CS42L43 SPI Driver"); MODULE_AUTHOR("Lucas Tanure <tanureal@opensource.cirrus.com>"); MODULE_AUTHOR("Maciej Strozek <mstrozek@opensource.cirrus.com>");