[PATCH] drivers: create a pin control subsystem v8

[PATCH] drivers: create a pin control subsystem v8

[Grant Likely]

On Wed, Sep 28, 2011 at 02:03:39PM +0200, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> This creates a subsystem for handling of pin control devices.
> These are devices that control different aspects of package
> pins.

Comments below, some a bit nitpicky, but overall I think it looks
good.  I haven't dug into it nearly deeply enough though.  :-(

[...]
> +/**
> + * Looks up a pin control device matching a certain device name or
> + * pure device pointer.

May as well actually do kerneldoc formatting here on the comment
blocks.

> + */
> +struct pinctrl_dev *get_pctldev_from_dev(struct device *dev,
> +					 const char *devname)
> +{
> +	struct pinctrl_dev *pctldev = NULL;
> +	bool found = false;
> +
> +	mutex_lock(&pinctrldev_list_mutex);
> +	list_for_each_entry(pctldev, &pinctrldev_list, node) {
> +		if (dev &&  &pctldev->dev == dev) {
> +			/* Matched on device pointer */
> +			found = true;
> +			break;
> +		}
> +
> +		if (devname &&
> +		    !strcmp(dev_name(&pctldev->dev), devname)) {
> +			/* Matched on device name */
> +			found = true;
> +			break;
> +		}
> +	}
> +	mutex_unlock(&pinctrldev_list_mutex);
> +
> +	if (found)
> +		return pctldev;
> +
> +	return NULL;
> +}

Nit: I'm not too fond of a single function doing both name and pointer
lookup at the same time.  Presumably the caller would have one or the
other and know what it needs to do.  I'd prefer to see one by-name
function and one by-reference.  I'm not going to make a big deal about
it though.

