gpio: zynq: add a to_irq implementation

This function is required by various gpio consumer implementations as
well as the generic gpiolib-sysfs "edge" attribute.

Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
---
 drivers/gpio/gpio-zynq.c | 8 ++++++++
 1 file changed, 8 insertions(+)

czw., 16 maj 2019 o 20:23 Jean-Francois Dagenais
<jeff.dagenais@gmail.com> napisał(a):
>
> This function is required by various gpio consumer implementations as
> well as the generic gpiolib-sysfs "edge" attribute.
>
> Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
> ---
>  drivers/gpio/gpio-zynq.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
> index 9392edaeec3f..19ba8801a4f2 100644
> --- a/drivers/gpio/gpio-zynq.c
> +++ b/drivers/gpio/gpio-zynq.c
> @@ -379,6 +379,13 @@ static int zynq_gpio_get_direction(struct gpio_chip *chip, unsigned int pin)
>         return !(reg & BIT(bank_pin_num));
>  }
>
> +static int zynq_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct zynq_gpio *gpio = gpiochip_get_data(chip);
> +
> +       return irq_find_mapping(gpio->chip.irq.domain, offset);
> +}
> +
>  /**
>   * zynq_gpio_irq_mask - Disable the interrupts for a gpio pin
>   * @irq_data:  per irq and chip data passed down to chip functions
> @@ -870,6 +877,7 @@ static int zynq_gpio_probe(struct platform_device *pdev)
>         chip->direction_input = zynq_gpio_dir_in;
>         chip->direction_output = zynq_gpio_dir_out;
>         chip->get_direction = zynq_gpio_get_direction;
> +       chip->to_irq = zynq_gpio_to_irq;
>         chip->base = of_alias_get_id(pdev->dev.of_node, "gpio");
>         chip->ngpio = gpio->p_data->ngpio;
>
> --
> 2.11.0
>

Hi Jean-Francois,

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.

Bart

Hi Bart,
Thanks for your answer. See below.

> On May 17, 2019, at 03:36, Bartosz Golaszewski <bgolaszewski@baylibre.com> 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?

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;
 	}

pt., 17 maj 2019 o 17:55 Jean-Francois Dagenais
<jeff.dagenais@gmail.com> napisał(a):
>
> Hi Bart,
> Thanks for your answer. See below.
>
> > On May 17, 2019, at 03:36, Bartosz Golaszewski <bgolaszewski@baylibre.com> 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
>

Thank you for your thoughts!

> On May 20, 2019, at 02:38, Bartosz Golaszewski <bgolaszewski@baylibre.com> wrote:
> 
>> 
>> Any thoughts?
>> 
> 
> Yes! Don't use sysfs and especially don't add your own buggy
> interfaces?

aawww... but I like adding a bunch of "buggy" interfaces here and there to make
sure my customers get a challenge when using my product. :P

BTW, I've always been fully aware, when I made this change in gpiolib, that I
would not be upstreaming it. It's been years and we're quite happy with what it
allowed us to do. Only now, because I needed to use the "edge" attribute, have I
had any problems with this approach. So it has served us well this far. I was
just long-shot probing you the other day.

> Is there any reason you can't use libgpiod and the
> character device?

To be honest, I had only glanced quite a while back at gpiolib and thought it
was a bit overkill at that stage of our development to include another C library
and methods into the mix. Later, I forgot about it. Looking at it again here I
can see the command line tools do satisfy a great part of the "simplicity"
argument. I will most likely add gpiolib to my base yocto image, thanks!

> What does your own class provide that none of the
> upstream interfaces do?

I have not fully explored gpiolib's API yet so I may be missing something but
the hog/auto-export I use allows for the hardware pin to be described
functionally all the way to userspace and abstract what chip a line is on and
whether it is active_low or high.

So for example a line name could be MYSUBCIRCUIT_RESET. From the DTS, that line
name is given, but in the hog/auto-export, it would be shown at
/sys/class/gpio/mysubcircuit_enable. So a functional name is given
(mysubcircuit_enable), along with ACTIVE_LOW to abstract the line polarity. That
changes this particular gpio's function to a "cleaner" api (an enable gpio
rather than a reset gpio). All along, userspace is none the wiser in that
hardware "details", other than the gpio functional name, need not be present in
the userspace consumer.

This is akin to having positive boolean in a program. Hardware designers have
different physical reasons for making "not" signals (inverted). This should stay
in the DTS where we already describe the hardware.

If you can describe a pin "functionally" from DTS and still use gpiolib, I will
stand corrected.

Thanks for your feedback!