linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] pinctrl: intel: Implements gpio free function
@ 2019-03-21  2:35 zhuchangchun
  2019-03-21  8:44 ` Mika Westerberg
  0 siblings, 1 reply; 18+ messages in thread
From: zhuchangchun @ 2019-03-21  2:35 UTC (permalink / raw)
  To: mika.westerberg, andriy.shevchenko
  Cc: linus.walleij, linux-gpio, linux-kernel, hendychu, zhuchangchun

When we use the gpio to control some peripheral devices,and try to
export the gpio first,then unexport the gpio, we test the signal with
oscilloscope,and find the signal can't meet the requirements,because
after we unexported the gpio,the gpio's register(tx and rx)value can't
be recovered,and this will infruence the device work flow.

We check the gpio's unexport code work flow, then find the gpio's free
hook function has not been implemented, After we add pinmux_ops' free
function to set exported gpio to recover its original value,the problem
is fixed.

Signed-off-by: zhuchangchun <zhuchangchun@cvte.com>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 3b18181..002b9d3 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -480,6 +480,33 @@ static int intel_gpio_set_direction(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
+static int intel_gpio_free(struct pinctrl_dev *pctldev, unsigned int pin)
+{
+	struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	void __iomem *padcfg0;
+	unsigned long flags;
+	u32 value;
+
+	spin_lock_irqsave(&pctrl->lock, flags);
+
+	if (!intel_pad_usable(pctrl, pin)) {
+		spin_unlock_irqrestore(&pctrl->lock, flags);
+		return -EBUSY;
+	}
+	padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0);
+
+	/* Put the pad into GPIO mode */
+	value = readl(padcfg0) & ~PADCFG0_PMODE_MASK;
+	/* enable TX buffer and disable RX (this will be input) */
+	value |= PADCFG0_GPIORXDIS;
+	value &= ~PADCFG0_GPIOTXDIS;
+	writel(value, padcfg0);
+
+	spin_unlock_irqrestore(&pctrl->lock, flags);
+
+	return 0;
+}
+
 static const struct pinmux_ops intel_pinmux_ops = {
 	.get_functions_count = intel_get_functions_count,
 	.get_function_name = intel_get_function_name,
@@ -487,6 +514,7 @@ static const struct pinmux_ops intel_pinmux_ops = {
 	.set_mux = intel_pinmux_set_mux,
 	.gpio_request_enable = intel_gpio_request_enable,
 	.gpio_set_direction = intel_gpio_set_direction,
+	.free = intel_gpio_free,
 };
 
 static int intel_config_get(struct pinctrl_dev *pctldev, unsigned int pin,
-- 
2.7.4




^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH] pinctrl: intel: Implements gpio free function
  2019-03-21  2:35 [PATCH] pinctrl: intel: Implements gpio free function zhuchangchun
@ 2019-03-21  8:44 ` Mika Westerberg
  2019-03-21  9:23   ` Andy Shevchenko
       [not found]   ` <2019032119195575582546@cvte.com>
  0 siblings, 2 replies; 18+ messages in thread
From: Mika Westerberg @ 2019-03-21  8:44 UTC (permalink / raw)
  To: zhuchangchun
  Cc: andriy.shevchenko, linus.walleij, linux-gpio, linux-kernel, hendychu

On Thu, Mar 21, 2019 at 10:35:24AM +0800, zhuchangchun wrote:
> When we use the gpio to control some peripheral devices,and try to
> export the gpio first,then unexport the gpio, we test the signal with
> oscilloscope,and find the signal can't meet the requirements,because
> after we unexported the gpio,the gpio's register(tx and rx)value can't
> be recovered,and this will infruence the device work flow.

After you unexport GPIO it can go back to any previous mode it was. If
you need to use it as GPIO then why unexport it in the first place?

> We check the gpio's unexport code work flow, then find the gpio's free
> hook function has not been implemented, After we add pinmux_ops' free
> function to set exported gpio to recover its original value,the problem
> is fixed.