> +/**
> + * pinctrl_register() - register a pin controller device
> + * @pctldesc: descriptor for this pin controller
> + * @dev: parent device for this pin controller
> + * @driver_data: private pin controller data for this pin controller
> + */
> +struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
> +				    struct device *dev, void *driver_data)
> +{
> +	static atomic_t pinmux_no = ATOMIC_INIT(0);
> +	struct pinctrl_dev *pctldev;
> +	int ret;
> +
> +	if (pctldesc == NULL)
> +		return ERR_PTR(-EINVAL);
> +	if (pctldesc->name == NULL)
> +		return ERR_PTR(-EINVAL);
> +
> +	/* If we're implementing pinmuxing, check the ops for sanity */
> +	if (pctldesc->pmxops) {
> +		ret = pinmux_check_ops(pctldesc->pmxops);
> +		if (ret) {
> +			pr_err("%s pinmux ops lacks necessary functions\n",
> +			       pctldesc->name);
> +			return ERR_PTR(ret);
> +		}
> +	}
> +
> +	pctldev = kzalloc(sizeof(struct pinctrl_dev), GFP_KERNEL);
> +	if (pctldev == NULL)
> +		return ERR_PTR(-ENOMEM);
> +
> +	/* Initialize pin control device struct */
> +	pctldev->owner = pctldesc->owner;
> +	pctldev->desc = pctldesc;
> +	pctldev->driver_data = driver_data;
> +	INIT_RADIX_TREE(&pctldev->pin_desc_tree, GFP_KERNEL);
> +	spin_lock_init(&pctldev->pin_desc_tree_lock);
> +	INIT_LIST_HEAD(&pctldev->gpio_ranges);
> +	mutex_init(&pctldev->gpio_ranges_lock);
> +
> +	/* Register device with sysfs */
> +	pctldev->dev.parent = dev;
> +	pctldev->dev.bus = &pinctrl_bus;

I don't think there is an actual need for a pinctrl bus type.  There
aren't any drivers that are going to be bound to these things which is
the primary functionality that a bus type provides.  Am I missing
something?

> +	pctldev->dev.type = &pinctrl_type;
> +	dev_set_name(&pctldev->dev, "pinctrl.%d",
> +		     atomic_inc_return(&pinmux_no) - 1);
> +	ret = device_register(&pctldev->dev);
> +	if (ret != 0) {
> +		pr_err("error in device registration\n");
> +		put_device(&pctldev->dev);
> +		kfree(pctldev);
> +		goto out_err;
> +	}
> +	dev_set_drvdata(&pctldev->dev, pctldev);
> +
> +	/* Register all the pins */
> +	pr_debug("try to register %d pins on %s...\n",
> +		 pctldesc->npins, pctldesc->name);
> +	ret = pinctrl_register_pins(pctldev, pctldesc->pins, pctldesc->npins);
> +	if (ret) {
> +		pr_err("error during pin registration\n");
> +		pinctrl_free_pindescs(pctldev, pctldesc->pins,
> +				      pctldesc->npins);
> +		goto out_err;
> +	}
> +
> +	pinctrl_init_device_debugfs(pctldev);
> +	mutex_lock(&pinctrldev_list_mutex);
> +	list_add(&pctldev->node, &pinctrldev_list);
> +	mutex_unlock(&pinctrldev_list_mutex);
> +	pinmux_hog_maps(pctldev);
> +	return pctldev;
> +
> +out_err:
> +	put_device(&pctldev->dev);
> +	kfree(pctldev);

Once a device is initialized, it cannot be kfree()'ed directly.  The
.release method needs to do that.

> +/**
> + * pin_free() - release a single muxed in pin so something else can be muxed in
> + *	instead

Nit: the summary line in kerneldoc should fit on one line.

> +/**
> + * pinmux_register_mappings() - register a set of pinmux mappings
> + * @maps: the pinmux mappings table to register
> + * @num_maps: the number of maps in the mapping table
> + *
> + * Only call this once during initialization of your machine, the function is
> + * tagged as __init and won't be callable after init has completed. The map
> + * passed into this function will be owned by the pinmux core and cannot be
> + * free:d.
> + */
> +int __init
> +pinmux_register_mappings(struct pinmux_map const *maps, unsigned num_maps)

Nit: keep line breaks in the parameter lists.  More grep friendly.

g.

[PATCH] drivers: create a pin control subsystem v8

[Linus Walleij]

On Fri, Sep 30, 2011 at 4:07 AM, Grant Likely <grant.likely@secretlab.ca> wrote:

> Comments below, some a bit nitpicky, but overall I think it looks
> good. ?I haven't dug into it nearly deeply enough though. ?:-(

Hopefully we can patch the remaining bugs as we go along  :-)

>> +/**
>> + * Looks up a pin control device matching a certain device name or
>> + * pure device pointer.
>
> May as well actually do kerneldoc formatting here on the comment
> blocks.

OK.

>> +struct pinctrl_dev *get_pctldev_from_dev(struct device *dev,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?const char *devname)
>
> Nit: I'm not too fond of a single function doing both name and pointer
> lookup at the same time. ?Presumably the caller would have one or the
> other and know what it needs to do. ?I'd prefer to see one by-name
> function and one by-reference. ?I'm not going to make a big deal about
> it though.

The caller currently does not know what it has or
what to do.

This is basically an interator function that is called on
a member tuple of device and device name to check
which one you have and return a matching controller
device for the key you do have.

>> + ? ? /* Register device with sysfs */
>> + ? ? pctldev->dev.parent = dev;
>> + ? ? pctldev->dev.bus = &pinctrl_bus;
>
> I don't think there is an actual need for a pinctrl bus type. ?There
> aren't any drivers that are going to be bound to these things which is
> the primary functionality that a bus type provides. ?Am I missing
> something?

That is not the reason it's there actually, what we have
discussed on the mailing list is getting sysfs entries for the same
reason gpiolib registers a class: handle pin control from userspace,
we can already see that coming and I already have a use case
for it. (Modem SIM connector control from userspace daemon.)

