[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 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/gpiolib-swnode.c |   8 ++
 drivers/gpio/gpiolib.c        |   9 ++
 drivers/spi/spi-cs42l43.c     | 155 +++++++++++++++++++++++++++++++++-
 drivers/spi/spi.c             |   7 ++
 include/linux/gpio/consumer.h |   4 +
 include/linux/spi/spi.h       |   2 +
 6 files changed, 184 insertions(+), 1 deletion(-)

-- 
2.39.2

[PATCH v3 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>
---

No changes since v2.

Thanks,
Charles

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

diff --git a/drivers/gpio/gpiolib-swnode.c b/drivers/gpio/gpiolib-swnode.c
index fa52bdb1a29a3..801b5a660307f 100644
--- a/drivers/gpio/gpiolib-swnode.c
+++ b/drivers/gpio/gpiolib-swnode.c
@@ -17,6 +17,11 @@
 #include "gpiolib.h"
 #include "gpiolib-swnode.h"
 
+const struct software_node swnode_gpio_undefined = {
+	.name = "gpio-internal-undefined",
+};
+EXPORT_SYMBOL_GPL(swnode_gpio_undefined);
+
 static void swnode_format_propname(const char *con_id, char *propname,
 				   size_t max_size)
 {
@@ -40,6 +45,9 @@ static struct gpio_device *swnode_get_gpio_device(struct fwnode_handle *fwnode)
 	if (!gdev_node || !gdev_node->name)
 		return ERR_PTR(-EINVAL);
 
+	if (!strcmp(gdev_node->name, "gpio-internal-undefined"))
+		return ERR_PTR(-ENOENT);
+
 	gdev = gpio_device_find_by_label(gdev_node->name);
 	return gdev ?: ERR_PTR(-EPROBE_DEFER);
 }
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index ce94e37bcbee7..e3a7e2a3a323e 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4892,8 +4892,17 @@ DEFINE_SEQ_ATTRIBUTE(gpiolib);
 
 static int __init gpiolib_debugfs_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;
+	}
+
 	/* /sys/kernel/debug/gpio */
 	debugfs_create_file("gpio", 0444, NULL, NULL, &gpiolib_fops);
+
 	return 0;
 }
 subsys_initcall(gpiolib_debugfs_init);
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index db2dfbae8edbd..e685fac43398d 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 v3 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>
---

No changes since v2.

