Linus Walleij <linus.walleij@xxxxxxxxxx> writes:
> Simplify things by making the PXA MCI driver just use
> slot GPIO with descriptors instead of passing around the global
> GPIO numbers that we want to get rid of.
Hi Linus,
I'd like some improvement here and there. Let's tackle first the generic parts :
- patch split
I'd like the mach-pxa part to be in a separate patch. Looking at your patch,
that probably means, if I'm right, a split in 4 patches :
1) Add the GPIO_LOOKUP entries
2) Add the pxamci part to use GPIO tables
3) Remove the platform_data in mach-pxa
4) Remove the dead code from pxamci
That will enable easier "Fixes:" tags, and easier maintainance for me and
Ulf, as the fixes will go either on a patch for his tree or mine, so neither
of use will have to bother with other conflicts.
- GPIO tables
Don't use number, only defines.
In some of the mach-pxa files, you use :
> +static struct gpiod_lookup_table cmx270_mci_gpio_table = {
> + .dev_id = "pxa2xx-mci.0",
> + .table = {
> + /* Card detect on GPIO 83 */
> + GPIO_LOOKUP("gpio-pxa", 83, "cd", GPIO_ACTIVE_LOW),
> + { },
> + },
> +};
I'd like instead :
> +static struct gpiod_lookup_table cmx270_mci_gpio_table = {
> + .dev_id = "pxa2xx-mci.0",
> + .table = {
> + /* Card detect on GPIO 83 */
> + GPIO_LOOKUP("gpio-pxa", GPIO83_MMC_IRQ, "cd", GPIO_ACTIVE_LOW),
> + { },
> + },
> +};
- the zylonite specific case
This is the real showstopper here, which could block this conversion and
deserves your full brain power :)
I already had attempted this conversion in the past, and came to a limitation
in the GPIO_LOOKUP model. The problem is that there are 2 gpio expanders (see
zylonite_pxa300.c), struct zylonite_i2c_board_info[].
Therefore, the GPIO_LOOKUP is not guaranteed AFAIK to check the correct
expander to get the gpio ... That problem is a good one for the GPIO
maintainer :)
> +static struct gpiod_lookup_table zylonite_mci_gpio_table = {
> + .dev_id = "pxa2xx-mci.0",
> + .table = {
> + GPIO_LOOKUP("pca9539", 0, "cd", GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP("pca9539", 2, "wp", GPIO_ACTIVE_LOW),
> + { },
> + },
> +};
- the platform_data cleanup
> diff --git a/include/linux/platform_data/mmc-pxamci.h b/include/linux/platform_data/mmc-pxamci.h
> index 752f97c62ef2..db6c247d42d1 100644
> --- a/include/linux/platform_data/mmc-pxamci.h
> +++ b/include/linux/platform_data/mmc-pxamci.h
> @@ -15,8 +15,6 @@ struct pxamci_platform_data {
> int (*get_ro)(struct device *);
> int (*setpower)(struct device *, unsigned int);
> void (*exit)(struct device *, void *);
> - int gpio_card_detect; /* gpio detecting card insertion */
> - int gpio_card_ro; /* gpio detecting read only toggle */
> bool gpio_card_ro_invert; /* gpio ro is inverted */
> int gpio_power; /* gpio powering up MMC bus */
> bool gpio_power_invert; /* gpio power is inverted */
Given all the work you injected to remove the gpios, I wonder why
gpio_card_ro_invert survived ...
In the mean time where these points are addressed, I'll go more deeply through
the details of this patch.
Cheers.
--
Robert