So first it was registering a class, then Greg said classes are
deprecated and we should use a bus instead. So it is
registering a bus to get sysfs so we can get userspace
controls.

>> +out_err:
>> + ? ? put_device(&pctldev->dev);
>> + ? ? kfree(pctldev);
>
> Once a device is initialized, it cannot be kfree()'ed directly. ?The
> .release method needs to do that.

OK. And I already had a proper .release() method doing
exactly that, so deleting this.

>> +/**
>> + * pin_free() - release a single muxed in pin so something else can be muxed in
>> + * ? instead
>
> Nit: the summary line in kerneldoc should fit on one line.

OK.

Went over the code and fixed a few other sites too.

>> +int __init
>> +pinmux_register_mappings(struct pinmux_map const *maps, unsigned num_maps)
>
> Nit: keep line breaks in the parameter lists. ?More grep friendly.

OK fixed it.

Yours,
Linus Walleij

[PATCH] drivers: create a pin control subsystem v8

[Grant Likely]

On Fri, Sep 30, 2011 at 9:05 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Sep 30, 2011 at 4:07 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
>
>> Comments below, some a bit nitpicky, but overall I think it looks
>> good. ?I haven't dug into it nearly deeply enough though. ?:-(
>
> Hopefully we can patch the remaining bugs as we go along ?:-)
>
>>> +/**
>>> + * Looks up a pin control device matching a certain device name or
>>> + * pure device pointer.
>>
>> May as well actually do kerneldoc formatting here on the comment
>> blocks.
>
> OK.
>
>>> +struct pinctrl_dev *get_pctldev_from_dev(struct device *dev,
>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?const char *devname)
>>
>> Nit: I'm not too fond of a single function doing both name and pointer
>> lookup at the same time. ?Presumably the caller would have one or the
>> other and know what it needs to do. ?I'd prefer to see one by-name
>> function and one by-reference. ?I'm not going to make a big deal about
>> it though.
>
> The caller currently does not know what it has or
> what to do.
>
> This is basically an interator function that is called on
> a member tuple of device and device name to check
> which one you have and return a matching controller
> device for the key you do have.
>
>>> + ? ? /* Register device with sysfs */
>>> + ? ? pctldev->dev.parent = dev;
>>> + ? ? pctldev->dev.bus = &pinctrl_bus;
>>
>> I don't think there is an actual need for a pinctrl bus type. ?There
>> aren't any drivers that are going to be bound to these things which is
>> the primary functionality that a bus type provides. ?Am I missing
>> something?
>
> That is not the reason it's there actually, what we have
> discussed on the mailing list is getting sysfs entries for the same
> reason gpiolib registers a class: handle pin control from userspace,
> we can already see that coming and I already have a use case
> for it. (Modem SIM connector control from userspace daemon.)
>
> So first it was registering a class, then Greg said classes are
> deprecated and we should use a bus instead. So it is
> registering a bus to get sysfs so we can get userspace
> controls.

Sure, but you don't need the bus yet.  It can be added when it is
actually needed.  I'm not convinced that the sysfs approach is
actually the right interface here (I'm certainly not a fan of the gpio
sysfs i/f), and I'd rather not be putting in unneeded stuff until the
userspace i/f is hammered out.

g.

[PATCH] drivers: create a pin control subsystem v8

[Linus Walleij]

On Fri, Sep 30, 2011 at 7:07 PM, Grant Likely <grant.likely@secretlab.ca> wrote:

> [Me]
>> That is not the reason it's there actually, what we have
>> discussed on the mailing list is getting sysfs entries for the same
>> reason gpiolib registers a class: handle pin control from userspace,
>>(...)
>
> Sure, but you don't need the bus yet. ?It can be added when it is
> actually needed. ?I'm not convinced that the sysfs approach is
> actually the right interface here (I'm certainly not a fan of the gpio
> sysfs i/f), and I'd rather not be putting in unneeded stuff until the
> userspace i/f is hammered out.

