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

[Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx>]

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:
> > From: Maciej Strozek <mstrozek@xxxxxxxxxxxxxxxxxxxxx>
> > +#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.

> > +#include <linux/acpi.h>
> 
> You need array_size.h (and perhaps overflow.h) and property.h.

Good spot will add those.

> > +static struct spi_board_info ampl_info = {
> > +	.modalias		= "cs35l56",
> > +	.max_speed_hz		= 2000000,
> 
> Maybe HZ_PER_MHZ from units.h?

Can do.

> > +static const struct software_node_ref_args cs42l43_cs_refs[] = {
> Please, use SOFTWARE_NODE_REFERENCE().

> > +static const struct property_entry cs42l43_cs_props[] = {
> You want PROPERTY_ENTRY_REF_ARRAY().. 

Can do.

> > +static bool cs42l43_has_sidecar(struct fwnode_handle *fwnode)
> > +{
> > +	static const int func_smart_amp = 0x1;
> > +	struct fwnode_handle *child_fwnode, *ext_fwnode;
> > +	unsigned long long function;
> > +	unsigned int val;
> > +	int ret;
> 
> > +	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?

> > +	fwnode_for_each_child_node(fwnode, child_fwnode) {
> 
> > +		struct acpi_device *adev = to_acpi_device_node(child_fwnode);
> > +
> > +		ret = acpi_evaluate_integer(adev->handle, "_ADR", NULL, &function);
> > +		if (ACPI_FAILURE(ret) || function != func_smart_amp) {
> > +			fwnode_handle_put(fwnode);
> > +			continue;
> > +		}
> 
> acpi_get_local_address() (it has a stub for CONFIG_ACPI=n).

Thanks was looking for something like that not sure how I missed
that.

> > +		ext_fwnode = fwnode_get_named_child_node(child_fwnode,
> > +				"mipi-sdca-function-expansion-subproperties");
> > +		if (!ext_fwnode) {
> 
> > +			fwnode_handle_put(fwnode);
> 
> Are you sure?

oops, yeah those should all be child_fwnode.

> > +	if (has_sidecar) {
> > +		ret = software_node_register(&cs42l43_gpiochip_swnode);
> > +		if (ret) {
> > +			dev_err(priv->dev, "Failed to register gpio swnode: %d\n", ret);
> > +			return ret;
> > +		}
> 
> > +		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.

> > +		if (ret) {
> > +			dev_err(priv->dev, "Failed to add swnode: %d\n", ret);
> > +			goto err;
> > +		}
> 
> > +
> 
> Redundant blank line.

yup.

> > +	} else {
> > +		device_set_node(&priv->ctlr->dev, fwnode);
> > +	}
> >  
> >  	ret = devm_spi_register_controller(priv->dev, priv->ctlr);
> >  	if (ret) {
> >  		dev_err(priv->dev, "Failed to register SPI controller: %d\n", ret);
> > +		goto err;
> > +	}
> > +
> > +	if (has_sidecar) {
> > +		if (!spi_new_device(priv->ctlr, &ampl_info)) {
> > +			ret = -ENODEV;
> > +			dev_err(priv->dev, "Failed to create left amp slave\n");
> > +			goto err;
> > +		}
> > +
> > +		if (!spi_new_device(priv->ctlr, &ampr_info)) {
> > +			ret = -ENODEV;
> > +			dev_err(priv->dev, "Failed to create right amp slave\n");
> > +			goto err;
> > +		}
> >  	}
> >  
> > +	return 0;
> > +
> > +err:
> > +	if (has_sidecar)
> > +		software_node_unregister(&cs42l43_gpiochip_swnode);
> > +
> >  	return ret;
> >  }
> 
> Wondering why don't you use return dev_err_probe() / ret = dev_err_probe() /
> dev_err_probe(ret)?

Yeah some of those should be, will update.

> > +static int cs42l43_spi_remove(struct platform_device *pdev)
> > +{
> > +	struct cs42l43 *cs42l43 = dev_get_drvdata(pdev->dev.parent);
> 
> platform_get_drvdata()
> 
> > +	struct fwnode_handle *fwnode = dev_fwnode(cs42l43->dev);
> 
> Is this dev the same as &pdev->dev?

No, this is MFD parent device, to be fair could probably use
parent directly here. Will have a think about that.

Thanks,
Charles