Re: [PATCH] gpio: zynq: add a to_irq implementation

[Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>]

pt., 17 maj 2019 o 17:55 Jean-Francois Dagenais
<jeff.dagenais@xxxxxxxxx> napisał(a):
>
> Hi Bart,
> Thanks for your answer. See below.
>
> > On May 17, 2019, at 03:36, Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx> wrote:
> >
> > the default implementation for this function is already assigned
> > inside the call to gpiochip_irqchip_add() and it does the same thing
> > internally (looks up the mapping from the domain). If there's a
> > problem with this driver then the culprit lies somewhere else.
>
> Indeed. I totally missed that. So yeah, disregard my patch.
>
> But then my problem still remains...
>
> The root cause of the "edge" attribute missing is that the exported pins have
> been exported through my own version of gpio-hog-auto-sysfs-exported from DTS
> ("linux,gpio-export") patch :-/
> Inception came from: https://www.spinics.net/lists/devicetree/msg08604.html
>
> So under my gpio controller node:
>         boardid_0 {
>                 gpio-hog;
>                 linux,gpio-export;
>                 gpios = <33 GPIO_ACTIVE_HIGH>;
>                 input;
>                 line-name = "boardid_0";
>         };
>
> makes /sys/class/gpio/boardid_0 appear automatically. The DTS is a natural fit
> for such information in my opinion. No init script is required so it just works
> in all images I make (initrd or real rootfs) without extra dependencies. The
> cost is about 15 lines of code in the kernel.
>
> I modified of_gpiochip_add to flag "FLAG_AUTO_EXPORT" each linux,gpio-export
> marked hogged pins.
>
> Then in gpiochip_add_data, which runs after of_gpiochip_add, I call my
> gpiochip_auto_export to scan pins for "FLAG_AUTO_EXPORT" and export them.
>
> The problem is that when all this occurs from zynq_gpio_probe/gpiochip_add_data,
> gpiochip_irqchip_add has not been called yet, so to_irq is still NULL.
>
> Perhaps I should defer my auto-export operation to another point (like at the
> end of zynq_gpio_probe? But then I have to do the same for another i2c
> io-expander chip we have that also exports pins. Or a tasklet...? (yikes. no.)
>
> Any thoughts?
>

Yes! Don't use sysfs and especially don't add your own buggy
interfaces? Is there any reason you can't use libgpiod and the
character device? What does your own class provide that none of the
upstream interfaces do?

Bart

> If I can make it work correctly in gpiolib, any point in submitting a patch?
>
> Cheers!
>
> P.S. Here's my current workaround after your comeback:
>
> diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
> index 694d6884e451..ec8ca101041f 100644
> --- a/drivers/gpio/gpiolib-sysfs.c
> +++ b/drivers/gpio/gpiolib-sysfs.c
> @@ -364,8 +364,6 @@ static umode_t gpio_is_visible(struct kobject *kobj, struct attribute *attr,
>                 if (!show_direction)
>                         mode = 0;
>         } else if (attr == &dev_attr_edge.attr) {
> -               if (gpiod_to_irq(desc) < 0)
> -                       mode = 0;
>                 if (!show_direction && test_bit(FLAG_IS_OUT, &desc->flags))
>                         mode = 0;
>         }
> --
> 2.11.0
>