OK I buy that, I will post a v9 without bus...

Thanks!
Linus Walleij

[PATCH] drivers: create a pin control subsystem v8

[Linus Walleij]

2011/9/30 Grant Likely <grant.likely@secretlab.ca>:

>?I'm not convinced that the sysfs approach is
> actually the right interface here (I'm certainly not a fan of the gpio
> sysfs i/f), and I'd rather not be putting in unneeded stuff until the
> userspace i/f is hammered out.

Actually, thinking about it I cannot see what would be wrong
with /dev/gpio0 & friends in the first place.

Using sysfs as swiss army knife for custom I/O does not
seem like it would be long-term viable so thanks for this
observation, and I think we need /dev/gpio* put on some
mental roadmap somewhere.

Yours,
Linus Walleij

[PATCH] drivers: create a pin control subsystem v8

[Grant Likely]

On Sat, Oct 01, 2011 at 12:39:21PM +0200, Linus Walleij wrote:
> 2011/9/30 Grant Likely <grant.likely@secretlab.ca>:
> 
> >?I'm not convinced that the sysfs approach is
> > actually the right interface here (I'm certainly not a fan of the gpio
> > sysfs i/f), and I'd rather not be putting in unneeded stuff until the
> > userspace i/f is hammered out.
> 
> Actually, thinking about it I cannot see what would be wrong
> with /dev/gpio0 & friends in the first place.
> 
> Using sysfs as swiss army knife for custom I/O does not
> seem like it would be long-term viable so thanks for this
> observation, and I think we need /dev/gpio* put on some
> mental roadmap somewhere.

Agreed.  I don't want to be in the situation we are now with GPIO,
where every time I look at the sysfs interface I shudder.

g.

[PATCH] drivers: create a pin control subsystem v8

[Mike Frysinger]

On Tue, Oct 4, 2011 at 16:35, Grant Likely wrote:
> On Sat, Oct 01, 2011 at 12:39:21PM +0200, Linus Walleij wrote:
>> 2011/9/30 Grant Likely:
>> >?I'm not convinced that the sysfs approach is
>> > actually the right interface here (I'm certainly not a fan of the gpio
>> > sysfs i/f), and I'd rather not be putting in unneeded stuff until the
>> > userspace i/f is hammered out.
>>
>> Actually, thinking about it I cannot see what would be wrong
>> with /dev/gpio0 & friends in the first place.
>>
>> Using sysfs as swiss army knife for custom I/O does not
>> seem like it would be long-term viable so thanks for this
>> observation, and I think we need /dev/gpio* put on some
>> mental roadmap somewhere.
>
> Agreed. ?I don't want to be in the situation we are now with GPIO,
> where every time I look at the sysfs interface I shudder.

the problem with that is it doesn't scale.  if i have a device with
over 150 GPIOs on the SoC itself (obviously GPIO expanders can make
that much bigger), i don't want to see 150+ device nodes in /dev/.
that's a pretty big waste.  sysfs only allocates/frees resources when
userspace actually wants to utilize a GPIO.
-mike

[PATCH] drivers: create a pin control subsystem v8

[Linus Walleij]

On Sat, Oct 22, 2011 at 7:44 PM, Mike Frysinger <vapier.adi@gmail.com> wrote:
> On Tue, Oct 4, 2011 at 16:35, Grant Likely wrote:
>> On Sat, Oct 01, 2011 at 12:39:21PM +0200, Linus Walleij wrote:
>>> 2011/9/30 Grant Likely:
>>> >?I'm not convinced that the sysfs approach is
>>> > actually the right interface here (I'm certainly not a fan of the gpio
>>> > sysfs i/f), and I'd rather not be putting in unneeded stuff until the
>>> > userspace i/f is hammered out.
>>>
>>> Actually, thinking about it I cannot see what would be wrong
>>> with /dev/gpio0 & friends in the first place.
>>>
>>> Using sysfs as swiss army knife for custom I/O does not
>>> seem like it would be long-term viable so thanks for this
>>> observation, and I think we need /dev/gpio* put on some
>>> mental roadmap somewhere.
>>
>> Agreed. ?I don't want to be in the situation we are now with GPIO,
>> where every time I look at the sysfs interface I shudder.
>
> the problem with that is it doesn't scale. ?if i have a device with
> over 150 GPIOs on the SoC itself (obviously GPIO expanders can make
> that much bigger), i don't want to see 150+ device nodes in /dev/.
> that's a pretty big waste. ?sysfs only allocates/frees resources when
> userspace actually wants to utilize a GPIO.

