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

[Kuldip Dwivedi]

> -----Original Message-----
> From: Mark Brown <broonie@xxxxxxxxxx>
> Sent: Friday, August 21, 2020 7:37 PM
> To: kuldip dwivedi <kuldip.dwivedi@xxxxxxxxxxxxxxxx>
> Cc: linux-spi@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Qiang Zhao
> <qiang.zhao@xxxxxxx>; Pankaj Bansal <pankaj.bansal@xxxxxxx>; Varun Sethi
> <V.Sethi@xxxxxxx>; tanveer <tanveer.alam@xxxxxxxxxxxxxxxx>
> Subject: Re: [PATCH] spi: spi-fsl-dspi: Add ACPI support
>
> 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.
Yes, I got HID from NXP only.
>
> > -		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?
Understood. Will take care in v2 PATCH
>
> > -		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.
Will take care in v2 PATCH
>
> 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.