Re: [PATCH] spi: spi-fsl-dspi: Add ACPI support

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1639 bytes --]

On Fri, Aug 21, 2020 at 06:40:29PM +0530, kuldip dwivedi wrote:

> +static const struct acpi_device_id fsl_dspi_acpi_ids[] = {
> +	{ "NXP0005", .driver_data = (kernel_ulong_t)&devtype_data[LS2085A], },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(acpi, fsl_dspi_acpi_ids);

Does NXP know about this ID assignment from their namespace?  ACPI IDs
should be namespaced by whoever's assigning the ID to avoid collisions.

> -		ret = of_property_read_u32(np, "spi-num-chipselects", &cs_num);
> +		if (is_acpi_node(pdev->dev.fwnode))
> +			ret = device_property_read_u32(&pdev->dev,
> +					"spi-num-chipselects", &cs_num);
> +		else
> +			ret = of_property_read_u32(np,
> +					"spi-num-chipselects", &cs_num);

The whole point with the device property API is that it works with both
DT and ACPI without needing separate parsing, though in this case I'm
wondering why we'd need to specify this in an ACPI system at all?

> -		of_property_read_u32(np, "bus-num", &bus_num);
> +		if (is_acpi_node(pdev->dev.fwnode)) {
> +			ret = device_property_read_u32(&pdev->dev,
> +							"bus-num", &bus_num);

This is a bad idea for DT and I can't understand why you'd carry it over
for ACPI - why would an ACPI system ever care about this?  It's Linux
internal at the best of times.

It looks like you've done this by simply adding these device property
alternatives for every DT property.  This isn't how that API is intended
to be used and suggests that this isn't a thought through, idiomatic
ACPI binding.  I'd suggest looking at the Synquacer driver for an
example of how this would normally be done, I'd expect your ACPI code to
look very much like theirs.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]