I don't think this is what ->free callback should do (assuming we decide
to implement it since we don't implement ->release either). It is
supposed to reverse effects of ->request which is what it currently does ;-)

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] pinctrl: intel: Implements gpio free function
  2019-03-21  8:44 ` Mika Westerberg
@ 2019-03-21  9:23   ` Andy Shevchenko
  2019-03-22 18:32     ` Enrico Weigelt, metux IT consult
       [not found]   ` <2019032119195575582546@cvte.com>
  1 sibling, 1 reply; 18+ messages in thread
From: Andy Shevchenko @ 2019-03-21  9:23 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: zhuchangchun, linus.walleij, linux-gpio, linux-kernel, hendychu

On Thu, Mar 21, 2019 at 10:44:20AM +0200, Mika Westerberg wrote:
> On Thu, Mar 21, 2019 at 10:35:24AM +0800, zhuchangchun wrote:
> > When we use the gpio to control some peripheral devices,and try to
> > export the gpio first,then unexport the gpio, we test the signal with
> > oscilloscope,and find the signal can't meet the requirements,because
> > after we unexported the gpio,the gpio's register(tx and rx)value can't
> > be recovered,and this will infruence the device work flow.
> 
> After you unexport GPIO it can go back to any previous mode it was. If
> you need to use it as GPIO then why unexport it in the first place?

...and on top of that GPIO sysfs interface is deprecated.

> > We check the gpio's unexport code work flow, then find the gpio's free
> > hook function has not been implemented, After we add pinmux_ops' free
> > function to set exported gpio to recover its original value,the problem
> > is fixed.
> 
> I don't think this is what ->free callback should do (assuming we decide
> to implement it since we don't implement ->release either). It is
> supposed to reverse effects of ->request which is what it currently does ;-)

Exactly!

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Re: [PATCH] pinctrl: intel: Implements gpio free function
       [not found]   ` <2019032119195575582546@cvte.com>
@ 2019-03-21 12:03     ` Mika Westerberg
       [not found]       ` <2019032120213955866649@cvte.com>
  0 siblings, 1 reply; 18+ messages in thread
From: Mika Westerberg @ 2019-03-21 12:03 UTC (permalink / raw)
  To: zhuchangchun
  Cc: andriy.shevchenko, linus.walleij, linux-gpio, linux-kernel, hendychu

On Thu, Mar 21, 2019 at 07:20:56PM +0800, zhuchangchun@cvte.com wrote:
>    After you unexport GPIO it can go back to any previous mode it was. If
>    you need to use it as GPIO then why unexport it in the first place?
>    --> because we need use the GPIO for device reset, we want to recover
>    the GPIO value after reset done.If we don't unexport it, then reboot
>    the
>    intel SOC(not power off), the GPIO's value will stay until the GPIO be
>    reinit.
>    I tested the GPIO signal with oscilloscope, if  power off the SOC,
>    then
>    power on, the GPIO value can recover its original value. This will
>    infruence
>    the peripheral device working condition.

If I understand correctly you have some peripheral connected to Intel
based board and one GPIO is used to reset the peripheral.

You say it works fine if you power cycle the board but it does not when
you issue 'reboot' command. Did I understood the problem correctly?

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Re: [PATCH] pinctrl: intel: Implements gpio free function
       [not found]       ` <2019032120213955866649@cvte.com>
@ 2019-03-21 12:36         ` Mika Westerberg
       [not found]           ` <2019032121342663125658@cvte.com>
  0 siblings, 1 reply; 18+ messages in thread
From: Mika Westerberg @ 2019-03-21 12:36 UTC (permalink / raw)
  To: zhuchangchun
  Cc: andriy.shevchenko, linus.walleij, linux-gpio, linux-kernel, hendychu

On Thu, Mar 21, 2019 at 08:22:40PM +0800, zhuchangchun@cvte.com wrote:
>    On Thu, Mar 21, 2019 at 07:20:56PM +0800, zhuchangchun@cvte.com wrote:
>    >    After you unexport GPIO it can go back to any previous mode it
>    was. If
>    >    you need to use it as GPIO then why unexport it in the first
>    place?
>    >    --> because we need use the GPIO for device reset, we want to
>    recover
>    >    the GPIO value after reset done.If we don't unexport it, then
>    reboot
>    >    the
>    >    intel SOC(not power off), the GPIO's value will stay until the
>    GPIO be
>    >    reinit.
>    >    I tested the GPIO signal with oscilloscope, if  power off the SOC,
>    >    then
>    >    power on, the GPIO value can recover its original value. This will
>    >    infruence
>    >    the peripheral device working condition.
> 
>    If I understand correctly you have some peripheral connected to Intel
>    based board and one GPIO is used to reset the peripheral.
> 
>    You say it works fine if you power cycle the board but it does not when
>    you issue 'reboot' command. Did I understood the problem correctly?
> 
>    --> Yes ,  so I add the -> free callback, it fixes the problem.

