linux-gpio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] spi: cs42l43: Use actual ACPI firmware node for chip selects
@ 2025-11-19 16:40 Charles Keepax
  2025-11-19 16:45 ` Charles Keepax
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Charles Keepax @ 2025-11-19 16:40 UTC (permalink / raw)
  To: broonie, brgl, linus.walleij
  Cc: andy, p.zabel, linux-gpio, linux-spi, bartosz.golaszewski,
	linux-kernel, patches

On some systems the cs42l43 has amplifiers attached to its SPI
controller that are not properly defined in ACPI. Currently software
nodes are added to support this case, however, the chip selects
for these devices are specified using a bit of a hack. A software
node is added with the same name as the pinctrl driver, as the look
up was name based this caused the GPIO looks to return the pinctrl
driver even though the swnode is not associated with the pinctrl
driver. This was necessary as the swnodes did not support directly
linking to real firmware nodes.

Since commit e5d527be7e69 ("gpio: swnode: don't use the
swnode's name as the key for GPIO lookup") changed the lookup to
be fwnode based this hack will no longer find the pinctrl driver,
resulting in the driver not probing. But other patches also add support
for linking a swnode to a real fwnode node [1]. As such switch over to
just passing the real fwnode for the pinctrl property to avoid any
issues.

[1] https://lore.kernel.org/linux-gpio/20251106-reset-gpios-swnodes-v6-0-69aa852de9e4@linaro.org/

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

Ok here is what I would propose to fix this one, IMPORTANT NOTE: this
does depend on the first four patches of the linked chain which I don't
think are merged yet. But I would argue if we are removing the name
based look up, we should add support for fwnodes at the same time.

 drivers/spi/spi-cs42l43.c | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/drivers/spi/spi-cs42l43.c b/drivers/spi/spi-cs42l43.c
index 14307dd800b74..b7dd5f6ecc30e 100644
--- a/drivers/spi/spi-cs42l43.c
+++ b/drivers/spi/spi-cs42l43.c
@@ -52,12 +52,8 @@ static struct spi_board_info amp_info_template = {
 	.mode			= SPI_MODE_0,
 };
 
-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(NULL, 0, GPIO_ACTIVE_LOW),
 	SOFTWARE_NODE_REFERENCE(&swnode_gpio_undefined),
 };
 
@@ -324,11 +320,6 @@ static void cs42l43_release_of_node(void *data)
 	fwnode_handle_put(data);
 }
 
-static void cs42l43_release_sw_node(void *data)
-{
-	software_node_unregister(&cs42l43_gpiochip_swnode);
-}
-
 static int cs42l43_spi_probe(struct platform_device *pdev)
 {
 	struct cs42l43 *cs42l43 = dev_get_drvdata(pdev->dev.parent);
@@ -391,6 +382,9 @@ static int cs42l43_spi_probe(struct platform_device *pdev)
 	fwnode_property_read_u32(xu_fwnode, "01fa-sidecar-instances", &nsidecars);
 
 	if (nsidecars) {
+		struct software_node_ref_args *args;
+		struct property_entry *props;
+
 		ret = fwnode_property_read_u32(xu_fwnode, "01fa-spk-id-val", &spkid);
 		if (!ret) {
 			dev_dbg(priv->dev, "01fa-spk-id-val = %d\n", spkid);
@@ -403,17 +397,20 @@ static int cs42l43_spi_probe(struct platform_device *pdev)
 						     "Failed to get spk-id-gpios\n");
 		}
 
-		ret = software_node_register(&cs42l43_gpiochip_swnode);
-		if (ret)
-			return dev_err_probe(priv->dev, ret,
-					     "Failed to register gpio swnode\n");
+		props = devm_kmemdup(priv->dev, cs42l43_cs_props, sizeof(cs42l43_cs_props),
+				     GFP_KERNEL);
+		if (!props)
+			return -ENOMEM;
 
-		ret = devm_add_action_or_reset(priv->dev, cs42l43_release_sw_node, NULL);
-		if (ret)
-			return ret;
+		args = devm_kmemdup(priv->dev, cs42l43_cs_refs, sizeof(cs42l43_cs_refs),
+				    GFP_KERNEL);
+		if (!args)
+			return -ENOMEM;
+
+		args[0].fwnode = fwnode;
+		props->pointer = args;
 
-		ret = device_create_managed_software_node(&priv->ctlr->dev,
-							  cs42l43_cs_props, NULL);
+		ret = device_create_managed_software_node(&priv->ctlr->dev, props, NULL);
 		if (ret)
 			return dev_err_probe(priv->dev, ret, "Failed to add swnode\n");
 	} else {
-- 
2.47.3


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] spi: cs42l43: Use actual ACPI firmware node for chip selects
  2025-11-19 16:40 [PATCH] spi: cs42l43: Use actual ACPI firmware node for chip selects Charles Keepax
@ 2025-11-19 16:45 ` Charles Keepax
  2025-11-19 16:51   ` Andy Shevchenko
  2025-11-19 16:49 ` Andy Shevchenko
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 8+ messages in thread
From: Charles Keepax @ 2025-11-19 16:45 UTC (permalink / raw)
  To: broonie, brgl, linus.walleij
  Cc: andy, p.zabel, linux-gpio, linux-spi, bartosz.golaszewski,
	linux-kernel, patches

