[PATCH 0/3] Add bridged amplifiers to cs42l43

[PATCH 0/3] Add bridged amplifiers to cs42l43

[Charles Keepax]

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. There is at least an
SDCA extension unit DT entry we can key off.

The process of adding this is handled using a software node, firstly the
ability to add native chip selects to software nodes must be added.
Secondly, an additional flag for naming the SPI devices is added this
allows the machine driver to key to the correct amplifier. Then finally,
the cs42l43 SPI driver adds the two amplifiers directly onto its SPI
bus.

An additional series will follow soon to add the audio machine driver
parts (in the sof-sdw driver), however that is fairly orthogonal to
this part of the process, getting the actual amplifiers registered.

Thanks,
Charles

Series changes since v3:
 - Add Kconfig to make swnode conditionally built
 - Add define for swnode name
 - Add custom init and exit calls to register swnode
 - Use export namespaces
 - Always name swnode SPI devices after the node name
 - Correct some header includes
 - Use HZ_PER_MHZ
 - Use some swnode helper macros
 - Use acpi_get_local_address
 - Correct some handle puts
 - Add some dev_err_probes

Series changes since v2:
 - Add missing fwnode_handle_puts in driver/spi/spi-cs423l43.c

Series changes since v1:
 - Add missing statics in driver/spi/spi-cs42l43.c

Charles Keepax (2):
  gpio: swnode: Add ability to specify native chip selects for SPI
  spi: Add a mechanism to use the fwnode name for the SPI device

Maciej Strozek (1):
  spi: cs42l43: Add bridged cs35l56 amplifiers

 drivers/gpio/Kconfig          |   9 +++
 drivers/gpio/gpiolib-swnode.c |  38 ++++++++++
 drivers/spi/Kconfig           |   1 +
 drivers/spi/spi-cs42l43.c     | 139 +++++++++++++++++++++++++++++++++-
 drivers/spi/spi.c             |   6 ++
 include/linux/gpio/consumer.h |   4 +
 6 files changed, 195 insertions(+), 2 deletions(-)

-- 
2.39.2

[PATCH v4 1/3] gpio: swnode: Add ability to specify native chip selects for SPI

[Charles Keepax]

SPI devices can specify a cs-gpios property to enumerate their
chip selects. Under device tree, a zero entry in this property can
be used to specify that a particular chip select is using the SPI
controllers native chip select, for example:

        cs-gpios = <&gpio1 0 0>, <0>;

Here the second chip select is native. However, when using swnodes
there is currently no way to specify a native chip select. The
proposal here is to register a swnode_gpio_undefined software node,
that can be specified to allow the indication of a native chip
select. For example:

static const struct software_node_ref_args device_cs_refs[] = {
	{
		.node  = &device_gpiochip_swnode,
		.nargs = 2,
		.args  = { 0, GPIO_ACTIVE_LOW },
	},
	{
		.node  = &swnode_gpio_undefined,
		.nargs = 0,
	},
};

Register the swnode as the gpiolib is initialised and
check in swnode_get_gpio_device if the returned node matches
swnode_gpio_undefined and return -ENOENT, which matches the behaviour
of the device tree system when it encounters a 0 phandle.

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

Changes since v3:
 - Add Kconfig to make swnode conditionally built
 - Add define for swnode name
 - Add custom init and exit calls
 - Use export namespaces

Thanks,
Charles

 drivers/gpio/Kconfig          |  9 +++++++++
 drivers/gpio/gpiolib-swnode.c | 38 +++++++++++++++++++++++++++++++++++
 include/linux/gpio/consumer.h |  4 ++++
 3 files changed, 51 insertions(+)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index b50d0b470849..e9b977139674 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -103,6 +103,15 @@ config GPIO_REGMAP
 	select REGMAP
 	tristate
 
+config GPIO_SWNODE_UNDEFINED
+	bool "Undefined swnode GPIOs"
+	help
+	  This adds a special place holder for software nodes to contain an
+	  undefined GPIO reference, this is primarily used by SPI to allow a
+	  list of GPIO chip selects to mark a certain chip select as being
+	  controlled the SPI device's internal chip select mechanism and not
+	  a GPIO.
+
 # put drivers in the right section, in alphabetical order
 
 # This symbol is selected by both I2C and SPI expanders
diff --git a/drivers/gpio/gpiolib-swnode.c b/drivers/gpio/gpiolib-swnode.c
index fa52bdb1a29a..57722d829c61 100644
--- a/drivers/gpio/gpiolib-swnode.c
+++ b/drivers/gpio/gpiolib-swnode.c
@@ -17,6 +17,8 @@
 #include "gpiolib.h"
 #include "gpiolib-swnode.h"
 
