| Message ID | 20181113130102.11299-1-linus.walleij@linaro.org |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
| Series | ata: palmld: Convert to GPIO descriptors | expand |
On Tue, Nov 13, 2018 at 2:03 PM Linus Walleij <linus.walleij@linaro.org> wrote: > Instead of passing GPIO numbers directly to the PalmLD > ATA driver, pass GPIO descriptors from the board file and > handle these in the driver. > > Cc: Marek Vasut <marek.vasut@gmail.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> Ping on this patch. If nothing happens I will apply it to the GPIO tree. Yours, Linus Walleij
On 11/13/2018 02:01 PM, Linus Walleij wrote: > Instead of passing GPIO numbers directly to the PalmLD > ATA driver, pass GPIO descriptors from the board file and > handle these in the driver. > > Cc: Marek Vasut <marek.vasut@gmail.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> [...] > +++ b/drivers/ata/pata_palmld.c > @@ -26,17 +26,14 @@ > #include <linux/irq.h> > #include <linux/platform_device.h> > #include <linux/delay.h> > -#include <linux/gpio.h> > +#include <linux/gpio/consumer.h> > > #include <scsi/scsi_host.h> > #include <mach/palmld.h> > > #define DRV_NAME "pata_palmld" > > -static struct gpio palmld_hdd_gpios[] = { > - { GPIO_NR_PALMLD_IDE_PWEN, GPIOF_INIT_HIGH, "HDD Power" }, > - { GPIO_NR_PALMLD_IDE_RESET, GPIOF_INIT_LOW, "HDD Reset" }, > -}; > +static struct gpio_desc *palmld_pata_power; Can't this be placed in some per-driver-instance data ? Looks OK otherwise. [...]
On 11/13/18 6:01 AM, Linus Walleij wrote: > Instead of passing GPIO numbers directly to the PalmLD > ATA driver, pass GPIO descriptors from the board file and > handle these in the driver. Applied, thanks.
On 12/05/2018 01:16 AM, Jens Axboe wrote: > On 11/13/18 6:01 AM, Linus Walleij wrote: >> Instead of passing GPIO numbers directly to the PalmLD >> ATA driver, pass GPIO descriptors from the board file and >> handle these in the driver. > > Applied, thanks. I provided feedback, seems it was ignored :(
On 12/4/18 5:36 PM, Marek Vasut wrote: > On 12/05/2018 01:16 AM, Jens Axboe wrote: >> On 11/13/18 6:01 AM, Linus Walleij wrote: >>> Instead of passing GPIO numbers directly to the PalmLD >>> ATA driver, pass GPIO descriptors from the board file and >>> handle these in the driver. >> >> Applied, thanks. > > I provided feedback, seems it was ignored :( Sorry, I missed that then. Linus, if feedback needs to be addressed, just do an incremental patch.
On 12/05/2018 01:38 AM, Jens Axboe wrote: > On 12/4/18 5:36 PM, Marek Vasut wrote: >> On 12/05/2018 01:16 AM, Jens Axboe wrote: >>> On 11/13/18 6:01 AM, Linus Walleij wrote: >>>> Instead of passing GPIO numbers directly to the PalmLD >>>> ATA driver, pass GPIO descriptors from the board file and >>>> handle these in the driver. >>> >>> Applied, thanks. >> >> I provided feedback, seems it was ignored :( > > Sorry, I missed that then. Linus, if feedback needs to be addressed, > just do an incremental patch. Thanks
On Wed, Dec 5, 2018 at 1:38 AM Jens Axboe <axboe@kernel.dk> wrote: > On 12/4/18 5:36 PM, Marek Vasut wrote: > > On 12/05/2018 01:16 AM, Jens Axboe wrote: > >> On 11/13/18 6:01 AM, Linus Walleij wrote: > >>> Instead of passing GPIO numbers directly to the PalmLD > >>> ATA driver, pass GPIO descriptors from the board file and > >>> handle these in the driver. > >> > >> Applied, thanks. > > > > I provided feedback, seems it was ignored :( > > Sorry, I missed that then. Linus, if feedback needs to be addressed, > just do an incremental patch. OK such things happens, I'll make a follow-up patch creating a state container as requested by Marek. I first thought it's unintuitive since I suspect it is mechanically impossible to connect more than one ATA interface to the Palm LifeDrive (judging from how it looks). But it is also nice for consistency and such a small piece of work so why not :) Are you using this device with OpenEmbedded userspace or something different? Yours, Linus Walleij
On 12/05/2018 09:46 AM, Linus Walleij wrote: > On Wed, Dec 5, 2018 at 1:38 AM Jens Axboe <axboe@kernel.dk> wrote: >> On 12/4/18 5:36 PM, Marek Vasut wrote: >>> On 12/05/2018 01:16 AM, Jens Axboe wrote: >>>> On 11/13/18 6:01 AM, Linus Walleij wrote: >>>>> Instead of passing GPIO numbers directly to the PalmLD >>>>> ATA driver, pass GPIO descriptors from the board file and >>>>> handle these in the driver. >>>> >>>> Applied, thanks. >>> >>> I provided feedback, seems it was ignored :( >> >> Sorry, I missed that then. Linus, if feedback needs to be addressed, >> just do an incremental patch. > > OK such things happens, I'll make a follow-up patch creating > a state container as requested by Marek. > > I first thought it's unintuitive since I suspect it is mechanically > impossible to connect more than one ATA interface to the > Palm LifeDrive (judging from how it looks). You can always cut a hole in the back of the case :-) > But it is also nice > for consistency and such a small piece of work so why not :) Right > Are you using this device with OpenEmbedded userspace > or something different? I used OE, but it's been mostly catching dust recently.
diff --git a/arch/arm/mach-pxa/palm27x.h b/arch/arm/mach-pxa/palm27x.h index d4eac3d6ffb5..3316ed2016f3 100644 --- a/arch/arm/mach-pxa/palm27x.h +++ b/arch/arm/mach-pxa/palm27x.h @@ -12,6 +12,8 @@ #ifndef __INCLUDE_MACH_PALM27X__ #define __INCLUDE_MACH_PALM27X__ +#include <linux/gpio/machine.h> + #if defined(CONFIG_MMC_PXA) || defined(CONFIG_MMC_PXA_MODULE) extern void __init palm27x_mmc_init(int detect, int ro, int power, int power_inverted); diff --git a/arch/arm/mach-pxa/palmld.c b/arch/arm/mach-pxa/palmld.c index 980f2847f5b5..a37ceec22903 100644 --- a/arch/arm/mach-pxa/palmld.c +++ b/arch/arm/mach-pxa/palmld.c @@ -288,8 +288,20 @@ static struct platform_device palmld_ide_device = { .id = -1, }; +static struct gpiod_lookup_table palmld_ide_gpio_table = { + .dev_id = "pata_palmld", + .table = { + GPIO_LOOKUP("gpio-pxa", GPIO_NR_PALMLD_IDE_PWEN, + "power", GPIO_ACTIVE_HIGH), + GPIO_LOOKUP("gpio-pxa", GPIO_NR_PALMLD_IDE_RESET, + "reset", GPIO_ACTIVE_LOW), + { }, + }, +}; + static void __init palmld_ide_init(void) { + gpiod_add_lookup_table(&palmld_ide_gpio_table); platform_device_register(&palmld_ide_device); } #else diff --git a/drivers/ata/pata_palmld.c b/drivers/ata/pata_palmld.c index d071ab6864a8..e2933c324d41 100644 --- a/drivers/ata/pata_palmld.c +++ b/drivers/ata/pata_palmld.c @@ -26,17 +26,14 @@ #include <linux/irq.h> #include <linux/platform_device.h> #include <linux/delay.h> -#include <linux/gpio.h> +#include <linux/gpio/consumer.h> #include <scsi/scsi_host.h> #include <mach/palmld.h> #define DRV_NAME "pata_palmld" -static struct gpio palmld_hdd_gpios[] = { - { GPIO_NR_PALMLD_IDE_PWEN, GPIOF_INIT_HIGH, "HDD Power" }, - { GPIO_NR_PALMLD_IDE_RESET, GPIOF_INIT_LOW, "HDD Reset" }, -}; +static struct gpio_desc *palmld_pata_power; static struct scsi_host_template palmld_sht = { ATA_PIO_SHT(DRV_NAME), @@ -53,32 +50,34 @@ static int palmld_pata_probe(struct platform_device *pdev) struct ata_host *host; struct ata_port *ap; void __iomem *mem; + struct device *dev = &pdev->dev; + struct gpio_desc *reset; int ret; /* allocate host */ - host = ata_host_alloc(&pdev->dev, 1); - if (!host) { - ret = -ENOMEM; - goto err1; - } + host = ata_host_alloc(dev, 1); + if (!host) + return -ENOMEM; /* remap drive's physical memory address */ - mem = devm_ioremap(&pdev->dev, PALMLD_IDE_PHYS, 0x1000); - if (!mem) { - ret = -ENOMEM; - goto err1; + mem = devm_ioremap(dev, PALMLD_IDE_PHYS, 0x1000); + if (!mem) + return -ENOMEM; + + /* request and activate power and reset GPIOs */ + palmld_pata_power = devm_gpiod_get(dev, "power", GPIOD_OUT_HIGH); + if (IS_ERR(palmld_pata_power)) + return PTR_ERR(palmld_pata_power); + reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); + if (IS_ERR(reset)) { + gpiod_set_value(palmld_pata_power, 0); + return PTR_ERR(reset); } - /* request and activate power GPIO, IRQ GPIO */ - ret = gpio_request_array(palmld_hdd_gpios, - ARRAY_SIZE(palmld_hdd_gpios)); - if (ret) - goto err1; - - /* reset the drive */ - gpio_set_value(GPIO_NR_PALMLD_IDE_RESET, 0); + /* Assert reset to reset the drive */ + gpiod_set_value(reset, 1); msleep(30); - gpio_set_value(GPIO_NR_PALMLD_IDE_RESET, 1); + gpiod_set_value(reset, 0); msleep(30); /* setup the ata port */ @@ -98,14 +97,9 @@ static int palmld_pata_probe(struct platform_device *pdev) /* activate host */ ret = ata_host_activate(host, 0, NULL, IRQF_TRIGGER_RISING, &palmld_sht); + /* power down on failure */ if (ret) - goto err2; - - return ret; - -err2: - gpio_free_array(palmld_hdd_gpios, ARRAY_SIZE(palmld_hdd_gpios)); -err1: + gpiod_set_value(palmld_pata_power, 0); return ret; } @@ -114,9 +108,7 @@ static int palmld_pata_remove(struct platform_device *dev) ata_platform_remove_one(dev); /* power down the HDD */ - gpio_set_value(GPIO_NR_PALMLD_IDE_PWEN, 0); - - gpio_free_array(palmld_hdd_gpios, ARRAY_SIZE(palmld_hdd_gpios)); + gpiod_set_value(palmld_pata_power, 0); return 0; }
Instead of passing GPIO numbers directly to the PalmLD ATA driver, pass GPIO descriptors from the board file and handle these in the driver. Cc: Marek Vasut <marek.vasut@gmail.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- The hunk to arch/arm/mach-pxa/palm27x.h also appear in a patch converting MMC/SD card detect, see: https://marc.info/?l=linux-mmc&m=154203199305212&w=2 However I have taken great care that these two hunks are identical and will not lead to conflicts when merging the trees. --- arch/arm/mach-pxa/palm27x.h | 2 ++ arch/arm/mach-pxa/palmld.c | 12 ++++++++ drivers/ata/pata_palmld.c | 58 ++++++++++++++++--------------------- 3 files changed, 39 insertions(+), 33 deletions(-)