Thanks,
Charles

 drivers/spi/spi.c       | 7 +++++++
 include/linux/spi/spi.h | 2 ++
 2 files changed, 9 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index a2f01116ba092..a38a4960032b8 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -598,6 +598,12 @@ 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 (spi->use_fwnode_name && fwnode) {
+		dev_set_name(&spi->dev, "spi-%s", fwnode_get_name(fwnode));
+		return;
+	}
 
 	if (adev) {
 		dev_set_name(&spi->dev, "spi-%s", acpi_dev_name(adev));
@@ -830,6 +836,7 @@ struct spi_device *spi_new_device(struct spi_controller *ctlr,
 	 * spi->cs_index_mask as 0x01.
 	 */
 	proxy->cs_index_mask = 0x01;
+	proxy->use_fwnode_name = chip->use_fwnode_name;
 
 	if (chip->swnode) {
 		status = device_add_software_node(&proxy->dev, chip->swnode);
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index b589e25474393..265f5d0c15304 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -222,6 +222,7 @@ struct spi_device {
 	struct spi_delay	cs_setup;
 	struct spi_delay	cs_hold;
 	struct spi_delay	cs_inactive;
+	bool			use_fwnode_name;
 
 	/* The statistics */
 	struct spi_statistics __percpu	*pcpu_statistics;
@@ -1603,6 +1604,7 @@ struct spi_board_info {
 	char		modalias[SPI_NAME_SIZE];
 	const void	*platform_data;
 	const struct software_node *swnode;
+	bool		use_fwnode_name;
 	void		*controller_data;
 	int		irq;
 
-- 
2.39.2

[PATCH v3 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 v2:
 - Add missing fwnode_handle_puts in cs42l43_has_sidecar

Thanks,
Charles

 drivers/spi/spi-cs42l43.c | 155 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 154 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-cs42l43.c b/drivers/spi/spi-cs42l43.c
index aabef9fc84bdf..15e93ef7b74d0 100644
--- a/drivers/spi/spi-cs42l43.c
+++ b/drivers/spi/spi-cs42l43.c
@@ -5,6 +5,8 @@
 // Copyright (C) 2022-2023 Cirrus Logic, Inc. and
 //                         Cirrus Logic International Semiconductor Ltd.
 
+#include <dt-bindings/gpio/gpio.h>
+#include <linux/acpi.h>
 #include <linux/bits.h>
 #include <linux/bitfield.h>
 #include <linux/device.h>
@@ -39,6 +41,59 @@ 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		= 2000000,
+	.chip_select		= 0,
+	.mode			= SPI_MODE_0,
+	.swnode			= &ampl,
+	.use_fwnode_name	= true,
+};
+
+static struct spi_board_info ampr_info = {
+	.modalias		= "cs35l56",
+	.max_speed_hz		= 2000000,
+	.chip_select		= 1,
+	.mode			= SPI_MODE_0,
+	.swnode			= &ampr,
+	.use_fwnode_name	= true,
+};
+
+static const struct software_node cs42l43_gpiochip_swnode = {
+	.name			= "cs42l43-pinctrl",
+};
+
+static const struct software_node_ref_args cs42l43_cs_refs[] = {
+	{
+		.node		= &cs42l43_gpiochip_swnode,
+		.nargs		= 2,
+		.args		= { 0, GPIO_ACTIVE_LOW },
+	},
+	{
+		.node		= &swnode_gpio_undefined,
+		.nargs		= 0,
+	},
+};
+
+static const struct property_entry cs42l43_cs_props[] = {
+	{
+		.name		= "cs-gpios",
+		.length		= ARRAY_SIZE(cs42l43_cs_refs) *
+				  sizeof(struct software_node_ref_args),
+		.type		= DEV_PROP_REF,
+		.pointer	= cs42l43_cs_refs,
+	},
+	{}
+};
+
 static int cs42l43_spi_tx(struct regmap *regmap, const u8 *buf, unsigned int len)
 {
 	const u8 *end = buf + len;
@@ -203,6 +258,54 @@ static size_t cs42l43_spi_max_length(struct spi_device *spi)
 	return CS42L43_SPI_MAX_LENGTH;
 }
 
+#if IS_ENABLED(CONFIG_ACPI)
+static bool cs42l43_has_sidecar(struct fwnode_handle *fwnode)
+{
+	static const int func_smart_amp = 0x1;
+	struct fwnode_handle *child_fwnode, *ext_fwnode;
+	unsigned long long function;
+	unsigned int val;
+	int ret;
+
+	if (!is_acpi_node(fwnode))
+		return false;
+
+	fwnode_for_each_child_node(fwnode, child_fwnode) {
+		struct acpi_device *adev = to_acpi_device_node(child_fwnode);
+
+		ret = acpi_evaluate_integer(adev->handle, "_ADR", NULL, &function);
+		if (ACPI_FAILURE(ret) || function != func_smart_amp) {
+			fwnode_handle_put(fwnode);
+			continue;
+		}
+
+		ext_fwnode = fwnode_get_named_child_node(child_fwnode,
+				"mipi-sdca-function-expansion-subproperties");
+		if (!ext_fwnode) {
+			fwnode_handle_put(fwnode);
+			continue;
+		}
+
+		ret = fwnode_property_read_u32(ext_fwnode,
+					       "01fa-cirrus-sidecar-instances",
+					       &val);
+
+		fwnode_handle_put(ext_fwnode);
+		fwnode_handle_put(fwnode);
+
+		if (!ret)
+			return !!val;
+	}
+
+	return false;
+}
+#else
+static bool cs42l43_has_sidecar(struct fwnode_handle *fwnode)
+{
+	return false;
+}
+#endif
+
 static void cs42l43_release_of_node(void *data)
 {
 	fwnode_handle_put(data);
@@ -213,6 +316,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 +370,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) {
+			dev_err(priv->dev, "Failed to register gpio swnode: %d\n", ret);
+			return ret;
+		}
+
+		ret = device_create_managed_software_node(&priv->ctlr->dev, cs42l43_cs_props, NULL);
+		if (ret) {
+			dev_err(priv->dev, "Failed to add swnode: %d\n", ret);
+			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);
+		goto err;
+	}
+
+	if (has_sidecar) {
+		if (!spi_new_device(priv->ctlr, &ampl_info)) {
+			ret = -ENODEV;
+			dev_err(priv->dev, "Failed to create left amp slave\n");
+			goto err;
+		}
+
+		if (!spi_new_device(priv->ctlr, &ampr_info)) {
+			ret = -ENODEV;
+			dev_err(priv->dev, "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,6 +440,7 @@ 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);
 
-- 
2.39.2

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

[Andy Shevchenko]

Fri, Mar 29, 2024 at 11:47:28AM +0000, Charles Keepax kirjoitti:
> 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.

...

> +const struct software_node swnode_gpio_undefined = {
> +	.name = "gpio-internal-undefined",

This name is still too generic, perhaps "software-node-undefined-gpio"?

> +};
> +EXPORT_SYMBOL_GPL(swnode_gpio_undefined);

Maybe we can start using namespaces?

...

> +	if (!strcmp(gdev_node->name, "gpio-internal-undefined"))

Probably it needs a definition to avoid typos in case somebody wants to change
the name.

#define	GPIOLIB_SWNODE_UNDEFINED	"..."

?

> +		return ERR_PTR(-ENOENT);

...

>  static int __init gpiolib_debugfs_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;
> +	}
> +
>  	/* /sys/kernel/debug/gpio */
>  	debugfs_create_file("gpio", 0444, NULL, NULL, &gpiolib_fops);
> +
>  	return 0;
>  }
>  subsys_initcall(gpiolib_debugfs_init);

I believe now it's time to get __exitcall to be implemented. Actually the above
already needs that. Besides that, what you are adding doesn't belong to debugfs
AFAICS. And why it can't be done in gpiolib-swnode.c?

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

Fri, Mar 29, 2024 at 11:47:29AM +0000, Charles Keepax kirjoitti:
> 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.

...

>  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 (spi->use_fwnode_name && fwnode) {
> +		dev_set_name(&spi->dev, "spi-%s", fwnode_get_name(fwnode));
> +		return;
> +	}
>  
>  	if (adev) {
>  		dev_set_name(&spi->dev, "spi-%s", acpi_dev_name(adev));

This should be something like this

	struct device *dev = &spi->dev;
	struct fwnode_handle *fwnode = dev_fwnode(dev);

	if (is_acpi_device_node(fwnode)) {
		dev_set_name(dev, "spi-%s", acpi_dev_name(to_acpi_device_node(fwnode)));
		return;
	}

	if (is_software_node(fwnode)) {
		dev_set_name(dev, "spi-%s", fwnode_get_name(fwnode));
		return;
	}

i.o.w. we don't need to have two ways of checking fwnode type and you may get
rid of unneeded variable, and always use fwnode name for swnode.

...

> +	proxy->use_fwnode_name = chip->use_fwnode_name;

Unneeded variable. See above.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

Fri, Mar 29, 2024 at 11:47:30AM +0000, Charles Keepax kirjoitti:
> 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 <dt-bindings/gpio/gpio.h>

Hmm... Is it requirement by gpiolib-swnode? (I haven't looked at the use cases,
I'm not the author of this idea, hence the Q).

> +#include <linux/acpi.h>

You need array_size.h (and perhaps overflow.h) and property.h.

...

> +static struct spi_board_info ampl_info = {
> +	.modalias		= "cs35l56",
> +	.max_speed_hz		= 2000000,

Maybe HZ_PER_MHZ from units.h?

> +	.chip_select		= 0,
> +	.mode			= SPI_MODE_0,
> +	.swnode			= &ampl,
> +	.use_fwnode_name	= true,
> +};
> +
> +static struct spi_board_info ampr_info = {
> +	.modalias		= "cs35l56",
> +	.max_speed_hz		= 2000000,

Ditto.

> +	.chip_select		= 1,
> +	.mode			= SPI_MODE_0,
> +	.swnode			= &ampr,
> +	.use_fwnode_name	= true,
> +};

...

> +static const struct software_node_ref_args cs42l43_cs_refs[] = {

Please, use SOFTWARE_NODE_REFERENCE().

> +	{
> +		.node		= &cs42l43_gpiochip_swnode,
> +		.nargs		= 2,
> +		.args		= { 0, GPIO_ACTIVE_LOW },
> +	},
> +	{
> +		.node		= &swnode_gpio_undefined,
> +		.nargs		= 0,
> +	},
> +};
> +
> +static const struct property_entry cs42l43_cs_props[] = {
> +	{
> +		.name		= "cs-gpios",
> +		.length		= ARRAY_SIZE(cs42l43_cs_refs) *
> +				  sizeof(struct software_node_ref_args),
> +		.type		= DEV_PROP_REF,
> +		.pointer	= cs42l43_cs_refs,
> +	},

You want PROPERTY_ENTRY_REF_ARRAY().. 

> +	{}
> +};

...

> +#if IS_ENABLED(CONFIG_ACPI)

No need (see below)?

> +static bool cs42l43_has_sidecar(struct fwnode_handle *fwnode)
> +{
> +	static const int func_smart_amp = 0x1;
> +	struct fwnode_handle *child_fwnode, *ext_fwnode;
> +	unsigned long long function;
> +	unsigned int val;
> +	int ret;

> +	if (!is_acpi_node(fwnode))
> +		return false;

Dup, your loop will perform the same effectivelly.

> +	fwnode_for_each_child_node(fwnode, child_fwnode) {

> +		struct acpi_device *adev = to_acpi_device_node(child_fwnode);
> +
> +		ret = acpi_evaluate_integer(adev->handle, "_ADR", NULL, &function);
> +		if (ACPI_FAILURE(ret) || function != func_smart_amp) {
> +			fwnode_handle_put(fwnode);
> +			continue;
> +		}

acpi_get_local_address() (it has a stub for CONFIG_ACPI=n).

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

> +			fwnode_handle_put(fwnode);

Are you sure?

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

> +		fwnode_handle_put(fwnode);

Are you sure?

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

...

> -	device_set_node(&priv->ctlr->dev, fwnode);
> +	if (has_sidecar) {
> +		ret = software_node_register(&cs42l43_gpiochip_swnode);
> +		if (ret) {
> +			dev_err(priv->dev, "Failed to register gpio swnode: %d\n", ret);
> +			return ret;
> +		}

> +		ret = device_create_managed_software_node(&priv->ctlr->dev, cs42l43_cs_props, NULL);

No, this must not be used (I'm talking about _managed variant), this is a hack
for backward compatibility.

> +		if (ret) {
> +			dev_err(priv->dev, "Failed to add swnode: %d\n", ret);
> +			goto err;
> +		}

> +

Redundant blank line.

> +	} 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);
> +		goto err;
> +	}
> +
> +	if (has_sidecar) {
> +		if (!spi_new_device(priv->ctlr, &ampl_info)) {
> +			ret = -ENODEV;
> +			dev_err(priv->dev, "Failed to create left amp slave\n");
> +			goto err;
> +		}
> +
> +		if (!spi_new_device(priv->ctlr, &ampr_info)) {
> +			ret = -ENODEV;
> +			dev_err(priv->dev, "Failed to create right amp slave\n");
> +			goto err;
> +		}
>  	}
>  
> +	return 0;
> +
> +err:
> +	if (has_sidecar)
> +		software_node_unregister(&cs42l43_gpiochip_swnode);
> +
>  	return ret;
>  }

Wondering why don't you use return dev_err_probe() / ret = dev_err_probe() /
dev_err_probe(ret)?

> +static int cs42l43_spi_remove(struct platform_device *pdev)
> +{
> +	struct cs42l43 *cs42l43 = dev_get_drvdata(pdev->dev.parent);

platform_get_drvdata()

> +	struct fwnode_handle *fwnode = dev_fwnode(cs42l43->dev);

Is this dev the same as &pdev->dev?

> +	bool has_sidecar = cs42l43_has_sidecar(fwnode);
> +
> +	if (has_sidecar)
> +		software_node_unregister(&cs42l43_gpiochip_swnode);
> +
> +	return 0;
> +};

-- 
With Best Regards,
Andy Shevchenko

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

[Charles Keepax]

On Wed, Apr 03, 2024 at 11:29:27PM +0300, Andy Shevchenko wrote:
> Fri, Mar 29, 2024 at 11:47:29AM +0000, Charles Keepax kirjoitti:
> >  	struct acpi_device *adev = ACPI_COMPANION(&spi->dev);
> > +	struct fwnode_handle *fwnode = dev_fwnode(&spi->dev);
> > +
> > +	if (spi->use_fwnode_name && fwnode) {
> > +		dev_set_name(&spi->dev, "spi-%s", fwnode_get_name(fwnode));
> > +		return;
> > +	}
> >  
> >  	if (adev) {
> >  		dev_set_name(&spi->dev, "spi-%s", acpi_dev_name(adev));
> 
> This should be something like this
> 
> 	struct device *dev = &spi->dev;
> 	struct fwnode_handle *fwnode = dev_fwnode(dev);
> 
> 	if (is_acpi_device_node(fwnode)) {
> 		dev_set_name(dev, "spi-%s", acpi_dev_name(to_acpi_device_node(fwnode)));
> 		return;
> 	}
> 
> 	if (is_software_node(fwnode)) {
> 		dev_set_name(dev, "spi-%s", fwnode_get_name(fwnode));
> 		return;
> 	}
> 
> i.o.w. we don't need to have two ways of checking fwnode type and you may get
> rid of unneeded variable, and always use fwnode name for swnode.
> 
> ...
> 
> > +	proxy->use_fwnode_name = chip->use_fwnode_name;
> 
> Unneeded variable. See above.
> 

Hmm... I guess I was viewing this feature more as something that
users would opt into. So other firmware mechanisms could use it
if required, and so most swnode based controllers would still get
caught by the standard naming at the bottom of the function.

From my perspective it will do what I need either way, so happy
to update it to always use this for software nodes if consensus
goes that way.

Thanks,
Charles

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

[Charles Keepax]

On Wed, Apr 03, 2024 at 11:47:36PM +0300, Andy Shevchenko wrote:
> Fri, Mar 29, 2024 at 11:47:30AM +0000, Charles Keepax kirjoitti:
> > From: Maciej Strozek <mstrozek@opensource.cirrus.com>
> > +#include <dt-bindings/gpio/gpio.h>
> 
> Hmm... Is it requirement by gpiolib-swnode? (I haven't looked at the use cases,
> I'm not the author of this idea, hence the Q).

It's required for the GPIO_ACTIVE_LOW used in the swnode stuff.

> > +#include <linux/acpi.h>
> 
> You need array_size.h (and perhaps overflow.h) and property.h.

Good spot will add those.

> > +static struct spi_board_info ampl_info = {
> > +	.modalias		= "cs35l56",
> > +	.max_speed_hz		= 2000000,
> 
> Maybe HZ_PER_MHZ from units.h?

Can do.

> > +static const struct software_node_ref_args cs42l43_cs_refs[] = {
> Please, use SOFTWARE_NODE_REFERENCE().

> > +static const struct property_entry cs42l43_cs_props[] = {
> You want PROPERTY_ENTRY_REF_ARRAY().. 

Can do.

> > +static bool cs42l43_has_sidecar(struct fwnode_handle *fwnode)
> > +{
> > +	static const int func_smart_amp = 0x1;
> > +	struct fwnode_handle *child_fwnode, *ext_fwnode;
> > +	unsigned long long function;
> > +	unsigned int val;
> > +	int ret;
> 
> > +	if (!is_acpi_node(fwnode))
> > +		return false;
> 
> Dup, your loop will perform the same effectivelly.

Are you sure? Won't adev end up being NULL and the adev->handle
will dereference it?

> > +	fwnode_for_each_child_node(fwnode, child_fwnode) {
> 
> > +		struct acpi_device *adev = to_acpi_device_node(child_fwnode);
> > +
> > +		ret = acpi_evaluate_integer(adev->handle, "_ADR", NULL, &function);
> > +		if (ACPI_FAILURE(ret) || function != func_smart_amp) {
> > +			fwnode_handle_put(fwnode);
> > +			continue;
> > +		}
> 
> acpi_get_local_address() (it has a stub for CONFIG_ACPI=n).

Thanks was looking for something like that not sure how I missed
that.

> > +		ext_fwnode = fwnode_get_named_child_node(child_fwnode,
> > +				"mipi-sdca-function-expansion-subproperties");
> > +		if (!ext_fwnode) {
> 
> > +			fwnode_handle_put(fwnode);
> 
> Are you sure?

oops, yeah those should all be child_fwnode.

> > +	if (has_sidecar) {
> > +		ret = software_node_register(&cs42l43_gpiochip_swnode);
> > +		if (ret) {
> > +			dev_err(priv->dev, "Failed to register gpio swnode: %d\n", ret);
> > +			return ret;
> > +		}
> 
> > +		ret = device_create_managed_software_node(&priv->ctlr->dev, cs42l43_cs_props, NULL);
> 
> No, this must not be used (I'm talking about _managed variant), this is a hack
> for backward compatibility.

Hm... odd, feels like the function could use a comment or something
to say don't use me. But we can go back to managing it manually
no problems.

> > +		if (ret) {
> > +			dev_err(priv->dev, "Failed to add swnode: %d\n", ret);
> > +			goto err;
> > +		}
> 
> > +
> 
> Redundant blank line.

yup.

> > +	} 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);
> > +		goto err;
> > +	}
> > +
> > +	if (has_sidecar) {
> > +		if (!spi_new_device(priv->ctlr, &ampl_info)) {
> > +			ret = -ENODEV;
> > +			dev_err(priv->dev, "Failed to create left amp slave\n");
> > +			goto err;
> > +		}
> > +
> > +		if (!spi_new_device(priv->ctlr, &ampr_info)) {
> > +			ret = -ENODEV;
> > +			dev_err(priv->dev, "Failed to create right amp slave\n");
> > +			goto err;
> > +		}
> >  	}
> >  
> > +	return 0;
> > +
> > +err:
> > +	if (has_sidecar)
> > +		software_node_unregister(&cs42l43_gpiochip_swnode);
> > +
> >  	return ret;
> >  }
> 
> Wondering why don't you use return dev_err_probe() / ret = dev_err_probe() /
> dev_err_probe(ret)?

Yeah some of those should be, will update.

> > +static int cs42l43_spi_remove(struct platform_device *pdev)
> > +{
> > +	struct cs42l43 *cs42l43 = dev_get_drvdata(pdev->dev.parent);
> 
> platform_get_drvdata()
> 
> > +	struct fwnode_handle *fwnode = dev_fwnode(cs42l43->dev);
> 
> Is this dev the same as &pdev->dev?

No, this is MFD parent device, to be fair could probably use
parent directly here. Will have a think about that.

Thanks,
Charles

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

[Andy Shevchenko]

On Mon, Apr 8, 2024 at 6:35 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
> On Wed, Apr 03, 2024 at 11:47:36PM +0300, Andy Shevchenko wrote:
> > Fri, Mar 29, 2024 at 11:47:30AM +0000, Charles Keepax kirjoitti:

...

> > > +#include <dt-bindings/gpio/gpio.h>
> >
> > Hmm... Is it requirement by gpiolib-swnode? (I haven't looked at the use cases,
> > I'm not the author of this idea, hence the Q).
>
> It's required for the GPIO_ACTIVE_LOW used in the swnode stuff.

Seems to me like you are mistaken.
https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib-swnode.c#L85

...

> > > +   if (!is_acpi_node(fwnode))
> > > +           return false;
> >
> > Dup, your loop will perform the same effectivelly.
>
> Are you sure? Won't adev end up being NULL and the adev->handle
> will dereference it?

Yes, you need to check the ACPI dev to be not NULL there. Also note,
that is_acpi_node() is not the same as is_acpi_device_node().

> > > +   fwnode_for_each_child_node(fwnode, child_fwnode) {
> >
> > > +           struct acpi_device *adev = to_acpi_device_node(child_fwnode);

...

> > > +           ret = device_create_managed_software_node(&priv->ctlr->dev, cs42l43_cs_props, NULL);
> >
> > No, this must not be used (I'm talking about _managed variant), this is a hack
> > for backward compatibility.
>
> Hm... odd, feels like the function could use a comment or something
> to say don't use me. But we can go back to managing it manually
> no problems.

Ah, I was thinking that it inherited that from device_add_property()
(see 2338e7bcef44 ("device property: Remove device_add_properties()
API") for the details), but no, it seems okay to use. but then we
really need to be careful about lifetime of it wrt. other parts in
this code.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Mon, Apr 8, 2024 at 4:52 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
> On Wed, Apr 03, 2024 at 11:29:27PM +0300, Andy Shevchenko wrote:
> > Fri, Mar 29, 2024 at 11:47:29AM +0000, Charles Keepax kirjoitti:

...

> > >     struct acpi_device *adev = ACPI_COMPANION(&spi->dev);
> > > +   struct fwnode_handle *fwnode = dev_fwnode(&spi->dev);
> > > +
> > > +   if (spi->use_fwnode_name && fwnode) {
> > > +           dev_set_name(&spi->dev, "spi-%s", fwnode_get_name(fwnode));
> > > +           return;
> > > +   }
> > >
> > >     if (adev) {
> > >             dev_set_name(&spi->dev, "spi-%s", acpi_dev_name(adev));
> >
> > This should be something like this
> >
> >       struct device *dev = &spi->dev;
> >       struct fwnode_handle *fwnode = dev_fwnode(dev);
> >
> >       if (is_acpi_device_node(fwnode)) {
> >               dev_set_name(dev, "spi-%s", acpi_dev_name(to_acpi_device_node(fwnode)));
> >               return;
> >       }
> >
> >       if (is_software_node(fwnode)) {
> >               dev_set_name(dev, "spi-%s", fwnode_get_name(fwnode));
> >               return;
> >       }
> >
> > i.o.w. we don't need to have two ways of checking fwnode type and you may get
> > rid of unneeded variable, and always use fwnode name for swnode.
> >
> > ...
> >
> > > +   proxy->use_fwnode_name = chip->use_fwnode_name;
> >
> > Unneeded variable. See above.
>
> Hmm... I guess I was viewing this feature more as something that
> users would opt into. So other firmware mechanisms could use it
> if required, and so most swnode based controllers would still get
> caught by the standard naming at the bottom of the function.

software nodes can be used as a trampoline for old board files (in
order to make the driver cleaner and prepared for DT conversion of the
board file in question) or for quirks. In either case when we use them
we really want to have their data to be propagated unconditionally
(just based on the type), the (per-subsystem) gate is a carefully
placed mine on the minefield.

> From my perspective it will do what I need either way, so happy
> to update it to always use this for software nodes if consensus
> goes that way.

Not a maintainer here, ask Mark.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Mon, Apr 08, 2024 at 07:07:55PM +0300, Andy Shevchenko wrote:
> On Mon, Apr 8, 2024 at 6:35 PM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
> > On Wed, Apr 03, 2024 at 11:47:36PM +0300, Andy Shevchenko wrote:
> > > Fri, Mar 29, 2024 at 11:47:30AM +0000, Charles Keepax kirjoitti:

...

> > > > +#include <dt-bindings/gpio/gpio.h>
> > >
> > > Hmm... Is it requirement by gpiolib-swnode? (I haven't looked at the use cases,
> > > I'm not the author of this idea, hence the Q).
> >
> > It's required for the GPIO_ACTIVE_LOW used in the swnode stuff.
> 
> Seems to me like you are mistaken.
> https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib-swnode.c#L85

Okay, I stand corrected, under "native" the GPIO_* are assumed.

But what you need is to include gpio/property.h for that, and not directly
the DT header.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Mon, Apr 08, 2024 at 10:50:28PM +0300, Andy Shevchenko wrote:
> On Mon, Apr 08, 2024 at 07:07:55PM +0300, Andy Shevchenko wrote:
> > On Mon, Apr 8, 2024 at 6:35 PM Charles Keepax
> > <ckeepax@opensource.cirrus.com> wrote:
> > > On Wed, Apr 03, 2024 at 11:47:36PM +0300, Andy Shevchenko wrote:
> > > > Fri, Mar 29, 2024 at 11:47:30AM +0000, Charles Keepax kirjoitti:

...

> > > > > +#include <dt-bindings/gpio/gpio.h>
> > > >
> > > > Hmm... Is it requirement by gpiolib-swnode? (I haven't looked at the use cases,
> > > > I'm not the author of this idea, hence the Q).
> > >
> > > It's required for the GPIO_ACTIVE_LOW used in the swnode stuff.
> > 
> > Seems to me like you are mistaken.
> > https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib-swnode.c#L85
> 
> Okay, I stand corrected, under "native" the GPIO_* are assumed.
> 
> But what you need is to include gpio/property.h for that, and not directly
> the DT header.

Oh, my, we have two _almost_ identical definitions in machine.h and under DT.
So, I believe we are using ones from machine.h.

-- 
With Best Regards,
Andy Shevchenko

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

[Charles Keepax]

On Mon, Apr 08, 2024 at 11:07:43PM +0300, Andy Shevchenko wrote:
> On Mon, Apr 08, 2024 at 10:50:28PM +0300, Andy Shevchenko wrote:
> > On Mon, Apr 08, 2024 at 07:07:55PM +0300, Andy Shevchenko wrote:
> > > On Mon, Apr 8, 2024 at 6:35 PM Charles Keepax
> > > <ckeepax@opensource.cirrus.com> wrote:
> > > > On Wed, Apr 03, 2024 at 11:47:36PM +0300, Andy Shevchenko wrote:
> > > > > Fri, Mar 29, 2024 at 11:47:30AM +0000, Charles Keepax kirjoitti:
> 
> ...
> 
> > > > > > +#include <dt-bindings/gpio/gpio.h>
> > > > >
> > > > > Hmm... Is it requirement by gpiolib-swnode? (I haven't looked at the use cases,
> > > > > I'm not the author of this idea, hence the Q).
> > > >
> > > > It's required for the GPIO_ACTIVE_LOW used in the swnode stuff.
> > > 
> > > Seems to me like you are mistaken.
> > > https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib-swnode.c#L85
> > 
> > Okay, I stand corrected, under "native" the GPIO_* are assumed.
> > 
> > But what you need is to include gpio/property.h for that, and not directly
> > the DT header.
> 
> Oh, my, we have two _almost_ identical definitions in machine.h and under DT.
> So, I believe we are using ones from machine.h.
> 

Yeah that is my bad I should be using those instead.

Thanks,
Charles