| Message ID | 20240409132126.1117916-4-ckeepax@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add bridged amplifiers to cs42l43 | expand |
On Tue, Apr 09, 2024 at 02:21:26PM +0100, Charles Keepax wrote: > From: Maciej Strozek <mstrozek@opensource.cirrus.com> > > On some cs42l43 systems a couple of cs35l56 amplifiers are attached > to the cs42l43's SPI and I2S. On Windows the cs42l43 is controlled > by a SDCA class driver and these two amplifiers are controlled by > firmware running on the cs42l43. However, under Linux the decision > was made to interact with the cs42l43 directly, affording the user > greater control over the audio system. However, this has resulted > in an issue where these two bridged cs35l56 amplifiers are not > populated in ACPI and must be added manually. > > Check for the presence of the "01fa-cirrus-sidecar-instances" property > in the SDCA extension unit's ACPI properties to confirm the presence > of these two amplifiers and if they exist add them manually onto the > SPI bus. ... > +#include <linux/acpi.h> > +#include <linux/array_size.h> > #include <linux/bits.h> > #include <linux/bitfield.h> > #include <linux/device.h> > #include <linux/errno.h> > +#include <linux/gpio/machine.h> Shouldn't you include gpio/property.h as well? Ah, in the previous patch you put swnode to consumer.h instead of gpio/property.h. Please, fix that. > #include <linux/mfd/cs42l43.h> > #include <linux/mfd/cs42l43-regs.h> > #include <linux/mod_devicetable.h> > #include <linux/of.h> > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > +#include <linux/property.h> > #include <linux/regmap.h> > #include <linux/spi/spi.h> > #include <linux/units.h> ... > +static const struct software_node ampl = { > + .name = "cs35l56-left", > +}; > + > +static const struct software_node ampr = { > + .name = "cs35l56-right", > +}; What these swnodes are for? ... > +static bool cs42l43_has_sidecar(struct fwnode_handle *fwnode) > +{ > + static const u32 func_smart_amp = 0x1; > + struct fwnode_handle *child_fwnode, *ext_fwnode; > + unsigned int val; > + u32 function; > + int ret; > + > + fwnode_for_each_child_node(fwnode, child_fwnode) { > + struct acpi_device *adev = to_acpi_device_node(child_fwnode); > + > + if (!adev) > + continue; > + > + ret = acpi_get_local_address(adev->handle, &function); > + if (ret || function != func_smart_amp) { > + fwnode_handle_put(child_fwnode); Why? > + continue; > + } > + > + ext_fwnode = fwnode_get_named_child_node(child_fwnode, > + "mipi-sdca-function-expansion-subproperties"); > + if (!ext_fwnode) { > + fwnode_handle_put(child_fwnode); Ditto. > + continue; > + } > + > + ret = fwnode_property_read_u32(ext_fwnode, > + "01fa-cirrus-sidecar-instances", > + &val); > + > + fwnode_handle_put(ext_fwnode); > + fwnode_handle_put(child_fwnode); Ditto. Haven't you get reference underflow? > + > + if (!ret) > + return !!val; > + } > + > + return false; > +} ... > +MODULE_IMPORT_NS(GPIO_SWNODE); > + Stray blank line. > MODULE_DESCRIPTION("CS42L43 SPI Driver"); > MODULE_AUTHOR("Lucas Tanure <tanureal@opensource.cirrus.com>"); > MODULE_AUTHOR("Maciej Strozek <mstrozek@opensource.cirrus.com>");
Hi Charles, kernel test robot noticed the following build errors: [auto build test ERROR on broonie-spi/for-next] [also build test ERROR on brgl/gpio/for-next linus/master v6.9-rc3 next-20240410] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Charles-Keepax/gpio-swnode-Add-ability-to-specify-native-chip-selects-for-SPI/20240409-212316 base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next patch link: https://lore.kernel.org/r/20240409132126.1117916-4-ckeepax%40opensource.cirrus.com patch subject: [PATCH v4 3/3] spi: cs42l43: Add bridged cs35l56 amplifiers config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20240410/202404101443.tYCaeZAm-lkp@intel.com/config) compiler: m68k-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240410/202404101443.tYCaeZAm-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202404101443.tYCaeZAm-lkp@intel.com/ All errors (new ones prefixed by >>): drivers/spi/spi-cs42l43.c: In function 'cs42l43_has_sidecar': >> drivers/spi/spi-cs42l43.c:262:50: error: invalid use of undefined type 'struct acpi_device' 262 | ret = acpi_get_local_address(adev->handle, &function); | ^~ vim +262 drivers/spi/spi-cs42l43.c 247 248 static bool cs42l43_has_sidecar(struct fwnode_handle *fwnode) 249 { 250 static const u32 func_smart_amp = 0x1; 251 struct fwnode_handle *child_fwnode, *ext_fwnode; 252 unsigned int val; 253 u32 function; 254 int ret; 255 256 fwnode_for_each_child_node(fwnode, child_fwnode) { 257 struct acpi_device *adev = to_acpi_device_node(child_fwnode); 258 259 if (!adev) 260 continue; 261 > 262 ret = acpi_get_local_address(adev->handle, &function); 263 if (ret || function != func_smart_amp) { 264 fwnode_handle_put(child_fwnode); 265 continue; 266 } 267 268 ext_fwnode = fwnode_get_named_child_node(child_fwnode, 269 "mipi-sdca-function-expansion-subproperties"); 270 if (!ext_fwnode) { 271 fwnode_handle_put(child_fwnode); 272 continue; 273 } 274 275 ret = fwnode_property_read_u32(ext_fwnode, 276 "01fa-cirrus-sidecar-instances", 277 &val); 278 279 fwnode_handle_put(ext_fwnode); 280 fwnode_handle_put(child_fwnode); 281 282 if (!ret) 283 return !!val; 284 } 285 286 return false; 287 } 288
Wed, Apr 10, 2024 at 03:42:35PM +0800, kernel test robot kirjoitti: > Hi Charles, > > kernel test robot noticed the following build errors: > > [auto build test ERROR on broonie-spi/for-next] > [also build test ERROR on brgl/gpio/for-next linus/master v6.9-rc3 next-20240410] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: https://github.com/intel-lab-lkp/linux/commits/Charles-Keepax/gpio-swnode-Add-ability-to-specify-native-chip-selects-for-SPI/20240409-212316 > base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next > patch link: https://lore.kernel.org/r/20240409132126.1117916-4-ckeepax%40opensource.cirrus.com > patch subject: [PATCH v4 3/3] spi: cs42l43: Add bridged cs35l56 amplifiers > config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20240410/202404101443.tYCaeZAm-lkp@intel.com/config) > compiler: m68k-linux-gcc (GCC) 13.2.0 > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240410/202404101443.tYCaeZAm-lkp@intel.com/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: https://lore.kernel.org/oe-kbuild-all/202404101443.tYCaeZAm-lkp@intel.com/ > > All errors (new ones prefixed by >>): > > drivers/spi/spi-cs42l43.c: In function 'cs42l43_has_sidecar': > >> drivers/spi/spi-cs42l43.c:262:50: error: invalid use of undefined type 'struct acpi_device' > 262 | ret = acpi_get_local_address(adev->handle, &function); > | ^~ Oh, yes, this should take ACPI_HANDLE_FWNODE() (and the adev will gone AFAIU?).
On Tue, Apr 09, 2024 at 09:26:44PM +0300, Andy Shevchenko wrote: > On Tue, Apr 09, 2024 at 02:21:26PM +0100, Charles Keepax wrote: > > From: Maciej Strozek <mstrozek@opensource.cirrus.com> > > +#include <linux/acpi.h> > > +#include <linux/array_size.h> > > #include <linux/bits.h> > > #include <linux/bitfield.h> > > #include <linux/device.h> > > #include <linux/errno.h> > > +#include <linux/gpio/machine.h> > > Shouldn't you include gpio/property.h as well? > Ah, in the previous patch you put swnode to consumer.h instead of > gpio/property.h. Please, fix that. > Sorry not sure I follow here, nothing is using PROPERTY_ENTRY_GPIO and not sure why that is needed in the swnode patch either? > > #include <linux/mfd/cs42l43.h> > > #include <linux/mfd/cs42l43-regs.h> > > #include <linux/mod_devicetable.h> > > > #include <linux/of.h> > > #include <linux/platform_device.h> > > #include <linux/pm_runtime.h> > > +#include <linux/property.h> > > #include <linux/regmap.h> > > #include <linux/spi/spi.h> > > #include <linux/units.h> > > ... > > > +static const struct software_node ampl = { > > + .name = "cs35l56-left", > > +}; > > + > > +static const struct software_node ampr = { > > + .name = "cs35l56-right", > > +}; > > What these swnodes are for? > The two amps we are adding, not sure I entirely follow what you are asking here. We need the software nodes so we can name the amps something such that we can find them from the machine driver later. > ... > > > +static bool cs42l43_has_sidecar(struct fwnode_handle *fwnode) > > +{ > > + static const u32 func_smart_amp = 0x1; > > + struct fwnode_handle *child_fwnode, *ext_fwnode; > > + unsigned int val; > > + u32 function; > > + int ret; > > + > > + fwnode_for_each_child_node(fwnode, child_fwnode) { > > + struct acpi_device *adev = to_acpi_device_node(child_fwnode); > > + > > + if (!adev) > > + continue; > > + > > + ret = acpi_get_local_address(adev->handle, &function); > > + if (ret || function != func_smart_amp) { > > > + fwnode_handle_put(child_fwnode); > > Why? > Ah had missed the fwnode_for_each_child will do the put itself, will fix that up. > > +MODULE_IMPORT_NS(GPIO_SWNODE); > > > + > > Stray blank line. > Fair enough will remove. Thanks, Charles
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index 6e4b5f7e8adc..ffc4bd7a67e6 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -284,6 +284,7 @@ config SPI_COLDFIRE_QSPI config SPI_CS42L43 tristate "Cirrus Logic CS42L43 SPI controller" depends on MFD_CS42L43 && PINCTRL_CS42L43 + select GPIO_SWNODE_UNDEFINED help This enables support for the SPI controller inside the Cirrus Logic CS42L43 audio codec. diff --git a/drivers/spi/spi-cs42l43.c b/drivers/spi/spi-cs42l43.c index aabef9fc84bd..a4208c2dfc76 100644 --- a/drivers/spi/spi-cs42l43.c +++ b/drivers/spi/spi-cs42l43.c @@ -5,10 +5,13 @@ // Copyright (C) 2022-2023 Cirrus Logic, Inc. and // Cirrus Logic International Semiconductor Ltd. +#include <linux/acpi.h> +#include <linux/array_size.h> #include <linux/bits.h> #include <linux/bitfield.h> #include <linux/device.h> #include <linux/errno.h> +#include <linux/gpio/machine.h> #include <linux/mfd/cs42l43.h> #include <linux/mfd/cs42l43-regs.h> #include <linux/mod_devicetable.h> @@ -16,6 +19,7 @@ #include <linux/of.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> +#include <linux/property.h> #include <linux/regmap.h> #include <linux/spi/spi.h> #include <linux/units.h> @@ -39,6 +43,44 @@ static const unsigned int cs42l43_clock_divs[] = { 2, 2, 4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24, 26, 28, 30 }; +static const struct software_node ampl = { + .name = "cs35l56-left", +}; + +static const struct software_node ampr = { + .name = "cs35l56-right", +}; + +static struct spi_board_info ampl_info = { + .modalias = "cs35l56", + .max_speed_hz = 20 * HZ_PER_MHZ, + .chip_select = 0, + .mode = SPI_MODE_0, + .swnode = &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 +245,47 @@ static size_t cs42l43_spi_max_length(struct spi_device *spi) return CS42L43_SPI_MAX_LENGTH; } +static bool cs42l43_has_sidecar(struct fwnode_handle *fwnode) +{ + static const u32 func_smart_amp = 0x1; + struct fwnode_handle *child_fwnode, *ext_fwnode; + unsigned int val; + u32 function; + int ret; + + fwnode_for_each_child_node(fwnode, child_fwnode) { + struct acpi_device *adev = to_acpi_device_node(child_fwnode); + + if (!adev) + continue; + + ret = acpi_get_local_address(adev->handle, &function); + if (ret || function != func_smart_amp) { + fwnode_handle_put(child_fwnode); + continue; + } + + ext_fwnode = fwnode_get_named_child_node(child_fwnode, + "mipi-sdca-function-expansion-subproperties"); + if (!ext_fwnode) { + fwnode_handle_put(child_fwnode); + continue; + } + + ret = fwnode_property_read_u32(ext_fwnode, + "01fa-cirrus-sidecar-instances", + &val); + + fwnode_handle_put(ext_fwnode); + fwnode_handle_put(child_fwnode); + + if (!ret) + return !!val; + } + + return false; +} + static void cs42l43_release_of_node(void *data) { fwnode_handle_put(data); @@ -213,6 +296,7 @@ static int cs42l43_spi_probe(struct platform_device *pdev) struct cs42l43 *cs42l43 = dev_get_drvdata(pdev->dev.parent); struct cs42l43_spi *priv; struct fwnode_handle *fwnode = dev_fwnode(cs42l43->dev); + bool has_sidecar = cs42l43_has_sidecar(fwnode); int ret; priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); @@ -266,16 +350,64 @@ static int cs42l43_spi_probe(struct platform_device *pdev) } } - device_set_node(&priv->ctlr->dev, fwnode); + if (has_sidecar) { + ret = software_node_register(&cs42l43_gpiochip_swnode); + if (ret) { + return dev_err_probe(priv->dev, ret, + "Failed to register gpio swnode\n"); + } + + ret = device_create_managed_software_node(&priv->ctlr->dev, + cs42l43_cs_props, NULL); + if (ret) { + dev_err_probe(priv->dev, ret, "Failed to add swnode\n"); + goto err; + } + } else { + device_set_node(&priv->ctlr->dev, fwnode); + } ret = devm_spi_register_controller(priv->dev, priv->ctlr); if (ret) { - dev_err(priv->dev, "Failed to register SPI controller: %d\n", ret); + dev_err_probe(priv->dev, ret, "Failed to register SPI controller\n"); + goto err; + } + + if (has_sidecar) { + if (!spi_new_device(priv->ctlr, &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 +420,12 @@ static struct platform_driver cs42l43_spi_driver = { }, .probe = cs42l43_spi_probe, .id_table = cs42l43_spi_id_table, + .remove = cs42l43_spi_remove, }; module_platform_driver(cs42l43_spi_driver); +MODULE_IMPORT_NS(GPIO_SWNODE); + MODULE_DESCRIPTION("CS42L43 SPI Driver"); MODULE_AUTHOR("Lucas Tanure <tanureal@opensource.cirrus.com>"); MODULE_AUTHOR("Maciej Strozek <mstrozek@opensource.cirrus.com>");