All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] spi: mvebu_a3700_spi: add support for cs-gpios
@ 2020-09-29 18:34 George Hilliard
  2020-09-30  5:20 ` Stefan Roese
  0 siblings, 1 reply; 4+ messages in thread
From: George Hilliard @ 2020-09-29 18:34 UTC (permalink / raw)
  To: u-boot

The device tree has a way to specify GPIO lines as chip selects.  From
the binding docs:

    So if for example the controller has 2 CS lines, and the cs-gpios
    property looks like this:

    cs-gpios = <&gpio1 0 0> <0> <&gpio1 1 0> <&gpio1 2 0>;

    Then it should be configured so that num_chipselect = 4 with the
    following mapping:

    cs0 : &gpio1 0 0
    cs1 : native
    cs2 : &gpio1 1 0
    cs3 : &gpio1 2 0

Add support for this, while retaining backward-compatibility with
existing device trees; the driver will preserve existing behavior if a
cs-gpios list is not given, or if a particular line is specified as <0>
(native).

This implementation is inspired by similar implementations in
neighboring drivers for other platforms: atmega, mxc, etc.

Signed-off-by: George Hilliard <ghilliar@amazon.com>
---
 drivers/spi/mvebu_a3700_spi.c | 40 +++++++++++++++++++++++++++++------
 1 file changed, 34 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/mvebu_a3700_spi.c b/drivers/spi/mvebu_a3700_spi.c
index e860b9ec64..b82ed2ce7a 100644
--- a/drivers/spi/mvebu_a3700_spi.c
+++ b/drivers/spi/mvebu_a3700_spi.c
@@ -15,6 +15,7 @@
 #include <asm/io.h>
 #include <dm/device_compat.h>
 #include <linux/bitops.h>
+#include <asm/gpio.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -27,6 +28,7 @@ DECLARE_GLOBAL_DATA_PTR;
 #define MVEBU_SPI_A3700_SPI_EN_0		BIT(16)
 #define MVEBU_SPI_A3700_CLK_PRESCALE_MASK	0x1f
 
+#define MAX_CS_COUNT	4
 
 /* SPI registers */
 struct spi_reg {
@@ -39,16 +41,23 @@ struct spi_reg {
 struct mvebu_spi_platdata {
 	struct spi_reg *spireg;
 	struct clk clk;
+	struct gpio_desc cs_gpios[MAX_CS_COUNT];
 };
 
-static void spi_cs_activate(struct spi_reg *reg, int cs)
+static void spi_cs_activate(struct mvebu_spi_platdata *plat, int cs)
 {
-	setbits_le32(&reg->ctrl, MVEBU_SPI_A3700_SPI_EN_0 << cs);
+	if (CONFIG_IS_ENABLED(DM_GPIO) && dm_gpio_is_valid(&plat->cs_gpios[cs]))
+		dm_gpio_set_value(&plat->cs_gpios[cs], 1);
+	else
+		setbits_le32(&plat->spireg->ctrl, MVEBU_SPI_A3700_SPI_EN_0 << cs);
 }
 
-static void spi_cs_deactivate(struct spi_reg *reg, int cs)
+static void spi_cs_deactivate(struct mvebu_spi_platdata *plat, int cs)
 {
-	clrbits_le32(&reg->ctrl, MVEBU_SPI_A3700_SPI_EN_0 << cs);
+	if (CONFIG_IS_ENABLED(DM_GPIO) && dm_gpio_is_valid(&plat->cs_gpios[cs]))
+		dm_gpio_set_value(&plat->cs_gpios[cs], 0);
+	else
+		clrbits_le32(&plat->spireg->ctrl, MVEBU_SPI_A3700_SPI_EN_0 << cs);
 }
 
 /**
@@ -150,7 +159,7 @@ static int mvebu_spi_xfer(struct udevice *dev, unsigned int bitlen,
 	/* Activate CS */
 	if (flags & SPI_XFER_BEGIN) {
 		debug("SPI: activate cs.\n");
-		spi_cs_activate(reg, spi_chip_select(dev));
+		spi_cs_activate(plat, spi_chip_select(dev));
 	}
 
 	/* Send and/or receive */
@@ -169,7 +178,7 @@ static int mvebu_spi_xfer(struct udevice *dev, unsigned int bitlen,
 			return ret;
 
 		debug("SPI: deactivate cs.\n");
-		spi_cs_deactivate(reg, spi_chip_select(dev));
+		spi_cs_deactivate(plat, spi_chip_select(dev));
 	}
 
 	return 0;
@@ -247,6 +256,25 @@ static int mvebu_spi_probe(struct udevice *bus)
 
 	writel(data, &reg->cfg);
 
+	/* Set up CS GPIOs in device tree, if any */
+	if (CONFIG_IS_ENABLED(DM_GPIO) && gpio_get_list_count(bus, "cs-gpios") > 0) {
+		int i;
+
+		for (i = 0; i < ARRAY_SIZE(plat->cs_gpios); i++) {
+			ret = gpio_request_by_name(bus, "cs-gpios", i, &plat->cs_gpios[i], 0);
+			if (ret < 0 || !dm_gpio_is_valid(&plat->cs_gpios[i]))
+				// Use the native CS function for this line
+				continue;
+
+			ret = dm_gpio_set_dir_flags(&plat->cs_gpios[i],
+						    GPIOD_IS_OUT | GPIOD_ACTIVE_LOW);
+			if (ret) {
+				dev_err(bus, "Setting cs %d error\n", i);
+				return ret;
+			}
+		}
+	}
+
 	return 0;
 }
 
-- 
2.23.3

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH v2] spi: mvebu_a3700_spi: add support for cs-gpios
  2020-09-29 18:34 [PATCH v2] spi: mvebu_a3700_spi: add support for cs-gpios George Hilliard
@ 2020-09-30  5:20 ` Stefan Roese
  2020-09-30 14:25   ` Hilliard, George
  0 siblings, 1 reply; 4+ messages in thread
From: Stefan Roese @ 2020-09-30  5:20 UTC (permalink / raw)
  To: u-boot

Hi George,

thanks for the new version. One nitpicking comment below...

On 29.09.20 20:34, George Hilliard wrote:
> The device tree has a way to specify GPIO lines as chip selects.  From
> the binding docs:
> 
>      So if for example the controller has 2 CS lines, and the cs-gpios
>      property looks like this:
> 
>      cs-gpios = <&gpio1 0 0> <0> <&gpio1 1 0> <&gpio1 2 0>;
> 
>      Then it should be configured so that num_chipselect = 4 with the
>      following mapping:
> 
>      cs0 : &gpio1 0 0
>      cs1 : native
>      cs2 : &gpio1 1 0
>      cs3 : &gpio1 2 0
> 
> Add support for this, while retaining backward-compatibility with
> existing device trees; the driver will preserve existing behavior if a
> cs-gpios list is not given, or if a particular line is specified as <0>
> (native).
> 
> This implementation is inspired by similar implementations in
> neighboring drivers for other platforms: atmega, mxc, etc.
> 
> Signed-off-by: George Hilliard <ghilliar@amazon.com>
> ---
>   drivers/spi/mvebu_a3700_spi.c | 40 +++++++++++++++++++++++++++++------
>   1 file changed, 34 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/spi/mvebu_a3700_spi.c b/drivers/spi/mvebu_a3700_spi.c
> index e860b9ec64..b82ed2ce7a 100644
> --- a/drivers/spi/mvebu_a3700_spi.c
> +++ b/drivers/spi/mvebu_a3700_spi.c
> @@ -15,6 +15,7 @@
>   #include <asm/io.h>
>   #include <dm/device_compat.h>
>   #include <linux/bitops.h>
> +#include <asm/gpio.h>
>   
>   DECLARE_GLOBAL_DATA_PTR;
>   
> @@ -27,6 +28,7 @@ DECLARE_GLOBAL_DATA_PTR;
>   #define MVEBU_SPI_A3700_SPI_EN_0		BIT(16)
>   #define MVEBU_SPI_A3700_CLK_PRESCALE_MASK	0x1f
>   
> +#define MAX_CS_COUNT	4
>   
>   /* SPI registers */
>   struct spi_reg {
> @@ -39,16 +41,23 @@ struct spi_reg {
>   struct mvebu_spi_platdata {
>   	struct spi_reg *spireg;
>   	struct clk clk;
> +	struct gpio_desc cs_gpios[MAX_CS_COUNT];
>   };
>   
> -static void spi_cs_activate(struct spi_reg *reg, int cs)
> +static void spi_cs_activate(struct mvebu_spi_platdata *plat, int cs)
>   {
> -	setbits_le32(&reg->ctrl, MVEBU_SPI_A3700_SPI_EN_0 << cs);
> +	if (CONFIG_IS_ENABLED(DM_GPIO) && dm_gpio_is_valid(&plat->cs_gpios[cs]))
> +		dm_gpio_set_value(&plat->cs_gpios[cs], 1);
> +	else
> +		setbits_le32(&plat->spireg->ctrl, MVEBU_SPI_A3700_SPI_EN_0 << cs);
>   }
>   
> -static void spi_cs_deactivate(struct spi_reg *reg, int cs)
> +static void spi_cs_deactivate(struct mvebu_spi_platdata *plat, int cs)
>   {
> -	clrbits_le32(&reg->ctrl, MVEBU_SPI_A3700_SPI_EN_0 << cs);
> +	if (CONFIG_IS_ENABLED(DM_GPIO) && dm_gpio_is_valid(&plat->cs_gpios[cs]))
> +		dm_gpio_set_value(&plat->cs_gpios[cs], 0);
> +	else
> +		clrbits_le32(&plat->spireg->ctrl, MVEBU_SPI_A3700_SPI_EN_0 << cs);
>   }
>   
>   /**
> @@ -150,7 +159,7 @@ static int mvebu_spi_xfer(struct udevice *dev, unsigned int bitlen,
>   	/* Activate CS */
>   	if (flags & SPI_XFER_BEGIN) {
>   		debug("SPI: activate cs.\n");
> -		spi_cs_activate(reg, spi_chip_select(dev));
> +		spi_cs_activate(plat, spi_chip_select(dev));
>   	}
>   
>   	/* Send and/or receive */
> @@ -169,7 +178,7 @@ static int mvebu_spi_xfer(struct udevice *dev, unsigned int bitlen,
>   			return ret;
>   
>   		debug("SPI: deactivate cs.\n");
> -		spi_cs_deactivate(reg, spi_chip_select(dev));
> +		spi_cs_deactivate(plat, spi_chip_select(dev));
>   	}
>   
>   	return 0;
> @@ -247,6 +256,25 @@ static int mvebu_spi_probe(struct udevice *bus)
>   
>   	writel(data, &reg->cfg);
>   
> +	/* Set up CS GPIOs in device tree, if any */
> +	if (CONFIG_IS_ENABLED(DM_GPIO) && gpio_get_list_count(bus, "cs-gpios") > 0) {
> +		int i;
> +
> +		for (i = 0; i < ARRAY_SIZE(plat->cs_gpios); i++) {
> +			ret = gpio_request_by_name(bus, "cs-gpios", i, &plat->cs_gpios[i], 0);
> +			if (ret < 0 || !dm_gpio_is_valid(&plat->cs_gpios[i]))
> +				// Use the native CS function for this line
> +				continue;

Why do you introduce here C++ style comments? Even though they are
not banned any more (AFAIK), I see no reason to add multiple
comment styles in such a small patch. Additionally this is a multi-
line statement after the "if" and it needs parenthesis.

Other than that, please feel free to add my:

Reviewed-by: Stefan Roese <sr@denx.de>

to the next version.

Thanks,
Stefan

> +
> +			ret = dm_gpio_set_dir_flags(&plat->cs_gpios[i],
> +						    GPIOD_IS_OUT | GPIOD_ACTIVE_LOW);
> +			if (ret) {
> +				dev_err(bus, "Setting cs %d error\n", i);
> +				return ret;
> +			}
> +		}
> +	}
> +
>   	return 0;
>   }
>   
> 


Viele Gr??e,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH v2] spi: mvebu_a3700_spi: add support for cs-gpios
  2020-09-30  5:20 ` Stefan Roese
