diff mbox series

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

Message ID 20240411090628.2436389-5-ckeepax@opensource.cirrus.com (mailing list archive)
State Superseded
Headers show
Series Add bridged amplifiers to cs42l43 | expand

Commit Message

Charles Keepax April 11, 2024, 9:06 a.m. UTC
From: Maciej Strozek <mstrozek@opensource.cirrus.com>

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.

Signed-off-by: Maciej Strozek <mstrozek@opensource.cirrus.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

Changes since v4:
 - Drop some redundant fwnode_handle_puts
 - Remove stray blank line
 - Use ACPI_HANDLE_FWNODE

Thanks,
Charles

 drivers/spi/Kconfig       |   1 +
 drivers/spi/spi-cs42l43.c | 135 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 134 insertions(+), 2 deletions(-)

Comments

Andy Shevchenko April 11, 2024, 2:04 p.m. UTC | #1
On Thu, Apr 11, 2024 at 12:06 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
> From: Maciej Strozek <mstrozek@opensource.cirrus.com>
>
> 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?

> +       }
Andy Shevchenko April 11, 2024, 2:06 p.m. UTC | #2
On Thu, Apr 11, 2024 at 5:04 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Apr 11, 2024 at 12:06 PM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:

...

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

And still this leftover...

> > +               if (!ret)
> > +                       return !!val;
Andy Shevchenko April 11, 2024, 2:07 p.m. UTC | #3
On Thu, Apr 11, 2024 at 5:06 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Apr 11, 2024 at 5:04 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Thu, Apr 11, 2024 at 12:06 PM Charles Keepax
> > <ckeepax@opensource.cirrus.com> wrote:

...

> > > +               fwnode_handle_put(child_fwnode);
>
> And still this leftover...
>
> > > +               if (!ret)
> > > +                       return !!val;

To be precise it has to be refactored like

  if (ret)
    continue;

  put()
  return ...;
Charles Keepax April 11, 2024, 5:13 p.m. UTC | #4
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:
> > From: Maciej Strozek <mstrozek@opensource.cirrus.com>
> >
> > 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
Andy Shevchenko April 11, 2024, 6:17 p.m. UTC | #5
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:

...

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

Thank you for the patience, now I realise how it's connected. But
wouldn't the simple spi-<SPI ID>-<bus number>.<chip select> work?
Thinking loudly: Probably not as bus number is dynamic nowadays,

...

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

I see, just be aware that names for "root" swnodes must be globally
unique. Otherwise they will clash over sysfs folder namings.

...

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

Yes, when you have moved the newly added exported variable there,
include itt in addition to gpio/consumer.h.

...

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

There is no NULL pointer dereference. That's the point. And I
explained how ACPICA treats this.

...

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

Basically in the expected case there should be two symlinks: to
physical node and to swnode.
Charles Keepax April 15, 2024, 1:39 p.m. UTC | #6
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
Andy Shevchenko April 15, 2024, 4:03 p.m. UTC | #7
On Mon, Apr 15, 2024 at 4:39 PM Charles Keepax
<ckeepax@opensource.cirrus.com> 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@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:

...

> > > > > +               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
diff mbox series

Patch

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 554664efda86..17325e0b7bd5 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -284,6 +284,7 @@  config SPI_COLDFIRE_QSPI
 config SPI_CS42L43
 	tristate "Cirrus Logic CS42L43 SPI controller"
 	depends on MFD_CS42L43 && PINCTRL_CS42L43
+	select GPIO_SWNODE_UNDEFINED
 	help
 	  This enables support for the SPI controller inside the Cirrus Logic
 	  CS42L43 audio codec.
diff --git a/drivers/spi/spi-cs42l43.c b/drivers/spi/spi-cs42l43.c
index aabef9fc84bd..29049f3f1f64 100644
--- a/drivers/spi/spi-cs42l43.c
+++ b/drivers/spi/spi-cs42l43.c
@@ -5,10 +5,14 @@ 
 // Copyright (C) 2022-2023 Cirrus Logic, Inc. and
 //                         Cirrus Logic International Semiconductor Ltd.
 
