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

[Andy Shevchenko <andy@black.fi.intel.com>]

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>");

-- 
With Best Regards,
Andy Shevchenko