| Message ID | 20240329114730.360313-4-ckeepax@opensource.cirrus.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add bridged amplifiers to cs42l43 | expand |
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 = &l, > + .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 = &r, > + .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, &l_info)) { > + ret = -ENODEV; > + dev_err(priv->dev, "Failed to create left amp slave\n"); > + goto err; > + } > + > + if (!spi_new_device(priv->ctlr, &r_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; > +};
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.
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 = &l, + .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 = &r, + .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, &l_info)) { + ret = -ENODEV; + dev_err(priv->dev, "Failed to create left amp slave\n"); + goto err; + } + + if (!spi_new_device(priv->ctlr, &r_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);