I was more thinking along the lines of one device per GPIO controller,
then you ioctl() to ask /dev/gpio0 how many pins it has or so.

Yours,
Linus Walleij

[PATCH] drivers: create a pin control subsystem v8

[Grant Likely]

On Mon, Oct 24, 2011 at 09:26:38AM +0200, Linus Walleij wrote:
> On Sat, Oct 22, 2011 at 7:44 PM, Mike Frysinger <vapier.adi@gmail.com> wrote:
> > On Tue, Oct 4, 2011 at 16:35, Grant Likely wrote:
> >> On Sat, Oct 01, 2011 at 12:39:21PM +0200, Linus Walleij wrote:
> >>> 2011/9/30 Grant Likely:
> >>> >?I'm not convinced that the sysfs approach is
> >>> > actually the right interface here (I'm certainly not a fan of the gpio
> >>> > sysfs i/f), and I'd rather not be putting in unneeded stuff until the
> >>> > userspace i/f is hammered out.
> >>>
> >>> Actually, thinking about it I cannot see what would be wrong
> >>> with /dev/gpio0 & friends in the first place.
> >>>
> >>> Using sysfs as swiss army knife for custom I/O does not
> >>> seem like it would be long-term viable so thanks for this
> >>> observation, and I think we need /dev/gpio* put on some
> >>> mental roadmap somewhere.
> >>
> >> Agreed. ?I don't want to be in the situation we are now with GPIO,
> >> where every time I look at the sysfs interface I shudder.
> >
> > the problem with that is it doesn't scale. ?if i have a device with
> > over 150 GPIOs on the SoC itself (obviously GPIO expanders can make
> > that much bigger), i don't want to see 150+ device nodes in /dev/.
> > that's a pretty big waste. ?sysfs only allocates/frees resources when
> > userspace actually wants to utilize a GPIO.
> 
> I was more thinking along the lines of one device per GPIO controller,
> then you ioctl() to ask /dev/gpio0 how many pins it has or so.

And there is also the question of whether it is even a good idea to
export pinctrl manipulation to userspace.

g.

[PATCH] drivers: create a pin control subsystem v8

[Linus Walleij]

On Mon, Oct 24, 2011 at 9:36 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Mon, Oct 24, 2011 at 09:26:38AM +0200, Linus Walleij wrote:
(...)
>> I was more thinking along the lines of one device per GPIO controller,
>> then you ioctl() to ask /dev/gpio0 how many pins it has or so.
>
> And there is also the question of whether it is even a good idea to
> export pinctrl manipulation to userspace.

The application I've seen is in automatic control.

I think people do things like connect they GPIO pins to electrical
relays, plus on top of that they use all the stuff in drivers/staging/iio.

All that from userspace. Controlling entire factories and industrial
robots, weapon systems too, I'm afraid.

The control of these dangerous things runs on a realtime-patched
kernel, in a single userspace app with a few threads and they have
done some realtime-tetris scheduling the beast more or less
manually with SCHED_FIFO. Basically that app is all that runs on
the board, and its threads take precedence over everything else
on the system.

That is the typical beast that is poking around on the GPIO sysfs
interfaces...

Yours,
Linus Walleij

[PATCH] drivers: create a pin control subsystem v8

[Mike Frysinger]

