| Message ID | 20240410072102.1200869-1-andy.shevchenko@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/1] gpio: sch: Switch to memory mapped IO accessors | expand |
On Wed, Apr 10, 2024 at 9:21 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > Convert driver to use memory mapped IO accessors. > > Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> If this is the direction we want to go with port-mapped I/O then I'm in. Acked-by: Linus Walleij <linus.walleij@linaro.org> An ACK from William may be in order, he's usually keeping track of the port-mapped stuff. Yours, Linus Walleij
On Fri, Apr 12, 2024 at 10:35:38AM +0200, Linus Walleij wrote: > On Wed, Apr 10, 2024 at 9:21 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > > Convert driver to use memory mapped IO accessors. > > > > Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > If this is the direction we want to go with port-mapped I/O then > I'm in. > Acked-by: Linus Walleij <linus.walleij@linaro.org> Thanks! > An ACK from William may be in order, he's usually keeping track > of the port-mapped stuff. William, are you okay to provide one?
On Wed, Apr 10, 2024 at 10:21:02AM +0300, Andy Shevchenko wrote: > Convert driver to use memory mapped IO accessors. > > Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> Acked-by: William Breathitt Gray <wbg@kernel.org> A minor suggestion below, but I find this patch accepted as-is. > static int sch_gpio_probe(struct platform_device *pdev) > { > + struct device *dev = &pdev->dev; In general I think this is a good variable to define to simplify all the &pdev->dev appearing throughout this callback, but I'd rather have seen it as its own patch so we could change all the other uses of &pdev->dev at once without distracting from the memory-mapped I/O change of this particular patch. Not really necessary, but maybe at some point in the future a follow-up patch doing such a cleanup would be nice. William Breathitt Gray
On Sat, Apr 13, 2024 at 04:31:26AM -0400, William Breathitt Gray wrote: > On Wed, Apr 10, 2024 at 10:21:02AM +0300, Andy Shevchenko wrote: > > Convert driver to use memory mapped IO accessors. > > > > Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > Acked-by: William Breathitt Gray <wbg@kernel.org> Pushed to my review and testing queue, thank you! > A minor suggestion below, but I find this patch accepted as-is. > > > static int sch_gpio_probe(struct platform_device *pdev) > > { > > + struct device *dev = &pdev->dev; > > In general I think this is a good variable to define to simplify all the > &pdev->dev appearing throughout this callback, but I'd rather have seen > it as its own patch so we could change all the other uses of &pdev->dev > at once without distracting from the memory-mapped I/O change of this > particular patch. Not really necessary, but maybe at some point in the > future a follow-up patch doing such a cleanup would be nice. I avoid making unneeded churn on a line I have updated in this patch. That's why I introduced the local variable proactively. Yet, I can do another patch to clean up the driver based on the existence of this var.
On Sat, Apr 13, 2024 at 10:31 AM William Breathitt Gray <wbg@kernel.org> wrote: > > On Wed, Apr 10, 2024 at 10:21:02AM +0300, Andy Shevchenko wrote: > > Convert driver to use memory mapped IO accessors. > > > > Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > Acked-by: William Breathitt Gray <wbg@kernel.org> > > A minor suggestion below, but I find this patch accepted as-is. > > > static int sch_gpio_probe(struct platform_device *pdev) > > { > > + struct device *dev = &pdev->dev; > > In general I think this is a good variable to define to simplify all the > &pdev->dev appearing throughout this callback, but I'd rather have seen > it as its own patch so we could change all the other uses of &pdev->dev > at once without distracting from the memory-mapped I/O change of this > particular patch. Not really necessary, but maybe at some point in the > future a follow-up patch doing such a cleanup would be nice. > > William Breathitt Gray I applied it as is, if anyone wants it, this can be sent on top of it. Bart
On Wed, Apr 17, 2024 at 12:17 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > On Sat, Apr 13, 2024 at 10:31 AM William Breathitt Gray <wbg@kernel.org> wrote: ... > I applied it as is, if anyone wants it, this can be sent on top of it. Thanks, but I assumed this should go via my tree and as PR to you. At least I have it already in my for-next.
On Wed, Apr 17, 2024 at 10:05 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Wed, Apr 17, 2024 at 12:17 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Sat, Apr 13, 2024 at 10:31 AM William Breathitt Gray <wbg@kernel.org> wrote: > > ... > > > I applied it as is, if anyone wants it, this can be sent on top of it. > > Thanks, but I assumed this should go via my tree and as PR to you. At > least I have it already in my for-next. > You didn't respond in any way about picking it up. Can you just drop it from your branch? Bart
On Wed, Apr 17, 2024 at 9:19 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > On Wed, Apr 17, 2024 at 10:05 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Wed, Apr 17, 2024 at 12:17 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > On Sat, Apr 13, 2024 at 10:31 AM William Breathitt Gray <wbg@kernel.org> wrote: ... > > > I applied it as is, if anyone wants it, this can be sent on top of it. > > > > Thanks, but I assumed this should go via my tree and as PR to you. At > > least I have it already in my for-next. > > You didn't respond in any way about picking it up. Hmm... I'm the author of it and I'm a maintainer for that driver. I'm not sure if it's mandatory to respond for that purpose. Usually I asked the opposite, i.e. when I'm not going to pick the thing up. > Can you just drop > it from your branch? It's possible, but I will need to rebase, which is not a good thing to perform. What about just leaving it as is and letting git to (nicely) solve this?
On Wed, Apr 17, 2024 at 10:41 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Wed, Apr 17, 2024 at 9:19 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Wed, Apr 17, 2024 at 10:05 AM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Wed, Apr 17, 2024 at 12:17 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > On Sat, Apr 13, 2024 at 10:31 AM William Breathitt Gray <wbg@kernel.org> wrote: > > ... > > > > > I applied it as is, if anyone wants it, this can be sent on top of it. > > > > > > Thanks, but I assumed this should go via my tree and as PR to you. At > > > least I have it already in my for-next. > > > > You didn't respond in any way about picking it up. > > Hmm... I'm the author of it and I'm a maintainer for that driver. I'm > not sure if it's mandatory to respond for that purpose. Usually I > asked the opposite, i.e. when I'm not going to pick the thing up. > > > Can you just drop > > it from your branch? > > It's possible, but I will need to rebase, which is not a good thing to > perform. What about just leaving it as is and letting git to (nicely) > solve this? > > -- > With Best Regards, > Andy Shevchenko It won't be solved nicely, we'll get a warning about the same commit appearing twice with different hashes. Whatever, I dropped it from my tree, it was the HEAD anyway. Bart
On Wed, Apr 17, 2024 at 11:46 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > On Wed, Apr 17, 2024 at 10:41 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Wed, Apr 17, 2024 at 9:19 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > On Wed, Apr 17, 2024 at 10:05 AM Andy Shevchenko > > > <andy.shevchenko@gmail.com> wrote: > > > > On Wed, Apr 17, 2024 at 12:17 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > > On Sat, Apr 13, 2024 at 10:31 AM William Breathitt Gray <wbg@kernel.org> wrote: ... > > > > > I applied it as is, if anyone wants it, this can be sent on top of it. > > > > > > > > Thanks, but I assumed this should go via my tree and as PR to you. At > > > > least I have it already in my for-next. > > > > > > You didn't respond in any way about picking it up. > > > > Hmm... I'm the author of it and I'm a maintainer for that driver. I'm > > not sure if it's mandatory to respond for that purpose. Usually I > > asked the opposite, i.e. when I'm not going to pick the thing up. > > > > > Can you just drop > > > it from your branch? > > > > It's possible, but I will need to rebase, which is not a good thing to > > perform. What about just leaving it as is and letting git to (nicely) > > solve this? > > It won't be solved nicely, we'll get a warning about the same commit > appearing twice with different hashes. Oh, this sounds like a new check in Linux Next? Or somewhere else? > Whatever, I dropped it from my tree, it was the HEAD anyway. Thank you!
diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c index e48392074e4b8..0c765558d03fc 100644 --- a/drivers/gpio/gpio-sch.c +++ b/drivers/gpio/gpio-sch.c @@ -38,8 +38,8 @@ struct sch_gpio { struct gpio_chip chip; + void __iomem *regs; spinlock_t lock; - unsigned short iobase; unsigned short resume_base; /* GPE handling */ @@ -75,7 +75,7 @@ static int sch_gpio_reg_get(struct sch_gpio *sch, unsigned int gpio, unsigned in offset = sch_gpio_offset(sch, gpio, reg); bit = sch_gpio_bit(sch, gpio); - reg_val = !!(inb(sch->iobase + offset) & BIT(bit)); + reg_val = !!(ioread8(sch->regs + offset) & BIT(bit)); return reg_val; } @@ -89,12 +89,14 @@ static void sch_gpio_reg_set(struct sch_gpio *sch, unsigned int gpio, unsigned i offset = sch_gpio_offset(sch, gpio, reg); bit = sch_gpio_bit(sch, gpio); - reg_val = inb(sch->iobase + offset); + reg_val = ioread8(sch->regs + offset); if (val) - outb(reg_val | BIT(bit), sch->iobase + offset); + reg_val |= BIT(bit); else - outb((reg_val & ~BIT(bit)), sch->iobase + offset); + reg_val &= ~BIT(bit); + + iowrite8(reg_val, sch->regs + offset); } static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned int gpio_num) @@ -267,8 +269,8 @@ static u32 sch_gpio_gpe_handler(acpi_handle gpe_device, u32 gpe, void *context) spin_lock_irqsave(&sch->lock, flags); - core_status = inl(sch->iobase + CORE_BANK_OFFSET + GTS); - resume_status = inl(sch->iobase + RESUME_BANK_OFFSET + GTS); + core_status = ioread32(sch->regs + CORE_BANK_OFFSET + GTS); + resume_status = ioread32(sch->regs + RESUME_BANK_OFFSET + GTS); spin_unlock_irqrestore(&sch->lock, flags); @@ -319,9 +321,11 @@ static int sch_gpio_install_gpe_handler(struct sch_gpio *sch) static int sch_gpio_probe(struct platform_device *pdev) { + struct device *dev = &pdev->dev; struct gpio_irq_chip *girq; struct sch_gpio *sch; struct resource *res; + void __iomem *regs; int ret; sch = devm_kzalloc(&pdev->dev, sizeof(*sch), GFP_KERNEL); @@ -332,12 +336,13 @@ static int sch_gpio_probe(struct platform_device *pdev) if (!res) return -EBUSY; - if (!devm_request_region(&pdev->dev, res->start, resource_size(res), - pdev->name)) + regs = devm_ioport_map(dev, res->start, resource_size(res)); + if (!regs) return -EBUSY; + sch->regs = regs; + spin_lock_init(&sch->lock); - sch->iobase = res->start; sch->chip = sch_gpio_chip; sch->chip.label = dev_name(&pdev->dev); sch->chip.parent = &pdev->dev;
Convert driver to use memory mapped IO accessors. Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> --- drivers/gpio/gpio-sch.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-)