From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Charles Keepax <ckeepax@opensource.cirrus.com>
Cc: broonie@kernel.org, linus.walleij@linaro.org, brgl@bgdev.pl,
bard.liao@intel.com, linux-gpio@vger.kernel.org,
linux-spi@vger.kernel.org, patches@opensource.cirrus.com
Subject: Re: [PATCH v5 4/4] spi: cs42l43: Add bridged cs35l56 amplifiers
Date: Mon, 15 Apr 2024 19:03:20 +0300 [thread overview]
Message-ID: <CAHp75Veqr290PKBNi+QhuSAR+_me+WXBSvYQSSrkpw-fTrV0zA@mail.gmail.com> (raw)
In-Reply-To: <Zh0uH64AlEMJQyzz@ediswmail9.ad.cirrus.com>
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
next prev parent reply other threads:[~2024-04-15 16:03 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2024-04-11 11:46 ` [PATCH 0/4] Add bridged amplifiers to cs42l43 Andy Shevchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAHp75Veqr290PKBNi+QhuSAR+_me+WXBSvYQSSrkpw-fTrV0zA@mail.gmail.com \
--to=andy.shevchenko@gmail.com \
--cc=bard.liao@intel.com \
--cc=brgl@bgdev.pl \
--cc=broonie@kernel.org \
--cc=ckeepax@opensource.cirrus.com \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-spi@vger.kernel.org \
--cc=patches@opensource.cirrus.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).