Re: [PATCH 1/2] Add OF binding helpers for MMC drivers

2011-04-22 Thread Domenico Andreoli
On Thu, Apr 21, 2011 at 4:14 PM, Grant Likely grant.lik...@secretlab.ca wrote:
 On Thu, Apr 21, 2011 at 03:19:57PM +0200, Domenico Andreoli wrote:
 From: Domenico Andreoli cav...@gmail.com

 This patch adds helpers to manage OF binding of MMC DeviceTree configs.
 They don't cover all the MMC configuration cases, indeed are only a
 slight generalization of those found in the MMC-over-SPI driver. More
 will come later.

 Can the mmc-over-spi driver be generalized to use this code too?

I made a mess with the patches... the second one of this set _is_ mmc-over-spi
converted to this generalization. Instead I sent GPIO OF stuff... sorry.

 +/* Card detect and Read only Gpio data */
 +struct of_mmc_crg {
 +     int gpios[NUM_MMC_GPIOS];
 +     int alow_gpios[NUM_MMC_GPIOS];
 +     int cd_irq;
 +};

 Many drivers currently provide this data via a platform_data.  It
 would probably be more useful and require less code if both DT and
 non-DT users shared the same structure for MMC gpios.  The structure
 could both be embedded in pdata and populated by this DT parsing code.
 It would also mean that each driver wouldn't need separate DT and
 non-DT code for consuming the information.

I agree, indeed I'm not able to use these helpers within the s3c-mmc driver
without duplicating and writing ugly code.

I made some research so I have few patches of tries which made me learn
a bit. There is not a common management of these as well as of the
set_power() callbacks, to say those I learned about looking at S3C MMC.

In practice also MMC platform datas require some consolidation.

Kgene, Ben, do you plan to work on the S3C MCI driver? Can I spend some
time on it without duplicating the effort of anybody?

Regards,
Domenico

