[PATCH] gpiolib: Fix gpio_direction_* for single direction GPIOs

[PATCH] gpiolib: Fix gpio_direction_* for single direction GPIOs

[Ricardo Ribalda Delgado]

GPIOs with no programmable direction are not required to implement
direction_output nor direction_input.

If we try to set an output direction on an output-only GPIO or input
direction on an input-only GPIO simply return 0.

This allows this single direction GPIO to be used by libgpiod.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/gpio/gpiolib.c | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index a57300c1d649..4b45de883ada 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2512,19 +2512,27 @@ EXPORT_SYMBOL_GPL(gpiochip_free_own_desc);
 int gpiod_direction_input(struct gpio_desc *desc)
 {
 	struct gpio_chip	*chip;
-	int			status = -EINVAL;
+	int			status = 0;
 
 	VALIDATE_DESC(desc);
 	chip = desc->gdev->chip;
 
-	if (!chip->get || !chip->direction_input) {
+	if (!chip->get && chip->direction_input) {
 		gpiod_warn(desc,
-			"%s: missing get() or direction_input() operations\n",
+			"%s: missing get() and direction_input() operations\n",
 			__func__);
 		return -EIO;
 	}
 
-	status = chip->direction_input(chip, gpio_chip_hwgpio(desc));
+	if (chip->direction_input) {
+		status = chip->direction_input(chip, gpio_chip_hwgpio(desc));
+	} else if (chip->get_direction &&
+		  (chip->get_direction(chip, gpio_chip_hwgpio(desc)) != 1)) {
+		gpiod_warn(desc,
+			"%s: missing direction_input() operation\n",
+			__func__);
+		return -EIO;
+	}
 	if (status == 0)
 		clear_bit(FLAG_IS_OUT, &desc->flags);
 
@@ -2546,16 +2554,28 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
 {
 	struct gpio_chip *gc = desc->gdev->chip;
 	int val = !!value;
-	int ret;
+	int ret = 0;
 
-	if (!gc->set || !gc->direction_output) {
+	if (!gc->set && !gc->direction_output) {
 		gpiod_warn(desc,
-		       "%s: missing set() or direction_output() operations\n",
+		       "%s: missing set() and direction_output() operations\n",
 		       __func__);
 		return -EIO;
 	}
 
-	ret = gc->direction_output(gc, gpio_chip_hwgpio(desc), val);
+	if (gc->direction_output) {
+		ret = gc->direction_output(gc, gpio_chip_hwgpio(desc), val);
+	} else {
+		if (gc->get_direction &&
+		    gc->get_direction(gc, gpio_chip_hwgpio(desc))) {
+			gpiod_warn(desc,
+				"%s: missing direction_output() operation\n",
+				__func__);
+			return -EIO;
+		}
+		gc->set(gc, gpio_chip_hwgpio(desc), val);
+	}
+
 	if (!ret)
 		set_bit(FLAG_IS_OUT, &desc->flags);
 	trace_gpio_value(desc_to_gpio(desc), 0, val);
-- 
2.18.0

[PATCH v2] gpiolib: Show correct direction from the beginning

[Ricardo Ribalda Delgado]

Current code assumes that the direction is input if direction_input
function is set.
This might not be the case on GPIOs with programmable direction.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/gpio/gpiolib.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 4b45de883ada..00c17f64d9ff 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1352,7 +1352,12 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data,
 		 * it does, and in case chip->get_direction is not set, we may
 		 * expose the wrong direction in sysfs.
 		 */
-		desc->flags = !chip->direction_input ? (1 << FLAG_IS_OUT) : 0;
+		if (chip->get_direction && gpiochip_line_is_valid(chip, i))
+			desc->flags = !chip->get_direction(chip, i) ?
+					(1 << FLAG_IS_OUT) : 0;
+		else
+			desc->flags = !chip->direction_input ?
+					(1 << FLAG_IS_OUT) : 0;
 	}
 
 #ifdef CONFIG_PINCTRL
-- 
2.18.0

Re: [PATCH v2] gpiolib: Show correct direction from the beginning

[Timur Tabi]

Jeff, can you test these two patches on Amberwing to make sure that they 
don't cause an XPU violation on boot?

The call to gpiochip_line_is_valid() should return false on any GPIOs 
that aren't listed in the ACPI table.

My concern is that this patch might be calling gpiochip_line_is_valid() 
too early, before all the arrays have been set up.

Thanks.

On 9/21/18 5:36 AM, Ricardo Ribalda Delgado wrote:
> Current code assumes that the direction is input if direction_input
> function is set.
> This might not be the case on GPIOs with programmable direction.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
>   drivers/gpio/gpiolib.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 4b45de883ada..00c17f64d9ff 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1352,7 +1352,12 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data,
>   		 * it does, and in case chip->get_direction is not set, we may
>   		 * expose the wrong direction in sysfs.
>   		 */
> -		desc->flags = !chip->direction_input ? (1 << FLAG_IS_OUT) : 0;
> +		if (chip->get_direction && gpiochip_line_is_valid(chip, i))
> +			desc->flags = !chip->get_direction(chip, i) ?
> +					(1 << FLAG_IS_OUT) : 0;
> +		else
> +			desc->flags = !chip->direction_input ?
> +					(1 << FLAG_IS_OUT) : 0;
>   	}
>   
>   #ifdef CONFIG_PINCTRL
> 

Re: [PATCH] gpiolib: Fix gpio_direction_* for single direction GPIOs

[Linus Walleij]

Hi Ricardo,

thanks for the patch and sorry for taking time before responding.

On Fri, Sep 21, 2018 at 12:36 PM Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
> GPIOs with no programmable direction are not required to implement
> direction_output nor direction_input.
>
> If we try to set an output direction on an output-only GPIO or input
> direction on an input-only GPIO simply return 0.
>
> This allows this single direction GPIO to be used by libgpiod.
>
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>

It makes perfect sense, patch applied.

I'll go in and add some comments to the code so I understand it
right as well in the future.

Yours,
Linus Walleij

Re: [PATCH v2] gpiolib: Show correct direction from the beginning

[Stephen Boyd]

Quoting Ricardo Ribalda Delgado (2018-09-21 03:36:04)
> Current code assumes that the direction is input if direction_input
> function is set.
> This might not be the case on GPIOs with programmable direction.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---

Looks OK to me visually. I haven't tested it because I don't have access
to the locked down hardware anymore.

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH v2] gpiolib: Show correct direction from the beginning

[Timur Tabi]

On 9/27/18 1:51 AM, Stephen Boyd wrote:
> Looks OK to me visually. I haven't tested it because I don't have access
> to the locked down hardware anymore.

Same here.  Please wait for Jeff Hugo to test it before applying.  Thanks.

Re: [PATCH v2] gpiolib: Show correct direction from the beginning

[Jeffrey Hugo]

On 9/27/2018 6:19 AM, Timur Tabi wrote:
> On 9/27/18 1:51 AM, Stephen Boyd wrote:
>> Looks OK to me visually. I haven't tested it because I don't have access
>> to the locked down hardware anymore.
> 
> Same here.  Please wait for Jeff Hugo to test it before applying.  Thanks.

I guess its lucky I saw this then.

I should be able to test within a week.

-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v2] gpiolib: Show correct direction from the beginning