On Mon, Oct 24, 2011 at 03:48, Linus Walleij wrote:
> On Mon, Oct 24, 2011 at 9:36 AM, Grant Likely wrote:
>> On Mon, Oct 24, 2011 at 09:26:38AM +0200, Linus Walleij wrote:
> (...)
>>> I was more thinking along the lines of one device per GPIO controller,
>>> then you ioctl() to ask /dev/gpio0 how many pins it has or so.
>>
>> And there is also the question of whether it is even a good idea to
>> export pinctrl manipulation to userspace.
>
> The application I've seen is in automatic control.
>
> I think people do things like connect they GPIO pins to electrical
> relays, plus on top of that they use all the stuff in drivers/staging/iio.
>
> All that from userspace. Controlling entire factories and industrial
> robots, weapon systems too, I'm afraid.
>
> The control of these dangerous things runs on a realtime-patched
> kernel, in a single userspace app with a few threads and they have
> done some realtime-tetris scheduling the beast more or less
> manually with SCHED_FIFO. Basically that app is all that runs on
> the board, and its threads take precedence over everything else
> on the system.
>
> That is the typical beast that is poking around on the GPIO sysfs
> interfaces...

we all agree that GPIO from userspace makes sense.  the only complaint
i've seen so far against the GPIO sysfs interface that should be
addressed is the performance overhead.

but the question here is about pinctrl.  does userspace really need to
manipulate the pinmapping ?  if we agree on that, then the question is
on the userspace interface.

assuming we want this, i can't see the performance argument being made
here for pinctrl.  which means doing a sysfs interface here like we
already have with GPIO makes the most sense.  GPIO deals in "binary"
data for the most part (reading/writing 0/1 ints) so the string-based
sysfs parsing is a bit weird, but pinctrl deals with strings
everywhere for selecting mapping groups, so sysfs is the natural
answer.
-mike

[PATCH] drivers: create a pin control subsystem v8

[Linus Walleij]

On Mon, Oct 24, 2011 at 11:20 AM, Mike Frysinger <vapier.adi@gmail.com> wrote:

> but the question here is about pinctrl. ?does userspace really need to
> manipulate the pinmapping ? ?if we agree on that, then the question is
> on the userspace interface.
>
> assuming we want this, i can't see the performance argument being made
> here for pinctrl. ?which means doing a sysfs interface here like we
> already have with GPIO makes the most sense. ?GPIO deals in "binary"
> data for the most part (reading/writing 0/1 ints) so the string-based
> sysfs parsing is a bit weird, but pinctrl deals with strings
> everywhere for selecting mapping groups, so sysfs is the natural
> answer.

Hm now I feel I start to agree with you and come back to my
original proposition to do pinctrl in sysfs after all.

I wonder how soon we have a practical use case for this.
We do have this hacked-up driver in ux500:
http://git.linaro.org/gitweb?p=bsp/st-ericsson/linux-3.0-ux500.git;a=tree;f=drivers/staging/ab5500_sim;hb=HEAD

This controls a lot of SIM card pins from userspace...

Yours,
Linus Walleij

[PATCH] drivers: create a pin control subsystem v8

[Grant Likely]

On Mon, Oct 24, 2011 at 09:48:19AM +0200, Linus Walleij wrote:
> On Mon, Oct 24, 2011 at 9:36 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> > On Mon, Oct 24, 2011 at 09:26:38AM +0200, Linus Walleij wrote:
> (...)
> >> I was more thinking along the lines of one device per GPIO controller,
> >> then you ioctl() to ask /dev/gpio0 how many pins it has or so.
> >
> > And there is also the question of whether it is even a good idea to
> > export pinctrl manipulation to userspace.
> 
> The application I've seen is in automatic control.
> 
> I think people do things like connect they GPIO pins to electrical
> relays, plus on top of that they use all the stuff in drivers/staging/iio.
> 
> All that from userspace. Controlling entire factories and industrial
> robots, weapon systems too, I'm afraid.
> 
> The control of these dangerous things runs on a realtime-patched
> kernel, in a single userspace app with a few threads and they have
> done some realtime-tetris scheduling the beast more or less
> manually with SCHED_FIFO. Basically that app is all that runs on
> the board, and its threads take precedence over everything else
> on the system.
> 
> That is the typical beast that is poking around on the GPIO sysfs
> interfaces...