OK, so you then write 1 to the sysfs to assert reset, right? And in your
patch you change the mode to input effectively deasserting the reset.

Why not simply do following instead of unexport?

  # echo 0 > /sys/class/gpio/gpioXX/value

or even

  # echo 0 > /sys/class/gpio/gpioXX/value
  # echo in > /sys/class/gpio/gpioXX/direction

Am I missing something?

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Re: [PATCH] pinctrl: intel: Implements gpio free function
       [not found]           ` <2019032121342663125658@cvte.com>
@ 2019-03-21 13:56             ` Mika Westerberg
       [not found]               ` <2019032211131426883268@cvte.com>
  2019-03-22 18:35               ` Enrico Weigelt, metux IT consult
  0 siblings, 2 replies; 18+ messages in thread
From: Mika Westerberg @ 2019-03-21 13:56 UTC (permalink / raw)
  To: zhuchangchun
  Cc: andriy.shevchenko, linus.walleij, linux-gpio, linux-kernel, hendychu

On Thu, Mar 21, 2019 at 09:35:26PM +0800, zhuchangchun@cvte.com wrote:
>    --> I know your meaning, even though I've did following actions
>    instead of unexport,
> 
>    # echo 0 > /sys/class/gpio/gpioXX/value
>      # echo in > /sys/class/gpio/gpioXX/direction
> 
>    the padcfg0 value still can not roll back,because after export the
>    gpio,the following
> 
>    action as padcfg0 settings was done.
> 
>    Exactly  TX and RX register have been set as below in GPIO request:
> 
>    /* Disable TX buffer and enable RX (this will be input) */
> 
>    value &= ~PADCFG0_GPIORXDIS;
> 
>    value |= PADCFG0_GPIOTXDIS;
> 
>    this will infruence next reboot gpio signal.
> 
>    And could you pls see my ->free function implementation?

I checked it and you do this:

+       value |= PADCFG0_GPIORXDIS;
+       value &= ~PADCFG0_GPIOTXDIS;

which pretty much turns the pin GPIO output unconditionally. I don't
really think it is good idea. May work in your case but may cause
problems with others.

If you want to keep it as output and leave the value 1 (high) before you
issue reboot, I don't think you need to do anything via sysfs (as the
pin is already in that state and issuing reboot command should not
change anything because the driver does not have ->shutdown callback.

In other words, your peripheral expects reset GPIO to be asserted (high)
during reboot so unless the boot firmware resets the pin I don't see why
leaving it as is does not work for you.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Re: [PATCH] pinctrl: intel: Implements gpio free function
       [not found]               ` <2019032211131426883268@cvte.com>
@ 2019-03-22 10:42                 ` Mika Westerberg
       [not found]                   ` <2019032314505202825175@cvte.com>
  0 siblings, 1 reply; 18+ messages in thread
From: Mika Westerberg @ 2019-03-22 10:42 UTC (permalink / raw)
  To: zhuchangchun
  Cc: andriy.shevchenko, linus.walleij, linux-gpio, linux-kernel, hendychu

On Fri, Mar 22, 2019 at 11:14:14AM +0800, zhuchangchun@cvte.com wrote:
>    static int intel_gpio_set_direction(struct pinctrl_dev *pctldev,
>       struct pinctrl_gpio_range *range,
>       unsigned pin, bool input)
>    {
>    struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
>    void __iomem *padcfg0;
>    unsigned long flags;
>    u32 value;
>    spin_lock_irqsave(&pctrl->lock, flags);
>    padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0);
>    value = readl(padcfg0);
>    if (input)
>    value |= PADCFG0_GPIOTXDIS;
>    else
>    value &= ~PADCFG0_GPIOTXDIS;
>    writel(value, padcfg0);
>    spin_unlock_irqrestore(&pctrl->lock, flags);
>    return 0;
>    }
> 
>    From above,you can kown when you export a GPIO ,it will do request,
> 
>    and there will set TX and RX register at the time same time.
> 
>    when you try to set direction in and set value, TX register value can
>    roll back
> 
>    the value,but RX register was not set, so who will set RX value back??

I think you are looking at some older code. There is now function
__intel_gpio_set_direction() that is supposed to set both buffers
depending on the direction. It was introduced with commit 17fab473693e
("pinctrl: intel: Set pin direction properly").

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] pinctrl: intel: Implements gpio free function
  2019-03-21  9:23   ` Andy Shevchenko