+#define GPIOLIB_SWNODE_UNDEFINED_NAME "swnode-gpio-undefined"
+
 static void swnode_format_propname(const char *con_id, char *propname,
 				   size_t max_size)
 {
@@ -40,6 +42,13 @@ static struct gpio_device *swnode_get_gpio_device(struct fwnode_handle *fwnode)
 	if (!gdev_node || !gdev_node->name)
 		return ERR_PTR(-EINVAL);
 
+	/*
+	 * Check for special node that identifies undefined GPIOs, this is
+	 * primarily used as a key for internal chip selects in SPI bindings.
+	 */
+	if (!strcmp(gdev_node->name, GPIOLIB_SWNODE_UNDEFINED_NAME))
+		return ERR_PTR(-ENOENT);
+
 	gdev = gpio_device_find_by_label(gdev_node->name);
 	return gdev ?: ERR_PTR(-EPROBE_DEFER);
 }
@@ -121,3 +130,32 @@ int swnode_gpio_count(const struct fwnode_handle *fwnode, const char *con_id)
 
 	return count ?: -ENOENT;
 }
+
+#if IS_ENABLED(CONFIG_GPIO_SWNODE_UNDEFINED)
+/*
+ * A special node that identifies undefined GPIOs, this is primarily used as
+ * a key for internal chip selects in SPI bindings.
+ */
+const struct software_node swnode_gpio_undefined = {
+	.name = GPIOLIB_SWNODE_UNDEFINED_NAME,
+};
+EXPORT_SYMBOL_NS_GPL(swnode_gpio_undefined, GPIO_SWNODE);
+
+static int __init swnode_gpio_init(void)
+{
+	int ret;
+
+	ret = software_node_register(&swnode_gpio_undefined);
+	if (ret < 0)
+		pr_err("gpiolib: failed to register swnode: %d\n", ret);
+
+	return ret;
+}
+subsys_initcall(swnode_gpio_init);
+
+static void __exit swnode_gpio_cleanup(void)
+{
+	software_node_unregister(&swnode_gpio_undefined);
+}
+__exitcall(swnode_gpio_cleanup);
+#endif
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index db2dfbae8edb..e685fac43398 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -12,6 +12,8 @@ struct fwnode_handle;
 struct gpio_array;
 struct gpio_desc;
 
+struct software_node;
+
 /**
  * struct gpio_descs - Struct containing an array of descriptors that can be
  *                     obtained using gpiod_get_array()
@@ -54,6 +56,8 @@ enum gpiod_flags {
 	GPIOD_OUT_HIGH_OPEN_DRAIN = GPIOD_OUT_HIGH | GPIOD_FLAGS_BIT_OPEN_DRAIN,
 };
 
+extern const struct software_node swnode_gpio_undefined;
+
 #ifdef CONFIG_GPIOLIB
 
 /* Return the number of GPIOs associated with a device / function */
-- 
2.39.2

[PATCH v4 2/3] spi: Add a mechanism to use the fwnode name for the SPI device

[Charles Keepax]

Add a mechanism to force the use of the fwnode name for the name of the
SPI device itself. This is useful when devices need to be manually added
within the kernel.

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

Changes since v3:
 - Always name swnode SPI devices after the node name

Thanks,
Charles

 drivers/spi/spi.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index a2f01116ba09..52c70705d179 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -598,12 +598,18 @@ EXPORT_SYMBOL_GPL(spi_alloc_device);
 static void spi_dev_set_name(struct spi_device *spi)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&spi->dev);
+	struct fwnode_handle *fwnode = dev_fwnode(&spi->dev);
 
 	if (adev) {
 		dev_set_name(&spi->dev, "spi-%s", acpi_dev_name(adev));
 		return;
 	}
 
+	if (is_software_node(fwnode)) {
+		dev_set_name(&spi->dev, "spi-%s", fwnode_get_name(fwnode));
+		return;
+	}
+
 	dev_set_name(&spi->dev, "%s.%u", dev_name(&spi->controller->dev),
 		     spi_get_chipselect(spi, 0));
 }
-- 
2.39.2

[PATCH v4 3/3] spi: cs42l43: Add bridged cs35l56 amplifiers

[Charles Keepax]

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 v3:
 - Correct some header includes
 - Use HZ_PER_MHZ
 - Use some swnode helper macros
 - Use acpi_get_local_address
 - Correct some handle puts
 - Add some dev_err_probes

