[RFC PATCH] SPI/ACPI: DesignWare: Add ACPI support for Designware SPI driver

[RFC PATCH] SPI/ACPI: DesignWare: Add ACPI support for Designware SPI driver

[qiujiang]

This patch added ACPI support for DesignWare SPI mmio driver. It
was based the corresponding DT driver and compatible for this two
way. This patch has been tested on Hisilicon D02 board. It relies
on the GPIO patchset.

Signed-off-by: qiujiang <qiujiang@huawei.com>
---
 drivers/spi/spi-dw-mmio.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
index a6d7029..de542f0 100644
--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c
@@ -8,6 +8,7 @@
  * version 2, as published by the Free Software Foundation.
  */
 
+#include <linux/acpi.h>
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
@@ -36,7 +37,9 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
 	struct dw_spi *dws;
 	struct resource *mem;
 	int ret;
-	int num_cs;
+	int num_cs, i;
+	struct gpio_desc *gpiod;
+	char propname[32];
 
 	dwsmmio = devm_kzalloc(&pdev->dev, sizeof(struct dw_spi_mmio),
 			GFP_KERNEL);
@@ -84,8 +87,6 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
 	dws->num_cs = num_cs;
 
 	if (pdev->dev.of_node) {
-		int i;
-
 		for (i = 0; i < dws->num_cs; i++) {
 			int cs_gpio = of_get_named_gpio(pdev->dev.of_node,
 					"cs-gpios", i);
@@ -104,6 +105,18 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
 		}
 	}
 
+	if (ACPI_COMPANION(&pdev->dev)) {
+		for (i = 0; i < dws->num_cs; i++) {
+			snprintf(propname, sizeof(propname), "cs%d", i);
+			gpiod = devm_gpiod_get(&pdev->dev,
+				propname, GPIOD_ASIS);
+			if (IS_ERR(gpiod)) {
+				dev_err(&pdev->dev, "Get gpio desc failed!\n");
+				return PTR_ERR(gpiod);
+			}
+		}
+	}
+
 	ret = dw_spi_add_host(&pdev->dev, dws);
 	if (ret)
 		goto out;
@@ -132,12 +145,19 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, dw_spi_mmio_of_match);
 
+static const struct acpi_device_id dw_spi_mmio_acpi_match[] = {
+		{"HISI0171", 0},
+		{ }
+};
+MODULE_DEVICE_TABLE(acpi, dw_spi_mmio_acpi_match);
+
 static struct platform_driver dw_spi_mmio_driver = {
 	.probe		= dw_spi_mmio_probe,
 	.remove		= dw_spi_mmio_remove,
 	.driver		= {
 		.name	= DRIVER_NAME,
 		.of_match_table = dw_spi_mmio_of_match,
+		.acpi_match_table = ACPI_PTR(dw_spi_mmio_acpi_match),
 	},
 };
 module_platform_driver(dw_spi_mmio_driver);
-- 
1.9.1

Re: [RFC PATCH] SPI/ACPI: DesignWare: Add ACPI support for Designware SPI driver

[Mark Brown]

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

On Fri, Feb 05, 2016 at 03:11:20PM +0800, qiujiang wrote:

> This patch added ACPI support for DesignWare SPI mmio driver. It
> was based the corresponding DT driver and compatible for this two
> way. This patch has been tested on Hisilicon D02 board. It relies
> on the GPIO patchset.

Intel are heavy users of this driver on their systems which also use
ACPI.  Have you discussed this binding with them?  I've copied Andy and
Jarkko who've worked on the driver recently.

Please use subject lines matching the style for the subsystem.  This
makes it easier for people to identify relevant patches.

> +	char propname[32];

That's a magic number, where did it come from and why is it a magic
nummber?

> +	if (ACPI_COMPANION(&pdev->dev)) {
> +		for (i = 0; i < dws->num_cs; i++) {
> +			snprintf(propname, sizeof(propname), "cs%d", i);
> +			gpiod = devm_gpiod_get(&pdev->dev,
> +				propname, GPIOD_ASIS);
> +			if (IS_ERR(gpiod)) {
> +				dev_err(&pdev->dev, "Get gpio desc failed!\n");
> +				return PTR_ERR(gpiod);
> +			}
> +		}
> +	}