@ 2019-03-22 18:32     ` Enrico Weigelt, metux IT consult
  2019-03-22 19:06       ` Andy Shevchenko
  2019-04-03  4:13       ` Linus Walleij
  0 siblings, 2 replies; 18+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2019-03-22 18:32 UTC (permalink / raw)
  To: Andy Shevchenko, Mika Westerberg
  Cc: zhuchangchun, linus.walleij, linux-gpio, linux-kernel, hendychu

On 21.03.19 10:23, Andy Shevchenko wrote:

> ...and on top of that GPIO sysfs interface is deprecated.

I don't like the idea of deprecating this. It might not be enough for
all usecases, but for a lot of usecases, it's a very easy and simple
interfaces.

--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] pinctrl: intel: Implements gpio free function
  2019-03-21 13:56             ` Mika Westerberg
       [not found]               ` <2019032211131426883268@cvte.com>
@ 2019-03-22 18:35               ` Enrico Weigelt, metux IT consult
  1 sibling, 0 replies; 18+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2019-03-22 18:35 UTC (permalink / raw)
  To: Mika Westerberg, zhuchangchun
  Cc: andriy.shevchenko, linus.walleij, linux-gpio, linux-kernel, hendychu

On 21.03.19 14:56, Mika Westerberg wrote:
> I checked it and you do this:> > +       value |= PADCFG0_GPIORXDIS;> +       value &=
~PADCFG0_GPIOTXDIS;> > which pretty much turns the pin GPIO output
unconditionally. I don't> really think it is good idea. May work in your
case but may cause> problems with others.
Uh, please don't do this. In some cases it could be even dangerous.

In general, I'd question whether such an gpio should be userland-
controlled in the first place. Better write a driver for it, which
also handles the reset properly.


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] pinctrl: intel: Implements gpio free function
  2019-03-22 18:32     ` Enrico Weigelt, metux IT consult
@ 2019-03-22 19:06       ` Andy Shevchenko
  2019-03-25  9:36         ` Enrico Weigelt, metux IT consult
  2019-04-03  4:13       ` Linus Walleij
  1 sibling, 1 reply; 18+ messages in thread
From: Andy Shevchenko @ 2019-03-22 19:06 UTC (permalink / raw)
  To: Enrico Weigelt, metux IT consult
  Cc: Mika Westerberg, zhuchangchun, linus.walleij, linux-gpio,
	linux-kernel, hendychu

On Fri, Mar 22, 2019 at 07:32:28PM +0100, Enrico Weigelt, metux IT consult wrote:
> On 21.03.19 10:23, Andy Shevchenko wrote:
> 
> > ...and on top of that GPIO sysfs interface is deprecated.
> 
> I don't like the idea of deprecating this. It might not be enough for
> all usecases, but for a lot of usecases, it's a very easy and simple
> interfaces.

So, you probably late for more than year. Linus W. and others discussed that
a lot and the points of the choice are listed in documentation IIRC.