... which maybe should be encouraged to use some form of uio driver.  :-)

g.

[PATCH] drivers: create a pin control subsystem v8

[Tony Lindgren]

* Grant Likely <grant.likely@secretlab.ca> [111024 12:31]:
> On Mon, Oct 24, 2011 at 09:48:19AM +0200, Linus Walleij wrote:
> > On Mon, Oct 24, 2011 at 9:36 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> > > On Mon, Oct 24, 2011 at 09:26:38AM +0200, Linus Walleij wrote:
> > (...)
> > >> I was more thinking along the lines of one device per GPIO controller,
> > >> then you ioctl() to ask /dev/gpio0 how many pins it has or so.
> > >
> > > And there is also the question of whether it is even a good idea to
> > > export pinctrl manipulation to userspace.
> > 
> > The application I've seen is in automatic control.
> > 
> > I think people do things like connect they GPIO pins to electrical
> > relays, plus on top of that they use all the stuff in drivers/staging/iio.
> > 
> > All that from userspace. Controlling entire factories and industrial
> > robots, weapon systems too, I'm afraid.
> > 
> > The control of these dangerous things runs on a realtime-patched
> > kernel, in a single userspace app with a few threads and they have
> > done some realtime-tetris scheduling the beast more or less
> > manually with SCHED_FIFO. Basically that app is all that runs on
> > the board, and its threads take precedence over everything else
> > on the system.
> > 
> > That is the typical beast that is poking around on the GPIO sysfs
> > interfaces...
> 
> ... which maybe should be encouraged to use some form of uio driver.  :-)

Changing pins from userspace is extremely handy for debugging drivers
and PM. For normal use case there should not be any need except to
view the values.

But for debugging we should have some interface for changing
the values either via debugfs or some user space program.

Regards,

Tony

[PATCH] drivers: create a pin control subsystem v8

[Grant Likely]

On Tue, Oct 25, 2011 at 10:05:32AM +0200, Tony Lindgren wrote:
> * Grant Likely <grant.likely@secretlab.ca> [111024 12:31]:
> > On Mon, Oct 24, 2011 at 09:48:19AM +0200, Linus Walleij wrote:
> > > On Mon, Oct 24, 2011 at 9:36 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> > > > On Mon, Oct 24, 2011 at 09:26:38AM +0200, Linus Walleij wrote:
> > > (...)
> > > >> I was more thinking along the lines of one device per GPIO controller,
> > > >> then you ioctl() to ask /dev/gpio0 how many pins it has or so.
> > > >
> > > > And there is also the question of whether it is even a good idea to
> > > > export pinctrl manipulation to userspace.
> > > 
> > > The application I've seen is in automatic control.
> > > 
> > > I think people do things like connect they GPIO pins to electrical
> > > relays, plus on top of that they use all the stuff in drivers/staging/iio.
> > > 
> > > All that from userspace. Controlling entire factories and industrial
> > > robots, weapon systems too, I'm afraid.
> > > 
> > > The control of these dangerous things runs on a realtime-patched
> > > kernel, in a single userspace app with a few threads and they have
> > > done some realtime-tetris scheduling the beast more or less
> > > manually with SCHED_FIFO. Basically that app is all that runs on
> > > the board, and its threads take precedence over everything else
> > > on the system.
> > > 
> > > That is the typical beast that is poking around on the GPIO sysfs
> > > interfaces...
> > 
> > ... which maybe should be encouraged to use some form of uio driver.  :-)
> 
> Changing pins from userspace is extremely handy for debugging drivers
> and PM. For normal use case there should not be any need except to
> view the values.
> 
> But for debugging we should have some interface for changing
> the values either via debugfs or some user space program.