[Timur Tabi]

On 9/27/18 9:04 AM, Jeffrey Hugo wrote:
> 
> I guess its lucky I saw this then.

Did you not get this email: 
https://lore.kernel.org/patchwork/patch/989545/#1173771

Re: [PATCH v2] gpiolib: Show correct direction from the beginning

[Jeffrey Hugo]

On 9/27/2018 8:19 AM, Timur Tabi wrote:
> On 9/27/18 9:04 AM, Jeffrey Hugo wrote:
>>
>> I guess its lucky I saw this then.
> 
> Did you not get this email: 
> https://lore.kernel.org/patchwork/patch/989545/#1173771

Apparently I did.  I found it in my deleted items.  I must have 
accidentally done that when sorting through my backlog after Linaro 
Connect.  Sorry about that.

-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v2] gpiolib: Show correct direction from the beginning

[Jeffrey Hugo]

On 9/27/2018 8:04 AM, Jeffrey Hugo wrote:
> On 9/27/2018 6:19 AM, Timur Tabi wrote:
>> On 9/27/18 1:51 AM, Stephen Boyd wrote:
>>> Looks OK to me visually. I haven't tested it because I don't have access
>>> to the locked down hardware anymore.
>>
>> Same here.  Please wait for Jeff Hugo to test it before applying.  
>> Thanks.
> 
> I guess its lucky I saw this then.
> 
> I should be able to test within a week.
> 

