[PATCH 1/2] spi: tegra210-quad: use devm call for cdata memory

[PATCH 1/2] spi: tegra210-quad: use devm call for cdata memory

[Krishna Yarlagadda]

Use devm alloc call to allocate memory for spi controller data and
remove free calls from cleanup.

Signed-off-by: Krishna Yarlagadda <kyarlagadda@nvidia.com>
---
 drivers/spi/spi-tegra210-quad.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/spi/spi-tegra210-quad.c b/drivers/spi/spi-tegra210-quad.c
index c0f9a75..ce1bdb4 100644
--- a/drivers/spi/spi-tegra210-quad.c
+++ b/drivers/spi/spi-tegra210-quad.c
@@ -877,7 +877,7 @@ static struct tegra_qspi_client_data *tegra_qspi_parse_cdata_dt(struct spi_devic
 	struct tegra_qspi_client_data *cdata;
 	struct device_node *slave_np = spi->dev.of_node;
 
-	cdata = kzalloc(sizeof(*cdata), GFP_KERNEL);
+	cdata = devm_kzalloc(&spi->dev, sizeof(*cdata), GFP_KERNEL);
 	if (!cdata)
 		return NULL;
 
@@ -888,14 +888,6 @@ static struct tegra_qspi_client_data *tegra_qspi_parse_cdata_dt(struct spi_devic
 	return cdata;
 }
 
-static void tegra_qspi_cleanup(struct spi_device *spi)
-{
-	struct tegra_qspi_client_data *cdata = spi->controller_data;
-
-	spi->controller_data = NULL;
-	kfree(cdata);
-}
-
 static int tegra_qspi_setup(struct spi_device *spi)
 {
 	struct tegra_qspi *tqspi = spi_master_get_devdata(spi->master);
@@ -1229,7 +1221,6 @@ static int tegra_qspi_probe(struct platform_device *pdev)
 			    SPI_TX_DUAL | SPI_RX_DUAL | SPI_TX_QUAD | SPI_RX_QUAD;
 	master->bits_per_word_mask = SPI_BPW_MASK(32) | SPI_BPW_MASK(16) | SPI_BPW_MASK(8);
 	master->setup = tegra_qspi_setup;
-	master->cleanup = tegra_qspi_cleanup;
 	master->transfer_one_message = tegra_qspi_transfer_one_message;
 	master->num_chipselect = 1;
 	master->auto_runtime_pm = true;
-- 
2.7.4

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

[Krishna Yarlagadda]

Support ACPI device enumeration for Tegra QUAD SPI driver.
Also pick device features from device tree.

Signed-off-by: Krishna Yarlagadda <kyarlagadda@nvidia.com>
---
 drivers/spi/spi-tegra210-quad.c | 74 ++++++++++++++++++++++++++++-------------
 1 file changed, 51 insertions(+), 23 deletions(-)

diff --git a/drivers/spi/spi-tegra210-quad.c b/drivers/spi/spi-tegra210-quad.c
index ce1bdb4..20c1fa6 100644
--- a/drivers/spi/spi-tegra210-quad.c
+++ b/drivers/spi/spi-tegra210-quad.c
@@ -21,6 +21,8 @@
 #include <linux/of_device.h>
 #include <linux/reset.h>
 #include <linux/spi/spi.h>
+#include <linux/acpi.h>
+#include <linux/property.h>
 
 #define QSPI_COMMAND1				0x000
 #define QSPI_BIT_LENGTH(x)			(((x) & 0x1f) << 0)
@@ -767,7 +769,7 @@ static u32 tegra_qspi_setup_transfer_one(struct spi_device *spi, struct spi_tran
 	u32 tx_tap = 0, rx_tap = 0;
 	int req_mode;
 
-	if (speed != tqspi->cur_speed) {
+	if (!has_acpi_companion(tqspi->dev) && speed != tqspi->cur_speed) {
 		clk_set_rate(tqspi->clk, speed);
 		tqspi->cur_speed = speed;
 	}
@@ -875,16 +877,15 @@ static int tegra_qspi_start_transfer_one(struct spi_device *spi,
 static struct tegra_qspi_client_data *tegra_qspi_parse_cdata_dt(struct spi_device *spi)
 {
 	struct tegra_qspi_client_data *cdata;
-	struct device_node *slave_np = spi->dev.of_node;
 
 	cdata = devm_kzalloc(&spi->dev, sizeof(*cdata), GFP_KERNEL);
 	if (!cdata)
 		return NULL;
 
-	of_property_read_u32(slave_np, "nvidia,tx-clk-tap-delay",
-			     &cdata->tx_clk_tap_delay);
-	of_property_read_u32(slave_np, "nvidia,rx-clk-tap-delay",
-			     &cdata->rx_clk_tap_delay);
+	device_property_read_u32(&spi->dev, "nvidia,tx-clk-tap-delay",
+				 &cdata->tx_clk_tap_delay);
+	device_property_read_u32(&spi->dev, "nvidia,rx-clk-tap-delay",
+				 &cdata->rx_clk_tap_delay);
 	return cdata;
 }
 
@@ -943,14 +944,27 @@ static void tegra_qspi_dump_regs(struct tegra_qspi *tqspi)
 		tegra_qspi_readl(tqspi, QSPI_FIFO_STATUS));
 }
 
+static void tegra_qspi_reset(struct tegra_qspi *tqspi)
+{
+	if (tqspi->rst) {
+		reset_control_assert(tqspi->rst);
+		udelay(2);
+		reset_control_deassert(tqspi->rst);
+		return;
+	}
+#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
+}
+
 static void tegra_qspi_handle_error(struct tegra_qspi *tqspi)
 {
 	dev_err(tqspi->dev, "error in transfer, fifo status 0x%08x\n", tqspi->status_reg);
 	tegra_qspi_dump_regs(tqspi);
 	tegra_qspi_flush_fifos(tqspi, true);
-	reset_control_assert(tqspi->rst);
-	udelay(2);
-	reset_control_deassert(tqspi->rst);
+	tegra_qspi_reset(tqspi);
 }
 
 static void tegra_qspi_transfer_end(struct spi_device *spi)
@@ -1202,6 +1216,14 @@ static const struct of_device_id tegra_qspi_of_match[] = {
 
 MODULE_DEVICE_TABLE(of, tegra_qspi_of_match);
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id tegra_qspi_acpi_match[] = {
+	{ .id = "NVDA14E2", },
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, tegra_qspi_acpi_match);
+#endif
+
 static int tegra_qspi_probe(struct platform_device *pdev)
 {
 	struct spi_master	*master;
@@ -1242,18 +1264,20 @@ static int tegra_qspi_probe(struct platform_device *pdev)
 	qspi_irq = platform_get_irq(pdev, 0);
 	tqspi->irq = qspi_irq;
 
-	tqspi->clk = devm_clk_get(&pdev->dev, "qspi");
-	if (IS_ERR(tqspi->clk)) {
-		ret = PTR_ERR(tqspi->clk);
-		dev_err(&pdev->dev, "failed to get clock: %d\n", ret);
-		return ret;
-	}
+	if (!has_acpi_companion(tqspi->dev)) {
+		tqspi->clk = devm_clk_get(&pdev->dev, "qspi");
+		if (IS_ERR(tqspi->clk)) {
+			ret = PTR_ERR(tqspi->clk);
+			dev_err(&pdev->dev, "failed to get clock: %d\n", ret);
+			return ret;
+		}
 
-	tqspi->rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
-	if (IS_ERR(tqspi->rst)) {
-		ret = PTR_ERR(tqspi->rst);
-		dev_err(&pdev->dev, "failed to get reset control: %d\n", ret);
-		return ret;
+		tqspi->rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
+		if (IS_ERR(tqspi->rst)) {
+			dev_err(&pdev->dev, "failed to get reset control: %d\n", ret);
+			ret = PTR_ERR(tqspi->rst);
+			return ret;
+		}
 	}
 
 	tqspi->max_buf_size = QSPI_FIFO_DEPTH << 2;
@@ -1277,9 +1301,7 @@ static int tegra_qspi_probe(struct platform_device *pdev)
 		goto exit_pm_disable;
 	}
 
-	reset_control_assert(tqspi->rst);
-	udelay(2);
-	reset_control_deassert(tqspi->rst);
+	tegra_qspi_reset(tqspi);
 
 	tqspi->def_command1_reg = QSPI_M_S | QSPI_CS_SW_HW |  QSPI_CS_SW_VAL;
 	tegra_qspi_writel(tqspi, tqspi->def_command1_reg, QSPI_COMMAND1);
@@ -1353,6 +1375,10 @@ static int __maybe_unused tegra_qspi_resume(struct device *dev)
 	return spi_master_resume(master);
 }
 
+#ifdef CONFIG_ACPI
+static int __maybe_unused tegra_qspi_runtime_suspend(struct device *dev) { return 0; }
+static int __maybe_unused tegra_qspi_runtime_resume(struct device *dev) { return 0; }
+#else
 static int __maybe_unused tegra_qspi_runtime_suspend(struct device *dev)
 {
 	struct spi_master *master = dev_get_drvdata(dev);
@@ -1378,6 +1404,7 @@ static int __maybe_unused tegra_qspi_runtime_resume(struct device *dev)
 
 	return ret;
 }
+#endif
 
 static const struct dev_pm_ops tegra_qspi_pm_ops = {
 	SET_RUNTIME_PM_OPS(tegra_qspi_runtime_suspend, tegra_qspi_runtime_resume, NULL)
@@ -1389,6 +1416,7 @@ static struct platform_driver tegra_qspi_driver = {
 		.name		= "tegra-qspi",
 		.pm		= &tegra_qspi_pm_ops,
 		.of_match_table	= tegra_qspi_of_match,
+		.acpi_match_table = ACPI_PTR(tegra_qspi_acpi_match),
 	},
 	.probe =	tegra_qspi_probe,
 	.remove =	tegra_qspi_remove,
-- 
2.7.4

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 303 bytes --]

On Thu, Nov 25, 2021 at 03:25:52PM +0530, 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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: (subset) [PATCH 1/2] spi: tegra210-quad: use devm call for cdata memory

[Mark Brown]

On Thu, 25 Nov 2021 15:25:51 +0530, Krishna Yarlagadda wrote:
> Use devm alloc call to allocate memory for spi controller data and
> remove free calls from cleanup.
> 
> 

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/2] spi: tegra210-quad: use devm call for cdata memory
      commit: f89d2cc3967af9948ffc58e4cc9a1331f1c4971a

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

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

[Krishna Yarlagadda]

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: 25 November 2021 18:29
> To: Krishna Yarlagadda <kyarlagadda@nvidia.com>
> Cc: Laxman Dewangan <ldewangan@nvidia.com>; Thierry Reding
> <thierry.reding@gmail.com>; Jonathan Hunter <jonathanh@nvidia.com>;
> Sowjanya Komatineni <skomatineni@nvidia.com>; Philipp Zabel
> <p.zabel@pengutronix.de>; linux-tegra@vger.kernel.org; linux-
> spi@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2] spi: tegra210-quad: add acpi support
> 
> On Thu, Nov 25, 2021 at 03:25:52PM +0530, 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
-KY

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1088 bytes --]

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.

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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Krishna Yarlagadda]

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: 29 November 2021 17:57
> To: Krishna Yarlagadda <kyarlagadda@nvidia.com>; Philipp Zabel
> <p.zabel@pengutronix.de>
> Cc: Laxman Dewangan <ldewangan@nvidia.com>; Thierry Reding
> <thierry.reding@gmail.com>; Jonathan Hunter <jonathanh@nvidia.com>;
> Sowjanya Komatineni <skomatineni@nvidia.com>; linux-
> tegra@vger.kernel.org; linux-spi@vger.kernel.org; linux-
> kernel@vger.kernel.org
> 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.

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 766 bytes --]

