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

[Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxxx>
> > +#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