Nack.  Causes a XPU violation to the GPIO config registers.

-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v2] gpiolib: Show correct direction from the beginning

[Timur Tabi]

On Fri, Sep 28, 2018 at 2:14 PM Jeffrey Hugo <jhugo@codeaurora.org> wrote:
> Nack.  Causes a XPU violation to the GPIO config registers.

That doesn't surprise me at all.

I believe that the problem is that gpiochip_line_is_valid() is being
called before gpiochip_irqchip_init_valid_mask() is called, which
means that gpiochip_line_is_valid() always returns true.

Re: [PATCH v2] gpiolib: Show correct direction from the beginning

[Ricardo Ribalda Delgado]

Hi Timur

In fact  gpiochip_init_valid_mask is called some lines after in the
same function. We could reorder the function. Would that work for you?

The driver breaking is upstream? Is it possible to access the hardware?

Thanks

[Sorry for the two html mails in a row, I did not try to work with my
phone before :) ]
On Fri, Sep 28, 2018 at 9:23 PM Timur Tabi <timur@kernel.org> wrote:
>
> On Fri, Sep 28, 2018 at 2:14 PM Jeffrey Hugo <jhugo@codeaurora.org> wrote:
> > Nack.  Causes a XPU violation to the GPIO config registers.
>
> That doesn't surprise me at all.
>
> I believe that the problem is that gpiochip_line_is_valid() is being
> called before gpiochip_irqchip_init_valid_mask() is called, which
> means that gpiochip_line_is_valid() always returns true.



-- 
Ricardo Ribalda

Re: [PATCH v2] gpiolib: Show correct direction from the beginning

[Timur Tabi]

On 9/29/18 1:23 AM, Ricardo Ribalda Delgado wrote:
> In fact  gpiochip_init_valid_mask is called some lines after in the
> same function. We could reorder the function. Would that work for you?

It might.  It might break something else, though.

> The driver breaking is upstream?

Yes.

> Is it possible to access the hardware?

Linaro and some Linux OSVs should still have systems, but I usually just 
ask Jeff to test code for me.

Re: [PATCH v2] gpiolib: Show correct direction from the beginning

[Timur Tabi]

On 9/29/18 8:21 AM, Timur Tabi wrote:
> 
>> Is it possible to access the hardware?
> 
> Linaro and some Linux OSVs should still have systems, but I usually just 
> ask Jeff to test code for me.

Alternatively, you can just add valid_mask support to your driver, and 
add a check to your get_direction() function to see if it ever is asked 
to access an invalid GPIO.

Re: [PATCH v2] gpiolib: Show correct direction from the beginning

[Linus Walleij]

On Fri, Sep 28, 2018 at 9:30 PM Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:

> How do we proceed from here? Can you fix your driver somehow to
> init the valid mask before enabling the gpio?

Just include a hunk to the qcom driver reordering this call
at the same time. No need to make it separate patches,
it need to be tested together anyways.

I guess just switch the order of these two:

        ret = gpiochip_add_data(&pctrl->chip, pctrl);
        if (ret) {
                dev_err(pctrl->dev, "Failed register gpiochip\n");
                return ret;
        }

        ret = msm_gpio_init_valid_mask(chip, pctrl);
        if (ret) {
                dev_err(pctrl->dev, "Failed to setup irq valid bits\n");
                gpiochip_remove(&pctrl->chip);
                return ret;
        }