On Tue, Nov 30, 2021 at 01:50:07AM +0000, Krishna Yarlagadda wrote:

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

I can see that.  What I'm saying is that this seems bad and we should
instead be implementing this in common code.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Dmitry Osipenko]

30.11.2021 16:05, Mark Brown пишет:
> On Tue, Nov 30, 2021 at 01:50:07AM +0000, Krishna Yarlagadda wrote:
> 
>>> 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.

I see only two or three other drivers in kernel which do that.

> I can see that.  What I'm saying is that this seems bad and we should
> instead be implementing this in common code.
> 

The ifdefery, that this patch has, shouldn't be needed. We have a
similar ACPI patch for Tegra I2C [1] and it doesn't have that.

[1]
https://patchwork.ozlabs.org/project/linux-tegra/patch/1637859224-5179-1-git-send-email-akhilrajeev@nvidia.com/

I assume this patch could be reworked similarly to the I2C patch.

Agree that should be better to have a common reset driver for ACPI
instead of polluting each driver with a boilerplate code.

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 781 bytes --]

On Tue, Nov 30, 2021 at 05:14:07PM +0300, Dmitry Osipenko wrote:

> The ifdefery, that this patch has, shouldn't be needed. We have a
> similar ACPI patch for Tegra I2C [1] and it doesn't have that.

> [1]
> https://patchwork.ozlabs.org/project/linux-tegra/patch/1637859224-5179-1-git-send-email-akhilrajeev@nvidia.com/

> I assume this patch could be reworked similarly to the I2C patch.

Yes, that looks much cleaner.

> Agree that should be better to have a common reset driver for ACPI
> instead of polluting each driver with a boilerplate code.

Right, I think that'd be even better.  It's probably best to split
adding reset support to the driver out from adding the ACPI ID - they
shouldn't really have been merged in the first place TBH - and then try
this approach first.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]