On Wed, Nov 19, 2025 at 04:40:17PM +0000, Charles Keepax wrote:
> On some systems the cs42l43 has amplifiers attached to its SPI
> controller that are not properly defined in ACPI. Currently software
> nodes are added to support this case, however, the chip selects
> for these devices are specified using a bit of a hack. A software
> node is added with the same name as the pinctrl driver, as the look
> up was name based this caused the GPIO looks to return the pinctrl
> driver even though the swnode is not associated with the pinctrl
> driver. This was necessary as the swnodes did not support directly
> linking to real firmware nodes.
> 
> Since commit e5d527be7e69 ("gpio: swnode: don't use the
> swnode's name as the key for GPIO lookup") changed the lookup to
> be fwnode based this hack will no longer find the pinctrl driver,
> resulting in the driver not probing. But other patches also add support
> for linking a swnode to a real fwnode node [1]. As such switch over to
> just passing the real fwnode for the pinctrl property to avoid any
> issues.
> 
> [1] https://lore.kernel.org/linux-gpio/20251106-reset-gpios-swnodes-v6-0-69aa852de9e4@linaro.org/
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---

Apologies this probably should have a fixes tag, or two and I
probably should have marked it RFC. Lets have some discussion and
if people like the approach I will send a v2 with the tags
included.

Thanks,
Charles

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] spi: cs42l43: Use actual ACPI firmware node for chip selects
  2025-11-19 16:40 [PATCH] spi: cs42l43: Use actual ACPI firmware node for chip selects Charles Keepax
  2025-11-19 16:45 ` Charles Keepax
@ 2025-11-19 16:49 ` Andy Shevchenko
  2025-11-19 17:04   ` Charles Keepax
  2025-11-20  9:23 ` kernel test robot
  2025-11-20  9:29 ` Bartosz Golaszewski
  3 siblings, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2025-11-19 16:49 UTC (permalink / raw)
  To: Charles Keepax
  Cc: broonie, brgl, linus.walleij, andy, p.zabel, linux-gpio,
	linux-spi, bartosz.golaszewski, linux-kernel, patches

On Wed, Nov 19, 2025 at 6:40 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
>
> On some systems the cs42l43 has amplifiers attached to its SPI
> controller that are not properly defined in ACPI. Currently software
> nodes are added to support this case, however, the chip selects
> for these devices are specified using a bit of a hack. A software
> node is added with the same name as the pinctrl driver, as the look
> up was name based this caused the GPIO looks to return the pinctrl
> driver even though the swnode is not associated with the pinctrl
> driver. This was necessary as the swnodes did not support directly
> linking to real firmware nodes.
>
> Since commit e5d527be7e69 ("gpio: swnode: don't use the
> swnode's name as the key for GPIO lookup") changed the lookup to
> be fwnode based this hack will no longer find the pinctrl driver,
> resulting in the driver not probing. But other patches also add support
> for linking a swnode to a real fwnode node [1]. As such switch over to
> just passing the real fwnode for the pinctrl property to avoid any
> issues.

> [1] https://lore.kernel.org/linux-gpio/20251106-reset-gpios-swnodes-v6-0-69aa852de9e4@linaro.org/
>

This can be

Link: ... [1]

actually.

> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
>
> Ok here is what I would propose to fix this one, IMPORTANT NOTE: this
> does depend on the first four patches of the linked chain which I don't
> think are merged yet. But I would argue if we are removing the name
> based look up, we should add support for fwnodes at the same time.

You mean it has functional dependency and not a compile-time one?

...

>  static const struct software_node_ref_args cs42l43_cs_refs[] = {
> -       SOFTWARE_NODE_REFERENCE(&cs42l43_gpiochip_swnode, 0, GPIO_ACTIVE_LOW),
> +       SOFTWARE_NODE_REFERENCE(NULL, 0, GPIO_ACTIVE_LOW),
>         SOFTWARE_NODE_REFERENCE(&swnode_gpio_undefined),
>  };

Since it's a placeholder, we don't need it at all. See below.

...

> +               args = devm_kmemdup(priv->dev, cs42l43_cs_refs, sizeof(cs42l43_cs_refs),
> +                                   GFP_KERNEL);
> +               if (!args)
> +                       return -ENOMEM;
> +

> +               args[0].fwnode = fwnode;

You can assign entries directly here as

  args = devm_kmalloc_array(...);
  ...
  args[0] = SOFTWARE_NODE_REFERENCE(...);
  args[1] = SOFTWARE_NODE_REFERENCE(&swnode_gpio_undefined),

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] spi: cs42l43: Use actual ACPI firmware node for chip selects
  2025-11-19 16:45 ` Charles Keepax