Thanks,
Charles

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

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 6e4b5f7e8adc..ffc4bd7a67e6 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..a4208c2dfc76 100644
--- a/drivers/spi/spi-cs42l43.c
+++ b/drivers/spi/spi-cs42l43.c
@@ -5,10 +5,13 @@
 // 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/gpio/machine.h>
 #include <linux/mfd/cs42l43.h>
 #include <linux/mfd/cs42l43-regs.h>
 #include <linux/mod_devicetable.h>
@@ -16,6 +19,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 +43,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 +245,47 @@ 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) {
+		struct acpi_device *adev = to_acpi_device_node(child_fwnode);
+
+		if (!adev)
+			continue;
+
+		ret = acpi_get_local_address(adev->handle, &function);
+		if (ret || function != func_smart_amp) {
+			fwnode_handle_put(child_fwnode);
+			continue;
+		}
+
+		ext_fwnode = fwnode_get_named_child_node(child_fwnode,
+				"mipi-sdca-function-expansion-subproperties");
+		if (!ext_fwnode) {
+			fwnode_handle_put(child_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 +296,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 +350,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 +420,12 @@ 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>");
-- 
2.39.2

Re: [PATCH v4 1/3] gpio: swnode: Add ability to specify native chip selects for SPI

[Linus Walleij]

On Tue, Apr 9, 2024 at 3:21 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:

> SPI devices can specify a cs-gpios property to enumerate their
> chip selects. Under device tree, a zero entry in this property can
> be used to specify that a particular chip select is using the SPI
> controllers native chip select, for example:
>
>         cs-gpios = <&gpio1 0 0>, <0>;
>
> Here the second chip select is native. However, when using swnodes
> there is currently no way to specify a native chip select. The
> proposal here is to register a swnode_gpio_undefined software node,
> that can be specified to allow the indication of a native chip
> select. For example:
>
> static const struct software_node_ref_args device_cs_refs[] = {
>         {
>                 .node  = &device_gpiochip_swnode,
>                 .nargs = 2,
>                 .args  = { 0, GPIO_ACTIVE_LOW },
>         },
>         {
>                 .node  = &swnode_gpio_undefined,
>                 .nargs = 0,
>         },
> };
>
> Register the swnode as the gpiolib is initialised and
> check in swnode_get_gpio_device if the returned node matches
> swnode_gpio_undefined and return -ENOENT, which matches the behaviour
> of the device tree system when it encounters a 0 phandle.
>
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>

I love it.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v4 2/3] spi: Add a mechanism to use the fwnode name for the SPI device

[Charles Keepax]

On Tue, Apr 09, 2024 at 02:21:25PM +0100, Charles Keepax wrote:
> Add a mechanism to force the use of the fwnode name for the name of the
> SPI device itself. This is useful when devices need to be manually added
> within the kernel.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---

Apologies looking at this I really should have updated the commit
message as well. I will send a new rev soon if there are no new
comments.

Thanks,
Charles

Re: [PATCH v4 1/3] gpio: swnode: Add ability to specify native chip selects for SPI

[Andy Shevchenko]

On Tue, Apr 09, 2024 at 02:21:24PM +0100, Charles Keepax wrote:
> SPI devices can specify a cs-gpios property to enumerate their
> chip selects. Under device tree, a zero entry in this property can
> be used to specify that a particular chip select is using the SPI
> controllers native chip select, for example:
> 
>         cs-gpios = <&gpio1 0 0>, <0>;
> 
> Here the second chip select is native. However, when using swnodes
> there is currently no way to specify a native chip select. The
> proposal here is to register a swnode_gpio_undefined software node,
> that can be specified to allow the indication of a native chip
> select. For example:
> 
> static const struct software_node_ref_args device_cs_refs[] = {
> 	{
> 		.node  = &device_gpiochip_swnode,
> 		.nargs = 2,
> 		.args  = { 0, GPIO_ACTIVE_LOW },
> 	},
> 	{
> 		.node  = &swnode_gpio_undefined,
> 		.nargs = 0,
> 	},
> };
> 
> Register the swnode as the gpiolib is initialised and
> check in swnode_get_gpio_device if the returned node matches

swnode_get_gpio_device()

> swnode_gpio_undefined and return -ENOENT, which matches the behaviour
> of the device tree system when it encounters a 0 phandle.

...

> +config GPIO_SWNODE_UNDEFINED
> +	bool "Undefined swnode GPIOs"

Why is this user visible?

> +	help
> +	  This adds a special place holder for software nodes to contain an
> +	  undefined GPIO reference, this is primarily used by SPI to allow a
> +	  list of GPIO chip selects to mark a certain chip select as being
> +	  controlled the SPI device's internal chip select mechanism and not
> +	  a GPIO.

How are drivers supposed to work in case this is not selected?

...

> +EXPORT_SYMBOL_NS_GPL(swnode_gpio_undefined, GPIO_SWNODE);

+ export.h

> +static int __init swnode_gpio_init(void)

+ init.h

> +		pr_err("gpiolib: failed to register swnode: %d\n", ret);

+ printk.h

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/3] spi: Add a mechanism to use the fwnode name for the SPI device

[Andy Shevchenko]

On Tue, Apr 09, 2024 at 02:21:25PM +0100, Charles Keepax wrote:
> Add a mechanism to force the use of the fwnode name for the name of the
> SPI device itself. This is useful when devices need to be manually added
> within the kernel.

Same comment, we don't need two ways to handle fwnode type
(and effectivelly code duplication to some extent).

...

>  	struct acpi_device *adev = ACPI_COMPANION(&spi->dev);

>  	if (adev) {

Replace this to be is_acpi_device_node() check...

>  		dev_set_name(&spi->dev, "spi-%s", acpi_dev_name(adev));

...and derive adev from fwnode.

>  		return;
>  	}

> +	if (is_software_node(fwnode)) {
> +		dev_set_name(&spi->dev, "spi-%s", fwnode_get_name(fwnode));

While at this, you can also introduce

	struct device *dev = &spi->dev;

to make these dev_set_name() be shorter.

> +		return;
> +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 3/3] spi: cs42l43: Add bridged cs35l56 amplifiers

[Andy Shevchenko]

On Tue, Apr 09, 2024 at 02:21:26PM +0100, Charles Keepax 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.

...

> +#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/gpio/machine.h>

Shouldn't you include gpio/property.h as well?
Ah, in the previous patch you put swnode to consumer.h instead of
gpio/property.h. Please, fix that.

>  #include <linux/mfd/cs42l43.h>
>  #include <linux/mfd/cs42l43-regs.h>
>  #include <linux/mod_devicetable.h>

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

...

> +static const struct software_node ampl = {
> +	.name			= "cs35l56-left",
> +};
> +
> +static const struct software_node ampr = {
> +	.name			= "cs35l56-right",
> +};

What these swnodes are for?

...

> +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) {
> +		struct acpi_device *adev = to_acpi_device_node(child_fwnode);
> +
> +		if (!adev)
> +			continue;
> +
> +		ret = acpi_get_local_address(adev->handle, &function);
> +		if (ret || function != func_smart_amp) {

> +			fwnode_handle_put(child_fwnode);

Why?

> +			continue;
> +		}
> +
> +		ext_fwnode = fwnode_get_named_child_node(child_fwnode,
> +				"mipi-sdca-function-expansion-subproperties");
> +		if (!ext_fwnode) {

> +			fwnode_handle_put(child_fwnode);

Ditto.

> +			continue;
> +		}
> +
> +		ret = fwnode_property_read_u32(ext_fwnode,
> +					       "01fa-cirrus-sidecar-instances",
> +					       &val);
> +
> +		fwnode_handle_put(ext_fwnode);

> +		fwnode_handle_put(child_fwnode);

Ditto.

Haven't you get reference underflow?

> +
> +		if (!ret)
> +			return !!val;
> +	}
> +