-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Re: [PATCH] pinctrl: intel: Implements gpio free function
       [not found]                   ` <2019032314505202825175@cvte.com>
@ 2019-03-23 16:48                     ` andriy.shevchenko
       [not found]                       ` <2019032517511048990383@cvte.com>
  0 siblings, 1 reply; 18+ messages in thread
From: andriy.shevchenko @ 2019-03-23 16:48 UTC (permalink / raw)
  To: zhuchangchun
  Cc: Mika Westerberg, linus.walleij, linux-gpio, linux-kernel, hendychu

On Sat, Mar 23, 2019 at 02:51:52PM +0800, zhuchangchun@cvte.com wrote:
> On Fri, Mar 22, 2019 at 11:14:14AM +0800, zhuchangchun@cvte.com wrote:

> >    From above,you can kown when you export a GPIO ,it will do request,
> >
> >    and there will set TX and RX register at the time same time.
> >
> >    when you try to set direction in and set value, TX register value can
> >    roll back
> >
> >    the value,but RX register was not set, so who will set RX value back??
>  
> I think you are looking at some older code. There is now function
> __intel_gpio_set_direction() that is supposed to set both buffers
> depending on the direction. It was introduced with commit 17fab473693e
> ("pinctrl: intel: Set pin direction properly").
> 
> 
> --> Yes ,I see the latest master branch, the __intel_gpio_set_direction will
> set RX and TX, but I still think we need to implement free function,cause
> it will help many other engineers,especailly for some manufactories use some
> module,and this modules embeded its own driver but not can be modified,
> if they want use the gpio control the module, they may meet the same problem.

What problem?
Is it reproducible on latest vanilla kernel?

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] pinctrl: intel: Implements gpio free function
  2019-03-22 19:06       ` Andy Shevchenko
@ 2019-03-25  9:36         ` Enrico Weigelt, metux IT consult
  2019-03-25 11:45           ` Andy Shevchenko
  0 siblings, 1 reply; 18+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2019-03-25  9:36 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Mika Westerberg, zhuchangchun, linus.walleij, linux-gpio,
	linux-kernel, hendychu

On 22.03.19 20:06, Andy Shevchenko wrote:
> On Fri, Mar 22, 2019 at 07:32:28PM +0100, Enrico Weigelt, metux IT consult wrote:
>> On 21.03.19 10:23, Andy Shevchenko wrote:
>>
>>> ...and on top of that GPIO sysfs interface is deprecated.
>>
>> I don't like the idea of deprecating this. It might not be enough for
>> all usecases, but for a lot of usecases, it's a very easy and simple
>> interfaces.
> 
> So, you probably late for more than year. Linus W. and others discussed that
> a lot and the points of the choice are listed in documentation IIRC.

If "deprecated" means there just won't be any new features, but
everything remains as it is, I can live w/ that. But But having to
rewrite lots of applications in the field for the new interface would
be really bad.

Note that the dev interface is *much* more complex than the sysfs one.
For example, it needs ioctl()s, so this can't be done just w/ a few
lines of shellscript anymore. (which is very common in many embedded
devices)


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] pinctrl: intel: Implements gpio free function
  2019-03-25  9:36         ` Enrico Weigelt, metux IT consult
@ 2019-03-25 11:45           ` Andy Shevchenko
  0 siblings, 0 replies; 18+ messages in thread
From: Andy Shevchenko @ 2019-03-25 11:45 UTC (permalink / raw)
  To: Enrico Weigelt, metux IT consult
  Cc: Mika Westerberg, zhuchangchun, linus.walleij, linux-gpio,
	linux-kernel, hendychu

On Mon, Mar 25, 2019 at 10:36:26AM +0100, Enrico Weigelt, metux IT consult wrote:
> On 22.03.19 20:06, Andy Shevchenko wrote:
> > On Fri, Mar 22, 2019 at 07:32:28PM +0100, Enrico Weigelt, metux IT consult wrote:
> >> On 21.03.19 10:23, Andy Shevchenko wrote:
> >>
> >>> ...and on top of that GPIO sysfs interface is deprecated.
> >>
> >> I don't like the idea of deprecating this. It might not be enough for
> >> all usecases, but for a lot of usecases, it's a very easy and simple
> >> interfaces.
> > 
> > So, you probably late for more than year. Linus W. and others discussed that
> > a lot and the points of the choice are listed in documentation IIRC.
> 
> If "deprecated" means there just won't be any new features, but
> everything remains as it is, I can live w/ that. But But having to
> rewrite lots of applications in the field for the new interface would
> be really bad.

If you don't want to have new features and OK with broken behaviour, yes,
that's fine.

> Note that the dev interface is *much* more complex than the sysfs one.
> For example, it needs ioctl()s, so this can't be done just w/ a few
> lines of shellscript anymore. (which is very common in many embedded
> devices)