@ 2025-11-19 16:51   ` Andy Shevchenko
  0 siblings, 0 replies; 8+ messages in thread
From: Andy Shevchenko @ 2025-11-19 16:51 UTC (permalink / raw)
  To: Charles Keepax
  Cc: broonie, brgl, linus.walleij, andy, p.zabel, linux-gpio,
	linux-spi, bartosz.golaszewski, linux-kernel, patches

On Wed, Nov 19, 2025 at 6:45 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
> On Wed, Nov 19, 2025 at 04:40:17PM +0000, Charles Keepax wrote:

...

> Apologies this probably should have a fixes tag, or two and I
> probably should have marked it RFC. Lets have some discussion and
> if people like the approach I will send a v2 with the tags
> included.

At least I don't see anything quite wrong with it. But I probably need
to have a fresh look when you send a v2.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] spi: cs42l43: Use actual ACPI firmware node for chip selects
  2025-11-19 16:49 ` Andy Shevchenko
@ 2025-11-19 17:04   ` Charles Keepax
  0 siblings, 0 replies; 8+ messages in thread
From: Charles Keepax @ 2025-11-19 17:04 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: broonie, brgl, linus.walleij, andy, p.zabel, linux-gpio,
	linux-spi, bartosz.golaszewski, linux-kernel, patches

On Wed, Nov 19, 2025 at 06:49:57PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 19, 2025 at 6:40 PM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
> >
> > On some systems the cs42l43 has amplifiers attached to its SPI
> > controller that are not properly defined in ACPI. Currently software
> > nodes are added to support this case, however, the chip selects
> > for these devices are specified using a bit of a hack. A software
> > node is added with the same name as the pinctrl driver, as the look
> > up was name based this caused the GPIO looks to return the pinctrl
> > driver even though the swnode is not associated with the pinctrl
> > driver. This was necessary as the swnodes did not support directly
> > linking to real firmware nodes.
> >
> > Since commit e5d527be7e69 ("gpio: swnode: don't use the
> > swnode's name as the key for GPIO lookup") changed the lookup to
> > be fwnode based this hack will no longer find the pinctrl driver,
> > resulting in the driver not probing. But other patches also add support
> > for linking a swnode to a real fwnode node [1]. As such switch over to
> > just passing the real fwnode for the pinctrl property to avoid any
> > issues.
> 
> > [1] https://lore.kernel.org/linux-gpio/20251106-reset-gpios-swnodes-v6-0-69aa852de9e4@linaro.org/
> >
> 
> This can be
> 
> Link: ... [1]
> 
> actually.

Thanks will fixup for v2.

> > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> > ---
> >
> > Ok here is what I would propose to fix this one, IMPORTANT NOTE: this
> > does depend on the first four patches of the linked chain which I don't
> > think are merged yet. But I would argue if we are removing the name
> > based look up, we should add support for fwnodes at the same time.
> 
> You mean it has functional dependency and not a compile-time one?

Apologies that wasn't clear, both.

> > +               args[0].fwnode = fwnode;
> 
> You can assign entries directly here as
> 
>   args = devm_kmalloc_array(...);
>   ...
>   args[0] = SOFTWARE_NODE_REFERENCE(...);
>   args[1] = SOFTWARE_NODE_REFERENCE(&swnode_gpio_undefined),

Yes thank you I will fix that up for v2 as well, I was a little
rushing this one out.

Thanks,
Charles

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] spi: cs42l43: Use actual ACPI firmware node for chip selects
  2025-11-19 16:40 [PATCH] spi: cs42l43: Use actual ACPI firmware node for chip selects Charles Keepax
  2025-11-19 16:45 ` Charles Keepax
  2025-11-19 16:49 ` Andy Shevchenko
@ 2025-11-20  9:23 ` kernel test robot
  2025-11-20  9:29 ` Bartosz Golaszewski
  3 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2025-11-20  9:23 UTC (permalink / raw)
  To: Charles Keepax, broonie, brgl, linus.walleij
  Cc: oe-kbuild-all, andy, p.zabel, linux-gpio, linux-spi,
	bartosz.golaszewski, linux-kernel, patches

Hi Charles,

kernel test robot noticed the following build errors:

[auto build test ERROR on broonie-spi/for-next]
[also build test ERROR on linus/master v6.18-rc6 next-20251119]
[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/spi-cs42l43-Use-actual-ACPI-firmware-node-for-chip-selects/20251120-005808
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
patch link:    https://lore.kernel.org/r/20251119164017.1115791-1-ckeepax%40opensource.cirrus.com
patch subject: [PATCH] spi: cs42l43: Use actual ACPI firmware node for chip selects
config: sparc-randconfig-6002-20251120 (https://download.01.org/0day-ci/archive/20251120/202511201700.S7ieteHi-lkp@intel.com/config)
compiler: sparc-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251120/202511201700.S7ieteHi-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/202511201700.S7ieteHi-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/spi/spi-cs42l43.c: In function 'cs42l43_spi_probe':
>> drivers/spi/spi-cs42l43.c:410:25: error: 'struct software_node_ref_args' has no member named 'fwnode'; did you mean 'node'?
     410 |                 args[0].fwnode = fwnode;
         |                         ^~~~~~
         |                         node


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

   322	
   323	static int cs42l43_spi_probe(struct platform_device *pdev)
   324	{
   325		struct cs42l43 *cs42l43 = dev_get_drvdata(pdev->dev.parent);
   326		struct cs42l43_spi *priv;
   327		struct fwnode_handle *fwnode = dev_fwnode(cs42l43->dev);
   328		struct fwnode_handle *xu_fwnode __free(fwnode_handle) = cs42l43_find_xu_node(fwnode);
   329		int nsidecars = 0;
   330		int spkid = -EINVAL;
   331		int ret;
   332	
   333		priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
   334		if (!priv)
   335			return -ENOMEM;
   336	
   337		priv->ctlr = devm_spi_alloc_host(&pdev->dev, sizeof(*priv->ctlr));
   338		if (!priv->ctlr)
   339			return -ENOMEM;
   340	
   341		spi_controller_set_devdata(priv->ctlr, priv);
   342	
   343		priv->dev = &pdev->dev;
   344		priv->regmap = cs42l43->regmap;
   345	
   346		priv->ctlr->prepare_message = cs42l43_prepare_message;
   347		priv->ctlr->prepare_transfer_hardware = cs42l43_prepare_transfer_hardware;
   348		priv->ctlr->unprepare_transfer_hardware = cs42l43_unprepare_transfer_hardware;
   349		priv->ctlr->transfer_one = cs42l43_transfer_one;
   350		priv->ctlr->set_cs = cs42l43_set_cs;
   351		priv->ctlr->max_transfer_size = cs42l43_spi_max_length;
   352		priv->ctlr->mode_bits = SPI_3WIRE | SPI_MODE_X_MASK;
   353		priv->ctlr->flags = SPI_CONTROLLER_HALF_DUPLEX;
   354		priv->ctlr->bits_per_word_mask = SPI_BPW_MASK(8) | SPI_BPW_MASK(16) |
   355						 SPI_BPW_MASK(32);
   356		priv->ctlr->min_speed_hz = CS42L43_SPI_ROOT_HZ /
   357					   cs42l43_clock_divs[ARRAY_SIZE(cs42l43_clock_divs) - 1];
   358		priv->ctlr->max_speed_hz = CS42L43_SPI_ROOT_HZ / cs42l43_clock_divs[0];
   359		priv->ctlr->use_gpio_descriptors = true;
   360		priv->ctlr->auto_runtime_pm = true;
   361	
   362		ret = devm_pm_runtime_enable(priv->dev);
   363		if (ret)
   364			return ret;
   365	
   366		pm_runtime_idle(priv->dev);
   367	
   368		regmap_write(priv->regmap, CS42L43_TRAN_CONFIG6, CS42L43_FIFO_SIZE - 1);
   369		regmap_write(priv->regmap, CS42L43_TRAN_CONFIG7, CS42L43_FIFO_SIZE - 1);
   370	
   371		// Disable Watchdog timer and enable stall
   372		regmap_write(priv->regmap, CS42L43_SPI_CONFIG3, 0);
   373		regmap_write(priv->regmap, CS42L43_SPI_CONFIG4, CS42L43_SPI_STALL_ENA_MASK);
   374	
   375		if (is_of_node(fwnode)) {
   376			fwnode = fwnode_get_named_child_node(fwnode, "spi");
   377			ret = devm_add_action_or_reset(priv->dev, cs42l43_release_of_node, fwnode);
   378			if (ret)
   379				return ret;
   380		}
   381	
   382		fwnode_property_read_u32(xu_fwnode, "01fa-sidecar-instances", &nsidecars);
   383	
   384		if (nsidecars) {
   385			struct software_node_ref_args *args;
   386			struct property_entry *props;
   387	
   388			ret = fwnode_property_read_u32(xu_fwnode, "01fa-spk-id-val", &spkid);
   389			if (!ret) {
   390				dev_dbg(priv->dev, "01fa-spk-id-val = %d\n", spkid);
   391			} else if (ret != -EINVAL) {
   392				return dev_err_probe(priv->dev, ret, "Failed to get spk-id-val\n");
   393			} else {
   394				ret = cs42l43_get_speaker_id_gpios(priv, &spkid);
   395				if (ret < 0)
   396					return dev_err_probe(priv->dev, ret,
   397							     "Failed to get spk-id-gpios\n");
   398			}
   399	
   400			props = devm_kmemdup(priv->dev, cs42l43_cs_props, sizeof(cs42l43_cs_props),
   401					     GFP_KERNEL);
   402			if (!props)
   403				return -ENOMEM;
   404	
   405			args = devm_kmemdup(priv->dev, cs42l43_cs_refs, sizeof(cs42l43_cs_refs),
   406					    GFP_KERNEL);
   407			if (!args)
   408				return -ENOMEM;
   409	
 > 410			args[0].fwnode = fwnode;
   411			props->pointer = args;
   412	
   413			ret = device_create_managed_software_node(&priv->ctlr->dev, props, NULL);
   414			if (ret)
   415				return dev_err_probe(priv->dev, ret, "Failed to add swnode\n");
   416		} else {
   417			device_set_node(&priv->ctlr->dev, fwnode);
   418		}
   419	
   420		ret = devm_spi_register_controller(priv->dev, priv->ctlr);
   421		if (ret)
   422			return dev_err_probe(priv->dev, ret,
   423					     "Failed to register SPI controller\n");
   424	
   425		if (nsidecars) {
   426			struct spi_board_info *ampl_info;
   427			struct spi_board_info *ampr_info;
   428	
   429			ampl_info = cs42l43_create_bridge_amp(priv, "cs35l56-left", 0, spkid);
   430			if (!ampl_info)
   431				return -ENOMEM;
   432	
   433			ampr_info = cs42l43_create_bridge_amp(priv, "cs35l56-right", 1, spkid);
   434			if (!ampr_info)
   435				return -ENOMEM;
   436	
   437			if (!spi_new_device(priv->ctlr, ampl_info))
   438				return dev_err_probe(priv->dev, -ENODEV,
   439						     "Failed to create left amp slave\n");
   440	
   441			if (!spi_new_device(priv->ctlr, ampr_info))
   442				return dev_err_probe(priv->dev, -ENODEV,
   443						     "Failed to create right amp slave\n");
   444		}
   445	
   446		return 0;
   447	}
   448	

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] spi: cs42l43: Use actual ACPI firmware node for chip selects
  2025-11-19 16:40 [PATCH] spi: cs42l43: Use actual ACPI firmware node for chip selects Charles Keepax
                   ` (2 preceding siblings ...)
  2025-11-20  9:23 ` kernel test robot
@ 2025-11-20  9:29 ` Bartosz Golaszewski
  2025-11-20 10:06   ` Charles Keepax
  3 siblings, 1 reply; 8+ messages in thread
From: Bartosz Golaszewski @ 2025-11-20  9:29 UTC (permalink / raw)
  To: Charles Keepax
  Cc: broonie, linus.walleij, andy, p.zabel, linux-gpio, linux-spi,
	bartosz.golaszewski, linux-kernel, patches

On Wed, Nov 19, 2025 at 5:40 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
>
> On some systems the cs42l43 has amplifiers attached to its SPI
> controller that are not properly defined in ACPI. Currently software
> nodes are added to support this case, however, the chip selects
> for these devices are specified using a bit of a hack. A software
> node is added with the same name as the pinctrl driver, as the look
> up was name based this caused the GPIO looks to return the pinctrl
> driver even though the swnode is not associated with the pinctrl
> driver. This was necessary as the swnodes did not support directly
> linking to real firmware nodes.
>
> Since commit e5d527be7e69 ("gpio: swnode: don't use the
> swnode's name as the key for GPIO lookup") changed the lookup to
> be fwnode based this hack will no longer find the pinctrl driver,
> resulting in the driver not probing. But other patches also add support
> for linking a swnode to a real fwnode node [1]. As such switch over to
> just passing the real fwnode for the pinctrl property to avoid any
> issues.
>
> [1] https://lore.kernel.org/linux-gpio/20251106-reset-gpios-swnodes-v6-0-69aa852de9e4@linaro.org/
>
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
>
> Ok here is what I would propose to fix this one, IMPORTANT NOTE: this
> does depend on the first four patches of the linked chain which I don't
> think are merged yet. But I would argue if we are removing the name
> based look up, we should add support for fwnodes at the same time.
>

I would then suggest an ack from Mark and making it part of the larger
series coming after swnode changes but before GPIO.

If it works for you better than the ones I proposed then I'm fine with it.

>  drivers/spi/spi-cs42l43.c | 35 ++++++++++++++++-------------------
>  1 file changed, 16 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/spi/spi-cs42l43.c b/drivers/spi/spi-cs42l43.c
> index 14307dd800b74..b7dd5f6ecc30e 100644
> --- a/drivers/spi/spi-cs42l43.c
> +++ b/drivers/spi/spi-cs42l43.c
> @@ -52,12 +52,8 @@ static struct spi_board_info amp_info_template = {
>         .mode                   = SPI_MODE_0,
>  };
>
> -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(NULL, 0, GPIO_ACTIVE_LOW),
>         SOFTWARE_NODE_REFERENCE(&swnode_gpio_undefined),
>  };
>
> @@ -324,11 +320,6 @@ static void cs42l43_release_of_node(void *data)
>         fwnode_handle_put(data);
>  }
>
> -static void cs42l43_release_sw_node(void *data)
> -{
> -       software_node_unregister(&cs42l43_gpiochip_swnode);
> -}
> -
>  static int cs42l43_spi_probe(struct platform_device *pdev)
>  {
>         struct cs42l43 *cs42l43 = dev_get_drvdata(pdev->dev.parent);
> @@ -391,6 +382,9 @@ static int cs42l43_spi_probe(struct platform_device *pdev)
>         fwnode_property_read_u32(xu_fwnode, "01fa-sidecar-instances", &nsidecars);
>
>         if (nsidecars) {
> +               struct software_node_ref_args *args;
> +               struct property_entry *props;
> +
>                 ret = fwnode_property_read_u32(xu_fwnode, "01fa-spk-id-val", &spkid);
>                 if (!ret) {
>                         dev_dbg(priv->dev, "01fa-spk-id-val = %d\n", spkid);
> @@ -403,17 +397,20 @@ static int cs42l43_spi_probe(struct platform_device *pdev)
>                                                      "Failed to get spk-id-gpios\n");
>                 }
>
> -               ret = software_node_register(&cs42l43_gpiochip_swnode);
> -               if (ret)
> -                       return dev_err_probe(priv->dev, ret,
> -                                            "Failed to register gpio swnode\n");
> +               props = devm_kmemdup(priv->dev, cs42l43_cs_props, sizeof(cs42l43_cs_props),
> +                                    GFP_KERNEL);
> +               if (!props)
> +                       return -ENOMEM;

You don't need to allocate it for more than the duration of this
function, device_create_managed_software_node() makes a deep copy of
the properties. They can be on the stack.

>
> -               ret = devm_add_action_or_reset(priv->dev, cs42l43_release_sw_node, NULL);
> -               if (ret)
> -                       return ret;
> +               args = devm_kmemdup(priv->dev, cs42l43_cs_refs, sizeof(cs42l43_cs_refs),
> +                                   GFP_KERNEL);
> +               if (!args)
> +                       return -ENOMEM;

Same here.

> +
> +               args[0].fwnode = fwnode;
> +               props->pointer = args;
>
> -               ret = device_create_managed_software_node(&priv->ctlr->dev,
> -                                                         cs42l43_cs_props, NULL);
> +               ret = device_create_managed_software_node(&priv->ctlr->dev, props, NULL);
>                 if (ret)
>                         return dev_err_probe(priv->dev, ret, "Failed to add swnode\n");
>         } else {
> --
> 2.47.3
>

This is looking good, if you post a v2 and it's reviewed, I can resend
my series with this included and maybe it'll still make v6.19.

Bart

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] spi: cs42l43: Use actual ACPI firmware node for chip selects
  2025-11-20  9:29 ` Bartosz Golaszewski
