Re: [PATCH v3 3/3] spi: cs42l43: Add bridged cs35l56 amplifiers

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Mon, Apr 8, 2024 at 6:35 PM Charles Keepax
<ckeepax@xxxxxxxxxxxxxxxxxxxxx> 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

...

> > > +   if (!is_acpi_node(fwnode))
> > > +           return false;
> >
> > Dup, your loop will perform the same effectivelly.
>
> Are you sure? Won't adev end up being NULL and the adev->handle
> will dereference it?

Yes, you need to check the ACPI dev to be not NULL there. Also note,
that is_acpi_node() is not the same as is_acpi_device_node().

> > > +   fwnode_for_each_child_node(fwnode, child_fwnode) {
> >
> > > +           struct acpi_device *adev = to_acpi_device_node(child_fwnode);

...

> > > +           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.
>
> Hm... odd, feels like the function could use a comment or something
> to say don't use me. But we can go back to managing it manually
> no problems.

Ah, I was thinking that it inherited that from device_add_property()
(see 2338e7bcef44 ("device property: Remove device_add_properties()
API") for the details), but no, it seems okay to use. but then we
really need to be careful about lifetime of it wrt. other parts in
this code.

-- 
With Best Regards,
Andy Shevchenko