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

[Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx>]

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:
> > From: Maciej Strozek <mstrozek@xxxxxxxxxxxxxxxxxxxxx>
> >
> > 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?
> 

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.

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

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

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

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.

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

Thanks,
Charles