> Do we need to make more severe changes on the core?

Don't think so.

Yours,
Linus Walleij

Re: [PATCH v2] gpiolib: Show correct direction from the beginning

[Ricardo Ribalda Delgado]

Hi Linus
On Mon, Oct 1, 2018 at 1:54 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Fri, Sep 28, 2018 at 9:30 PM Ricardo Ribalda Delgado
> <ricardo.ribalda@gmail.com> wrote:
>
> > How do we proceed from here? Can you fix your driver somehow to
> > init the valid mask before enabling the gpio?
>
> Just include a hunk to the qcom driver reordering this call
> at the same time. No need to make it separate patches,
> it need to be tested together anyways.
>
> I guess just switch the order of these two:
>
>         ret = gpiochip_add_data(&pctrl->chip, pctrl);
>         if (ret) {
>                 dev_err(pctrl->dev, "Failed register gpiochip\n");
>                 return ret;
>         }
>
>         ret = msm_gpio_init_valid_mask(chip, pctrl);
>         if (ret) {
>                 dev_err(pctrl->dev, "Failed to setup irq valid bits\n");
>                 gpiochip_remove(&pctrl->chip);
>                 return ret;
>         }
>

the problem is that valid_mask is not a long/integer, is a struct that
needs to be malloced, and is malloc at gpiochip_add_data :(

Maybe we need a callback from the driver to init that mask just after
the allocation?

A fast grep shows that the only driver using need_valid_mask (not for
irq) is msm:

ricardo@neopili:~/curro/kernel-upstream$ git grep "need_valid_mask ="
| grep -v irq
drivers/gpio/gpiolib.c: gpiochip->need_valid_mask = true;
drivers/pinctrl/intel/pinctrl-cherryview.c: bool need_valid_mask =
!dmi_check_system(chv_no_valid_mask);
drivers/pinctrl/qcom/pinctrl-msm.c: chip->need_valid_mask =
msm_gpio_needs_valid_mask(pctrl);

so hacking something in the driver might not be a terrible idea.



Also

> > Do we need to make more severe changes on the core?
>
> Don't think so.
>
> Yours,
> Linus Walleij



-- 
Ricardo Ribalda

Re: [PATCH v2] gpiolib: Show correct direction from the beginning

[Jeffrey Hugo]

On 10/1/2018 7:36 AM, Ricardo Ribalda Delgado wrote:
> Hi Linus
> On Mon, Oct 1, 2018 at 1:54 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>>
>> On Fri, Sep 28, 2018 at 9:30 PM Ricardo Ribalda Delgado
>> <ricardo.ribalda@gmail.com> wrote:
>>
>>> How do we proceed from here? Can you fix your driver somehow to
>>> init the valid mask before enabling the gpio?
>>
>> Just include a hunk to the qcom driver reordering this call
>> at the same time. No need to make it separate patches,
>> it need to be tested together anyways.
>>
>> I guess just switch the order of these two:
>>
>>          ret = gpiochip_add_data(&pctrl->chip, pctrl);
>>          if (ret) {
>>                  dev_err(pctrl->dev, "Failed register gpiochip\n");
>>                  return ret;
>>          }
>>
>>          ret = msm_gpio_init_valid_mask(chip, pctrl);
>>          if (ret) {
>>                  dev_err(pctrl->dev, "Failed to setup irq valid bits\n");
>>                  gpiochip_remove(&pctrl->chip);
>>                  return ret;
>>          }
>>
> 
> the problem is that valid_mask is not a long/integer, is a struct that
> needs to be malloced, and is malloc at gpiochip_add_data :(
> 
> Maybe we need a callback from the driver to init that mask just after
> the allocation?
> 
> A fast grep shows that the only driver using need_valid_mask (not for
> irq) is msm:
> 
> ricardo@neopili:~/curro/kernel-upstream$ git grep "need_valid_mask ="
> | grep -v irq
> drivers/gpio/gpiolib.c: gpiochip->need_valid_mask = true;
> drivers/pinctrl/intel/pinctrl-cherryview.c: bool need_valid_mask =
> !dmi_check_system(chv_no_valid_mask);
> drivers/pinctrl/qcom/pinctrl-msm.c: chip->need_valid_mask =
> msm_gpio_needs_valid_mask(pctrl);
> 
> so hacking something in the driver might not be a terrible idea.
> 

IMO, hack up the driver and I'll test it.  We can figure out what is 
needed to work, then determine what is the proper solution for that.

-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v2] gpiolib: Show correct direction from the beginning

[Linus Walleij]

On Mon, Oct 1, 2018 at 3:36 PM Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
> On Mon, Oct 1, 2018 at 1:54 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Fri, Sep 28, 2018 at 9:30 PM Ricardo Ribalda Delgado
> > <ricardo.ribalda@gmail.com> wrote:
> >
> > > How do we proceed from here? Can you fix your driver somehow to
> > > init the valid mask before enabling the gpio?
> >
> > Just include a hunk to the qcom driver reordering this call
> > at the same time. No need to make it separate patches,
> > it need to be tested together anyways.
> >
> > I guess just switch the order of these two:
> >
> >         ret = gpiochip_add_data(&pctrl->chip, pctrl);
> >         if (ret) {
> >                 dev_err(pctrl->dev, "Failed register gpiochip\n");
> >                 return ret;
> >         }
> >
> >         ret = msm_gpio_init_valid_mask(chip, pctrl);
> >         if (ret) {
> >                 dev_err(pctrl->dev, "Failed to setup irq valid bits\n");
> >                 gpiochip_remove(&pctrl->chip);
> >                 return ret;
> >         }
> >
>
> the problem is that valid_mask is not a long/integer, is a struct that
> needs to be malloced, and is malloc at gpiochip_add_data :(

I don't get it, but maybe I'm not smart enough.

gpiochip_add_data() doesn't allocate anything, it
just adds a already allocated (or static!) gpio_chip
to the gpiolib subsystem.

In fact I think it is wrong to set up the mask after
calling gpiolob_add_data(), because of exactly the
type of problem you're seeing.

Don't get confused by the &pctrl->chip
vs just chip variables, it's just some sloppiness.

Yours,
Linus Walleij

Re: [PATCH v2] gpiolib: Show correct direction from the beginning

[Ricardo Ribalda Delgado]

Hi
On Mon, Oct 1, 2018 at 11:20 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Mon, Oct 1, 2018 at 3:36 PM Ricardo Ribalda Delgado
> <ricardo.ribalda@gmail.com> wrote:
> > On Mon, Oct 1, 2018 at 1:54 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> > > On Fri, Sep 28, 2018 at 9:30 PM Ricardo Ribalda Delgado
> > > <ricardo.ribalda@gmail.com> wrote:
> > >
> > > > How do we proceed from here? Can you fix your driver somehow to
> > > > init the valid mask before enabling the gpio?
> > >
> > > Just include a hunk to the qcom driver reordering this call
> > > at the same time. No need to make it separate patches,
> > > it need to be tested together anyways.
> > >
> > > I guess just switch the order of these two:
> > >
> > >         ret = gpiochip_add_data(&pctrl->chip, pctrl);
> > >         if (ret) {
> > >                 dev_err(pctrl->dev, "Failed register gpiochip\n");
> > >                 return ret;
> > >         }
> > >
> > >         ret = msm_gpio_init_valid_mask(chip, pctrl);
> > >         if (ret) {
> > >                 dev_err(pctrl->dev, "Failed to setup irq valid bits\n");
> > >                 gpiochip_remove(&pctrl->chip);
> > >                 return ret;
> > >         }
> > >
> >
> > the problem is that valid_mask is not a long/integer, is a struct that
> > needs to be malloced, and is malloc at gpiochip_add_data :(
>
> I don't get it, but maybe I'm not smart enough.

Dont take my job, I am the not smart of the conversation :P

>
> gpiochip_add_data() doesn't allocate anything, it
> just adds a already allocated (or static!) gpio_chip
> to the gpiolib subsystem.
>

Take a look to gpiochip_add_data_with_key()
  ->gpiochip_init_valid_mask()
       -> gpiochip->valid_mask = gpiochip_allocate_mask(gpiochip);



> In fact I think it is wrong to set up the mask after
> calling gpiolob_add_data(), because of exactly the
> type of problem you're seeing.

I agree, and I believe that the cleaner way would be to add a function
pointer that replaces:

gpiochip_allocate_mask()
  bitmap_fill(p, chip->ngpio);

with a proper initalization from the driver

But as today the only driver that seems to be using valid_mask is msm,
so perhaps a hack is something better and then when we have a second
driver that requires it we figure out the real requirements. But it is
definately your decision ;)


>
> Don't get confused by the &pctrl->chip
> vs just chip variables, it's just some sloppiness.
>
> Yours,
> Linus Walleij

Thanks!

-- 
Ricardo Ribalda

Re: [PATCH v2] gpiolib: Show correct direction from the beginning

[Linus Walleij]

On Tue, Oct 2, 2018 at 9:15 AM Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
> On Mon, Oct 1, 2018 at 11:20 PM Linus Walleij <linus.walleij@linaro.org> wrote:

> > gpiochip_add_data() doesn't allocate anything, it
> > just adds a already allocated (or static!) gpio_chip
> > to the gpiolib subsystem.
>
> Take a look to gpiochip_add_data_with_key()
>   ->gpiochip_init_valid_mask()
>        -> gpiochip->valid_mask = gpiochip_allocate_mask(gpiochip);

Aha I see...

> > In fact I think it is wrong to set up the mask after
> > calling gpiolob_add_data(), because of exactly the
> > type of problem you're seeing.
>
> I agree, and I believe that the cleaner way would be to add a function
> pointer that replaces:
>
> gpiochip_allocate_mask()
>   bitmap_fill(p, chip->ngpio);
>
> with a proper initalization from the driver

OK

> But as today the only driver that seems to be using valid_mask is msm,
> so perhaps a hack is something better and then when we have a second
> driver that requires it we figure out the real requirements. But it is
> definately your decision ;)

I would just add some exported function to gpiolib to do what you
need so you can set up the valid_mask before calling
gpiochip_add*.

Yours,
Linus Walleij

Re: [PATCH v2] gpiolib: Show correct direction from the beginning

[Timur Tabi]

On 10/2/18 2:38 AM, Linus Walleij wrote:
>> But as today the only driver that seems to be using valid_mask is msm,
>> so perhaps a hack is something better and then when we have a second
>> driver that requires it we figure out the real requirements. But it is
>> definately your decision;)

Please note that MSM is supposed to be the *first* driver, not the only, 
driver that needs valid_mask.  So let's not make any code changes that 
limit this feature to the MSM driver.

> I would just add some exported function to gpiolib to do what you
> need so you can set up the valid_mask before calling
> gpiochip_add*.

I think that should be okay.  Drivers should know pretty early whether 
they need valid_mask or not.

Re: [PATCH v2] gpiolib: Show correct direction from the beginning

[Linus Walleij]

On Tue, Oct 2, 2018 at 2:26 PM Timur Tabi <timur@kernel.org> wrote:
> On 10/2/18 2:38 AM, Linus Walleij wrote:
> >> But as today the only driver that seems to be using valid_mask is msm,
> >> so perhaps a hack is something better and then when we have a second
> >> driver that requires it we figure out the real requirements. But it is
> >> definately your decision;)
>
> Please note that MSM is supposed to be the *first* driver, not the only,
> driver that needs valid_mask.  So let's not make any code changes that
> limit this feature to the MSM driver.

I just recently encouraged Thierry to reuse this for the Tegra186
driver:
https://marc.info/?l=linux-gpio&m=153787164826821&w=2

Yours,
Linus Walleij