I do it with a shell script, hint: libgpiod is a part of almost all alive Linux
distributions.

P.S. I don't think I would continue wasting time on the topic, since there is
nothing proposed how to improve the case.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Re: [PATCH] pinctrl: intel: Implements gpio free function
       [not found]                       ` <2019032517511048990383@cvte.com>
@ 2019-03-25 11:48                         ` andriy.shevchenko
  0 siblings, 0 replies; 18+ messages in thread
From: andriy.shevchenko @ 2019-03-25 11:48 UTC (permalink / raw)
  To: zhuchangchun
  Cc: Mika Westerberg, linus.walleij, linux-gpio, linux-kernel, hendychu

On Mon, Mar 25, 2019 at 05:52:10PM +0800, zhuchangchun@cvte.com wrote:
> On Sat, Mar 23, 2019 at 02:51:52PM +0800, zhuchangchun@cvte.com wrote:
> > On Fri, Mar 22, 2019 at 11:14:14AM +0800, zhuchangchun@cvte.com wrote:
>  
> > >    From above,you can kown when you export a GPIO ,it will do request,
> > >
> > >    and there will set TX and RX register at the time same time.
> > >
> > >    when you try to set direction in and set value, TX register value can
> > >    roll back
> > >
> > >    the value,but RX register was not set, so who will set RX value back??
> > 
> > I think you are looking at some older code. There is now function
> > __intel_gpio_set_direction() that is supposed to set both buffers
> > depending on the direction. It was introduced with commit 17fab473693e
> > ("pinctrl: intel: Set pin direction properly").
> >
> >
> > --> Yes ,I see the latest master branch, the __intel_gpio_set_direction will
> > set RX and TX, but I still think we need to implement free function,cause
> > it will help many other engineers,especailly for some manufactories use some
> > module,and this modules embeded its own driver but not can be modified,
> > if they want use the gpio control the module, they may meet the same problem.
>  
> What problem?
> Is it reproducible on latest vanilla kernel?
> 
> --> Not yet, I mean if someone use export GPIO ,then forget to set direction in, 
> and then set GPIO unexport directly, the GPIO buffer status may influence 
> the device work flow.

If you would like to return pin back to the previous state, it should be done
in generic way in the pin control subsystem.

Since there is no problem, nothing to fix then.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] pinctrl: intel: Implements gpio free function
  2019-03-22 18:32     ` Enrico Weigelt, metux IT consult
  2019-03-22 19:06       ` Andy Shevchenko
@ 2019-04-03  4:13       ` Linus Walleij
  2019-04-04 10:51         ` Enrico Weigelt, metux IT consult
  1 sibling, 1 reply; 18+ messages in thread
From: Linus Walleij @ 2019-04-03  4:13 UTC (permalink / raw)
  To: Enrico Weigelt, metux IT consult
  Cc: Andy Shevchenko, Mika Westerberg, zhuchangchun,
	open list:GPIO SUBSYSTEM, linux-kernel, hendychu

On Sat, Mar 23, 2019 at 1:32 AM Enrico Weigelt, metux IT consult
<lkml@metux.net> wrote:
> On 21.03.19 10:23, Andy Shevchenko wrote:
>
> > ...and on top of that GPIO sysfs interface is deprecated.
>
> I don't like the idea of deprecating this. It might not be enough for
> all usecases, but for a lot of usecases, it's a very easy and simple
> interfaces.

Yeah it is deprecated for years.

It is not like I don't see the simpleness and utility.

The reason why it is not good for anyone is simple:

Export some GPIOs in sysfs. You script crashes. Who is going
to unexport them now? (Answer: noone. Restart machine?)

Two scripts at the same time try to use the same GPIO. Such as happen
to start the same script twice. What happens
now? (Answer: both, with interesting results.)

Sure you can make more elaborate scripts that check for stuff already
being exported etc.

But the chardev on the other hand will protect you from all this.

If the program crashes, the lines will be free:ed.

If two scripts try to access the same GPIO, they will be denied.

