> -----Original Message-----
> From: Mark Brown <broo...@kernel.org>
> Sent: Friday, August 21, 2020 7:37 PM
> To: kuldip dwivedi <kuldip.dwiv...@puresoftware.com>
> Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; Qiang Zhao
> <qiang.z...@nxp.com>; Pankaj Bansal <pankaj.ban...@nxp.com>; Varun Sethi
> <v.se...@nxp.com>; tanveer <tanveer.a...@puresoftware.com>
> 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.
