Re: [PATCH v2 3/6] pinctrl: Add a possibility to configure pins from a gpiolib based drivers

[Linus Walleij]

On Fri, Jan 13, 2017 at 5:33 PM, Mika Westerberg
 wrote:
> On Fri, Jan 13, 2017 at 04:36:42PM +0100, Linus Walleij wrote:
>> On Thu, Jan 12, 2017 at 10:22 AM, Mika Westerberg
>>  wrote:
>>
>> > Hmm, looking at users of .set_debounce() I can see that the debounce
>> > time can be quite large. For example some signals which are connected to
>> > physical push-buttons may need > 64ms debounce time.
>> >
>> > However, the current pinconfig value is defined to be unsigned long
>> > which on 32-bit architecture is 32-bits. From that the higher 16-bits
>> > are used as config leaving the value to be 16-bits. This gives maximum
>> > debounce time of 65535us. I don't think it can cover all the uses of
>> > .set_debounce(). This could also be problematic when specifying values
>> > for pull resistors.
>> >
>> > One solution is to convert the packed value to be u64 instead, leaving
>> > up to 48-bits for the value. Alternatively we could provide a scale
>> > field with the packed format.
>>
>> Hm yeah as long as all in-kernel users survive I don't see why we
>> couldn't just make it 64bit. Is it a big deal?
>
> As long as everyone is using those macros and inline functions from
> pinconf-generic.h, I think the conversion should be pretty
> straightforward.

I think I just make it a strict requirement that if people want to use
the pinctrl back-end for GPIO they simply have to support generic
pin control. It's not like they have something else already, and
converting a driver is not any unreasonable amount of work.

Yours,
Linus Walleij

Re: [PATCH v2 3/6] pinctrl: Add a possibility to configure pins from a gpiolib based drivers

[Linus Walleij]

On Fri, Jan 13, 2017 at 5:33 PM, Mika Westerberg
 wrote:
> On Fri, Jan 13, 2017 at 04:36:42PM +0100, Linus Walleij wrote:
>> On Thu, Jan 12, 2017 at 10:22 AM, Mika Westerberg
>>  wrote:
>>
>> > Hmm, looking at users of .set_debounce() I can see that the debounce
>> > time can be quite large. For example some signals which are connected to
>> > physical push-buttons may need > 64ms debounce time.
>> >
>> > However, the current pinconfig value is defined to be unsigned long
>> > which on 32-bit architecture is 32-bits. From that the higher 16-bits
>> > are used as config leaving the value to be 16-bits. This gives maximum
>> > debounce time of 65535us. I don't think it can cover all the uses of
>> > .set_debounce(). This could also be problematic when specifying values
>> > for pull resistors.
>> >
>> > One solution is to convert the packed value to be u64 instead, leaving
>> > up to 48-bits for the value. Alternatively we could provide a scale
>> > field with the packed format.
>>
>> Hm yeah as long as all in-kernel users survive I don't see why we
>> couldn't just make it 64bit. Is it a big deal?
>
> As long as everyone is using those macros and inline functions from
> pinconf-generic.h, I think the conversion should be pretty
> straightforward.

I think I just make it a strict requirement that if people want to use
the pinctrl back-end for GPIO they simply have to support generic
pin control. It's not like they have something else already, and
converting a driver is not any unreasonable amount of work.

Yours,
Linus Walleij

Re: [PATCH v2 3/6] pinctrl: Add a possibility to configure pins from a gpiolib based drivers

[Mika Westerberg]