@ 2025-11-20 10:06   ` Charles Keepax
  0 siblings, 0 replies; 8+ messages in thread
From: Charles Keepax @ 2025-11-20 10:06 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: broonie, linus.walleij, andy, p.zabel, linux-gpio, linux-spi,
	bartosz.golaszewski, linux-kernel, patches

On Thu, Nov 20, 2025 at 10:29:41AM +0100, Bartosz Golaszewski wrote:
> On Wed, Nov 19, 2025 at 5:40 PM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
> > +               props = devm_kmemdup(priv->dev, cs42l43_cs_props, sizeof(cs42l43_cs_props),
> > +                                    GFP_KERNEL);
> > +               if (!props)
> > +                       return -ENOMEM;
> 
> You don't need to allocate it for more than the duration of this
> function, device_create_managed_software_node() makes a deep copy of
> the properties. They can be on the stack.

Good point, thanks will fixup for v2. Should be able to send that
later today.

> This is looking good, if you post a v2 and it's reviewed, I can resend
> my series with this included and maybe it'll still make v6.19.

Cool yeah I am fine with you pulling this into your series once I
have sent the v2.

Thanks,
Charles

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2025-11-20 10:07 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-19 16:40 [PATCH] spi: cs42l43: Use actual ACPI firmware node for chip selects Charles Keepax
2025-11-19 16:45 ` Charles Keepax
2025-11-19 16:51   ` Andy Shevchenko
2025-11-19 16:49 ` Andy Shevchenko
2025-11-19 17:04   ` Charles Keepax
2025-11-20  9:23 ` kernel test robot
2025-11-20  9:29 ` Bartosz Golaszewski
2025-11-20 10:06   ` Charles Keepax

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 NNTP newsgroup(s).