| Message ID | 1374794808-21890-2-git-send-email-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 26/07/13 09:26, Laurent Pinchart wrote: > Add support for passing CD/RO GPIO numbers directly to the mmc_spi > driver instead of relying solely on board code callbacks to retrieve the > CD/RO signals values. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/mmc/host/mmc_spi.c | 32 +++++++++++++++++++++--------- > drivers/mmc/host/of_mmc_spi.c | 46 +++++++++++-------------------------------- > include/linux/spi/mmc_spi.h | 11 +++++++++++ > 3 files changed, 46 insertions(+), 43 deletions(-) > > diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c > index 74145d1..4e83908 100644 > --- a/drivers/mmc/host/mmc_spi.c > +++ b/drivers/mmc/host/mmc_spi.c > @@ -36,6 +36,7 @@ > > #include <linux/mmc/host.h> > #include <linux/mmc/mmc.h> /* for R1_SPI_* bit values */ > +#include <linux/mmc/slot-gpio.h> > > #include <linux/spi/spi.h> > #include <linux/spi/mmc_spi.h> > @@ -1278,11 +1279,8 @@ static int mmc_spi_get_ro(struct mmc_host *mmc) > > if (host->pdata && host->pdata->get_ro) > return !!host->pdata->get_ro(mmc->parent); > - /* > - * Board doesn't support read only detection; let the mmc core > - * decide what to do. > - */ > - return -ENOSYS; > + else > + return mmc_gpio_get_ro(mmc); Why not just have the board file assign mmc_gpio_get_ro as host->pdata->get_ro? This would eliminate the need for the MMC_SPI_USE_RO_GPIO flag. Also, if host->pdata->get_ro is not set then this will assume mmc_gpio_get_ro is valid, even if MMC_SPI_USE_RO_GPIO is not set. I'm guessing it will end up returning -ENOSYS, but they way the code reads is that if the host doesn't have get_ro function set, then it is must be a gpio. Same applies for the other callbacks. ~Ryan
Hi Ryan, On Friday 26 July 2013 09:45:26 Ryan Mallon wrote: > On 26/07/13 09:26, Laurent Pinchart wrote: > > Add support for passing CD/RO GPIO numbers directly to the mmc_spi > > driver instead of relying solely on board code callbacks to retrieve the > > CD/RO signals values. > > > > Signed-off-by: Laurent Pinchart > > <laurent.pinchart+renesas@ideasonboard.com> > > --- > > > > drivers/mmc/host/mmc_spi.c | 32 +++++++++++++++++++++--------- > > drivers/mmc/host/of_mmc_spi.c | 46 ++++++++++---------------------------- > > include/linux/spi/mmc_spi.h | 11 +++++++++++ > > 3 files changed, 46 insertions(+), 43 deletions(-) > > > > diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c > > index 74145d1..4e83908 100644 > > --- a/drivers/mmc/host/mmc_spi.c > > +++ b/drivers/mmc/host/mmc_spi.c > > @@ -36,6 +36,7 @@ > > > > #include <linux/mmc/host.h> > > #include <linux/mmc/mmc.h> /* for R1_SPI_* bit values */ > > +#include <linux/mmc/slot-gpio.h> > > #include <linux/spi/spi.h> > > #include <linux/spi/mmc_spi.h> > > > > @@ -1278,11 +1279,8 @@ static int mmc_spi_get_ro(struct mmc_host *mmc) > > if (host->pdata && host->pdata->get_ro) > > return !!host->pdata->get_ro(mmc->parent); > > > > - /* > > - * Board doesn't support read only detection; let the mmc core > > - * decide what to do. > > - */ > > - return -ENOSYS; > > + else > > + return mmc_gpio_get_ro(mmc); > > Why not just have the board file assign mmc_gpio_get_ro as > host->pdata->get_ro? This would eliminate the need for the > MMC_SPI_USE_RO_GPIO flag. Because the idea is to get rid of board callbacks and rely on proper kernel abstraction layers such as the GPIO subsystem. This will be especially important when adding device tree support to the mmc_spi driver. > Also, if host->pdata->get_ro is not set then this will assume > mmc_gpio_get_ro is valid, even if MMC_SPI_USE_RO_GPIO is not set. I'm > guessing it will end up returning -ENOSYS, but they way the code reads is > that if the host doesn't have get_ro function set, then it is must be a > gpio. mmc_gpio_get_ro() will indeed return -ENOSYS when the MMC_SPI_USE_RO_GPIO flag isn't set, as the mmc_spi driver will not call mmc_gpio_request_ro() in that case. This patch hides the -ENOSYS value from the mmc_spi_get_ro() and mmc_spi_get_cd() functions, perhaps making them slightly confusing, but patch 06/12 then gets rid of those functions, so I don't think it's a bit issue. > Same applies for the other callbacks.
On 26/07/13 10:04, Laurent Pinchart wrote: > Hi Ryan, > > On Friday 26 July 2013 09:45:26 Ryan Mallon wrote: >> On 26/07/13 09:26, Laurent Pinchart wrote: >>> Add support for passing CD/RO GPIO numbers directly to the mmc_spi >>> driver instead of relying solely on board code callbacks to retrieve the >>> CD/RO signals values. >>> >>> Signed-off-by: Laurent Pinchart >>> <laurent.pinchart+renesas@ideasonboard.com> >>> --- >>> >>> drivers/mmc/host/mmc_spi.c | 32 +++++++++++++++++++++--------- >>> drivers/mmc/host/of_mmc_spi.c | 46 ++++++++++---------------------------- >>> include/linux/spi/mmc_spi.h | 11 +++++++++++ >>> 3 files changed, 46 insertions(+), 43 deletions(-) >>> >>> diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c >>> index 74145d1..4e83908 100644 >>> --- a/drivers/mmc/host/mmc_spi.c >>> +++ b/drivers/mmc/host/mmc_spi.c >>> @@ -36,6 +36,7 @@ >>> >>> #include <linux/mmc/host.h> >>> #include <linux/mmc/mmc.h> /* for R1_SPI_* bit values */ >>> +#include <linux/mmc/slot-gpio.h> >>> #include <linux/spi/spi.h> >>> #include <linux/spi/mmc_spi.h> >>> >>> @@ -1278,11 +1279,8 @@ static int mmc_spi_get_ro(struct mmc_host *mmc) >>> if (host->pdata && host->pdata->get_ro) >>> return !!host->pdata->get_ro(mmc->parent); >>> >>> - /* >>> - * Board doesn't support read only detection; let the mmc core >>> - * decide what to do. >>> - */ >>> - return -ENOSYS; >>> + else >>> + return mmc_gpio_get_ro(mmc); >> >> Why not just have the board file assign mmc_gpio_get_ro as >> host->pdata->get_ro? This would eliminate the need for the >> MMC_SPI_USE_RO_GPIO flag. > > Because the idea is to get rid of board callbacks and rely on proper kernel > abstraction layers such as the GPIO subsystem. This will be especially > important when adding device tree support to the mmc_spi driver. > >> Also, if host->pdata->get_ro is not set then this will assume >> mmc_gpio_get_ro is valid, even if MMC_SPI_USE_RO_GPIO is not set. I'm >> guessing it will end up returning -ENOSYS, but they way the code reads is >> that if the host doesn't have get_ro function set, then it is must be a >> gpio. > > mmc_gpio_get_ro() will indeed return -ENOSYS when the MMC_SPI_USE_RO_GPIO flag > isn't set, as the mmc_spi driver will not call mmc_gpio_request_ro() in that > case. > > This patch hides the -ENOSYS value from the mmc_spi_get_ro() and > mmc_spi_get_cd() functions, perhaps making them slightly confusing, but patch > 06/12 then gets rid of those functions, so I don't think it's a bit issue. Ah, I didn't read far enough along in the patch series :-). So all mmc spi ro/cd controls are now assumed to be gpios? ~Ryan
Hi Ryan, On Friday 26 July 2013 10:23:01 Ryan Mallon wrote: > On 26/07/13 10:04, Laurent Pinchart wrote: > > On Friday 26 July 2013 09:45:26 Ryan Mallon wrote: > >> On 26/07/13 09:26, Laurent Pinchart wrote: > >>> Add support for passing CD/RO GPIO numbers directly to the mmc_spi > >>> driver instead of relying solely on board code callbacks to retrieve the > >>> CD/RO signals values. > >>> > >>> Signed-off-by: Laurent Pinchart > >>> <laurent.pinchart+renesas@ideasonboard.com> > >>> --- > >>> > >>> drivers/mmc/host/mmc_spi.c | 32 +++++++++++++++++++++--------- > >>> drivers/mmc/host/of_mmc_spi.c | 46 ++++++++++-------------------------- > >>> include/linux/spi/mmc_spi.h | 11 +++++++++++ > >>> 3 files changed, 46 insertions(+), 43 deletions(-) > >>> > >>> diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c > >>> index 74145d1..4e83908 100644 > >>> --- a/drivers/mmc/host/mmc_spi.c > >>> +++ b/drivers/mmc/host/mmc_spi.c > >>> @@ -36,6 +36,7 @@ > >>> > >>> #include <linux/mmc/host.h> > >>> #include <linux/mmc/mmc.h> /* for R1_SPI_* bit values */ > >>> +#include <linux/mmc/slot-gpio.h> > >>> #include <linux/spi/spi.h> > >>> #include <linux/spi/mmc_spi.h> > >>> > >>> @@ -1278,11 +1279,8 @@ static int mmc_spi_get_ro(struct mmc_host *mmc) > >>> if (host->pdata && host->pdata->get_ro) > >>> return !!host->pdata->get_ro(mmc->parent); > >>> > >>> - /* > >>> - * Board doesn't support read only detection; let the mmc core > >>> - * decide what to do. > >>> - */ > >>> - return -ENOSYS; > >>> + else > >>> + return mmc_gpio_get_ro(mmc); > >> > >> Why not just have the board file assign mmc_gpio_get_ro as > >> host->pdata->get_ro? This would eliminate the need for the > >> MMC_SPI_USE_RO_GPIO flag. > > > > Because the idea is to get rid of board callbacks and rely on proper > > kernel abstraction layers such as the GPIO subsystem. This will be > > especially important when adding device tree support to the mmc_spi > > driver. > > > >> Also, if host->pdata->get_ro is not set then this will assume > >> mmc_gpio_get_ro is valid, even if MMC_SPI_USE_RO_GPIO is not set. I'm > >> guessing it will end up returning -ENOSYS, but they way the code reads is > >> that if the host doesn't have get_ro function set, then it is must be a > >> gpio. > > > > mmc_gpio_get_ro() will indeed return -ENOSYS when the MMC_SPI_USE_RO_GPIO > > flag isn't set, as the mmc_spi driver will not call mmc_gpio_request_ro() > > in that case. > > > > This patch hides the -ENOSYS value from the mmc_spi_get_ro() and > > mmc_spi_get_cd() functions, perhaps making them slightly confusing, but > > patch 06/12 then gets rid of those functions, so I don't think it's a bit > > issue. > > Ah, I didn't read far enough along in the patch series :-). So all mmc spi > ro/cd controls are now assumed to be gpios? That's correct. It's the case in practice for all the boards we support in mainline. If the situation changes in the future for a new board (hardware engineers tend to have strange design ideas sometimes) a custom GPIO driver might be needed (for FPGA/CPLD-controlled GPIOs for instance).
diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c index 74145d1..4e83908 100644 --- a/drivers/mmc/host/mmc_spi.c +++ b/drivers/mmc/host/mmc_spi.c @@ -36,6 +36,7 @@ #include <linux/mmc/host.h> #include <linux/mmc/mmc.h> /* for R1_SPI_* bit values */ +#include <linux/mmc/slot-gpio.h> #include <linux/spi/spi.h> #include <linux/spi/mmc_spi.h> @@ -1278,11 +1279,8 @@ static int mmc_spi_get_ro(struct mmc_host *mmc) if (host->pdata && host->pdata->get_ro) return !!host->pdata->get_ro(mmc->parent); - /* - * Board doesn't support read only detection; let the mmc core - * decide what to do. - */ - return -ENOSYS; + else + return mmc_gpio_get_ro(mmc); } static int mmc_spi_get_cd(struct mmc_host *mmc) @@ -1291,7 +1289,8 @@ static int mmc_spi_get_cd(struct mmc_host *mmc) if (host->pdata && host->pdata->get_cd) return !!host->pdata->get_cd(mmc->parent); - return -ENOSYS; + else + return mmc_gpio_get_cd(mmc); } static const struct mmc_host_ops mmc_spi_ops = { @@ -1324,6 +1323,7 @@ static int mmc_spi_probe(struct spi_device *spi) struct mmc_host *mmc; struct mmc_spi_host *host; int status; + bool has_ro = false; /* We rely on full duplex transfers, mostly to reduce * per-transfer overheads (by making fewer transfers). @@ -1448,18 +1448,32 @@ static int mmc_spi_probe(struct spi_device *spi) } /* pass platform capabilities, if any */ - if (host->pdata) + if (host->pdata) { mmc->caps |= host->pdata->caps; + mmc->caps2 |= host->pdata->caps2; + } status = mmc_add_host(mmc); if (status != 0) goto fail_add_host; + if (host->pdata && host->pdata->flags & MMC_SPI_USE_CD_GPIO) { + status = mmc_gpio_request_cd(mmc, host->pdata->cd_gpio); + if (status != 0) + goto fail_add_host; + } + + if (host->pdata && host->pdata->flags & MMC_SPI_USE_RO_GPIO) { + has_ro = true; + status = mmc_gpio_request_ro(mmc, host->pdata->ro_gpio); + if (status != 0) + goto fail_add_host; + } + dev_info(&spi->dev, "SD/MMC host %s%s%s%s%s\n", dev_name(&mmc->class_dev), host->dma_dev ? "" : ", no DMA", - (host->pdata && host->pdata->get_ro) - ? "" : ", no WP", + has_ro ? "" : ", no WP", (host->pdata && host->pdata->setpower) ? "" : ", no poweroff", (mmc->caps & MMC_CAP_NEEDS_POLL) diff --git a/drivers/mmc/host/of_mmc_spi.c b/drivers/mmc/host/of_mmc_spi.c index d720b5e..6e218fb 100644 --- a/drivers/mmc/host/of_mmc_spi.c +++ b/drivers/mmc/host/of_mmc_spi.c @@ -50,25 +50,6 @@ static struct of_mmc_spi *to_of_mmc_spi(struct device *dev) return container_of(dev->platform_data, struct of_mmc_spi, pdata); } -static int of_mmc_spi_read_gpio(struct device *dev, int gpio_num) -{ - struct of_mmc_spi *oms = to_of_mmc_spi(dev); - bool active_low = oms->alow_gpios[gpio_num]; - bool value = gpio_get_value(oms->gpios[gpio_num]); - - return active_low ^ value; -} - -static int of_mmc_spi_get_cd(struct device *dev) -{ - return of_mmc_spi_read_gpio(dev, CD_GPIO); -} - -static int of_mmc_spi_get_ro(struct device *dev) -{ - return of_mmc_spi_read_gpio(dev, WP_GPIO); -} - static int of_mmc_spi_init(struct device *dev, irqreturn_t (*irqhandler)(int, void *), void *mmc) { @@ -130,20 +111,22 @@ struct mmc_spi_platform_data *mmc_spi_get_pdata(struct spi_device *spi) if (!gpio_is_valid(oms->gpios[i])) continue; - ret = gpio_request(oms->gpios[i], dev_name(dev)); - if (ret < 0) { - oms->gpios[i] = -EINVAL; - continue; - } - if (gpio_flags & OF_GPIO_ACTIVE_LOW) oms->alow_gpios[i] = true; } - if (gpio_is_valid(oms->gpios[CD_GPIO])) - oms->pdata.get_cd = of_mmc_spi_get_cd; - if (gpio_is_valid(oms->gpios[WP_GPIO])) - oms->pdata.get_ro = of_mmc_spi_get_ro; + if (gpio_is_valid(oms->gpios[CD_GPIO])) { + oms->pdata.cd_gpio = oms->gpios[CD_GPIO]; + oms->pdata.flags |= MMC_SPI_USE_CD_GPIO; + if (!oms->alow_gpios[CD_GPIO]) + oms->pdata.caps2 |= MMC_CAP2_CD_ACTIVE_HIGH; + } + if (gpio_is_valid(oms->gpios[WP_GPIO])) { + oms->pdata.ro_gpio = oms->gpios[WP_GPIO]; + oms->pdata.flags |= MMC_SPI_USE_RO_GPIO; + if (!oms->alow_gpios[WP_GPIO]) + oms->pdata.caps2 |= MMC_CAP2_RO_ACTIVE_HIGH; + } oms->detect_irq = irq_of_parse_and_map(np, 0); if (oms->detect_irq != 0) { @@ -166,15 +149,10 @@ void mmc_spi_put_pdata(struct spi_device *spi) struct device *dev = &spi->dev; struct device_node *np = dev->of_node; struct of_mmc_spi *oms = to_of_mmc_spi(dev); - int i; if (!dev->platform_data || !np) return; - for (i = 0; i < ARRAY_SIZE(oms->gpios); i++) { - if (gpio_is_valid(oms->gpios[i])) - gpio_free(oms->gpios[i]); - } kfree(oms); dev->platform_data = NULL; } diff --git a/include/linux/spi/mmc_spi.h b/include/linux/spi/mmc_spi.h index 32be8db..a54da4a 100644 --- a/include/linux/spi/mmc_spi.h +++ b/include/linux/spi/mmc_spi.h @@ -7,6 +7,11 @@ struct device; struct mmc_host; +#define MMC_SPI_USE_CD_GPIO (1 << 0) +#define MMC_SPI_USE_RO_GPIO (1 << 1) +#define MMC_SPI_CD_GPIO_ACTIVE_LOW (1 << 2) +#define MMC_SPI_RO_GPIO_ACTIVE_LOW (1 << 3) + /* Put this in platform_data of a device being used to manage an MMC/SD * card slot. (Modeled after PXA mmc glue; see that for usage examples.) * @@ -30,8 +35,14 @@ struct mmc_spi_platform_data { */ int (*get_cd)(struct device *); + /* Card Detect and Read Only GPIOs */ + unsigned int flags; + unsigned int cd_gpio; + unsigned int ro_gpio; + /* Capabilities to pass into mmc core (e.g. MMC_CAP_NEEDS_POLL). */ unsigned long caps; + unsigned long caps2; /* how long to debounce card detect, in msecs */ u16 detect_delay;
Add support for passing CD/RO GPIO numbers directly to the mmc_spi driver instead of relying solely on board code callbacks to retrieve the CD/RO signals values. Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- drivers/mmc/host/mmc_spi.c | 32 +++++++++++++++++++++--------- drivers/mmc/host/of_mmc_spi.c | 46 +++++++++++-------------------------------- include/linux/spi/mmc_spi.h | 11 +++++++++++ 3 files changed, 46 insertions(+), 43 deletions(-)