On Fri, Jan 13, 2017 at 04:36:42PM +0100, Linus Walleij wrote:
> On Thu, Jan 12, 2017 at 10:22 AM, Mika Westerberg
>  wrote:
> 
> > Hmm, looking at users of .set_debounce() I can see that the debounce
> > time can be quite large. For example some signals which are connected to
> > physical push-buttons may need > 64ms debounce time.
> >
> > However, the current pinconfig value is defined to be unsigned long
> > which on 32-bit architecture is 32-bits. From that the higher 16-bits
> > are used as config leaving the value to be 16-bits. This gives maximum
> > debounce time of 65535us. I don't think it can cover all the uses of
> > .set_debounce(). This could also be problematic when specifying values
> > for pull resistors.
> >
> > One solution is to convert the packed value to be u64 instead, leaving
> > up to 48-bits for the value. Alternatively we could provide a scale
> > field with the packed format.
> 
> Hm yeah as long as all in-kernel users survive I don't see why we
> couldn't just make it 64bit. Is it a big deal?

As long as everyone is using those macros and inline functions from
pinconf-generic.h, I think the conversion should be pretty
straightforward.

Re: [PATCH v2 3/6] pinctrl: Add a possibility to configure pins from a gpiolib based drivers

[Mika Westerberg]

On Fri, Jan 13, 2017 at 04:36:42PM +0100, Linus Walleij wrote:
> On Thu, Jan 12, 2017 at 10:22 AM, Mika Westerberg
>  wrote:
> 
> > Hmm, looking at users of .set_debounce() I can see that the debounce
> > time can be quite large. For example some signals which are connected to
> > physical push-buttons may need > 64ms debounce time.
> >
> > However, the current pinconfig value is defined to be unsigned long
> > which on 32-bit architecture is 32-bits. From that the higher 16-bits
> > are used as config leaving the value to be 16-bits. This gives maximum
> > debounce time of 65535us. I don't think it can cover all the uses of
> > .set_debounce(). This could also be problematic when specifying values
> > for pull resistors.
> >
> > One solution is to convert the packed value to be u64 instead, leaving
> > up to 48-bits for the value. Alternatively we could provide a scale
> > field with the packed format.
> 
> Hm yeah as long as all in-kernel users survive I don't see why we
> couldn't just make it 64bit. Is it a big deal?

As long as everyone is using those macros and inline functions from
pinconf-generic.h, I think the conversion should be pretty
straightforward.

Re: [PATCH v2 3/6] pinctrl: Add a possibility to configure pins from a gpiolib based drivers

[Linus Walleij]

On Thu, Jan 12, 2017 at 10:22 AM, Mika Westerberg
 wrote:

> Hmm, looking at users of .set_debounce() I can see that the debounce
> time can be quite large. For example some signals which are connected to
> physical push-buttons may need > 64ms debounce time.
>
> However, the current pinconfig value is defined to be unsigned long
> which on 32-bit architecture is 32-bits. From that the higher 16-bits
> are used as config leaving the value to be 16-bits. This gives maximum
> debounce time of 65535us. I don't think it can cover all the uses of
> .set_debounce(). This could also be problematic when specifying values
> for pull resistors.
>
> One solution is to convert the packed value to be u64 instead, leaving
> up to 48-bits for the value. Alternatively we could provide a scale
> field with the packed format.

Hm yeah as long as all in-kernel users survive I don't see why we
couldn't just make it 64bit. Is it a big deal?

A scale field (multiplier) can also work, I don't know which is most
elegant.

Yours,
Linus Walleij

Re: [PATCH v2 3/6] pinctrl: Add a possibility to configure pins from a gpiolib based drivers

[Linus Walleij]

On Thu, Jan 12, 2017 at 10:22 AM, Mika Westerberg
 wrote:

> Hmm, looking at users of .set_debounce() I can see that the debounce
> time can be quite large. For example some signals which are connected to
> physical push-buttons may need > 64ms debounce time.
>
> However, the current pinconfig value is defined to be unsigned long
> which on 32-bit architecture is 32-bits. From that the higher 16-bits
> are used as config leaving the value to be 16-bits. This gives maximum
> debounce time of 65535us. I don't think it can cover all the uses of
> .set_debounce(). This could also be problematic when specifying values
> for pull resistors.
>
> One solution is to convert the packed value to be u64 instead, leaving
> up to 48-bits for the value. Alternatively we could provide a scale
> field with the packed format.