> +	return false;
> +}

...

> +MODULE_IMPORT_NS(GPIO_SWNODE);

> +

Stray blank line.

>  MODULE_DESCRIPTION("CS42L43 SPI Driver");
>  MODULE_AUTHOR("Lucas Tanure <tanureal@opensource.cirrus.com>");
>  MODULE_AUTHOR("Maciej Strozek <mstrozek@opensource.cirrus.com>");

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 3/3] spi: cs42l43: Add bridged cs35l56 amplifiers

[kernel test robot]

Hi Charles,

kernel test robot noticed the following build errors:

[auto build test ERROR on broonie-spi/for-next]
[also build test ERROR on brgl/gpio/for-next linus/master v6.9-rc3 next-20240410]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Charles-Keepax/gpio-swnode-Add-ability-to-specify-native-chip-selects-for-SPI/20240409-212316
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
patch link:    https://lore.kernel.org/r/20240409132126.1117916-4-ckeepax%40opensource.cirrus.com
patch subject: [PATCH v4 3/3] spi: cs42l43: Add bridged cs35l56 amplifiers
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20240410/202404101443.tYCaeZAm-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240410/202404101443.tYCaeZAm-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202404101443.tYCaeZAm-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/spi/spi-cs42l43.c: In function 'cs42l43_has_sidecar':
>> drivers/spi/spi-cs42l43.c:262:50: error: invalid use of undefined type 'struct acpi_device'
     262 |                 ret = acpi_get_local_address(adev->handle, &function);
         |                                                  ^~


