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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 09 Sep 2015 07:55:24 +0200
From:	Oleksij Rempel <linux@...pel-privat.de>
To:	Linus Walleij <linus.walleij@...aro.org>
CC:	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 1/2] pinctrl: Add driver for Alphascale asm9260 pinctrl

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 <linux@...pel-privat.de> 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?

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?

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


Download attachment "signature.asc" of type "application/pgp-signature" (214 bytes)