And to spice things up, I only add new stuff like open drain and
driving several lines at the same time only to the chardev.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] pinctrl: intel: Implements gpio free function
  2019-04-03  4:13       ` Linus Walleij
@ 2019-04-04 10:51         ` Enrico Weigelt, metux IT consult
  2019-04-04 11:52           ` Andy Shevchenko
  2019-04-04 16:03           ` Linus Walleij
  0 siblings, 2 replies; 18+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2019-04-04 10:51 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Andy Shevchenko, Mika Westerberg, zhuchangchun,
	open list:GPIO SUBSYSTEM, linux-kernel, hendychu

On 03.04.19 06:13, Linus Walleij wrote:

> But the chardev on the other hand will protect you from all this.> > If the program crashes, the lines will be free:ed.> > If two scripts
try to access the same GPIO, they will be denied.
Right, when you want this concurrency protection and cleanup stiff
the chardev is the better choice. But I've already had several cases
where the simplicity of the sysfs interface is a big win - all you need
few trivial fs operations.

That's also nice for exporting in a grid, eg. via 9P (eg. nice for
quickly building up HIL environments)

ioctls have the bad side effect that they can't be exported via
network in a generic way - your remote fs protocol must support all of
them - even worse: it needs to cope with overlapping ioctl-nr's that
can have entirely different data structures depending on the actual
device. And now try to do that w/ reasonable effort and w/o creating
a shared memory between server and client :p

Another interesting usecase is permission handling:

a) some ioctls need special privileges (oh, how I hate all these "if
   (!capable(CAP_SYS_ADMIN)) ..." lines in the drivers), but you wanna
   give some unprivileged user access to them
b) you wanna give an unprivileged user access to specific gpio's,
   but not to all at once.

With pure filesystem based approach, you can easly define permissions
for each filesystem object.

Yes, these usecases aren't so common for average Jon Doe, but the gpio
subsystem is used that way, out there in the field, and it would be bad
if that functionality would go away someday.


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] pinctrl: intel: Implements gpio free function
  2019-04-04 10:51         ` Enrico Weigelt, metux IT consult
@ 2019-04-04 11:52           ` Andy Shevchenko
  2019-04-04 16:03           ` Linus Walleij
  1 sibling, 0 replies; 18+ messages in thread
From: Andy Shevchenko @ 2019-04-04 11:52 UTC (permalink / raw)
  To: Enrico Weigelt, metux IT consult
  Cc: Linus Walleij, Mika Westerberg, zhuchangchun,
	open list:GPIO SUBSYSTEM, linux-kernel, hendychu,
	Bartosz Golaszewski

On Thu, Apr 04, 2019 at 12:51:35PM +0200, Enrico Weigelt, metux IT consult wrote:
> On 03.04.19 06:13, Linus Walleij wrote:
> 
> > But the chardev on the other hand will protect you from all this.> > If the program crashes, the lines will be free:ed.> > If two scripts
> try to access the same GPIO, they will be denied.
> Right, when you want this concurrency protection and cleanup stiff
> the chardev is the better choice. But I've already had several cases
> where the simplicity of the sysfs interface is a big win - all you need
> few trivial fs operations.
> 
> That's also nice for exporting in a grid, eg. via 9P (eg. nice for
> quickly building up HIL environments)
> 
> ioctls have the bad side effect that they can't be exported via
> network in a generic way - your remote fs protocol must support all of
> them - even worse: it needs to cope with overlapping ioctl-nr's that
> can have entirely different data structures depending on the actual
> device. And now try to do that w/ reasonable effort and w/o creating
> a shared memory between server and client :p
> 
> Another interesting usecase is permission handling:
> 
> a) some ioctls need special privileges (oh, how I hate all these "if
>    (!capable(CAP_SYS_ADMIN)) ..." lines in the drivers), but you wanna
>    give some unprivileged user access to them
> b) you wanna give an unprivileged user access to specific gpio's,
>    but not to all at once.
> 
> With pure filesystem based approach, you can easly define permissions
> for each filesystem object.

Also you may consider gpiod daemon and it's socket interface, for example.
Yes, complicated, but solves above problems AFAICT.

I guess the best person, missed in Cc, Bartosz, can tell more about user space
interaction.

And btw gpiod still a good to have for other even local cases:
https://github.com/brgl/libgpiod/issues/29
https://github.com/brgl/libgpiod/issues/40
...

> Yes, these usecases aren't so common for average Jon Doe, but the gpio
> subsystem is used that way, out there in the field, and it would be bad
> if that functionality would go away someday.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] pinctrl: intel: Implements gpio free function
  2019-04-04 10:51         ` Enrico Weigelt, metux IT consult
  2019-04-04 11:52           ` Andy Shevchenko
@ 2019-04-04 16:03           ` Linus Walleij
  1 sibling, 0 replies; 18+ messages in thread
