Re: [PATCH v5 4/4] spi: cs42l43: Add bridged cs35l56 amplifiers

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Thu, Apr 11, 2024 at 8:13 PM Charles Keepax
<ckeepax@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx> 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.

-- 
With Best Regards,
Andy Shevchenko