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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

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?

...

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

> +       }

-- 
With Best Regards,
Andy Shevchenko