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]

Message-ID: <20200821140718.GH4870@sirena.org.uk>
Date:   Fri, 21 Aug 2020 15:07:19 +0100
From:   Mark Brown <broonie@...nel.org>
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.

> -		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.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)