vim +262 drivers/spi/spi-cs42l43.c

   247	
   248	static bool cs42l43_has_sidecar(struct fwnode_handle *fwnode)
   249	{
   250		static const u32 func_smart_amp = 0x1;
   251		struct fwnode_handle *child_fwnode, *ext_fwnode;
   252		unsigned int val;
   253		u32 function;
   254		int ret;
   255	
   256		fwnode_for_each_child_node(fwnode, child_fwnode) {
   257			struct acpi_device *adev = to_acpi_device_node(child_fwnode);
   258	
   259			if (!adev)
   260				continue;
   261	
 > 262			ret = acpi_get_local_address(adev->handle, &function);
   263			if (ret || function != func_smart_amp) {
   264				fwnode_handle_put(child_fwnode);
   265				continue;
   266			}
   267	
   268			ext_fwnode = fwnode_get_named_child_node(child_fwnode,
   269					"mipi-sdca-function-expansion-subproperties");
   270			if (!ext_fwnode) {
   271				fwnode_handle_put(child_fwnode);
   272				continue;
   273			}
   274	
   275			ret = fwnode_property_read_u32(ext_fwnode,
   276						       "01fa-cirrus-sidecar-instances",
   277						       &val);
   278	
   279			fwnode_handle_put(ext_fwnode);
   280			fwnode_handle_put(child_fwnode);
   281	
   282			if (!ret)
   283				return !!val;
   284		}
   285	
   286		return false;
   287	}
   288	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v4 3/3] spi: cs42l43: Add bridged cs35l56 amplifiers

[Andy Shevchenko]

Wed, Apr 10, 2024 at 03:42:35PM +0800, kernel test robot kirjoitti:
> Hi Charles,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on broonie-spi/for-next]
> [also build test ERROR on brgl/gpio/for-next linus/master v6.9-rc3 next-20240410]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Charles-Keepax/gpio-swnode-Add-ability-to-specify-native-chip-selects-for-SPI/20240409-212316
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
> patch link:    https://lore.kernel.org/r/20240409132126.1117916-4-ckeepax%40opensource.cirrus.com
> patch subject: [PATCH v4 3/3] spi: cs42l43: Add bridged cs35l56 amplifiers
> config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20240410/202404101443.tYCaeZAm-lkp@intel.com/config)
> compiler: m68k-linux-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240410/202404101443.tYCaeZAm-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202404101443.tYCaeZAm-lkp@intel.com/
> 
> All errors (new ones prefixed by >>):
> 
>    drivers/spi/spi-cs42l43.c: In function 'cs42l43_has_sidecar':
> >> drivers/spi/spi-cs42l43.c:262:50: error: invalid use of undefined type 'struct acpi_device'
>      262 |                 ret = acpi_get_local_address(adev->handle, &function);
>          |                                                  ^~

Oh, yes, this should take ACPI_HANDLE_FWNODE() (and the adev will gone AFAIU?).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/3] spi: Add a mechanism to use the fwnode name for the SPI device

[Charles Keepax]