From: Linus Walleij @ 2019-04-04 16:03 UTC (permalink / raw)
  To: Enrico Weigelt, metux IT consult
  Cc: Andy Shevchenko, Mika Westerberg, zhuchangchun,
	open list:GPIO SUBSYSTEM, linux-kernel, hendychu,
	Bartosz Golaszewski

On Thu, Apr 4, 2019 at 5:51 PM Enrico Weigelt, metux IT consult
<lkml@metux.net> wrote:
> On 03.04.19 06:13, Linus Walleij wrote:
>
> > But the chardev on the other hand will protect you from all this.
> > If the program crashes, the lines will be free:ed.
> > If two scripts try to access the same GPIO, they will be denied.

> Right, when you want this concurrency protection and cleanup stiff
> the chardev is the better choice. But I've already had several cases
> where the simplicity of the sysfs interface is a big win - all you need
> few trivial fs operations.

It is admittedly a tradeoff. But if we want something that is hacky
and dangerous it should be moved over to debugfs not sitting
around in sysfs. In debugfs we give users enough rope all the
time.

> Another interesting usecase is permission handling:
>
> a) some ioctls need special privileges (oh, how I hate all these "if
>    (!capable(CAP_SYS_ADMIN)) ..." lines in the drivers), but you wanna
>    give some unprivileged user access to them
> b) you wanna give an unprivileged user access to specific gpio's,
>    but not to all at once.
>
> With pure filesystem based approach, you can easly define permissions
> for each filesystem object.

This was a comment when we designed the chardev as well, but
noone really could give a good usecase for this. Often GPIOs they
want to protect should not have been made available to userspace
or anyone else in the first place and we now have ways inside
the kernel to take GPIOs aside (valid_mask) we can also use
hogs (at least in the device tree) to set up GPIOs so that they are
held perpetually by the kernel (until reboot) so that userspace
cannot try to use them.

I can't see that any more access control granularity than the
whole gpiochip chardev is really used, not anymore than
you want to have access control on every indidual pixel on
a framebuffer or each individual block on a block device.

Things like requesting and flipping several GPIOs at once
could become explosively complex to implement with
individual access permissions on each GPIO line, and this
is on the other hand a very real and important use case.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2019-04-04 16:03 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-21  2:35 [PATCH] pinctrl: intel: Implements gpio free function zhuchangchun
2019-03-21  8:44 ` Mika Westerberg
2019-03-21  9:23   ` Andy Shevchenko
2019-03-22 18:32     ` Enrico Weigelt, metux IT consult
2019-03-22 19:06       ` Andy Shevchenko
2019-03-25  9:36         ` Enrico Weigelt, metux IT consult
2019-03-25 11:45           ` Andy Shevchenko
2019-04-03  4:13       ` Linus Walleij
2019-04-04 10:51         ` Enrico Weigelt, metux IT consult
2019-04-04 11:52           ` Andy Shevchenko
2019-04-04 16:03           ` Linus Walleij
     [not found]   ` <2019032119195575582546@cvte.com>
2019-03-21 12:03     ` Mika Westerberg
     [not found]       ` <2019032120213955866649@cvte.com>
2019-03-21 12:36         ` Mika Westerberg
     [not found]           ` <2019032121342663125658@cvte.com>
2019-03-21 13:56             ` Mika Westerberg
     [not found]               ` <2019032211131426883268@cvte.com>
2019-03-22 10:42                 ` Mika Westerberg
     [not found]                   ` <2019032314505202825175@cvte.com>
2019-03-23 16:48                     ` andriy.shevchenko
     [not found]                       ` <2019032517511048990383@cvte.com>
2019-03-25 11:48                         ` andriy.shevchenko
2019-03-22 18:35               ` Enrico Weigelt, metux IT consult

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).