| Message ID | 20181112141239.19646-8-linus.walleij@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Convert MMC slot GPIO to descriptors | expand |
On Mon, 12 Nov 2018 at 15:13, Linus Walleij <linus.walleij@linaro.org> wrote: > > Simplify things by making the S3CMCI driver just use > slot GPIO with descriptors instead of passing around the global > GPIO numbers that we want to get rid of. > > Getting the names of the GPIO chips into the machine > descriptor tables was a bit of a challenge but I think > I have them right. > > The platform data supports passing in inversion flags, but > no platform is using them, and it is highly unlikely > that we will add more, so drop them. The long term plan > is to let the inversion flags on the GPIO machine > descriptor do the job. > > Even if the slot GPIO core discards the inversion > semantics on the GPIO lines we take special care to > specify them right. > > Cc: Jaehoon Chung <jh80.chung@samsung.com> > Cc: Sylwester Nawrocki <s.nawrocki@samsung.com> > Cc: Krzysztof Kozlowski <k.kozlowski@samsung.com> This address does not work since long time. mailmap should point you to current one (krzk@kernel.org)... > Cc: Sergio Prado <sergio.prado@e-labworks.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > Some testing of this would be greatly appreciated. These are legacy platforms (although I heard that still used by some users/companies) and I don't have the boards. I can just quickly look at the code. Does this patch depend on anything? What tree shall take it? > --- > arch/arm/mach-s3c24xx/mach-at2440evb.c | 14 +++++++++-- > arch/arm/mach-s3c24xx/mach-h1940.c | 15 +++++++++-- > arch/arm/mach-s3c24xx/mach-mini2440.c | 16 +++++++++--- > arch/arm/mach-s3c24xx/mach-n30.c | 15 +++++++++-- > arch/arm/mach-s3c24xx/mach-rx1950.c | 15 +++++++++-- > drivers/mmc/host/s3cmci.c | 32 ++++++++---------------- > include/linux/platform_data/mmc-s3cmci.h | 4 --- > 7 files changed, 75 insertions(+), 36 deletions(-) > > diff --git a/arch/arm/mach-s3c24xx/mach-at2440evb.c b/arch/arm/mach-s3c24xx/mach-at2440evb.c > index 68a4fa94257a..2d370f7f75fa 100644 > --- a/arch/arm/mach-s3c24xx/mach-at2440evb.c > +++ b/arch/arm/mach-s3c24xx/mach-at2440evb.c > @@ -9,7 +9,7 @@ > > #include <linux/kernel.h> > #include <linux/types.h> > -#include <linux/gpio.h> > +#include <linux/gpio/machine.h> > #include <linux/interrupt.h> > #include <linux/list.h> > #include <linux/timer.h> > @@ -136,7 +136,16 @@ static struct platform_device at2440evb_device_eth = { > }; > > static struct s3c24xx_mci_pdata at2440evb_mci_pdata __initdata = { > - .gpio_detect = S3C2410_GPG(10), > + /* Intentionally left blank */ > +}; > + > +static struct gpiod_lookup_table at2440evb_mci_gpio_table = { > + .dev_id = "s3c2410-sdi", > + .table = { > + /* Card detect S3C2410_GPG(10) */ > + GPIO_LOOKUP("GPIOG", 10, "cd", GPIO_ACTIVE_LOW), I missed few changes around this... What is the "chip_label" here? Name of bank? In general the bank names follow different name convention: https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/samsung/pinctrl-s3c24xx.c#L58 https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/samsung/pinctrl-s3c24xx.c#L563 so I think it should be "gpg". > + { }, > + }, > }; > > /* 7" LCD panel */ > @@ -200,6 +209,7 @@ static void __init at2440evb_init_time(void) > static void __init at2440evb_init(void) > { > s3c24xx_fb_set_platdata(&at2440evb_fb_info); > + gpiod_add_lookup_table(&at2440evb_mci_gpio_table); > s3c24xx_mci_set_platdata(&at2440evb_mci_pdata); > s3c_nand_set_platdata(&at2440evb_nand_info); > s3c_i2c0_set_platdata(NULL); > diff --git a/arch/arm/mach-s3c24xx/mach-h1940.c b/arch/arm/mach-s3c24xx/mach-h1940.c > index e064c73a57d3..8d9d8e7c71d4 100644 > --- a/arch/arm/mach-s3c24xx/mach-h1940.c > +++ b/arch/arm/mach-s3c24xx/mach-h1940.c > @@ -18,6 +18,7 @@ > #include <linux/platform_device.h> > #include <linux/io.h> > #include <linux/gpio.h> > +#include <linux/gpio/machine.h> > #include <linux/input.h> > #include <linux/gpio_keys.h> > #include <linux/pwm.h> > @@ -459,12 +460,21 @@ static void h1940_set_mmc_power(unsigned char power_mode, unsigned short vdd) > } > > static struct s3c24xx_mci_pdata h1940_mmc_cfg __initdata = { > - .gpio_detect = S3C2410_GPF(5), > - .gpio_wprotect = S3C2410_GPH(8), > .set_power = h1940_set_mmc_power, > .ocr_avail = MMC_VDD_32_33, > }; > > +static struct gpiod_lookup_table h1940_mmc_gpio_table = { > + .dev_id = "s3c2410-sdi", > + .table = { > + /* Card detect S3C2410_GPF(5) */ > + GPIO_LOOKUP("GPIOF", 5, "cd", GPIO_ACTIVE_LOW), > + /* Write protect S3C2410_GPH(8) */ > + GPIO_LOOKUP("GPIOH", 8, "wp", GPIO_ACTIVE_LOW), > + { }, > + }, > +}; > + > static struct pwm_lookup h1940_pwm_lookup[] = { > PWM_LOOKUP("samsung-pwm", 0, "pwm-backlight", NULL, 36296, > PWM_POLARITY_NORMAL), > @@ -680,6 +690,7 @@ static void __init h1940_init(void) > u32 tmp; > > s3c24xx_fb_set_platdata(&h1940_fb_info); > + gpiod_add_lookup_table(&h1940_mmc_gpio_table); > s3c24xx_mci_set_platdata(&h1940_mmc_cfg); > s3c24xx_udc_set_platdata(&h1940_udc_cfg); > s3c24xx_ts_set_platdata(&h1940_ts_cfg); > diff --git a/arch/arm/mach-s3c24xx/mach-mini2440.c b/arch/arm/mach-s3c24xx/mach-mini2440.c > index 50d67d760efd..8c5f222126d2 100644 > --- a/arch/arm/mach-s3c24xx/mach-mini2440.c > +++ b/arch/arm/mach-s3c24xx/mach-mini2440.c > @@ -15,6 +15,7 @@ > #include <linux/timer.h> > #include <linux/init.h> > #include <linux/gpio.h> > +#include <linux/gpio/machine.h> > #include <linux/input.h> > #include <linux/io.h> > #include <linux/serial_core.h> > @@ -234,13 +235,21 @@ static struct s3c2410fb_mach_info mini2440_fb_info __initdata = { > /* MMC/SD */ > > static struct s3c24xx_mci_pdata mini2440_mmc_cfg __initdata = { > - .gpio_detect = S3C2410_GPG(8), > - .gpio_wprotect = S3C2410_GPH(8), > - .wprotect_invert = 1, > .set_power = NULL, > .ocr_avail = MMC_VDD_32_33|MMC_VDD_33_34, > }; > > +static struct gpiod_lookup_table mini2440_mmc_gpio_table = { > + .dev_id = "s3c2410-sdi", > + .table = { > + /* Card detect S3C2410_GPG(8) */ > + GPIO_LOOKUP("GPIOG", 8, "cd", GPIO_ACTIVE_LOW), > + /* Write protect S3C2410_GPH(8) */ > + GPIO_LOOKUP("GPIOH", 8, "wp", GPIO_ACTIVE_LOW), What about the wprotect_invert? You mentioned that no platform is using them but it was in mini2440_mmc_cfg. Best regards, Krzysztof
On Tue, Nov 13, 2018 at 4:05 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > Cc: Krzysztof Kozlowski <k.kozlowski@samsung.com> > > This address does not work since long time. mailmap should point you > to current one (krzk@kernel.org)... Sorry, was just looking in the kernel logs for the file... :/ > Does this patch depend on anything? What tree shall take it? It does not depend on anything but should go into the MMC tree as at the end of the series I delete the legacy helper functions for global GPIOs. > > +static struct gpiod_lookup_table at2440evb_mci_gpio_table = { > > + .dev_id = "s3c2410-sdi", > > + .table = { > > + /* Card detect S3C2410_GPG(10) */ > > + GPIO_LOOKUP("GPIOG", 10, "cd", GPIO_ACTIVE_LOW), > > I missed few changes around this... What is the "chip_label" here? > Name of bank? No it is whatever .label is set to inside struct gpio_chip > In general the bank names follow different name > convention: > https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/samsung/pinctrl-s3c24xx.c#L58 > https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/samsung/pinctrl-s3c24xx.c#L563 > so I think it should be "gpg". Hm since I can't run these systems I try to parse the code in my head. Not always successfully... This board seems to use gpio-samsung, in arch/arm/plat-samsung/gpio.c by the call to samsung_gpiolib_init(). If I understand that code correctly, the label is set to "GPG" (capital letters). I'll respin with this change, thanks! Yours, Linus Walleij
diff --git a/arch/arm/mach-s3c24xx/mach-at2440evb.c b/arch/arm/mach-s3c24xx/mach-at2440evb.c index 68a4fa94257a..2d370f7f75fa 100644 --- a/arch/arm/mach-s3c24xx/mach-at2440evb.c +++ b/arch/arm/mach-s3c24xx/mach-at2440evb.c @@ -9,7 +9,7 @@ #include <linux/kernel.h> #include <linux/types.h> -#include <linux/gpio.h> +#include <linux/gpio/machine.h> #include <linux/interrupt.h> #include <linux/list.h> #include <linux/timer.h> @@ -136,7 +136,16 @@ static struct platform_device at2440evb_device_eth = { }; static struct s3c24xx_mci_pdata at2440evb_mci_pdata __initdata = { - .gpio_detect = S3C2410_GPG(10), + /* Intentionally left blank */ +}; + +static struct gpiod_lookup_table at2440evb_mci_gpio_table = { + .dev_id = "s3c2410-sdi", + .table = { + /* Card detect S3C2410_GPG(10) */ + GPIO_LOOKUP("GPIOG", 10, "cd", GPIO_ACTIVE_LOW), + { }, + }, }; /* 7" LCD panel */ @@ -200,6 +209,7 @@ static void __init at2440evb_init_time(void) static void __init at2440evb_init(void) { s3c24xx_fb_set_platdata(&at2440evb_fb_info); + gpiod_add_lookup_table(&at2440evb_mci_gpio_table); s3c24xx_mci_set_platdata(&at2440evb_mci_pdata); s3c_nand_set_platdata(&at2440evb_nand_info); s3c_i2c0_set_platdata(NULL); diff --git a/arch/arm/mach-s3c24xx/mach-h1940.c b/arch/arm/mach-s3c24xx/mach-h1940.c index e064c73a57d3..8d9d8e7c71d4 100644 --- a/arch/arm/mach-s3c24xx/mach-h1940.c +++ b/arch/arm/mach-s3c24xx/mach-h1940.c @@ -18,6 +18,7 @@ #include <linux/platform_device.h> #include <linux/io.h> #include <linux/gpio.h> +#include <linux/gpio/machine.h> #include <linux/input.h> #include <linux/gpio_keys.h> #include <linux/pwm.h> @@ -459,12 +460,21 @@ static void h1940_set_mmc_power(unsigned char power_mode, unsigned short vdd) } static struct s3c24xx_mci_pdata h1940_mmc_cfg __initdata = { - .gpio_detect = S3C2410_GPF(5), - .gpio_wprotect = S3C2410_GPH(8), .set_power = h1940_set_mmc_power, .ocr_avail = MMC_VDD_32_33, }; +static struct gpiod_lookup_table h1940_mmc_gpio_table = { + .dev_id = "s3c2410-sdi", + .table = { + /* Card detect S3C2410_GPF(5) */ + GPIO_LOOKUP("GPIOF", 5, "cd", GPIO_ACTIVE_LOW), + /* Write protect S3C2410_GPH(8) */ + GPIO_LOOKUP("GPIOH", 8, "wp", GPIO_ACTIVE_LOW), + { }, + }, +}; + static struct pwm_lookup h1940_pwm_lookup[] = { PWM_LOOKUP("samsung-pwm", 0, "pwm-backlight", NULL, 36296, PWM_POLARITY_NORMAL), @@ -680,6 +690,7 @@ static void __init h1940_init(void) u32 tmp; s3c24xx_fb_set_platdata(&h1940_fb_info); + gpiod_add_lookup_table(&h1940_mmc_gpio_table); s3c24xx_mci_set_platdata(&h1940_mmc_cfg); s3c24xx_udc_set_platdata(&h1940_udc_cfg); s3c24xx_ts_set_platdata(&h1940_ts_cfg); diff --git a/arch/arm/mach-s3c24xx/mach-mini2440.c b/arch/arm/mach-s3c24xx/mach-mini2440.c index 50d67d760efd..8c5f222126d2 100644 --- a/arch/arm/mach-s3c24xx/mach-mini2440.c +++ b/arch/arm/mach-s3c24xx/mach-mini2440.c @@ -15,6 +15,7 @@ #include <linux/timer.h> #include <linux/init.h> #include <linux/gpio.h> +#include <linux/gpio/machine.h> #include <linux/input.h> #include <linux/io.h> #include <linux/serial_core.h> @@ -234,13 +235,21 @@ static struct s3c2410fb_mach_info mini2440_fb_info __initdata = { /* MMC/SD */ static struct s3c24xx_mci_pdata mini2440_mmc_cfg __initdata = { - .gpio_detect = S3C2410_GPG(8), - .gpio_wprotect = S3C2410_GPH(8), - .wprotect_invert = 1, .set_power = NULL, .ocr_avail = MMC_VDD_32_33|MMC_VDD_33_34, }; +static struct gpiod_lookup_table mini2440_mmc_gpio_table = { + .dev_id = "s3c2410-sdi", + .table = { + /* Card detect S3C2410_GPG(8) */ + GPIO_LOOKUP("GPIOG", 8, "cd", GPIO_ACTIVE_LOW), + /* Write protect S3C2410_GPH(8) */ + GPIO_LOOKUP("GPIOH", 8, "wp", GPIO_ACTIVE_LOW), + { }, + }, +}; + /* NAND Flash on MINI2440 board */ static struct mtd_partition mini2440_default_nand_part[] __initdata = { @@ -696,6 +705,7 @@ static void __init mini2440_init(void) } s3c24xx_udc_set_platdata(&mini2440_udc_cfg); + gpiod_add_lookup_table(&mini2440_mmc_gpio_table); s3c24xx_mci_set_platdata(&mini2440_mmc_cfg); s3c_nand_set_platdata(&mini2440_nand_info); s3c_i2c0_set_platdata(NULL); diff --git a/arch/arm/mach-s3c24xx/mach-n30.c b/arch/arm/mach-s3c24xx/mach-n30.c index eec51fadb14a..ffa20f52aa83 100644 --- a/arch/arm/mach-s3c24xx/mach-n30.c +++ b/arch/arm/mach-s3c24xx/mach-n30.c @@ -17,6 +17,7 @@ #include <linux/gpio_keys.h> #include <linux/init.h> #include <linux/gpio.h> +#include <linux/gpio/machine.h> #include <linux/input.h> #include <linux/interrupt.h> #include <linux/platform_device.h> @@ -350,12 +351,21 @@ static void n30_sdi_set_power(unsigned char power_mode, unsigned short vdd) } static struct s3c24xx_mci_pdata n30_mci_cfg __initdata = { - .gpio_detect = S3C2410_GPF(1), - .gpio_wprotect = S3C2410_GPG(10), .ocr_avail = MMC_VDD_32_33, .set_power = n30_sdi_set_power, }; +static struct gpiod_lookup_table n30_mci_gpio_table = { + .dev_id = "s3c2410-sdi", + .table = { + /* Card detect S3C2410_GPF(1) */ + GPIO_LOOKUP("GPIOF", 1, "cd", GPIO_ACTIVE_LOW), + /* Write protect S3C2410_GPG(10) */ + GPIO_LOOKUP("GPIOG", 10, "wp", GPIO_ACTIVE_LOW), + { }, + }, +}; + static struct platform_device *n30_devices[] __initdata = { &s3c_device_lcd, &s3c_device_wdt, @@ -549,6 +559,7 @@ static void __init n30_init(void) s3c24xx_fb_set_platdata(&n30_fb_info); s3c24xx_udc_set_platdata(&n30_udc_cfg); + gpiod_add_lookup_table(&n30_mci_gpio_table); s3c24xx_mci_set_platdata(&n30_mci_cfg); s3c_i2c0_set_platdata(&n30_i2ccfg); diff --git a/arch/arm/mach-s3c24xx/mach-rx1950.c b/arch/arm/mach-s3c24xx/mach-rx1950.c index 7f5a18fa305b..534e9c1d8161 100644 --- a/arch/arm/mach-s3c24xx/mach-rx1950.c +++ b/arch/arm/mach-s3c24xx/mach-rx1950.c @@ -14,6 +14,7 @@ #include <linux/timer.h> #include <linux/init.h> #include <linux/gpio.h> +#include <linux/gpio/machine.h> #include <linux/platform_device.h> #include <linux/serial_core.h> #include <linux/serial_s3c.h> @@ -558,12 +559,21 @@ static void rx1950_set_mmc_power(unsigned char power_mode, unsigned short vdd) } static struct s3c24xx_mci_pdata rx1950_mmc_cfg __initdata = { - .gpio_detect = S3C2410_GPF(5), - .gpio_wprotect = S3C2410_GPH(8), .set_power = rx1950_set_mmc_power, .ocr_avail = MMC_VDD_32_33, }; +static struct gpiod_lookup_table rx1950_mmc_gpio_table = { + .dev_id = "s3c2410-sdi", + .table = { + /* Card detect S3C2410_GPF(5) */ + GPIO_LOOKUP("GPIOF", 5, "cd", GPIO_ACTIVE_LOW), + /* Write protect S3C2410_GPH(8) */ + GPIO_LOOKUP("GPIOH", 8, "wp", GPIO_ACTIVE_LOW), + { }, + }, +}; + static struct mtd_partition rx1950_nand_part[] = { [0] = { .name = "Boot0", @@ -762,6 +772,7 @@ static void __init rx1950_init_machine(void) s3c24xx_fb_set_platdata(&rx1950_lcd_cfg); s3c24xx_udc_set_platdata(&rx1950_udc_cfg); s3c24xx_ts_set_platdata(&rx1950_ts_cfg); + gpiod_add_lookup_table(&rx1950_mmc_gpio_table); s3c24xx_mci_set_platdata(&rx1950_mmc_cfg); s3c_i2c0_set_platdata(NULL); s3c_nand_set_platdata(&rx1950_nand_info); diff --git a/drivers/mmc/host/s3cmci.c b/drivers/mmc/host/s3cmci.c index f77493604312..529b48d66d43 100644 --- a/drivers/mmc/host/s3cmci.c +++ b/drivers/mmc/host/s3cmci.c @@ -26,7 +26,6 @@ #include <linux/io.h> #include <linux/of.h> #include <linux/of_device.h> -#include <linux/of_gpio.h> #include <linux/mmc/slot-gpio.h> #include <plat/gpio-cfg.h> @@ -1542,28 +1541,19 @@ static int s3cmci_probe_pdata(struct s3cmci_host *host) if (pdata->no_detect) mmc->caps |= MMC_CAP_NEEDS_POLL; - if (pdata->wprotect_invert) - mmc->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH; - - if (pdata->detect_invert) - mmc->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH; - - if (gpio_is_valid(pdata->gpio_detect)) { - ret = mmc_gpio_request_cd(mmc, pdata->gpio_detect, 0); - if (ret) { - dev_err(&pdev->dev, "error requesting GPIO for CD %d\n", - ret); - return ret; - } + /* If we get -ENOENT we have no card detect GPIO line */ + ret = mmc_gpiod_request_cd(mmc, "cd", 0, false, 0, NULL); + if (ret != -ENOENT) { + dev_err(&pdev->dev, "error requesting GPIO for CD %d\n", + ret); + return ret; } - if (gpio_is_valid(pdata->gpio_wprotect)) { - ret = mmc_gpio_request_ro(mmc, pdata->gpio_wprotect); - if (ret) { - dev_err(&pdev->dev, "error requesting GPIO for WP %d\n", - ret); - return ret; - } + ret = mmc_gpiod_request_ro(host->mmc, "wp", 0, false, 0, NULL); + if (ret != -ENOENT) { + dev_err(&pdev->dev, "error requesting GPIO for WP %d\n", + ret); + return ret; } return 0; diff --git a/include/linux/platform_data/mmc-s3cmci.h b/include/linux/platform_data/mmc-s3cmci.h index b68d9f0bdd9e..87274a267fe4 100644 --- a/include/linux/platform_data/mmc-s3cmci.h +++ b/include/linux/platform_data/mmc-s3cmci.h @@ -30,12 +30,8 @@ struct s3c24xx_mci_pdata { unsigned int no_wprotect:1; unsigned int no_detect:1; - unsigned int wprotect_invert:1; - unsigned int detect_invert:1; /* set => detect active high */ unsigned int use_dma:1; - unsigned int gpio_detect; - unsigned int gpio_wprotect; unsigned long ocr_avail; void (*set_power)(unsigned char power_mode, unsigned short vdd);
Simplify things by making the S3CMCI driver just use slot GPIO with descriptors instead of passing around the global GPIO numbers that we want to get rid of. Getting the names of the GPIO chips into the machine descriptor tables was a bit of a challenge but I think I have them right. The platform data supports passing in inversion flags, but no platform is using them, and it is highly unlikely that we will add more, so drop them. The long term plan is to let the inversion flags on the GPIO machine descriptor do the job. Even if the slot GPIO core discards the inversion semantics on the GPIO lines we take special care to specify them right. Cc: Jaehoon Chung <jh80.chung@samsung.com> Cc: Sylwester Nawrocki <s.nawrocki@samsung.com> Cc: Krzysztof Kozlowski <k.kozlowski@samsung.com> Cc: Sergio Prado <sergio.prado@e-labworks.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- Some testing of this would be greatly appreciated. --- arch/arm/mach-s3c24xx/mach-at2440evb.c | 14 +++++++++-- arch/arm/mach-s3c24xx/mach-h1940.c | 15 +++++++++-- arch/arm/mach-s3c24xx/mach-mini2440.c | 16 +++++++++--- arch/arm/mach-s3c24xx/mach-n30.c | 15 +++++++++-- arch/arm/mach-s3c24xx/mach-rx1950.c | 15 +++++++++-- drivers/mmc/host/s3cmci.c | 32 ++++++++---------------- include/linux/platform_data/mmc-s3cmci.h | 4 --- 7 files changed, 75 insertions(+), 36 deletions(-) -- 2.17.2