Hm yeah as long as all in-kernel users survive I don't see why we
couldn't just make it 64bit. Is it a big deal?

A scale field (multiplier) can also work, I don't know which is most
elegant.

Yours,
Linus Walleij

Re: [PATCH v2 3/6] pinctrl: Add a possibility to configure pins from a gpiolib based drivers

[Mika Westerberg]

On Wed, Jan 11, 2017 at 03:33:04PM +0200, Mika Westerberg wrote:
> > But let's first pause and discuss this, because I have some stuff on my
> > mind here.
> > 
> > First this kernel-internal ABI from :
> > 
> > struct gpio_chip {
> > (...)
> > int (*set_debounce)(struct gpio_chip *chip,
> > unsigned offset,
> > unsigned debounce);
> > int (*set_single_ended)(struct gpio_chip *chip,
> > unsigned offset,
> > enum single_ended_mode 
> > mode);
> > (...)
> > 
> > It's not going to scale. We need to replace this with something like
> > 
> > int (*set_config)(struct gpio_chip *chip, unsigned offset, unsigned
> > long config);
> > 
> > Where "config" takes the packed format described in
> > 
> > and nothing else, anything else is just inviting disaster.
> > 
> > We can also later add:
> > 
> > int (*get_config)(struct gpio_chip *chip, unsigned offset, unsigned
> > long *config);
> > 
> > We can then  set and get arbitrary configs on GPIO lines, and the
> > drivers can simply implement a switch() for the configs they handle
> > else return -ENOTSUPP.
> > 
> > But right now only set_config() would be enough.
> > 
> > Maybe stuff needs to be split out of that header to be shared between
> > GPIO and pinctrl but hopefully you could just include it.
> > 
> > Then we change all in-kernel users of these two APIs over to set_config().
> > 
> > THEN we can think about cross-calling to pin control using the API
> > from this patch. It should be a simple matter of just passing along the
> > same config argument since we're using generic pin config.
> > 
> > It's not like it's impossible to merge this patch first, but I want to get 
> > some
> > order here.
> > 
> > Are you convenient with doing the above patch as part of this series, or
> > shall I do it first so you can rebase on it? (Will take some time if I
> > do it...)
> 
> Sure, I can take a look at it.

Hmm, looking at users of .set_debounce() I can see that the debounce
time can be quite large. For example some signals which are connected to
physical push-buttons may need > 64ms debounce time.

However, the current pinconfig value is defined to be unsigned long
which on 32-bit architecture is 32-bits. From that the higher 16-bits
are used as config leaving the value to be 16-bits. This gives maximum
debounce time of 65535us. I don't think it can cover all the uses of
.set_debounce(). This could also be problematic when specifying values
for pull resistors.

One solution is to convert the packed value to be u64 instead, leaving
up to 48-bits for the value. Alternatively we could provide a scale
field with the packed format.

What do you think?

Re: [PATCH v2 3/6] pinctrl: Add a possibility to configure pins from a gpiolib based drivers

[Mika Westerberg]

On Wed, Jan 11, 2017 at 03:33:04PM +0200, Mika Westerberg wrote:
> > But let's first pause and discuss this, because I have some stuff on my
> > mind here.
> > 
> > First this kernel-internal ABI from :
> > 
> > struct gpio_chip {
> > (...)
> > int (*set_debounce)(struct gpio_chip *chip,
> > unsigned offset,
> > unsigned debounce);
> > int (*set_single_ended)(struct gpio_chip *chip,
> > unsigned offset,
> > enum single_ended_mode 
> > mode);
> > (...)
> > 
> > It's not going to scale. We need to replace this with something like
> > 
> > int (*set_config)(struct gpio_chip *chip, unsigned offset, unsigned
> > long config);
> > 
> > Where "config" takes the packed format described in
> > 
> > and nothing else, anything else is just inviting disaster.
> > 
> > We can also later add:
> > 
> > int (*get_config)(struct gpio_chip *chip, unsigned offset, unsigned
> > long *config);
> > 
> > We can then  set and get arbitrary configs on GPIO lines, and the
> > drivers can simply implement a switch() for the configs they handle
> > else return -ENOTSUPP.
> > 
> > But right now only set_config() would be enough.
> > 
> > Maybe stuff needs to be split out of that header to be shared between
> > GPIO and pinctrl but hopefully you could just include it.
> > 
> > Then we change all in-kernel users of these two APIs over to set_config().
> > 
> > THEN we can think about cross-calling to pin control using the API
> > from this patch. It should be a simple matter of just passing along the
> > same config argument since we're using generic pin config.
> > 
> > It's not like it's impossible to merge this patch first, but I want to get 
> > some
> > order here.
> > 
> > Are you convenient with doing the above patch as part of this series, or
> > shall I do it first so you can rebase on it? (Will take some time if I
> > do it...)
> 
> Sure, I can take a look at it.

Hmm, looking at users of .set_debounce() I can see that the debounce
time can be quite large. For example some signals which are connected to
physical push-buttons may need > 64ms debounce time.

However, the current pinconfig value is defined to be unsigned long
which on 32-bit architecture is 32-bits. From that the higher 16-bits
are used as config leaving the value to be 16-bits. This gives maximum
debounce time of 65535us. I don't think it can cover all the uses of
.set_debounce(). This could also be problematic when specifying values
for pull resistors.

One solution is to convert the packed value to be u64 instead, leaving
up to 48-bits for the value. Alternatively we could provide a scale
field with the packed format.

What do you think?

Re: [PATCH v2 3/6] pinctrl: Add a possibility to configure pins from a gpiolib based drivers

[Mika Westerberg]

On Wed, Jan 11, 2017 at 02:06:56PM +0100, Linus Walleij wrote:
> On Tue, Jan 10, 2017 at 3:31 PM, Mika Westerberg
>  wrote:
> 
> > When a GPIO driver is backed by a pinctrl driver the GPIO driver often
> > needs to call the pinctrl driver to configure certain things, like
> > whether the pin is used as input or output. In addition to this there
> > are other pin configurations applicable to GPIOs such as debounce
> > timeout.
> >
> > To support this we introduce a new function pinctrl_gpio_set_config()
> > that can be used by gpiolib based driver to pass configuration requests
> > to the backing pinctrl driver.
> >
> > Signed-off-by: Mika Westerberg 
> 
> OK so this is needed.

The alternative would be to just handle this internally in
pinctrl-intel.c but I thought it is better to make the functionality
available for other drivers as well.

> But let's first pause and discuss this, because I have some stuff on my
> mind here.
> 
> First this kernel-internal ABI from :
> 
> struct gpio_chip {
> (...)
> int (*set_debounce)(struct gpio_chip *chip,
> unsigned offset,
> unsigned debounce);
> int (*set_single_ended)(struct gpio_chip *chip,
> unsigned offset,
> enum single_ended_mode mode);
> (...)
> 
> It's not going to scale. We need to replace this with something like
> 
> int (*set_config)(struct gpio_chip *chip, unsigned offset, unsigned
> long config);
> 
> Where "config" takes the packed format described in
> 
> and nothing else, anything else is just inviting disaster.
> 
> We can also later add:
> 
> int (*get_config)(struct gpio_chip *chip, unsigned offset, unsigned
> long *config);
> 
> We can then  set and get arbitrary configs on GPIO lines, and the
> drivers can simply implement a switch() for the configs they handle
> else return -ENOTSUPP.
> 
> But right now only set_config() would be enough.
> 
> Maybe stuff needs to be split out of that header to be shared between
> GPIO and pinctrl but hopefully you could just include it.
> 
> Then we change all in-kernel users of these two APIs over to set_config().
> 
> THEN we can think about cross-calling to pin control using the API
> from this patch. It should be a simple matter of just passing along the
> same config argument since we're using generic pin config.
> 
> It's not like it's impossible to merge this patch first, but I want to get 
> some
> order here.
> 
> Are you convenient with doing the above patch as part of this series, or
> shall I do it first so you can rebase on it? (Will take some time if I
> do it...)

Sure, I can take a look at it.

> We need this because GPIO is going to need more and more config
> to be done by pinctrl on its behalf, and it will have to go all the
> way to userspace in many cases, so we need this infrastructure in
> place.

OK.

Re: [PATCH v2 3/6] pinctrl: Add a possibility to configure pins from a gpiolib based drivers

[Mika Westerberg]

On Wed, Jan 11, 2017 at 02:06:56PM +0100, Linus Walleij wrote:
> On Tue, Jan 10, 2017 at 3:31 PM, Mika Westerberg
>  wrote:
> 
> > When a GPIO driver is backed by a pinctrl driver the GPIO driver often
> > needs to call the pinctrl driver to configure certain things, like
> > whether the pin is used as input or output. In addition to this there
> > are other pin configurations applicable to GPIOs such as debounce
> > timeout.
> >
> > To support this we introduce a new function pinctrl_gpio_set_config()
> > that can be used by gpiolib based driver to pass configuration requests
> > to the backing pinctrl driver.
> >
> > Signed-off-by: Mika Westerberg 
> 
> OK so this is needed.

The alternative would be to just handle this internally in
pinctrl-intel.c but I thought it is better to make the functionality
available for other drivers as well.

> But let's first pause and discuss this, because I have some stuff on my
> mind here.
> 
> First this kernel-internal ABI from :
> 
> struct gpio_chip {
> (...)
> int (*set_debounce)(struct gpio_chip *chip,
> unsigned offset,
> unsigned debounce);
> int (*set_single_ended)(struct gpio_chip *chip,
> unsigned offset,
> enum single_ended_mode mode);
> (...)
> 
> It's not going to scale. We need to replace this with something like
> 
> int (*set_config)(struct gpio_chip *chip, unsigned offset, unsigned
> long config);
> 
> Where "config" takes the packed format described in
> 
> and nothing else, anything else is just inviting disaster.
> 
> We can also later add:
> 
> int (*get_config)(struct gpio_chip *chip, unsigned offset, unsigned
> long *config);
> 
> We can then  set and get arbitrary configs on GPIO lines, and the
> drivers can simply implement a switch() for the configs they handle
> else return -ENOTSUPP.
> 
> But right now only set_config() would be enough.
> 
> Maybe stuff needs to be split out of that header to be shared between
> GPIO and pinctrl but hopefully you could just include it.
> 
> Then we change all in-kernel users of these two APIs over to set_config().
> 
> THEN we can think about cross-calling to pin control using the API
> from this patch. It should be a simple matter of just passing along the
> same config argument since we're using generic pin config.
> 
> It's not like it's impossible to merge this patch first, but I want to get 
> some
> order here.
> 
> Are you convenient with doing the above patch as part of this series, or
> shall I do it first so you can rebase on it? (Will take some time if I
> do it...)

Sure, I can take a look at it.

> We need this because GPIO is going to need more and more config
> to be done by pinctrl on its behalf, and it will have to go all the
> way to userspace in many cases, so we need this infrastructure in
> place.

OK.

Re: [PATCH v2 3/6] pinctrl: Add a possibility to configure pins from a gpiolib based drivers

[Linus Walleij]

On Tue, Jan 10, 2017 at 3:31 PM, Mika Westerberg
 wrote:

> When a GPIO driver is backed by a pinctrl driver the GPIO driver often
> needs to call the pinctrl driver to configure certain things, like
> whether the pin is used as input or output. In addition to this there
> are other pin configurations applicable to GPIOs such as debounce
> timeout.
>
> To support this we introduce a new function pinctrl_gpio_set_config()
> that can be used by gpiolib based driver to pass configuration requests
> to the backing pinctrl driver.
>
> Signed-off-by: Mika Westerberg 

OK so this is needed.

But let's first pause and discuss this, because I have some stuff on my
mind here.

First this kernel-internal ABI from :

struct gpio_chip {
(...)
int (*set_debounce)(struct gpio_chip *chip,
unsigned offset,
unsigned debounce);
int (*set_single_ended)(struct gpio_chip *chip,
unsigned offset,
enum single_ended_mode mode);
(...)

It's not going to scale. We need to replace this with something like

int (*set_config)(struct gpio_chip *chip, unsigned offset, unsigned
long config);

Where "config" takes the packed format described in

and nothing else, anything else is just inviting disaster.

We can also later add:

int (*get_config)(struct gpio_chip *chip, unsigned offset, unsigned
long *config);

We can then  set and get arbitrary configs on GPIO lines, and the
drivers can simply implement a switch() for the configs they handle
else return -ENOTSUPP.

But right now only set_config() would be enough.

Maybe stuff needs to be split out of that header to be shared between
GPIO and pinctrl but hopefully you could just include it.

Then we change all in-kernel users of these two APIs over to set_config().

THEN we can think about cross-calling to pin control using the API
from this patch. It should be a simple matter of just passing along the
same config argument since we're using generic pin config.

It's not like it's impossible to merge this patch first, but I want to get some
order here.

Are you convenient with doing the above patch as part of this series, or
shall I do it first so you can rebase on it? (Will take some time if I
do it...)

We need this because GPIO is going to need more and more config
to be done by pinctrl on its behalf, and it will have to go all the
way to userspace in many cases, so we need this infrastructure in
place.

Yours,
Linus Walleij

Re: [PATCH v2 3/6] pinctrl: Add a possibility to configure pins from a gpiolib based drivers

[Linus Walleij]

On Tue, Jan 10, 2017 at 3:31 PM, Mika Westerberg
 wrote:

> When a GPIO driver is backed by a pinctrl driver the GPIO driver often
> needs to call the pinctrl driver to configure certain things, like
> whether the pin is used as input or output. In addition to this there
> are other pin configurations applicable to GPIOs such as debounce
> timeout.
>
> To support this we introduce a new function pinctrl_gpio_set_config()
> that can be used by gpiolib based driver to pass configuration requests
> to the backing pinctrl driver.
>
> Signed-off-by: Mika Westerberg 

OK so this is needed.

But let's first pause and discuss this, because I have some stuff on my
mind here.

First this kernel-internal ABI from :

struct gpio_chip {
(...)
int (*set_debounce)(struct gpio_chip *chip,
unsigned offset,
unsigned debounce);
int (*set_single_ended)(struct gpio_chip *chip,
unsigned offset,
enum single_ended_mode mode);
(...)

It's not going to scale. We need to replace this with something like

int (*set_config)(struct gpio_chip *chip, unsigned offset, unsigned
long config);

Where "config" takes the packed format described in

and nothing else, anything else is just inviting disaster.

We can also later add:

int (*get_config)(struct gpio_chip *chip, unsigned offset, unsigned
long *config);

We can then  set and get arbitrary configs on GPIO lines, and the
drivers can simply implement a switch() for the configs they handle
else return -ENOTSUPP.

But right now only set_config() would be enough.

Maybe stuff needs to be split out of that header to be shared between
GPIO and pinctrl but hopefully you could just include it.

Then we change all in-kernel users of these two APIs over to set_config().

THEN we can think about cross-calling to pin control using the API
from this patch. It should be a simple matter of just passing along the
same config argument since we're using generic pin config.

It's not like it's impossible to merge this patch first, but I want to get some
order here.

Are you convenient with doing the above patch as part of this series, or
shall I do it first so you can rebase on it? (Will take some time if I
do it...)

We need this because GPIO is going to need more and more config
to be done by pinctrl on its behalf, and it will have to go all the
way to userspace in many cases, so we need this infrastructure in
place.

Yours,
Linus Walleij