| Message ID | 20200929183431.93477-1-ghilliar@amazon.com |
|---|---|
| State | Superseded |
| Delegated to: | Stefan Roese |
| Headers | show |
| Series | [v2] spi: mvebu_a3700_spi: add support for cs-gpios | expand |
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(®->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(®->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, ®->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
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@denx.de George
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@denx.de > > George > Viele Grüße, Stefan
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(®->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(®->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, ®->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; }
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(-)