On Tue, Apr 09, 2024 at 09:06:56PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 09, 2024 at 02:21:25PM +0100, Charles Keepax wrote:
> > Add a mechanism to force the use of the fwnode name for the name of the
> > SPI device itself. This is useful when devices need to be manually added
> > within the kernel.
> 
> Same comment, we don't need two ways to handle fwnode type
> (and effectivelly code duplication to some extent).
> 
> ...
> 
> >  	struct acpi_device *adev = ACPI_COMPANION(&spi->dev);
> 
> >  	if (adev) {
> 
> Replace this to be is_acpi_device_node() check...
> 
> >  		dev_set_name(&spi->dev, "spi-%s", acpi_dev_name(adev));
> 
> ...and derive adev from fwnode.
> 

I had been hoping to not modify the path I wasn't using but fair
enough if you are sure this is a fine substitution.

> >  		return;
> >  	}
> 
> > +	if (is_software_node(fwnode)) {
> > +		dev_set_name(&spi->dev, "spi-%s", fwnode_get_name(fwnode));
> 
> While at this, you can also introduce
> 
> 	struct device *dev = &spi->dev;
> 
> to make these dev_set_name() be shorter.
> 

Sure.

Thanks,
Charles

Re: [PATCH v4 1/3] gpio: swnode: Add ability to specify native chip selects for SPI

[Charles Keepax]

On Tue, Apr 09, 2024 at 09:03:30PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 09, 2024 at 02:21:24PM +0100, Charles Keepax wrote:
> > SPI devices can specify a cs-gpios property to enumerate their
> > chip selects. Under device tree, a zero entry in this property can
> > be used to specify that a particular chip select is using the SPI
> > controllers native chip select, for example:
> > 
> >         cs-gpios = <&gpio1 0 0>, <0>;
> > 
> > Here the second chip select is native. However, when using swnodes
> > there is currently no way to specify a native chip select. The
> > proposal here is to register a swnode_gpio_undefined software node,
> > that can be specified to allow the indication of a native chip
> > select. For example:
> > 
> > static const struct software_node_ref_args device_cs_refs[] = {
> > 	{
> > 		.node  = &device_gpiochip_swnode,
> > 		.nargs = 2,
> > 		.args  = { 0, GPIO_ACTIVE_LOW },
> > 	},
> > 	{
> > 		.node  = &swnode_gpio_undefined,
> > 		.nargs = 0,
> > 	},
> > };
> > 
> > Register the swnode as the gpiolib is initialised and
> > check in swnode_get_gpio_device if the returned node matches
> 
> swnode_get_gpio_device()
> 

sorry yeah will fix that up.

> > swnode_gpio_undefined and return -ENOENT, which matches the behaviour
> > of the device tree system when it encounters a 0 phandle.
> 
> ...
> 
> > +config GPIO_SWNODE_UNDEFINED
> > +	bool "Undefined swnode GPIOs"
> 
> Why is this user visible?
> 

Yeah a good shout no reason for it to be.

> > +	help
> > +	  This adds a special place holder for software nodes to contain an
> > +	  undefined GPIO reference, this is primarily used by SPI to allow a
> > +	  list of GPIO chip selects to mark a certain chip select as being
> > +	  controlled the SPI device's internal chip select mechanism and not
> > +	  a GPIO.
> 
> How are drivers supposed to work in case this is not selected?
> 
> ...

Well they don't :-)

Thanks,
Charles

Re: [PATCH v4 3/3] spi: cs42l43: Add bridged cs35l56 amplifiers

[Charles Keepax]

On Tue, Apr 09, 2024 at 09:26:44PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 09, 2024 at 02:21:26PM +0100, Charles Keepax wrote:
> > From: Maciej Strozek <mstrozek@opensource.cirrus.com>
> > +#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/gpio/machine.h>
> 
> Shouldn't you include gpio/property.h as well?
> Ah, in the previous patch you put swnode to consumer.h instead of
> gpio/property.h. Please, fix that.
> 

Sorry not sure I follow here, nothing is using
PROPERTY_ENTRY_GPIO and not sure why that is needed in the swnode
patch either?

> >  #include <linux/mfd/cs42l43.h>
> >  #include <linux/mfd/cs42l43-regs.h>
> >  #include <linux/mod_devicetable.h>
> 
> >  #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>
> 
> ...
> 
> > +static const struct software_node ampl = {
> > +	.name			= "cs35l56-left",
> > +};
> > +
> > +static const struct software_node ampr = {
> > +	.name			= "cs35l56-right",
> > +};
> 
> What these swnodes are for?
> 

The two amps we are adding, not sure I entirely follow what you
are asking here. We need the software nodes so we can name the
amps something such that we can find them from the machine driver
later.

> ...
> 
> > +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) {
> > +		struct acpi_device *adev = to_acpi_device_node(child_fwnode);
> > +
> > +		if (!adev)
> > +			continue;
> > +
> > +		ret = acpi_get_local_address(adev->handle, &function);
> > +		if (ret || function != func_smart_amp) {
> 
> > +			fwnode_handle_put(child_fwnode);
> 
> Why?
> 

Ah had missed the fwnode_for_each_child will do the put itself,
will fix that up.

> > +MODULE_IMPORT_NS(GPIO_SWNODE);
> 
> > +
> 
> Stray blank line.
> 

Fair enough will remove.

Thanks,
Charles