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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>
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
<[email protected]> 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 <[email protected]>
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
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 <[email protected]>
> ---
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 <[email protected]>
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.
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.
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
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.
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.
On Fri, Sep 28, 2018 at 2:14 PM Jeffrey Hugo <[email protected]> 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.
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 <[email protected]> wrote:
>
> On Fri, Sep 28, 2018 at 2:14 PM Jeffrey Hugo <[email protected]> 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
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.
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.
On Fri, Sep 28, 2018 at 9:30 PM Ricardo Ribalda Delgado
<[email protected]> 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
Hi Linus
On Mon, Oct 1, 2018 at 1:54 PM Linus Walleij <[email protected]> wrote:
>
> On Fri, Sep 28, 2018 at 9:30 PM Ricardo Ribalda Delgado
> <[email protected]> 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
On 10/1/2018 7:36 AM, Ricardo Ribalda Delgado wrote:
> Hi Linus
> On Mon, Oct 1, 2018 at 1:54 PM Linus Walleij <[email protected]> wrote:
>>
>> On Fri, Sep 28, 2018 at 9:30 PM Ricardo Ribalda Delgado
>> <[email protected]> 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.
On Mon, Oct 1, 2018 at 3:36 PM Ricardo Ribalda Delgado
<[email protected]> wrote:
> On Mon, Oct 1, 2018 at 1:54 PM Linus Walleij <[email protected]> wrote:
> > On Fri, Sep 28, 2018 at 9:30 PM Ricardo Ribalda Delgado
> > <[email protected]> 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
Hi
On Mon, Oct 1, 2018 at 11:20 PM Linus Walleij <[email protected]> wrote:
>
> On Mon, Oct 1, 2018 at 3:36 PM Ricardo Ribalda Delgado
> <[email protected]> wrote:
> > On Mon, Oct 1, 2018 at 1:54 PM Linus Walleij <[email protected]> wrote:
> > > On Fri, Sep 28, 2018 at 9:30 PM Ricardo Ribalda Delgado
> > > <[email protected]> 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
On Tue, Oct 2, 2018 at 9:15 AM Ricardo Ribalda Delgado
<[email protected]> wrote:
> On Mon, Oct 1, 2018 at 11:20 PM Linus Walleij <[email protected]> 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
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.
On Tue, Oct 2, 2018 at 2:26 PM Timur Tabi <[email protected]> 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