I'm not seeing anywhere where we store the GPIO in this loop.  It is
therefore unclear to me how the chip select is going to work?

> +static const struct acpi_device_id dw_spi_mmio_acpi_match[] = {
> +		{"HISI0171", 0},
> +		{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, dw_spi_mmio_acpi_match);

I really do wish ACPI had some more sensible system for allocating
device IDs so the tables were a little more legible. :(

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

Re: [RFC PATCH] SPI/ACPI: DesignWare: Add ACPI support for Designware SPI driver

[Jiang Qiu]

Hi Mark,

Many thanks for your review, I'm so sorry for late reply because The 
Chinese
new year holiday. See my replies below.

Best Regards
Jiang

在 2016/2/5 19:09, Mark Brown 写道:
> On Fri, Feb 05, 2016 at 03:11:20PM +0800, qiujiang wrote:
>
>> This patch added ACPI support for DesignWare SPI mmio driver. It
>> was based the corresponding DT driver and compatible for this two
>> way. This patch has been tested on Hisilicon D02 board. It relies
>> on the GPIO patchset.
> Intel are heavy users of this driver on their systems which also use
> ACPI.  Have you discussed this binding with them?  I've copied Andy and
> Jarkko who've worked on the driver recently.
  I'm going to ask Andy to get some ideas that how to use this spi-dw-mmio
  driver by ACPI binding.
>
> Please use subject lines matching the style for the subsystem.  This
> makes it easier for people to identify relevant patches.
Thanks for the reminder, I will fix it in the next version.
>
>> +	char propname[32];
> That's a magic number, where did it come from and why is it a magic
> nummber?
I'm sorry for here without any comments. This number define is come from
gpiolib.c. It means the max size of gpio property name. The reference code
located in line 1815 of gpiolib.c.
>> +	if (ACPI_COMPANION(&pdev->dev)) {
>> +		for (i = 0; i < dws->num_cs; i++) {
>> +			snprintf(propname, sizeof(propname), "cs%d", i);
>> +			gpiod = devm_gpiod_get(&pdev->dev,
>> +				propname, GPIOD_ASIS);
>> +			if (IS_ERR(gpiod)) {
>> +				dev_err(&pdev->dev, "Get gpio desc failed!\n");
>> +				return PTR_ERR(gpiod);
>> +			}
>> +		}
>> +	}
> I'm not seeing anywhere where we store the GPIO in this loop.  It is
> therefore unclear to me how the chip select is going to work?
In DT binding, of_get_named_gpio and devm_gpio_request were used to 
parse gpio
pins defined in DTs and then request these pins. Similarly, for ACPI, 
devm_gpiod_get
can do that two operation in a single function. It is a unified 
interface to ACPI and DT
binding.

If the gpiod is valid, the corresponding gpio pins has been requested. 
We do not need
to save this gpiod any more.

which gpio pin was used is defined in spi_device, named cs_gpio, the 
configuration to the
gpio pins will be done in the setup callback routine of each device. 
What the spi master
should do is just request these pins to the gpio subsystem.
>> +static const struct acpi_device_id dw_spi_mmio_acpi_match[] = {
>> +		{"HISI0171", 0},
>> +		{ }
>> +};
>> +MODULE_DEVICE_TABLE(acpi, dw_spi_mmio_acpi_match);
> I really do wish ACPI had some more sensible system for allocating
> device IDs so the tables were a little more legible. :(
This is really a question, I will do this feedback to ACPI maintainers.


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] SPI/ACPI: DesignWare: Add ACPI support for Designware SPI driver

[Mark Brown]

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

On Sun, Feb 14, 2016 at 05:31:14PM +0800, Jiang Qiu wrote:
> 在 2016/2/5 19:09, Mark Brown 写道:
> >On Fri, Feb 05, 2016 at 03:11:20PM +0800, qiujiang wrote:

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.  Your mail has lines wrapped alternately at
normal length and half length which is difficult to read, I've reflowed
this time.

> >>+	char propname[32];

> >That's a magic number, where did it come from and why is it a magic
> >nummber?

> I'm sorry for here without any comments. This number define is come from
> gpiolib.c. It means the max size of gpio property name. The reference code
> located in line 1815 of gpiolib.c.

That really isn't helping here.  The problem is that you've got a random
magic number thrown in here with no documentation.

> >I'm not seeing anywhere where we store the GPIO in this loop.  It is
> >therefore unclear to me how the chip select is going to work?

> In DT binding, of_get_named_gpio and devm_gpio_request were used to
> parse gpio pins defined in DTs and then request these pins. Similarly,
> for ACPI, devm_gpiod_get can do that two operation in a single
> function. It is a unified interface to ACPI and DT binding.

> If the gpiod is valid, the corresponding gpio pins has been requested.
> We do not need to save this gpiod any more.

No, that makes no sense at all.  If we're requesting a GPIO to use as
chip select we can't just then ignore the GPIO, we need to control it at
some point and...

> which gpio pin was used is defined in spi_device, named cs_gpio, the
> configuration to the gpio pins will be done in the setup callback
> routine of each device. What the spi master should do is just request
> these pins to the gpio subsystem.

...this would be a complete failure of abstraction - you appear to be
suggesting that the client devices should also separately request the
GPIOs and set them up.  That's at best going to be redundant and is
likely to introduce bugs.

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

Re: [RFC PATCH] SPI/ACPI: DesignWare: Add ACPI support for Designware SPI driver

[Hanjun Guo]

On 2016/2/5 19:09, Mark Brown wrote:
> On Fri, Feb 05, 2016 at 03:11:20PM +0800, qiujiang wrote:
>
>> This patch added ACPI support for DesignWare SPI mmio driver. It
>> was based the corresponding DT driver and compatible for this two
>> way. This patch has been tested on Hisilicon D02 board. It relies
>> on the GPIO patchset.
> Intel are heavy users of this driver on their systems which also use
> ACPI.  Have you discussed this binding with them?  I've copied Andy and
> Jarkko who've worked on the driver recently.

There is no ACPI code in this driver yet, seems "PRP0001" is used to reuse
this driver directly.

[...]
>
>> +static const struct acpi_device_id dw_spi_mmio_acpi_match[] = {
>> +		{"HISI0171", 0},
>> +		{ }
>> +};
>> +MODULE_DEVICE_TABLE(acpi, dw_spi_mmio_acpi_match);
> I really do wish ACPI had some more sensible system for allocating
> device IDs so the tables were a little more legible. :(

Hmm, I agree. but for now, every OEM can request ACPI or PNP ID with specific
prefixes, and they can manage it by themselves, that would flexible for OEM verdors
to develop firmware and driver.

For this driver in particular, if DesignWare provides the ACPI or PNP ID, that would
be great for all OEMs can reuse it.

Thanks
Hanjun


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] SPI/ACPI: DesignWare: Add ACPI support for Designware SPI driver

[Mark Brown]

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

On Mon, Feb 15, 2016 at 04:27:51PM +0800, Hanjun Guo wrote:
> On 2016/2/5 19:09, Mark Brown wrote:

> > Intel are heavy users of this driver on their systems which also use
> > ACPI.  Have you discussed this binding with them?  I've copied Andy and
> > Jarkko who've worked on the driver recently.

> There is no ACPI code in this driver yet, seems "PRP0001" is used to reuse
> this driver directly.

No, on Intel systems the devices appear on the PCI bus.

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

Re: [RFC PATCH] SPI/ACPI: DesignWare: Add ACPI support for Designware SPI driver

[One Thousand Gnomes]

On Mon, 15 Feb 2016 17:45:40 +0000
Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> On Mon, Feb 15, 2016 at 04:27:51PM +0800, Hanjun Guo wrote:
> > On 2016/2/5 19:09, Mark Brown wrote:  
> 
> > > Intel are heavy users of this driver on their systems which also use
> > > ACPI.  Have you discussed this binding with them?  I've copied Andy and
> > > Jarkko who've worked on the driver recently.  
> 
> > There is no ACPI code in this driver yet, seems "PRP0001" is used to reuse
> > this driver directly.  
> 
> No, on Intel systems the devices appear on the PCI bus.

They may be ACPI or PCI enumerated - depends upon the system and
firmware. See drivers/acpi/acpi_lpss.c


Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] SPI/ACPI: DesignWare: Add ACPI support for Designware SPI driver

[Andy Shevchenko]

On Fri, Feb 5, 2016 at 9:11 AM, qiujiang <qiujiang@huawei.com> wrote:
> This patch added ACPI support for DesignWare SPI mmio driver. It
> was based the corresponding DT driver and compatible for this two
> way. This patch has been tested on Hisilicon D02 board. It relies
> on the GPIO patchset.

My comments below.

> @@ -84,8 +87,6 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
>         dws->num_cs = num_cs;
>
>         if (pdev->dev.of_node) {
> -               int i;
> -
>                 for (i = 0; i < dws->num_cs; i++) {
>                         int cs_gpio = of_get_named_gpio(pdev->dev.of_node,
>                                         "cs-gpios", i);

It seems the driver was never validated with more than one chip select.
Perhaps someone has to switch to use of_spi_register_master() here.

> @@ -104,6 +105,18 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
>                 }
>         }
>
> +       if (ACPI_COMPANION(&pdev->dev)) {
> +               for (i = 0; i < dws->num_cs; i++) {
> +                       snprintf(propname, sizeof(propname), "cs%d", i);
> +                       gpiod = devm_gpiod_get(&pdev->dev,
> +                               propname, GPIOD_ASIS);
> +                       if (IS_ERR(gpiod)) {
> +                               dev_err(&pdev->dev, "Get gpio desc failed!\n");
> +                               return PTR_ERR(gpiod);
> +                       }
> +               }
> +       }

Like Mark noticed there is also same issue. Do you indeed check the
configuration with different chip select signals?

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH] SPI/ACPI: DesignWare: Add ACPI support for Designware SPI driver

