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

[Charles Keepax <ckeepax@opensource.cirrus.com>]

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