+#include <linux/acpi.h>
+#include <linux/array_size.h>
 #include <linux/bits.h>
 #include <linux/bitfield.h>
 #include <linux/device.h>
 #include <linux/errno.h>
+#include <linux/fwnode.h>
+#include <linux/gpio/machine.h>
 #include <linux/mfd/cs42l43.h>
 #include <linux/mfd/cs42l43-regs.h>
 #include <linux/mod_devicetable.h>
@@ -16,6 +20,7 @@ 
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/spi/spi.h>
 #include <linux/units.h>
@@ -39,6 +44,44 @@  static const unsigned int cs42l43_clock_divs[] = {
 	2, 2, 4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24, 26, 28, 30
 };
 
+static const struct software_node ampl = {
+	.name			= "cs35l56-left",
+};
+
+static const struct software_node ampr = {
+	.name			= "cs35l56-right",
+};
+
+static struct spi_board_info ampl_info = {
+	.modalias		= "cs35l56",
+	.max_speed_hz		= 20 * HZ_PER_MHZ,
+	.chip_select		= 0,
+	.mode			= SPI_MODE_0,
+	.swnode			= &ampl,
+};
+
+static struct spi_board_info ampr_info = {
+	.modalias		= "cs35l56",
+	.max_speed_hz		= 20 * HZ_PER_MHZ,
+	.chip_select		= 1,
+	.mode			= SPI_MODE_0,
+	.swnode			= &ampr,
+};
+
+static const struct software_node cs42l43_gpiochip_swnode = {
+	.name			= "cs42l43-pinctrl",
+};
+
+static const struct software_node_ref_args cs42l43_cs_refs[] = {
+	SOFTWARE_NODE_REFERENCE(&cs42l43_gpiochip_swnode, 0, GPIO_ACTIVE_LOW),
+	SOFTWARE_NODE_REFERENCE(&swnode_gpio_undefined),
+};
+
+static const struct property_entry cs42l43_cs_props[] = {
+	PROPERTY_ENTRY_REF_ARRAY("cs-gpios", cs42l43_cs_refs),
+	{}
+};
+
 static int cs42l43_spi_tx(struct regmap *regmap, const u8 *buf, unsigned int len)
 {
 	const u8 *end = buf + len;
@@ -203,6 +246,43 @@  static size_t cs42l43_spi_max_length(struct spi_device *spi)
 	return CS42L43_SPI_MAX_LENGTH;
 }
 
+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;
+
+		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;
+}
+
 static void cs42l43_release_of_node(void *data)
 {
 	fwnode_handle_put(data);
@@ -213,6 +293,7 @@  static int cs42l43_spi_probe(struct platform_device *pdev)
 	struct cs42l43 *cs42l43 = dev_get_drvdata(pdev->dev.parent);
 	struct cs42l43_spi *priv;
 	struct fwnode_handle *fwnode = dev_fwnode(cs42l43->dev);
+	bool has_sidecar = cs42l43_has_sidecar(fwnode);
 	int ret;
 
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
@@ -266,16 +347,64 @@  static int cs42l43_spi_probe(struct platform_device *pdev)
 		}
 	}
 
-	device_set_node(&priv->ctlr->dev, fwnode);
+	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);
 	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;
 }
 
+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;
+};
+
 static const struct platform_device_id cs42l43_spi_id_table[] = {
 	{ "cs42l43-spi", },
 	{}
@@ -288,9 +417,11 @@  static struct platform_driver cs42l43_spi_driver = {
 	},
 	.probe		= cs42l43_spi_probe,
 	.id_table	= cs42l43_spi_id_table,
+	.remove		= cs42l43_spi_remove,
 };
 module_platform_driver(cs42l43_spi_driver);
 
+MODULE_IMPORT_NS(GPIO_SWNODE);
 MODULE_DESCRIPTION("CS42L43 SPI Driver");
 MODULE_AUTHOR("Lucas Tanure <tanureal@opensource.cirrus.com>");
 MODULE_AUTHOR("Maciej Strozek <mstrozek@opensource.cirrus.com>");