Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Kuldip Dwivedi <kuldip.dwivedi@puresoftware.com>
References: <20200821131029.11440-1-kuldip.dwivedi@puresoftware.com> <20200821140718.GH4870@sirena.org.uk>
In-Reply-To: <20200821140718.GH4870@sirena.org.uk>
MIME-Version: 1.0
Thread-Index: AQF7/ycK6cTpjLXBKD+ruXNZEs6GEAE4wSq+qe9WNoA=
Date:   Sat, 22 Aug 2020 19:37:25 +0530
Message-ID: <c810740d75f64e308fd362e6c6a5f437@mail.gmail.com>
Subject: RE: [PATCH] spi: spi-fsl-dspi: Add ACPI support
To:     Mark Brown <broonie@kernel.org>
Cc:     linux-spi@vger.kernel.org, linux-kernel@vger.kernel.org,
        Qiang Zhao <qiang.zhao@nxp.com>,
        Pankaj Bansal <pankaj.bansal@nxp.com>,
        Varun Sethi <V.Sethi@nxp.com>,
        Tanveer Alam <tanveer.alam@puresoftware.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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