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;
Date:   Sat, 22 Aug 2020 18:21:18 +0300
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Kuldip Dwivedi <kuldip.dwivedi@puresoftware.com>
Cc:     Mark Brown <broonie@kernel.org>, 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>
Subject: Re: [PATCH] spi: spi-fsl-dspi: Add ACPI support
Message-ID: <20200822152118.rlwbcgfk4abjldtg@skbuf>
References: <20200821131029.11440-1-kuldip.dwivedi@puresoftware.com>
 <20200821140718.GH4870@sirena.org.uk>
 <c810740d75f64e308fd362e6c6a5f437@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c810740d75f64e308fd362e6c6a5f437@mail.gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, Aug 22, 2020 at 07:37:25PM +0530, Kuldip Dwivedi wrote:
> > -----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
> >

IMO there is zero reason for the existence of the "spi-num-chipselects"
property even for DT. We should deprecate it (start ignoring it in
existing device tree deployments) and populate struct
fsl_dspi_devtype_data with that info based on SoC compatible string.

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

Yes, definitely bloatware from the old days. I think this driver needs
the existing device tree bindings rethought a little bit before
mindlessly porting them to ACPI.

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

Speaking of which, on what SPI peripherals was this tested?
I am not sure how other controllers deal with this, but DSPI has, by
default, no CS-to-SCK and a SCK-to-CS delays. Those must be explicitly
requested through the custom "fsl,spi-cs-sck-delay" and
"fsl,spi-sck-cs-delay" DT bindings for each individual SPI peripheral.
Some peripherals just don't work when the CS-to-SCK and SCK-to-CS delays
are zero, and I don't see the ACPI variant of the driver attempting to
read those properties, hence the question.

Thanks,
-Vladimir