Thread (28 messages) 28 messages, 3 authors, 2015-09-25

Re: [PATCH v4 1/2] pinctrl: Add driver for Alphascale asm9260 pinctrl

From: Oleksij Rempel <hidden>
Date: 2015-09-09 05:55:40
Also in: linux-devicetree, lkml

Hi, finally i'm able to continue to work on it.

Am 13.05.2015 um 13:00 schrieb Linus Walleij:
On Tue, May 12, 2015 at 6:25 PM, Oleksij Rempel [off-list ref] wrote:
quoted
Am 05.05.2015 um 17:12 schrieb Linus Walleij:
quoted
quoted
Just reference the statically defined array by a pointer instead,
this just takes up a lot o memory for no reason.
This two arrays have different types this is why i convert it.
priv->pin_desc[i].name - here i copy pointer any ways, and
priv->pin_desc[i].number can be smaller then pointer.
I probably do not understand what you're trying to do, sorry :(

Why is it necessary for the driver to copy one description of
the pin into another?
If i understand it correctly, pinctrl_pin_desc is essential part of
pinmux framework. Theoretically i should define just statical array of
this struct, but by this number of pins and functions it hard to keep it
readable and error free. So i decided to create asm9260_mux_table which
contains every thing i need. The side effect is higher memory usage
since i need to create pinctrl_pin_desc on fly.

May be i miss some thing?
quoted
quoted
Mory copying. I don't see why this is necessary at all.
I hadn't seen the point to define groups statically, especially because
they are used only  to make curious user happy. So, memory will be used
only if you request the list over sysfs. Or miss some thing?
pinctrl does not even use sysfs.
please read debugfs.
The group names are usually there for matching with a function,
it is part of the core functionality. The group name + function name
matching is even more obvious in the dt case.

They also make things easier to read in debugfs yes, but
the core of the crux is to make it easy to config function+groups
states with e.g. DT or board files.
quoted
quoted
quoted
+static struct pinmux_ops asm9260_pinmux_ops = {
+       .get_functions_count    = asm9260_pinctrl_get_funcs_count,
+       .get_function_name      = asm9260_pinctrl_get_func_name,
+       .get_function_groups    = asm9260_pinctrl_get_func_groups,
+       .set_mux                = asm9260_pinctrl_set_mux,
+       /* TODO: should we care about gpios here? gpio_request_enable? */
I think you should, if you also have a matching GPIO driver.
I fear it would cause unpredictable bugs. GPIO mode is just one of mux
modes. If some one will request gpio some busy or dangerous line it
would do more harm then use. So, i assume limiting this only to device
tree would be better.
Device tree or not doesn't matter, .gpio_request_enable() is used
as a shortcut to mux in GPIO pins.

If the simultaneous use of a pin for a device and GPIO bothers
you there is nowadays (linux-next or my devel branch) a .strict
option in pinmux_ops that you can set to disallow simultaneous
use by devices and GPIO of the same pin.

Yours,
Linus Walleij

-- 
Regards,
Oleksij

Attachments

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help