* [PATCH 0/4] Add bridged amplifiers to cs42l43
@ 2024-04-11 9:06 Charles Keepax
2024-04-11 9:06 ` [PATCH v5 1/4] gpio: swnode: Add ability to specify native chip selects for SPI Charles Keepax
` (4 more replies)
0 siblings, 5 replies; 23+ messages in thread
From: Charles Keepax @ 2024-04-11 9:06 UTC (permalink / raw
To: broonie, linus.walleij, brgl
Cc: andy.shevchenko, bard.liao, linux-gpio, linux-spi, patches
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 v4:
- Remove extraneous fwnode_handle_puts in driver/spi/spi-cs42l43.c
- Make Kconfig for swnode undef gpios not user visible
- Add some missing headers
- Add patch to update handling in spi_dev_set_name
- Remove stray blank line
- Use ACPI_HANDLE_FWNODE
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
Charles Keepax (3):
gpio: swnode: Add ability to specify native chip selects for SPI
spi: Switch to using is_acpi_device_node() in spi_dev_set_name()
spi: Update swnode based SPI devices to use the fwnode name
Maciej Strozek (1):
spi: cs42l43: Add bridged cs35l56 amplifiers
drivers/gpio/Kconfig | 3 +
drivers/gpio/gpiolib-swnode.c | 40 ++++++++++
drivers/spi/Kconfig | 1 +
drivers/spi/spi-cs42l43.c | 135 +++++++++++++++++++++++++++++++++-
drivers/spi/spi.c | 13 +++-
include/linux/gpio/consumer.h | 4 +
6 files changed, 191 insertions(+), 5 deletions(-)
--
2.39.2
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v5 1/4] gpio: swnode: Add ability to specify native chip selects for SPI
2024-04-11 9:06 [PATCH 0/4] Add bridged amplifiers to cs42l43 Charles Keepax
@ 2024-04-11 9:06 ` Charles Keepax
2024-04-11 13:25 ` Andy Shevchenko
2024-04-11 9:06 ` [PATCH v5 2/4] spi: Switch to using is_acpi_device_node() in spi_dev_set_name() Charles Keepax
` (3 subsequent siblings)
4 siblings, 1 reply; 23+ messages in thread
From: Charles Keepax @ 2024-04-11 9:06 UTC (permalink / raw
To: broonie, linus.walleij, brgl
Cc: andy.shevchenko, bard.liao, linux-gpio, linux-spi, patches
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.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
Changes since v4:
- Make GPIO_SWNODE_UNDEFINED not user visible
- Minor fixes to commit message
- Add some missing headers
Thanks,
Charles
drivers/gpio/Kconfig | 3 +++
drivers/gpio/gpiolib-swnode.c | 40 +++++++++++++++++++++++++++++++++++
include/linux/gpio/consumer.h | 4 ++++
3 files changed, 47 insertions(+)
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index b50d0b470849..c44a6b57aefa 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -103,6 +103,9 @@ config GPIO_REGMAP
select REGMAP
tristate
+config GPIO_SWNODE_UNDEFINED
+ bool
+
# 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..add5f8962e8d 100644
--- a/drivers/gpio/gpiolib-swnode.c
+++ b/drivers/gpio/gpiolib-swnode.c
@@ -6,6 +6,8 @@
*/
#include <linux/err.h>
#include <linux/errno.h>
+#include <linux/export.h>
+#include <linux/init.h>
#include <linux/kernel.h>
#include <linux/printk.h>
#include <linux/property.h>
@@ -17,6 +19,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 +44,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 +132,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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v5 2/4] spi: Switch to using is_acpi_device_node() in spi_dev_set_name()
2024-04-11 9:06 [PATCH 0/4] Add bridged amplifiers to cs42l43 Charles Keepax
2024-04-11 9:06 ` [PATCH v5 1/4] gpio: swnode: Add ability to specify native chip selects for SPI Charles Keepax
@ 2024-04-11 9:06 ` Charles Keepax
2024-04-11 13:30 ` Andy Shevchenko
2024-04-11 9:06 ` [PATCH v5 3/4] spi: Update swnode based SPI devices to use the fwnode name Charles Keepax
` (2 subsequent siblings)
4 siblings, 1 reply; 23+ messages in thread
From: Charles Keepax @ 2024-04-11 9:06 UTC (permalink / raw
To: broonie, linus.walleij, brgl
Cc: andy.shevchenko, bard.liao, linux-gpio, linux-spi, patches
Use the more modern is_acpi_device_node() rather than checking
ACPI_COMPANION().
Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
New since v4 of the series.
Thanks,
Charles
drivers/spi/spi.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index a2f01116ba09..05b33901eaa9 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -12,6 +12,7 @@
#include <linux/dmaengine.h>
#include <linux/dma-mapping.h>
#include <linux/export.h>
+#include <linux/fwnode.h>
#include <linux/gpio/consumer.h>
#include <linux/highmem.h>
#include <linux/idr.h>
@@ -597,10 +598,11 @@ 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 device *dev = &spi->dev;
+ struct fwnode_handle *fwnode = dev_fwnode(dev);
- if (adev) {
- dev_set_name(&spi->dev, "spi-%s", acpi_dev_name(adev));
+ if (is_acpi_device_node(fwnode)) {
+ dev_set_name(dev, "spi-%s", acpi_dev_name(to_acpi_device_node(fwnode)));
return;
}
--
2.39.2
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v5 3/4] spi: Update swnode based SPI devices to use the fwnode name
2024-04-11 9:06 [PATCH 0/4] Add bridged amplifiers to cs42l43 Charles Keepax
2024-04-11 9:06 ` [PATCH v5 1/4] gpio: swnode: Add ability to specify native chip selects for SPI Charles Keepax
2024-04-11 9:06 ` [PATCH v5 2/4] spi: Switch to using is_acpi_device_node() in spi_dev_set_name() Charles Keepax
@ 2024-04-11 9:06 ` Charles Keepax
2024-04-11 13:33 ` Andy Shevchenko
2024-04-11 9:06 ` [PATCH v5 4/4] spi: cs42l43: Add bridged cs35l56 amplifiers Charles Keepax
2024-04-11 11:46 ` [PATCH 0/4] Add bridged amplifiers to cs42l43 Andy Shevchenko
4 siblings, 1 reply; 23+ messages in thread
From: Charles Keepax @ 2024-04-11 9:06 UTC (permalink / raw
To: broonie, linus.walleij, brgl
Cc: andy.shevchenko, bard.liao, linux-gpio, linux-spi, patches
Update the name for software node based SPI devices to use the fwnode
name as the device name. This is helpful since swnode devices are
usually added within the kernel, and the kernel often then requires a
predictable name such that it can refer back to the device.
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
No changes since v4.
Thanks,
Charles
drivers/spi/spi.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 05b33901eaa9..bcc9d3ab7cd9 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -606,6 +606,11 @@ static void spi_dev_set_name(struct spi_device *spi)
return;
}
+ if (is_software_node(fwnode)) {
+ dev_set_name(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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v5 4/4] spi: cs42l43: Add bridged cs35l56 amplifiers
2024-04-11 9:06 [PATCH 0/4] Add bridged amplifiers to cs42l43 Charles Keepax
` (2 preceding siblings ...)
2024-04-11 9:06 ` [PATCH v5 3/4] spi: Update swnode based SPI devices to use the fwnode name Charles Keepax
@ 2024-04-11 9:06 ` Charles Keepax
2024-04-11 14:04 ` Andy Shevchenko
2024-04-11 11:46 ` [PATCH 0/4] Add bridged amplifiers to cs42l43 Andy Shevchenko
4 siblings, 1 reply; 23+ messages in thread
From: Charles Keepax @ 2024-04-11 9:06 UTC (permalink / raw
To: broonie, linus.walleij, brgl
Cc: andy.shevchenko, bard.liao, linux-gpio, linux-spi, patches
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(-)
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 = &l,
+};
+
+static struct spi_board_info ampr_info = {
+ .modalias = "cs35l56",
+ .max_speed_hz = 20 * HZ_PER_MHZ,
+ .chip_select = 1,
+ .mode = SPI_MODE_0,
+ .swnode = &r,
+};
+
+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, &l_info)) {
+ ret = dev_err_probe(priv->dev, -ENODEV,
+ "Failed to create left amp slave\n");
+ goto err;
+ }
+
+ if (!spi_new_device(priv->ctlr, &r_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>");
--
2.39.2
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 0/4] Add bridged amplifiers to cs42l43
2024-04-11 9:06 [PATCH 0/4] Add bridged amplifiers to cs42l43 Charles Keepax
` (3 preceding siblings ...)
2024-04-11 9:06 ` [PATCH v5 4/4] spi: cs42l43: Add bridged cs35l56 amplifiers Charles Keepax
@ 2024-04-11 11:46 ` Andy Shevchenko
4 siblings, 0 replies; 23+ messages in thread
From: Andy Shevchenko @ 2024-04-11 11:46 UTC (permalink / raw
To: Charles Keepax
Cc: broonie, linus.walleij, brgl, bard.liao, linux-gpio, linux-spi,
patches
On Thu, Apr 11, 2024 at 12:06 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
>
> 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.
Quick note: I dunno how you prepared your series, but the cover letter
missed the versioning. `git format-patch -v<N> ...` does it correctly,
where <X> is the version number. You may also see how I do with a
script [1].
[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 1/4] gpio: swnode: Add ability to specify native chip selects for SPI
2024-04-11 9:06 ` [PATCH v5 1/4] gpio: swnode: Add ability to specify native chip selects for SPI Charles Keepax
@ 2024-04-11 13:25 ` Andy Shevchenko
2024-04-11 16:44 ` Charles Keepax
0 siblings, 1 reply; 23+ messages in thread
From: Andy Shevchenko @ 2024-04-11 13:25 UTC (permalink / raw
To: Charles Keepax
Cc: broonie, linus.walleij, brgl, bard.liao, linux-gpio, linux-spi,
patches
On Thu, Apr 11, 2024 at 12:06 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
Here, the
> 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.
...
> +config GPIO_SWNODE_UNDEFINED
> + bool
But why did you remove the help? Please, put it back.
...
> + /*
> + * Check for special node that identifies undefined GPIOs, this is
for a special
> + * 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);
This is a dead code when the respective config option is not selected.
Or actually a potential flaw if somebody else names their swnode like
this.
...
> + ret = software_node_register(&swnode_gpio_undefined);
> + if (ret < 0)
> + pr_err("gpiolib: failed to register swnode: %d\n", ret);
Instead of this prefix, define pr_fmt()
> + return ret;
...
> --- a/include/linux/gpio/consumer.h
> +++ b/include/linux/gpio/consumer.h
Why? I already said that the best place is gpio/property.h as it's
exactly for swnode related code.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 2/4] spi: Switch to using is_acpi_device_node() in spi_dev_set_name()
2024-04-11 9:06 ` [PATCH v5 2/4] spi: Switch to using is_acpi_device_node() in spi_dev_set_name() Charles Keepax
@ 2024-04-11 13:30 ` Andy Shevchenko
2024-04-11 16:56 ` Charles Keepax
0 siblings, 1 reply; 23+ messages in thread
From: Andy Shevchenko @ 2024-04-11 13:30 UTC (permalink / raw
To: Charles Keepax
Cc: broonie, linus.walleij, brgl, bard.liao, linux-gpio, linux-spi,
patches
On Thu, Apr 11, 2024 at 12:06 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
>
> Use the more modern is_acpi_device_node() rather than checking
> ACPI_COMPANION().
I don't think it's valuable on its own. There is no clear motivation
why to do that, I suggested it exactly in the conjunction of not
introducing two ways of fwnode type check. That said, you probably
want to elaborate the motivation in the commit message if you want to
keep it separate.
...
> +#include <linux/fwnode.h>
This header is not supposed to be included by the end users. property.h is.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 3/4] spi: Update swnode based SPI devices to use the fwnode name
2024-04-11 9:06 ` [PATCH v5 3/4] spi: Update swnode based SPI devices to use the fwnode name Charles Keepax
@ 2024-04-11 13:33 ` Andy Shevchenko
2024-04-11 17:04 ` Charles Keepax
0 siblings, 1 reply; 23+ messages in thread
From: Andy Shevchenko @ 2024-04-11 13:33 UTC (permalink / raw
To: Charles Keepax
Cc: broonie, linus.walleij, brgl, bard.liao, linux-gpio, linux-spi,
patches
On Thu, Apr 11, 2024 at 12:06 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
>
> Update the name for software node based SPI devices to use the fwnode
> name as the device name. This is helpful since swnode devices are
> usually added within the kernel, and the kernel often then requires a
> predictable name such that it can refer back to the device.
...
> + if (is_software_node(fwnode)) {
> + dev_set_name(dev, "spi-%s", fwnode_get_name(fwnode));
Wouldn't %pfwP / %pfw work?
Thinking more about this, maybe even the ACPI case also can be combined?
See for the details
87526603c892 ("irqdomain: Get rid of special treatment for ACPI in
__irq_domain_add()")
9ed78b05f998 ("irqdomain: Allow software nodes for IRQ domain creation")
> + return;
> + }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 4/4] spi: cs42l43: Add bridged cs35l56 amplifiers
2024-04-11 9:06 ` [PATCH v5 4/4] spi: cs42l43: Add bridged cs35l56 amplifiers Charles Keepax
@ 2024-04-11 14:04 ` Andy Shevchenko
2024-04-11 14:06 ` Andy Shevchenko
2024-04-11 17:13 ` Charles Keepax
0 siblings, 2 replies; 23+ messages in thread
From: Andy Shevchenko @ 2024-04-11 14:04 UTC (permalink / raw
To: Charles Keepax
Cc: broonie, linus.walleij, brgl, bard.liao, linux-gpio, linux-spi,
patches
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?
> + }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 4/4] spi: cs42l43: Add bridged cs35l56 amplifiers
2024-04-11 14:04 ` Andy Shevchenko
@ 2024-04-11 14:06 ` Andy Shevchenko
2024-04-11 14:07 ` Andy Shevchenko
2024-04-11 17:13 ` Charles Keepax
1 sibling, 1 reply; 23+ messages in thread
From: Andy Shevchenko @ 2024-04-11 14:06 UTC (permalink / raw
To: Charles Keepax
Cc: broonie, linus.walleij, brgl, bard.liao, linux-gpio, linux-spi,
patches
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;
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 4/4] spi: cs42l43: Add bridged cs35l56 amplifiers
2024-04-11 14:06 ` Andy Shevchenko
@ 2024-04-11 14:07 ` Andy Shevchenko
0 siblings, 0 replies; 23+ messages in thread
From: Andy Shevchenko @ 2024-04-11 14:07 UTC (permalink / raw
To: Charles Keepax
Cc: broonie, linus.walleij, brgl, bard.liao, linux-gpio, linux-spi,
patches
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 ...;
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 1/4] gpio: swnode: Add ability to specify native chip selects for SPI
2024-04-11 13:25 ` Andy Shevchenko
@ 2024-04-11 16:44 ` Charles Keepax
2024-04-11 16:50 ` Andy Shevchenko
0 siblings, 1 reply; 23+ messages in thread
From: Charles Keepax @ 2024-04-11 16:44 UTC (permalink / raw
To: Andy Shevchenko
Cc: broonie, linus.walleij, brgl, bard.liao, linux-gpio, linux-spi,
patches
On Thu, Apr 11, 2024 at 04:25:25PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 11, 2024 at 12:06 PM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
> > Here the second chip select is native. However, when using swnodes
>
> Here, the
>
Can do.
> > +config GPIO_SWNODE_UNDEFINED
> > + bool
>
> But why did you remove the help? Please, put it back.
>
Sorry didn't realise you still wanted it will pop it back.
> ...
>
> > + /*
> > + * Check for special node that identifies undefined GPIOs, this is
>
> for a special
Can do.
>
> > + * 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);
>
> This is a dead code when the respective config option is not selected.
> Or actually a potential flaw if somebody else names their swnode like
> this.
>
Can add a check for the config.
> ...
>
> > + ret = software_node_register(&swnode_gpio_undefined);
> > + if (ret < 0)
> > + pr_err("gpiolib: failed to register swnode: %d\n", ret);
>
> Instead of this prefix, define pr_fmt()
>
Little iffy on this, there are other prints in gpiolib that do it
this way as well, I guess I could add a patch to convert
everything but its starting to get a little out of the thrust of
what I am doing here.
> > + return ret;
>
> ...
>
> > --- a/include/linux/gpio/consumer.h
> > +++ b/include/linux/gpio/consumer.h
>
> Why? I already said that the best place is gpio/property.h as it's
> exactly for swnode related code.
>
Oh I see that's what you meant by those comments, as per my email
I wasn't exactly sure. Will move them I don't mind where they go.
Thanks,
Charles
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 1/4] gpio: swnode: Add ability to specify native chip selects for SPI
2024-04-11 16:44 ` Charles Keepax
@ 2024-04-11 16:50 ` Andy Shevchenko
2024-04-11 16:58 ` Charles Keepax
0 siblings, 1 reply; 23+ messages in thread
From: Andy Shevchenko @ 2024-04-11 16:50 UTC (permalink / raw
To: Charles Keepax
Cc: broonie, linus.walleij, brgl, bard.liao, linux-gpio, linux-spi,
patches
On Thu, Apr 11, 2024 at 7:44 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
> On Thu, Apr 11, 2024 at 04:25:25PM +0300, Andy Shevchenko wrote:
> > On Thu, Apr 11, 2024 at 12:06 PM Charles Keepax
> > <ckeepax@opensource.cirrus.com> wrote:
...
> > > +config GPIO_SWNODE_UNDEFINED
> > > + bool
> >
> > But why did you remove the help? Please, put it back.
>
> Sorry didn't realise you still wanted it will pop it back.
No, I don't want it to be user-selectable.
This is defined by a non-empty summary near to the type of the option
(bool, tristate). The help text is orthogonal to this.
...
> > > + if (!strcmp(gdev_node->name, GPIOLIB_SWNODE_UNDEFINED_NAME))
> > > + return ERR_PTR(-ENOENT);
> >
> > This is a dead code when the respective config option is not selected.
> > Or actually a potential flaw if somebody else names their swnode like
> > this.
>
> Can add a check for the config.
Maybe something like
if (IS_ENABLED(...) &&
!strcmp(...))
...
> > > + ret = software_node_register(&swnode_gpio_undefined);
> > > + if (ret < 0)
> > > + pr_err("gpiolib: failed to register swnode: %d\n", ret);
> >
> > Instead of this prefix, define pr_fmt()
>
> Little iffy on this, there are other prints in gpiolib that do it
> this way as well, I guess I could add a patch to convert
> everything but its starting to get a little out of the thrust of
> what I am doing here.
That's why I'm talking only about this (gpiolib-swnode) module where
you can have it as
"gpiolib: swnode: " fmt
or alike
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 2/4] spi: Switch to using is_acpi_device_node() in spi_dev_set_name()
2024-04-11 13:30 ` Andy Shevchenko
@ 2024-04-11 16:56 ` Charles Keepax
2024-04-11 18:09 ` Andy Shevchenko
0 siblings, 1 reply; 23+ messages in thread
From: Charles Keepax @ 2024-04-11 16:56 UTC (permalink / raw
To: Andy Shevchenko
Cc: broonie, linus.walleij, brgl, bard.liao, linux-gpio, linux-spi,
patches
On Thu, Apr 11, 2024 at 04:30:13PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 11, 2024 at 12:06 PM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
> >
> > Use the more modern is_acpi_device_node() rather than checking
> > ACPI_COMPANION().
>
> I don't think it's valuable on its own. There is no clear motivation
> why to do that, I suggested it exactly in the conjunction of not
> introducing two ways of fwnode type check. That said, you probably
> want to elaborate the motivation in the commit message if you want to
> keep it separate.
>
I am really tempted to just drop this, its not necessary for my
changes and changes something that is unrelated to them. At the
least it belongs in a separate patch.
> ...
>
> > +#include <linux/fwnode.h>
>
> This header is not supposed to be included by the end users. property.h is.
>
Fair enough will update, although I really feel these headers
could use some annotation if they are not supposed to be directly
included. Either include everything you use or just include a top
level header makes sense but this weird mixture we seem to use is
very confusing and I don't have a big enough brain to remember
every header.
Thanks,
Charles
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 1/4] gpio: swnode: Add ability to specify native chip selects for SPI
2024-04-11 16:50 ` Andy Shevchenko
@ 2024-04-11 16:58 ` Charles Keepax
0 siblings, 0 replies; 23+ messages in thread
From: Charles Keepax @ 2024-04-11 16:58 UTC (permalink / raw
To: Andy Shevchenko
Cc: broonie, linus.walleij, brgl, bard.liao, linux-gpio, linux-spi,
patches
On Thu, Apr 11, 2024 at 07:50:46PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 11, 2024 at 7:44 PM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
> > On Thu, Apr 11, 2024 at 04:25:25PM +0300, Andy Shevchenko wrote:
> > > On Thu, Apr 11, 2024 at 12:06 PM Charles Keepax
> > > <ckeepax@opensource.cirrus.com> wrote:
>
> ...
>
> > > > +config GPIO_SWNODE_UNDEFINED
> > > > + bool
> > >
> > > But why did you remove the help? Please, put it back.
> >
> > Sorry didn't realise you still wanted it will pop it back.
>
> No, I don't want it to be user-selectable.
>
Yeah I get that just didn't realise you also wanted the help,
they are technically orthogonal but most non-user selectable
things don't define a help.
> Maybe something like
>
> if (IS_ENABLED(...) &&
> !strcmp(...))
>
Aye that is what I just added.
> > > > + ret = software_node_register(&swnode_gpio_undefined);
> > > > + if (ret < 0)
> > > > + pr_err("gpiolib: failed to register swnode: %d\n", ret);
> > >
> > > Instead of this prefix, define pr_fmt()
> >
> > Little iffy on this, there are other prints in gpiolib that do it
> > this way as well, I guess I could add a patch to convert
> > everything but its starting to get a little out of the thrust of
> > what I am doing here.
>
> That's why I'm talking only about this (gpiolib-swnode) module where
> you can have it as
>
> "gpiolib: swnode: " fmt
>
> or alike
>
Alright will add this too.
Thanks,
Charles
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 3/4] spi: Update swnode based SPI devices to use the fwnode name
2024-04-11 13:33 ` Andy Shevchenko
@ 2024-04-11 17:04 ` Charles Keepax
2024-04-11 18:07 ` Andy Shevchenko
0 siblings, 1 reply; 23+ messages in thread
From: Charles Keepax @ 2024-04-11 17:04 UTC (permalink / raw
To: Andy Shevchenko
Cc: broonie, linus.walleij, brgl, bard.liao, linux-gpio, linux-spi,
patches
On Thu, Apr 11, 2024 at 04:33:05PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 11, 2024 at 12:06 PM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
> >
> > Update the name for software node based SPI devices to use the fwnode
> > name as the device name. This is helpful since swnode devices are
> > usually added within the kernel, and the kernel often then requires a
> > predictable name such that it can refer back to the device.
>
> ...
>
> > + if (is_software_node(fwnode)) {
> > + dev_set_name(dev, "spi-%s", fwnode_get_name(fwnode));
>
> Wouldn't %pfwP / %pfw work?
>
Oh I was totally unaware of those will have a look, thanks.
> Thinking more about this, maybe even the ACPI case also can be combined?
> See for the details
>
> 87526603c892 ("irqdomain: Get rid of special treatment for ACPI in
> __irq_domain_add()")
> 9ed78b05f998 ("irqdomain: Allow software nodes for IRQ domain creation")
>
Maybe, I am not super confident that acpi_dev_name will always
return the same thing as fwnode_get_name and I don't really want
to risk renaming everything on the ACPI side.
Thanks,
Charles
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 4/4] spi: cs42l43: Add bridged cs35l56 amplifiers
2024-04-11 14:04 ` Andy Shevchenko
2024-04-11 14:06 ` Andy Shevchenko
@ 2024-04-11 17:13 ` Charles Keepax
2024-04-11 18:17 ` Andy Shevchenko
1 sibling, 1 reply; 23+ messages in thread
From: Charles Keepax @ 2024-04-11 17:13 UTC (permalink / raw
To: Andy Shevchenko
Cc: broonie, linus.walleij, brgl, bard.liao, linux-gpio, linux-spi,
patches
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 3/4] spi: Update swnode based SPI devices to use the fwnode name
2024-04-11 17:04 ` Charles Keepax
@ 2024-04-11 18:07 ` Andy Shevchenko
0 siblings, 0 replies; 23+ messages in thread
From: Andy Shevchenko @ 2024-04-11 18:07 UTC (permalink / raw
To: Charles Keepax
Cc: broonie, linus.walleij, brgl, bard.liao, linux-gpio, linux-spi,
patches
On Thu, Apr 11, 2024 at 8:04 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
> On Thu, Apr 11, 2024 at 04:33:05PM +0300, Andy Shevchenko wrote:
> > On Thu, Apr 11, 2024 at 12:06 PM Charles Keepax
> > <ckeepax@opensource.cirrus.com> wrote:
...
> > Thinking more about this, maybe even the ACPI case also can be combined?
> > See for the details
> >
> > 87526603c892 ("irqdomain: Get rid of special treatment for ACPI in
> > __irq_domain_add()")
> > 9ed78b05f998 ("irqdomain: Allow software nodes for IRQ domain creation")
>
> Maybe, I am not super confident that acpi_dev_name will always
> return the same thing as fwnode_get_name and I don't really want
> to risk renaming everything on the ACPI side.
Scratch this. They are completely different.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 2/4] spi: Switch to using is_acpi_device_node() in spi_dev_set_name()
2024-04-11 16:56 ` Charles Keepax
@ 2024-04-11 18:09 ` Andy Shevchenko
0 siblings, 0 replies; 23+ messages in thread
From: Andy Shevchenko @ 2024-04-11 18:09 UTC (permalink / raw
To: Charles Keepax
Cc: broonie, linus.walleij, brgl, bard.liao, linux-gpio, linux-spi,
patches
On Thu, Apr 11, 2024 at 7:56 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
> On Thu, Apr 11, 2024 at 04:30:13PM +0300, Andy Shevchenko wrote:
> > On Thu, Apr 11, 2024 at 12:06 PM Charles Keepax
> > <ckeepax@opensource.cirrus.com> wrote:
> > >
> > > Use the more modern is_acpi_device_node() rather than checking
> > > ACPI_COMPANION().
> >
> > I don't think it's valuable on its own. There is no clear motivation
> > why to do that, I suggested it exactly in the conjunction of not
> > introducing two ways of fwnode type check. That said, you probably
> > want to elaborate the motivation in the commit message if you want to
> > keep it separate.
>
> I am really tempted to just drop this, its not necessary for my
> changes and changes something that is unrelated to them. At the
> least it belongs in a separate patch.
Okay, just elaborate in the commit message that this is done to make
new comer not to invent its own way for fwnode type check,
...
> > > +#include <linux/fwnode.h>
> >
> > This header is not supposed to be included by the end users. property.h is.
>
> Fair enough will update, although I really feel these headers
> could use some annotation if they are not supposed to be directly
> included. Either include everything you use or just include a top
> level header makes sense but this weird mixture we seem to use is
> very confusing and I don't have a big enough brain to remember
> every header.
Rough hint is to look into the contents. But I agree, that is a complete mess.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 4/4] spi: cs42l43: Add bridged cs35l56 amplifiers
2024-04-11 17:13 ` Charles Keepax
@ 2024-04-11 18:17 ` Andy Shevchenko
2024-04-15 13:39 ` Charles Keepax
0 siblings, 1 reply; 23+ messages in thread
From: Andy Shevchenko @ 2024-04-11 18:17 UTC (permalink / raw
To: Charles Keepax
Cc: broonie, linus.walleij, brgl, bard.liao, linux-gpio, linux-spi,
patches
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.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 4/4] spi: cs42l43: Add bridged cs35l56 amplifiers
2024-04-11 18:17 ` Andy Shevchenko
@ 2024-04-15 13:39 ` Charles Keepax
2024-04-15 16:03 ` Andy Shevchenko
0 siblings, 1 reply; 23+ messages in thread
From: Charles Keepax @ 2024-04-15 13:39 UTC (permalink / raw
To: Andy Shevchenko
Cc: broonie, linus.walleij, brgl, bard.liao, linux-gpio, linux-spi,
patches
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 4/4] spi: cs42l43: Add bridged cs35l56 amplifiers
2024-04-15 13:39 ` Charles Keepax
@ 2024-04-15 16:03 ` Andy Shevchenko
0 siblings, 0 replies; 23+ messages in thread
From: Andy Shevchenko @ 2024-04-15 16:03 UTC (permalink / raw
To: Charles Keepax
Cc: broonie, linus.walleij, brgl, bard.liao, linux-gpio, linux-spi,
patches
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
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2024-04-15 16:03 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-11 9:06 [PATCH 0/4] Add bridged amplifiers to cs42l43 Charles Keepax
2024-04-11 9:06 ` [PATCH v5 1/4] gpio: swnode: Add ability to specify native chip selects for SPI Charles Keepax
2024-04-11 13:25 ` Andy Shevchenko
2024-04-11 16:44 ` Charles Keepax
2024-04-11 16:50 ` Andy Shevchenko
2024-04-11 16:58 ` Charles Keepax
2024-04-11 9:06 ` [PATCH v5 2/4] spi: Switch to using is_acpi_device_node() in spi_dev_set_name() Charles Keepax
2024-04-11 13:30 ` Andy Shevchenko
2024-04-11 16:56 ` Charles Keepax
2024-04-11 18:09 ` Andy Shevchenko
2024-04-11 9:06 ` [PATCH v5 3/4] spi: Update swnode based SPI devices to use the fwnode name Charles Keepax
2024-04-11 13:33 ` Andy Shevchenko
2024-04-11 17:04 ` Charles Keepax
2024-04-11 18:07 ` Andy Shevchenko
2024-04-11 9:06 ` [PATCH v5 4/4] spi: cs42l43: Add bridged cs35l56 amplifiers Charles Keepax
2024-04-11 14:04 ` Andy Shevchenko
2024-04-11 14:06 ` Andy Shevchenko
2024-04-11 14:07 ` Andy Shevchenko
2024-04-11 17:13 ` Charles Keepax
2024-04-11 18:17 ` Andy Shevchenko
2024-04-15 13:39 ` Charles Keepax
2024-04-15 16:03 ` Andy Shevchenko
2024-04-11 11:46 ` [PATCH 0/4] Add bridged amplifiers to cs42l43 Andy Shevchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).