-[ Domenico Andreoli, aka cavok
 --[ http://cavokz.wordpress.com/gpgkey/
   ---[ 3A0F 2F80 F79C 678A 8936  4FEE 0677 9033 A20E BC50
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/2] Add OF binding helpers for MMC drivers

2011-04-21 Thread Grant Likely
On Thu, Apr 21, 2011 at 03:19:57PM +0200, Domenico Andreoli wrote:
 From: Domenico Andreoli cav...@gmail.com
 
 This patch adds helpers to manage OF binding of MMC DeviceTree configs.
 They don't cover all the MMC configuration cases, indeed are only a
 slight generalization of those found in the MMC-over-SPI driver. More
 will come later.

Can the mmc-over-spi driver be generalized to use this code too?

 
 Signed-off-by: Domenico Andreoli cav...@gmail.com
 ---
  drivers/of/Kconfig |4 +
  drivers/of/Makefile|1 +
  drivers/of/of_mmc.c|  165 
 +
  include/linux/of_mmc.h |   50 +++

Personally I think it makes more sense for this code to live in 
drivers/mmc/core.

  4 files changed, 220 insertions(+)
 
 Index: b/drivers/of/Kconfig
 ===
 --- a/drivers/of/Kconfig
 +++ b/drivers/of/Kconfig
 @@ -59,6 +59,10 @@ config OF_I2C
   help
 OpenFirmware I2C accessors
  
 +config OF_MMC
 + depends on MMC
 + def_bool y
 +
  config OF_NET
   depends on NETDEVICES
   def_bool y
 Index: b/drivers/of/Makefile
 ===
 --- a/drivers/of/Makefile
 +++ b/drivers/of/Makefile
 @@ -7,6 +7,7 @@ obj-$(CONFIG_OF_DEVICE) += device.o plat
  obj-$(CONFIG_OF_GPIO)   += gpio.o
  obj-$(CONFIG_OF_CLOCK)   += clock.o
  obj-$(CONFIG_OF_I2C) += of_i2c.o
 +obj-$(CONFIG_OF_MMC) += of_mmc.o
  obj-$(CONFIG_OF_NET) += of_net.o
  obj-$(CONFIG_OF_SPI) += of_spi.o
  obj-$(CONFIG_OF_MDIO)+= of_mdio.o
 Index: b/drivers/of/of_mmc.c
 ===
 --- /dev/null
 +++ b/drivers/of/of_mmc.c
 @@ -0,0 +1,165 @@
 +/*
 + * OF helpers for the MMC API
 + *
 + * Copyright (c) 2011 Domenico Andreoli
 + *
 + * Heavily inspired by the OF support to the MMC-over-SPI driver made
 + * by Anton Vorontsov
 + *
 + * This program is free software; you can redistribute  it and/or modify it
 + * under  the terms of  the GNU General  Public License as published by the
 + * Free Software Foundation;  either version 2 of the  License, or (at your
 + * option) any later version.
 + */
 +
 +#include linux/kernel.h
 +#include linux/module.h
 +#include linux/device.h
 +#include linux/err.h
 +#include linux/slab.h
 +#include linux/mmc/core.h
 +#include linux/gpio.h
 +#include linux/of.h
 +#include linux/of_mmc.h
 +#include linux/of_gpio.h
 +
 +static int of_read_mmc_gpio(struct of_mmc_crg *crg, int gpio_num)
 +{
 + int value, active_low;
 +
 + BUG_ON(gpio_num = NUM_MMC_GPIOS);
 +
 + /* hitting this means that DeviceTree left this gpio unspecified
 +  * by purpose but driver didn't take any measure to define its
 +  * behavior (i.e. aborting probe phase or disabling the feature).
 +  * driver needs to call of_is_valid_mmc_crg() for each expected
 +  * gpio to detect this case.
 +  */
 + if (WARN_ON(crg-gpios[gpio_num]  0))
 + return -1;
 +
 + value = gpio_get_value(crg-gpios[gpio_num]);
 + active_low = crg-alow_gpios[gpio_num];
 + return value ^ active_low;
 +}
 +
 +int of_get_mmc_cd_gpio(struct of_mmc_crg *crg)
 +{
 + return of_read_mmc_gpio(crg, CD_MMC_GPIO);
 +}
 +EXPORT_SYMBOL(of_get_mmc_cd_gpio);
 +
 +int of_get_mmc_ro_gpio(struct of_mmc_crg *crg)
 +{
 + return of_read_mmc_gpio(crg, WP_MMC_GPIO);
 +}
 +EXPORT_SYMBOL(of_get_mmc_ro_gpio);
 +
 +int of_is_valid_mmc_crg(struct of_mmc_crg *crg, int gpio_num)
 +{
 + BUG_ON(gpio_num = NUM_MMC_GPIOS);
 + return gpio_is_valid(crg-gpios[gpio_num]);
 +}
 +EXPORT_SYMBOL(of_is_valid_mmc_crg);
 +
 +int of_get_mmc_crg(struct device *dev, struct device_node *np,
 +   int cd_off, struct of_mmc_crg *crg)
 +{
 + int *gpio, *alow;
 + int i, ret;
 +
 + memset(crg, 0, sizeof(*crg));
 + crg-cd_irq = -1;
 +
 + gpio = crg-gpios;
 + alow = crg-alow_gpios;
 + for (i = 0; i  NUM_MMC_GPIOS; i++, gpio++, alow++) {
 + enum of_gpio_flags gpio_flags;
 + *gpio = of_get_gpio_flags(np, cd_off+i, gpio_flags);
 + *alow = !!(gpio_flags  OF_GPIO_ACTIVE_LOW);
 +
 + if (*gpio == -EEXIST || *gpio == -ENOENT) {
 + /* driver needs to define proper meaning of this missing
 +gpio (i.e. abort probe or disable the feature) */
 + pr_debug(%s: gpio #%d is not specified\n, __func__, 
 i);
 + continue;
 + }
 + if (*gpio  0) {
 + pr_debug(%s: invalid configuration\n, __func__);
 + ret = *gpio;
 + break;
 + }
 + if (!gpio_is_valid(*gpio)) {
 + pr_debug(%s: gpio #%d is not valid: %d\n, __func__, 
 i, *gpio);
 + ret = -EINVAL;
 + break;
 + }
 + ret =