@ 2020-09-30 14:25   ` Hilliard, George
  2020-09-30 15:35     ` Stefan Roese
  0 siblings, 1 reply; 4+ messages in thread
From: Hilliard, George @ 2020-09-30 14:25 UTC (permalink / raw)
  To: u-boot

On Sep 30, 2020, at 12:20 AM, Stefan Roese <sr@denx.de> wrote:
> 
> Hi George,
> 
> thanks for the new version. One nitpicking comment below...
> 
> On 29.09.20 20:34, George Hilliard wrote:
>> 
>> +     /* Set up CS GPIOs in device tree, if any */
>> +     if (CONFIG_IS_ENABLED(DM_GPIO) && gpio_get_list_count(bus, "cs-gpios") > 0) {
>> +             int i;
>> +
>> +             for (i = 0; i < ARRAY_SIZE(plat->cs_gpios); i++) {
>> +                     ret = gpio_request_by_name(bus, "cs-gpios", i, &plat->cs_gpios[i], 0);
>> +                     if (ret < 0 || !dm_gpio_is_valid(&plat->cs_gpios[i]))
>> +                             // Use the native CS function for this line
>> +                             continue;
> 
> Why do you introduce here C++ style comments? Even though they are
> not banned any more (AFAIK), I see no reason to add multiple
> comment styles in such a small patch. Additionally this is a multi-
> line statement after the "if" and it needs parenthesis.

Both good points. The C++ style is by force of habit alone, no real reason for it here.

I suppose "multi-line" means multiple lines of text, not multiple C statements.

> 
> Other than that, please feel free to add my:
> 
> Reviewed-by: Stefan Roese <sr@denx.de>
> 
> to the next version.

Your review is much appreciated!

> 
> Thanks,
> Stefan
> 
> Viele Gr??e,
> Stefan
> 
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de

George

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH v2] spi: mvebu_a3700_spi: add support for cs-gpios
  2020-09-30 14:25   ` Hilliard, George
@ 2020-09-30 15:35     ` Stefan Roese
  0 siblings, 0 replies; 4+ messages in thread
From: Stefan Roese @ 2020-09-30 15:35 UTC (permalink / raw)
  To: u-boot

On 30.09.20 16:25, Hilliard, George wrote:
> On Sep 30, 2020, at 12:20 AM, Stefan Roese <sr@denx.de> wrote:
>>
>> Hi George,
>>
>> thanks for the new version. One nitpicking comment below...
>>
>> On 29.09.20 20:34, George Hilliard wrote:
>>>
>>> +     /* Set up CS GPIOs in device tree, if any */
>>> +     if (CONFIG_IS_ENABLED(DM_GPIO) && gpio_get_list_count(bus, "cs-gpios") > 0) {
>>> +             int i;
>>> +
>>> +             for (i = 0; i < ARRAY_SIZE(plat->cs_gpios); i++) {
>>> +                     ret = gpio_request_by_name(bus, "cs-gpios", i, &plat->cs_gpios[i], 0);
>>> +                     if (ret < 0 || !dm_gpio_is_valid(&plat->cs_gpios[i]))
>>> +                             // Use the native CS function for this line
>>> +                             continue;
>>
>> Why do you introduce here C++ style comments? Even though they are
>> not banned any more (AFAIK), I see no reason to add multiple
>> comment styles in such a small patch. Additionally this is a multi-
>> line statement after the "if" and it needs parenthesis.
> 
> Both good points. The C++ style is by force of habit alone, no real reason for it here.
> 
> I suppose "multi-line" means multiple lines of text, not multiple C statements.

Yes. AFAIU, it's just related to the number of lines following an
"if" (etc) statement. Which makes perfect sense IMHO. ;)

>> Other than that, please feel free to add my:
>>
>> Reviewed-by: Stefan Roese <sr@denx.de>
>>
>> to the next version.
> 
> Your review is much appreciated!

Thanks.

I do have another comment, for the next patch(es) you might send:

Please add a patch history to your patches when sending updated
versions. Please see here:

http://www.denx.de/wiki/view/U-Boot/Patches#Sending_updated_patch_versions

No need to resend this v3 patch though.

Thanks,
Stefan

>>
>> Thanks,
>> Stefan
>>
>> Viele Gr??e,
>> Stefan
>>
>> --
>> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de
> 
> George
> 


Viele Gr??e,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-09-30 15:35 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-29 18:34 [PATCH v2] spi: mvebu_a3700_spi: add support for cs-gpios George Hilliard
2020-09-30  5:20 ` Stefan Roese
2020-09-30 14:25   ` Hilliard, George
2020-09-30 15:35     ` Stefan Roese

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.