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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Mon, Apr 15, 2024 at 5:09 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.

FWIW,
Reviewed-by: Andy Shevchenko <andy@xxxxxxxxxx>
with the caveats:
- assuming primary - secondary fwnode expectations are correct
- no support for more than expected (2?) amplifiers as per global
naming of swnodes

See also the comment below.

...

> +       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;
> +               }
> +       } else {
> +               device_set_node(&priv->ctlr->dev, fwnode);
> +       }
>
>         ret = devm_spi_register_controller(priv->dev, priv->ctlr);

Left this chunk for the context below.

>         if (ret) {
> -               dev_err(priv->dev, "Failed to register SPI controller: %d\n", ret);
> +               dev_err_probe(priv->dev, ret, "Failed to register SPI controller\n");
> +               goto err;
> +       }
> +
> +       if (has_sidecar) {
> +               if (!spi_new_device(priv->ctlr, &ampl_info)) {
> +                       ret = dev_err_probe(priv->dev, -ENODEV,
> +                                           "Failed to create left amp slave\n");
> +                       goto err;
> +               }
> +
> +               if (!spi_new_device(priv->ctlr, &ampr_info)) {
> +                       ret = dev_err_probe(priv->dev, -ENODEV,
> +                                           "Failed to create right amp slave\n");
> +                       goto err;
> +               }
>         }
>
> +       return 0;

...

> +err:
> +       if (has_sidecar)
> +               software_node_unregister(&cs42l43_gpiochip_swnode);
> +
>         return ret;

This is wrong in terms of ordering.

...

> +static int cs42l43_spi_remove(struct platform_device *pdev)
> +{
> +       struct cs42l43 *cs42l43 = dev_get_drvdata(pdev->dev.parent);
> +       struct fwnode_handle *fwnode = dev_fwnode(cs42l43->dev);
> +       bool has_sidecar = cs42l43_has_sidecar(fwnode);
> +
> +       if (has_sidecar)
> +               software_node_unregister(&cs42l43_gpiochip_swnode);
> +
> +       return 0;

As this one.

> +};

You will remove the software node before removing the controller, this
seems incorrect ordering to me. What you need is to wrap by
devm_add_action_or_reset() and it won't be any remove() callback
needed.

-- 
With Best Regards,
Andy Shevchenko