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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 22 Aug 2020 19:37:25 +0530
From:   Kuldip Dwivedi <kuldip.dwivedi@...esoftware.com>
To:     Mark Brown <broonie@...nel.org>
Cc:     linux-spi@...r.kernel.org, linux-kernel@...r.kernel.org,
        Qiang Zhao <qiang.zhao@....com>,
        Pankaj Bansal <pankaj.bansal@....com>,
        Varun Sethi <V.Sethi@....com>,
        Tanveer Alam <tanveer.alam@...esoftware.com>
Subject: RE: [PATCH] spi: spi-fsl-dspi: Add ACPI support

> -----Original Message-----
> From: Mark Brown <broonie@...nel.org>
> Sent: Friday, August 21, 2020 7:37 PM
> To: kuldip dwivedi <kuldip.dwivedi@...esoftware.com>
> Cc: linux-spi@...r.kernel.org; linux-kernel@...r.kernel.org; Qiang Zhao
> <qiang.zhao@....com>; Pankaj Bansal <pankaj.bansal@....com>; Varun Sethi
> <V.Sethi@....com>; tanveer <tanveer.alam@...esoftware.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.