[Mark Brown]

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

On Fri, Feb 05, 2016 at 05:55:42PM +0200, Andy Shevchenko wrote:

> Like Mark noticed there is also same issue. Do you indeed check the
> configuration with different chip select signals?

It's not clear to me that *any* chip select signals work, we get a GPIO
descriptor back but I didn't see any code that ever used the value.

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

Re: [RFC PATCH] SPI/ACPI: DesignWare: Add ACPI support for Designware SPI driver

[Jiang Qiu]

Hi Andy,

Sorry for late relpy because Chinese new year holiday. See my replies below.


Best Regards
Jiang

在 2016/2/5 23:55, Andy Shevchenko 写道:
> On Fri, Feb 5, 2016 at 9:11 AM, qiujiang <qiujiang@huawei.com> wrote:
>> This patch added ACPI support for DesignWare SPI mmio driver. It
>> was based the corresponding DT driver and compatible for this two
>> way. This patch has been tested on Hisilicon D02 board. It relies
>> on the GPIO patchset.
> My comments below.
As Mark mentioned, I want to ask you how to use this spi-dw-mmio driver for
ACPI binding? Dose it need any other extra patchset?
>
>> @@ -84,8 +87,6 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
>>          dws->num_cs = num_cs;
>>
>>          if (pdev->dev.of_node) {
>> -               int i;
>> -
>>                  for (i = 0; i < dws->num_cs; i++) {
>>                          int cs_gpio = of_get_named_gpio(pdev->dev.of_node,
>>                                          "cs-gpios", i);
> It seems the driver was never validated with more than one chip select.
> Perhaps someone has to switch to use of_spi_register_master() here.
of_spi_register_master() will be executed in the spi_register_master(), 
but it just saved the
cs_gpios to the spi_master and not used it any more.
>> @@ -104,6 +105,18 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
>>                  }
>>          }
>>
>> +       if (ACPI_COMPANION(&pdev->dev)) {
>> +               for (i = 0; i < dws->num_cs; i++) {
>> +                       snprintf(propname, sizeof(propname), "cs%d", i);
>> +                       gpiod = devm_gpiod_get(&pdev->dev,
>> +                               propname, GPIOD_ASIS);
>> +                       if (IS_ERR(gpiod)) {
>> +                               dev_err(&pdev->dev, "Get gpio desc failed!\n");
>> +                               return PTR_ERR(gpiod);
>> +                       }
>> +               }
>> +       }
> Like Mark noticed there is also same issue. Do you indeed check the
> configuration with different chip select signals?
As a spi master driver, it seems that multi-chip select must be 
supported, so this check is
necessary, I think.
>