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

[Linus Walleij]

On Tue, May 12, 2015 at 6:25 PM, Oleksij Rempel <linux@xxxxxxxxxxxxxxxx> wrote:
> Am 05.05.2015 um 17:12 schrieb Linus Walleij:

>> 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?

>> 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.

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.

>>> +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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/