| Message ID | 1430106270-17142-1-git-send-email-mwelling@ieee.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Apr 26, 2015 at 10:44:30PM -0500, Michael Welling wrote: > + if (gpio_is_valid(spi->cs_gpio)) { > + gpio_set_value(spi->cs_gpio, (cs_active) ? > + !!(spi->mode & SPI_CS_HIGH) : > + !(spi->mode & SPI_CS_HIGH)); > + } Two problems here. One is that the above logic statement is just not readable (the repitition of hecks, the ternery operator, the indentation...) and the other is that the core chipselect support already handles GPIO chipselects so you should really be converting the driver to use that. At the very least the code needs to be legible though.
On Mon, Apr 27, 2015 at 08:55:50PM +0100, Mark Brown wrote: > On Sun, Apr 26, 2015 at 10:44:30PM -0500, Michael Welling wrote: > > > + if (gpio_is_valid(spi->cs_gpio)) { > > + gpio_set_value(spi->cs_gpio, (cs_active) ? > > + !!(spi->mode & SPI_CS_HIGH) : > > + !(spi->mode & SPI_CS_HIGH)); > > + } > > Two problems here. One is that the above logic statement is just not > readable (the repitition of hecks, the ternery operator, the > indentation...) and the other is that the core chipselect support > already handles GPIO chipselects so you should really be converting the > driver to use that. At the very least the code needs to be legible > though. Before I send another patch how does this look? if (gpio_is_valid(spi->cs_gpio)) { if (cs_active) gpio_set_value(spi->cs_gpio, spi->mode & SPI_CS_HIGH); else gpio_set_value(spi->cs_gpio, !(spi->mode & SPI_CS_HIGH)); } If I were to attempt to convert the driver to use the core chipselect support, how would I go about doing it? Is there another driver that I can use for reference? -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On 28.04.2015, at 03:21, Michael Welling <mwelling@ieee.org> wrote: > If I were to attempt to convert the driver to use the core chipselect support, > how would I go about doing it? > > Is there another driver that I can use for reference? You may look into this patch: e34ff011c70e5f4ef219141711142d5111ae6ebb for the spi-bcm2835 driver, which did the conversion to the new transfer_one interface (and framework based GPIO chipselects). For most parts all you have to do is take the contents of the loop over all the spi_transfers inside your master->transfer_one_message method and create a new method with it and assign it to master->transfer_one. -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/27/2015 10:21 PM, Michael Welling wrote: > On Mon, Apr 27, 2015 at 08:55:50PM +0100, Mark Brown wrote: >> On Sun, Apr 26, 2015 at 10:44:30PM -0500, Michael Welling wrote: >> >>> + if (gpio_is_valid(spi->cs_gpio)) { >>> + gpio_set_value(spi->cs_gpio, (cs_active) ? >>> + !!(spi->mode & SPI_CS_HIGH) : >>> + !(spi->mode & SPI_CS_HIGH)); >>> + } >> >> Two problems here. One is that the above logic statement is just not >> readable (the repitition of hecks, the ternery operator, the >> indentation...) and the other is that the core chipselect support >> already handles GPIO chipselects so you should really be converting the >> driver to use that. At the very least the code needs to be legible >> though. > > Before I send another patch how does this look? > > if (gpio_is_valid(spi->cs_gpio)) { > if (cs_active) > gpio_set_value(spi->cs_gpio, spi->mode & SPI_CS_HIGH); > else > gpio_set_value(spi->cs_gpio, !(spi->mode & SPI_CS_HIGH)); > } > > If I were to attempt to convert the driver to use the core chipselect support, > how would I go about doing it? > > Is there another driver that I can use for reference? We've recently done that for spi-img-spfi.c.
On Mon, Apr 27, 2015 at 08:21:50PM -0500, Michael Welling wrote: > Before I send another patch how does this look? > if (gpio_is_valid(spi->cs_gpio)) { > if (cs_active) > gpio_set_value(spi->cs_gpio, spi->mode & SPI_CS_HIGH); > else > gpio_set_value(spi->cs_gpio, !(spi->mode & SPI_CS_HIGH)); > } That's good but please have braces around the inner if for clarity. > If I were to attempt to convert the driver to use the core chipselect support, > how would I go about doing it? > Is there another driver that I can use for reference? Martin and Ezequiel already gave you some good examples - in general anything providing the set_cs() operation should be reasonable to look at.
On Tue, Apr 28, 2015 at 07:32:21AM +0200, Martin Sperl wrote: > > > On 28.04.2015, at 03:21, Michael Welling <mwelling@ieee.org> wrote: > > If I were to attempt to convert the driver to use the core chipselect support, > > how would I go about doing it? > > > > Is there another driver that I can use for reference? > You may look into this patch: e34ff011c70e5f4ef219141711142d5111ae6ebb > for the spi-bcm2835 driver, which did the conversion to the new transfer_one > interface (and framework based GPIO chipselects). I will take a look. Thanks. > > For most parts all you have to do is take the contents of the loop over all the > spi_transfers inside your master->transfer_one_message method and create a new > method with it and assign it to master->transfer_one. > -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 28, 2015 at 11:04:09AM -0300, Ezequiel Garcia wrote: > On 04/27/2015 10:21 PM, Michael Welling wrote: > > On Mon, Apr 27, 2015 at 08:55:50PM +0100, Mark Brown wrote: > >> On Sun, Apr 26, 2015 at 10:44:30PM -0500, Michael Welling wrote: > >> > >>> + if (gpio_is_valid(spi->cs_gpio)) { > >>> + gpio_set_value(spi->cs_gpio, (cs_active) ? > >>> + !!(spi->mode & SPI_CS_HIGH) : > >>> + !(spi->mode & SPI_CS_HIGH)); > >>> + } > >> > >> Two problems here. One is that the above logic statement is just not > >> readable (the repitition of hecks, the ternery operator, the > >> indentation...) and the other is that the core chipselect support > >> already handles GPIO chipselects so you should really be converting the > >> driver to use that. At the very least the code needs to be legible > >> though. > > > > Before I send another patch how does this look? > > > > if (gpio_is_valid(spi->cs_gpio)) { > > if (cs_active) > > gpio_set_value(spi->cs_gpio, spi->mode & SPI_CS_HIGH); > > else > > gpio_set_value(spi->cs_gpio, !(spi->mode & SPI_CS_HIGH)); > > } > > > > If I were to attempt to convert the driver to use the core chipselect support, > > how would I go about doing it? > > > > Is there another driver that I can use for reference? > > We've recently done that for spi-img-spfi.c. > Okay, thanks for pointing that out. > -- > Ezequiel -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 28, 2015 at 03:34:25PM +0100, Mark Brown wrote: > On Mon, Apr 27, 2015 at 08:21:50PM -0500, Michael Welling wrote: > > > Before I send another patch how does this look? > > > if (gpio_is_valid(spi->cs_gpio)) { > > if (cs_active) > > gpio_set_value(spi->cs_gpio, spi->mode & SPI_CS_HIGH); > > else > > gpio_set_value(spi->cs_gpio, !(spi->mode & SPI_CS_HIGH)); > > } > > That's good but please have braces around the inner if for clarity. I try to follow the CodingStyle documentation and it says differently: http://lxr.free-electrons.com/source/Documentation/CodingStyle#L156 > > > If I were to attempt to convert the driver to use the core chipselect support, > > how would I go about doing it? > > > Is there another driver that I can use for reference? > > Martin and Ezequiel already gave you some good examples - in general > anything providing the set_cs() operation should be reasonable to look > at. I was trying to minimise the impact of the change but will look into using this method. I have also been looking at ways of allowing more GPIO chipselects than DMA channels. Currently this driver is limited to one chipselect per DMA channel. Do you have any ideas on how to accomplish this? The number of chipselects on each channel is very restrictive on the mcspi. By using multiple GPIO chipselects per DMA channel it opens many more applications. -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 28, 2015 at 06:06:43PM -0500, Michael Welling wrote: > On Tue, Apr 28, 2015 at 03:34:25PM +0100, Mark Brown wrote: > > > if (gpio_is_valid(spi->cs_gpio)) { > > > if (cs_active) > > > gpio_set_value(spi->cs_gpio, spi->mode & SPI_CS_HIGH); > > > else > > > gpio_set_value(spi->cs_gpio, !(spi->mode & SPI_CS_HIGH)); > > > } > > That's good but please have braces around the inner if for clarity. > I try to follow the CodingStyle documentation and it says differently: > http://lxr.free-electrons.com/source/Documentation/CodingStyle#L156 That's for free standing if statements, the reason I'm asking for braces at both levels here is that you've got a nested if - it's similar to having braces on both branches of an if, being consistent within the same statement. > I have also been looking at ways of allowing more GPIO chipselects than > DMA channels. Currently this driver is limited to one chipselect per > DMA channel. > Do you have any ideas on how to accomplish this? Just use a single channel for the controller rather than allocating them per device? It's not like the controller is going to be able to interact with multiple devics simultaneously.
diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c index d1a5b9f..5e388a8 100644 --- a/drivers/spi/spi-omap2-mcspi.c +++ b/drivers/spi/spi-omap2-mcspi.c @@ -35,6 +35,7 @@ #include <linux/gcd.h> #include <linux/spi/spi.h> +#include <linux/gpio.h> #include <linux/platform_data/spi-omap2-mcspi.h> @@ -246,6 +247,12 @@ static void omap2_mcspi_force_cs(struct spi_device *spi, int cs_active) { u32 l; + if (gpio_is_valid(spi->cs_gpio)) { + gpio_set_value(spi->cs_gpio, (cs_active) ? + !!(spi->mode & SPI_CS_HIGH) : + !(spi->mode & SPI_CS_HIGH)); + } + l = mcspi_cached_chconf0(spi); if (cs_active) l |= OMAP2_MCSPI_CHCONF_FORCE; @@ -1015,6 +1022,12 @@ static int omap2_mcspi_setup(struct spi_device *spi) if (ret < 0) return ret; + if (gpio_is_valid(spi->cs_gpio)) { + if (gpio_request(spi->cs_gpio, dev_name(&spi->dev)) == 0) + gpio_direction_output(spi->cs_gpio, + !(spi->mode & SPI_CS_HIGH)); + } + ret = omap2_mcspi_setup_transfer(spi, NULL); pm_runtime_mark_last_busy(mcspi->dev); pm_runtime_put_autosuspend(mcspi->dev);
This patch allows for GPIOs specified in the devicetree to be used as SPI chipselects on TI OMAP2 SoCs. Tested on the AM3354. Signed-off-by: Michael Welling <mwelling@ieee.org> --- v2: Considers the possible use of SPI_CS_HIGH during chip select activation. drivers/spi/spi-omap2-mcspi.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)