RE: [PATCH 2/2] spi: tegra210-quad: add acpi support

[Krishna Yarlagadda]

> -----Original Message-----
> From: Mark Brown <broonie@xxxxxxxxxx>
> Sent: 29 November 2021 17:57
> To: Krishna Yarlagadda <kyarlagadda@xxxxxxxxxx>; Philipp Zabel
> <p.zabel@xxxxxxxxxxxxxx>
> Cc: Laxman Dewangan <ldewangan@xxxxxxxxxx>; Thierry Reding
> <thierry.reding@xxxxxxxxx>; Jonathan Hunter <jonathanh@xxxxxxxxxx>;
> Sowjanya Komatineni <skomatineni@xxxxxxxxxx>; linux-
> tegra@xxxxxxxxxxxxxxx; linux-spi@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 2/2] spi: tegra210-quad: add acpi support
> 
> On Mon, Nov 29, 2021 at 09:09:30AM +0000, Krishna Yarlagadda wrote:
> 
> > > > +#ifdef CONFIG_ACPI
> > > > +	if (ACPI_FAILURE(acpi_evaluate_object(ACPI_HANDLE(tqspi->dev),
> > > > +					      "_RST", NULL, NULL)))
> > > > +		dev_err(tqspi->dev, "failed to reset device\n"); #endif
> 
> > > What happens when this runs on a DT system?
> 
> > For a DT system reset handle would be present and this code will not
> > run
> 
> This code is really unclearly structured, the early return doesn't match the
> normal kernel style and the ifdefs aren't helping with clarity.  Please
> restructure it so it's clear that *both* cases have checks for the correct
> firmware type being present.
I will move each reset method into their own function for readability.
> 
> That said frankly I'd expect this handling of ACPI reset to be pushed into the
> reset code, it's obviously not good to be open coding this in drivers when this
> looks like it's completely generic to any ACPI object so shouldn't be being
> open coded in individual driers especially with the ifdefery.  Shouldn't the
> reset API be able to figure out that an object with _RST has a reset control
> and provide access to it through the reset API?
Common reset apis are not handling _RST. Each driver is implementing
_RST method in ACPI and calling from drivers.