| Message ID | 20240411090628.2436389-1-ckeepax@opensource.cirrus.com |
|---|---|
| Headers | show |
| Series | Add bridged amplifiers to cs42l43 | expand |
On Thu, Apr 11, 2024 at 12:06 PM Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > > SPI devices can specify a cs-gpios property to enumerate their > chip selects. Under device tree, a zero entry in this property can > be used to specify that a particular chip select is using the SPI > controllers native chip select, for example: > > cs-gpios = <&gpio1 0 0>, <0>; > > Here the second chip select is native. However, when using swnodes Here, the > there is currently no way to specify a native chip select. The > proposal here is to register a swnode_gpio_undefined software node, > that can be specified to allow the indication of a native chip > select. For example: > > static const struct software_node_ref_args device_cs_refs[] = { > { > .node = &device_gpiochip_swnode, > .nargs = 2, > .args = { 0, GPIO_ACTIVE_LOW }, > }, > { > .node = &swnode_gpio_undefined, > .nargs = 0, > }, > }; > > Register the swnode as the gpiolib is initialised and check in > swnode_get_gpio_device() if the returned node matches > swnode_gpio_undefined and return -ENOENT, which matches the > behaviour of the device tree system when it encounters a 0 phandle. ... > +config GPIO_SWNODE_UNDEFINED > + bool But why did you remove the help? Please, put it back. ... > + /* > + * Check for special node that identifies undefined GPIOs, this is for a special > + * primarily used as a key for internal chip selects in SPI bindings. > + */ > + if (!strcmp(gdev_node->name, GPIOLIB_SWNODE_UNDEFINED_NAME)) > + return ERR_PTR(-ENOENT); This is a dead code when the respective config option is not selected. Or actually a potential flaw if somebody else names their swnode like this. ... > + ret = software_node_register(&swnode_gpio_undefined); > + if (ret < 0) > + pr_err("gpiolib: failed to register swnode: %d\n", ret); Instead of this prefix, define pr_fmt() > + return ret; ... > --- a/include/linux/gpio/consumer.h > +++ b/include/linux/gpio/consumer.h Why? I already said that the best place is gpio/property.h as it's exactly for swnode related code.
On Thu, Apr 11, 2024 at 12:06 PM Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > From: Maciej Strozek <mstrozek@opensource.cirrus.com> > > On some cs42l43 systems a couple of cs35l56 amplifiers are attached > to the cs42l43's SPI and I2S. On Windows the cs42l43 is controlled > by a SDCA class driver and these two amplifiers are controlled by > firmware running on the cs42l43. However, under Linux the decision > was made to interact with the cs42l43 directly, affording the user > greater control over the audio system. However, this has resulted > in an issue where these two bridged cs35l56 amplifiers are not > populated in ACPI and must be added manually. > > Check for the presence of the "01fa-cirrus-sidecar-instances" property > in the SDCA extension unit's ACPI properties to confirm the presence > of these two amplifiers and if they exist add them manually onto the > SPI bus. ... > +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? ... > +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). ... > + SOFTWARE_NODE_REFERENCE(&swnode_gpio_undefined), gpio/property.h for this one. ... > +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; 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. > + 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; > +} ... > + 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? > + }
On Thu, Apr 11, 2024 at 5:06 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Apr 11, 2024 at 5:04 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Thu, Apr 11, 2024 at 12:06 PM Charles Keepax > > <ckeepax@opensource.cirrus.com> wrote: ... > > > + fwnode_handle_put(child_fwnode); > > And still this leftover... > > > > + if (!ret) > > > + return !!val; To be precise it has to be refactored like if (ret) continue; put() return ...;
On Thu, Apr 11, 2024 at 04:25:25PM +0300, Andy Shevchenko wrote: > On Thu, Apr 11, 2024 at 12:06 PM Charles Keepax > <ckeepax@opensource.cirrus.com> wrote: > > Here the second chip select is native. However, when using swnodes > > Here, the > Can do. > > +config GPIO_SWNODE_UNDEFINED > > + bool > > But why did you remove the help? Please, put it back. > Sorry didn't realise you still wanted it will pop it back. > ... > > > + /* > > + * Check for special node that identifies undefined GPIOs, this is > > for a special Can do. > > > + * primarily used as a key for internal chip selects in SPI bindings. > > + */ > > + if (!strcmp(gdev_node->name, GPIOLIB_SWNODE_UNDEFINED_NAME)) > > + return ERR_PTR(-ENOENT); > > This is a dead code when the respective config option is not selected. > Or actually a potential flaw if somebody else names their swnode like > this. > Can add a check for the config. > ... > > > + ret = software_node_register(&swnode_gpio_undefined); > > + if (ret < 0) > > + pr_err("gpiolib: failed to register swnode: %d\n", ret); > > Instead of this prefix, define pr_fmt() > Little iffy on this, there are other prints in gpiolib that do it this way as well, I guess I could add a patch to convert everything but its starting to get a little out of the thrust of what I am doing here. > > + return ret; > > ... > > > --- a/include/linux/gpio/consumer.h > > +++ b/include/linux/gpio/consumer.h > > Why? I already said that the best place is gpio/property.h as it's > exactly for swnode related code. > Oh I see that's what you meant by those comments, as per my email I wasn't exactly sure. Will move them I don't mind where they go. Thanks, Charles
On Thu, Apr 11, 2024 at 04:30:13PM +0300, Andy Shevchenko wrote: > On Thu, Apr 11, 2024 at 12:06 PM Charles Keepax > <ckeepax@opensource.cirrus.com> wrote: > > > > Use the more modern is_acpi_device_node() rather than checking > > ACPI_COMPANION(). > > I don't think it's valuable on its own. There is no clear motivation > why to do that, I suggested it exactly in the conjunction of not > introducing two ways of fwnode type check. That said, you probably > want to elaborate the motivation in the commit message if you want to > keep it separate. > I am really tempted to just drop this, its not necessary for my changes and changes something that is unrelated to them. At the least it belongs in a separate patch. > ... > > > +#include <linux/fwnode.h> > > This header is not supposed to be included by the end users. property.h is. > Fair enough will update, although I really feel these headers could use some annotation if they are not supposed to be directly included. Either include everything you use or just include a top level header makes sense but this weird mixture we seem to use is very confusing and I don't have a big enough brain to remember every header. Thanks, Charles
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