Patchwork [3/3] spi: add ACPI support for SPI controller chip select lines(cs-gpios)

login
register
mail settings
Submitter Jay Fang
Date Dec. 3, 2018, 3:15 a.m.
Message ID <1543806951-61848-4-git-send-email-f.fangjian@huawei.com>
Download mbox | patch
Permalink /patch/670121/
State New
Headers show

Comments

Jay Fang - Dec. 3, 2018, 3:15 a.m.
This will also allow to use cs-gpios for chip select in ACPI code
with no modification in the driver binding, like it be used in DT.
We could share almost all of the code with the DT path.

Signed-off-by: Jay Fang <f.fangjian@huawei.com>
---
 drivers/spi/spi.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)
Mark Brown - Dec. 4, 2018, 4:33 p.m.
On Mon, Dec 03, 2018 at 11:15:51AM +0800, Jay Fang wrote:

> -static int of_spi_register_master(struct spi_controller *ctlr)
> -{
> +	if (IS_ENABLED(CONFIG_OF) && np) {
> +		for (i = 0; i < nb; i++)
> +			cs[i] = of_get_named_gpio(np, "cs-gpios", i);
> +	} else if (IS_ENABLED(CONFIG_ACPI) && ACPI_HANDLE(&ctlr->dev)) {
> +		for (i = 0; i < nb; i++) {
> +			desc = devm_gpiod_get_index(&ctlr->dev, "cs",
> +						    i, GPIOD_ASIS);
> +			if (IS_ERR(desc))
> +				continue;
> +			cs[i] = desc_to_gpio(desc);
> +		}
> +	}
>  	return 0;

Given that both properties are called "cs" won't devm_gpiod_get_index()
do the right thing for DT systems as well as ACPI allowing us to move
entirely to descriptors and remove the need for separate code paths here?
Linus Walleij - Dec. 5, 2018, 10:28 a.m.
On Tue, Dec 4, 2018 at 5:33 PM Mark Brown <broonie@kernel.org> wrote:
> On Mon, Dec 03, 2018 at 11:15:51AM +0800, Jay Fang wrote:
>
> > -static int of_spi_register_master(struct spi_controller *ctlr)
> > -{
> > +     if (IS_ENABLED(CONFIG_OF) && np) {
> > +             for (i = 0; i < nb; i++)
> > +                     cs[i] = of_get_named_gpio(np, "cs-gpios", i);
> > +     } else if (IS_ENABLED(CONFIG_ACPI) && ACPI_HANDLE(&ctlr->dev)) {
> > +             for (i = 0; i < nb; i++) {
> > +                     desc = devm_gpiod_get_index(&ctlr->dev, "cs",
> > +                                                 i, GPIOD_ASIS);
> > +                     if (IS_ERR(desc))
> > +                             continue;
> > +                     cs[i] = desc_to_gpio(desc);
> > +             }
> > +     }
> >       return 0;
>
> Given that both properties are called "cs" won't devm_gpiod_get_index()
> do the right thing for DT systems as well as ACPI allowing us to move
> entirely to descriptors and remove the need for separate code paths here?

Indeed moving the cs-gpios over to just using descriptors is on my
TODO list and I have a rough patch cooking:
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/commit/?h=gpio-descriptors-spi&id=963bff1e8feb020cac05325db84a37f81efdb0ea

I think maybe I should expediate this patch[set] so there is a clean
path forward for ACPI without having to do this:

> +                     cs[i] = desc_to_gpio(desc);

Which is essentially the waltz of two steps forward, one step back,
*shudder*

I'll try to get my patch into shape ASAP.

Yours,
Linus Walleij
Jay Fang - Dec. 6, 2018, 1:49 a.m.
Thank you Linus Walleij. And we will await your patchset.
Please cc to linuxarm.

Thank you,
Jay

On 2018/12/5 18:28, Linus Walleij wrote:
> On Tue, Dec 4, 2018 at 5:33 PM Mark Brown <broonie@kernel.org> wrote:
>> On Mon, Dec 03, 2018 at 11:15:51AM +0800, Jay Fang wrote:
>>
>>> -static int of_spi_register_master(struct spi_controller *ctlr)
>>> -{
>>> +     if (IS_ENABLED(CONFIG_OF) && np) {
>>> +             for (i = 0; i < nb; i++)
>>> +                     cs[i] = of_get_named_gpio(np, "cs-gpios", i);
>>> +     } else if (IS_ENABLED(CONFIG_ACPI) && ACPI_HANDLE(&ctlr->dev)) {
>>> +             for (i = 0; i < nb; i++) {
>>> +                     desc = devm_gpiod_get_index(&ctlr->dev, "cs",
>>> +                                                 i, GPIOD_ASIS);
>>> +                     if (IS_ERR(desc))
>>> +                             continue;
>>> +                     cs[i] = desc_to_gpio(desc);
>>> +             }
>>> +     }
>>>       return 0;
>>
>> Given that both properties are called "cs" won't devm_gpiod_get_index()
>> do the right thing for DT systems as well as ACPI allowing us to move
>> entirely to descriptors and remove the need for separate code paths here?
> 
> Indeed moving the cs-gpios over to just using descriptors is on my
> TODO list and I have a rough patch cooking:
> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/commit/?h=gpio-descriptors-spi&id=963bff1e8feb020cac05325db84a37f81efdb0ea
> 
> I think maybe I should expediate this patch[set] so there is a clean
> path forward for ACPI without having to do this:
> 
>> +                     cs[i] = desc_to_gpio(desc);
> 
> Which is essentially the waltz of two steps forward, one step back,
> *shudder*
> 
> I'll try to get my patch into shape ASAP.
> 
> Yours,
> Linus Walleij
> 
> .
>

Patch

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 6ca5940..81d404a 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2075,16 +2075,13 @@  struct spi_controller *__spi_alloc_controller(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(__spi_alloc_controller);
 
-#ifdef CONFIG_OF
-static int of_spi_register_master(struct spi_controller *ctlr)
+static int __spi_register_controller(struct spi_controller *ctlr)
 {
 	int nb, i, *cs;
 	struct device_node *np = ctlr->dev.of_node;
+	struct gpio_desc *desc;
 
-	if (!np)
-		return 0;
-
-	nb = of_gpio_named_count(np, "cs-gpios");
+	nb = gpiod_count(&ctlr->dev, "cs");
 	ctlr->num_chipselect = max_t(int, nb, ctlr->num_chipselect);
 
 	/* Return error only for an incorrectly formed cs-gpios property */
@@ -2103,17 +2100,20 @@  static int of_spi_register_master(struct spi_controller *ctlr)
 	for (i = 0; i < ctlr->num_chipselect; i++)
 		cs[i] = -ENOENT;
 
-	for (i = 0; i < nb; i++)
-		cs[i] = of_get_named_gpio(np, "cs-gpios", i);
-
-	return 0;
-}
-#else
-static int of_spi_register_master(struct spi_controller *ctlr)
-{
+	if (IS_ENABLED(CONFIG_OF) && np) {
+		for (i = 0; i < nb; i++)
+			cs[i] = of_get_named_gpio(np, "cs-gpios", i);
+	} else if (IS_ENABLED(CONFIG_ACPI) && ACPI_HANDLE(&ctlr->dev)) {
+		for (i = 0; i < nb; i++) {
+			desc = devm_gpiod_get_index(&ctlr->dev, "cs",
+						    i, GPIOD_ASIS);
+			if (IS_ERR(desc))
+				continue;
+			cs[i] = desc_to_gpio(desc);
+		}
+	}
 	return 0;
 }
-#endif
 
 static int spi_controller_check_ops(struct spi_controller *ctlr)
 {
@@ -2177,7 +2177,7 @@  int spi_register_controller(struct spi_controller *ctlr)
 		return status;
 
 	if (!spi_controller_is_slave(ctlr)) {
-		status = of_spi_register_master(ctlr);
+		status = __spi_register_controller(ctlr);
 		if (status)
 			return status;
 	}