RE: [PATCH v1] spi: spi-nxp-fspi: Add ACPI support

[Kuldip Dwivedi <kuldip.dwivedi@xxxxxxxxxxxxxxxx>]

> -----Original Message-----
> From: Mark Brown <broonie@xxxxxxxxxx>
> Sent: Wednesday, September 9, 2020 5:32 PM
> To: kuldip dwivedi <kuldip.dwivedi@xxxxxxxxxxxxxxxx>
> Cc: Ashish Kumar <ashish.kumar@xxxxxxx>; Yogesh Gaur
> <yogeshgaur.83@xxxxxxxxx>; linux-spi@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Varun Sethi <V.Sethi@xxxxxxx>; Arokia Samy
> <arokia.samy@xxxxxxx>
> Subject: Re: [PATCH v1] spi: spi-nxp-fspi: Add ACPI support
>
> On Tue, Sep 08, 2020 at 11:32:27AM +0530, kuldip dwivedi wrote:
>
> This appears to be v2 not v1?
This is separate Patch so v1 should be OK here. Earlier one was related to
DSPI.
https://lore.kernel.org/linux-spi/20200827113216.GA4674@xxxxxxxxxxxxx/T/#t

>
> > Currently NXP fspi  driver has support of DT only. Adding ACPI support
> > to the driver so that it can be used by UEFI firmware booting in ACPI
> > mode. This driver will be probed if any firmware will expose HID
> > "NXP0009" in DSDT table.
>
> As I said on your previous version:
>
> | 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, NXP is aware.
>
> Please don't ignore review comments, people are generally making them
for a
> reason and are likely to have the same concerns if issues remain
unaddressed.
> Having to repeat the same comments can get repetitive and make people
question
> the value of time spent reviewing.  If you disagree with the review
comments
> that's fine but you need to reply and discuss your concerns so that the
reviewer
> can understand your decisions.
This is new Patch for different IP (FSPI)  and scenario is different from
DSPI driver.
>
> > @@ -900,6 +910,8 @@ static int nxp_fspi_default_setup(struct nxp_fspi
*f)
> >  		return ret;
> >
> >  	/* Reset the module */
> > +	fspi_writel(f, FSPI_MCR0_SWRST, (base + FSPI_MCR0));
> > +
> >  	/* w1c register, wait unit clear */
> >  	ret = fspi_readl_poll_tout(f, f->iobase + FSPI_MCR0,
> >  				   FSPI_MCR0_SWRST, 0, POLL_TOUT, false);
>
> Why are you adding this reset?  How is it connected to adding ACPI
support - it
> looks like it should be a separate patch.
I observed a kernel panic in setting up the driver, and this fixed the
issue.