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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Mon, Apr 15, 2024 at 4:39 PM Charles Keepax
<ckeepax@xxxxxxxxxxxxxxxxxxxxx> 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@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:

...

> > > > > +               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