On Mon, Oct 2, 2017 at 10:47 PM, Timur Tabi [off-list ref] wrote:

On 10/02/2017 12:44 PM, Bjorn Andersson wrote:

quoted

quoted

+       /*
+        * If irq_need_valid_mask is true, then gpiochip_add_data() will
+        * initialize irq_valid_mask to all 1s.  We need to clear all the
+        * GPIOs that are unavailable, and we need to find each block
+        * of consecutive available GPIOs are add them as pin ranges.
+        */
+       if (chip->irq_need_valid_mask) {
+               for (i = 0; i < ngpio; i++)
+                       if (!groups[i].npins)
+                               clear_bit(i, pctrl->chip.irq_valid_mask);
+
+               while ((count = msm_gpio_get_next_range(pctrl, &start)))
{
+                       ret = gpiochip_add_pin_range(&pctrl->chip,
+
dev_name(pctrl->dev),
+                                                    start, start,
count);
+                       if (ret)
+                               break;
+                       start += count;

I do not fancy the idea of specifying a bitmap of valid irq pins and
then having the driver register the pin-ranges in-between.

But that's exactly what abx500_gpio_probe() in pinctrl-abx500.c does. Here's
even a reference to holes in the GPIO space:

This driver is not a good example of what is desireable.

I am sorry that the kernel contains a lot of bad examples. These
are historical artifacts, they cannot be used as an argument to write
code in the same style.

quoted

If we provide

a bitmap of validity to the core it should support using this for the
pins as well. (Which I believe is what Linus answered in the discussion
following patch 0/2)

So you want to change "gpio_chip" to add an "available" callback?  And every
time gpiolib wants to call a gpio_chip callback, it should call ->available
first?  Like this:

if (chip->available && chip->available())
        status = chip->direction_input(chip, gpio_chip_hwgpio(desc));

I mean that you add
unsigned long *line_valid_mask;
to struct gpio_chip using the same type of logic
as .irq_valid_mask. The mask should be optional.

Then the operation gpiod_get[_index]() will FAIL if the
line is not valid.

There is no need to check on every operation, there should
be no way to get a handle on the pin any other way.

All new code should be using descriptors at this point so we
only need to design for that case.

Yours,
Linus Walleij