I've got no issue with a debugfs interface, although it would probably
a good idea to put a big scary warning into klog when userspace starts
manipulating pinctrl setting.  Maybe should taint the kernel too.
Anything manipulating pinctrl, even more than gpio, *really* needs to
know what it is doing.  I'm not worried about hacking around when
doing board bringup and debug, but I'm all for barriers to actual
applications using it.

.... of course this also assumes that users have an easy to use
alternative that isn't as scary as exposing all of pinctl to
userspace.

g.

[PATCH] drivers: create a pin control subsystem v8

[Mark Brown]

On Tue, Oct 25, 2011 at 10:17:19AM +0200, Grant Likely wrote:

> I've got no issue with a debugfs interface, although it would probably
> a good idea to put a big scary warning into klog when userspace starts
> manipulating pinctrl setting.  Maybe should taint the kernel too.

Yes, we really should taint the kernel for any non-mediated interface -
never mind instability this stuff can cause physical damage if you get
it wrong.

[PATCH] drivers: create a pin control subsystem v8

[Mike Frysinger]

On Mon, Oct 24, 2011 at 03:26, Linus Walleij wrote:
> On Sat, Oct 22, 2011 at 7:44 PM, Mike Frysinger wrote:
>> On Tue, Oct 4, 2011 at 16:35, Grant Likely wrote:
>>> On Sat, Oct 01, 2011 at 12:39:21PM +0200, Linus Walleij wrote:
>>>> 2011/9/30 Grant Likely:
>>>> >?I'm not convinced that the sysfs approach is
>>>> > actually the right interface here (I'm certainly not a fan of the gpio
>>>> > sysfs i/f), and I'd rather not be putting in unneeded stuff until the
>>>> > userspace i/f is hammered out.
>>>>
>>>> Actually, thinking about it I cannot see what would be wrong
>>>> with /dev/gpio0 & friends in the first place.
>>>>
>>>> Using sysfs as swiss army knife for custom I/O does not
>>>> seem like it would be long-term viable so thanks for this
>>>> observation, and I think we need /dev/gpio* put on some
>>>> mental roadmap somewhere.
>>>
>>> Agreed. ?I don't want to be in the situation we are now with GPIO,
>>> where every time I look at the sysfs interface I shudder.
>>
>> the problem with that is it doesn't scale. ?if i have a device with
>> over 150 GPIOs on the SoC itself (obviously GPIO expanders can make
>> that much bigger), i don't want to see 150+ device nodes in /dev/.
>> that's a pretty big waste. ?sysfs only allocates/frees resources when
>> userspace actually wants to utilize a GPIO.
>
> I was more thinking along the lines of one device per GPIO controller,
> then you ioctl() to ask /dev/gpio0 how many pins it has or so.

that brings its own set of trade offs.  this might be OK from a
debugging point of view, but it means security wise we have to grant
access on a per-gpiochip basis instead of a per-gpio basis.  i think
the sysfs interface has this granularity of support already as the
root user can chmod/chown the files after exporting them.

Grant suggested we extend UIO to export GPIOs.  this would be a good
trade off i think -- sysfs is a good on-the-fly debugging/scripting
interface, but UIO gets us the performance.  sysfs overhead can be
mitigated by using pwrite/pread, but without pwritev/preadv, we're
stuck with 1-transition-per-syscall.
-mike

[PATCH] drivers: create a pin control subsystem v8

[Stephen Warren]

Linus Walleij wrote at Wednesday, September 28, 2011 6:04 AM:
> This creates a subsystem for handling of pin control devices.
> These are devices that control different aspects of package
> pins.

I still haven't had a chance to look through the .c files, just the docs
and headers, but they look good to me now, so:

Acked-by: Stephen Warren <swarren@nvidia.com>

-- 
nvpublic