| 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
| ||
|
Date: Tue, 28 Mar 2017 12:43:34 +0200
From: Gregory CLEMENT <gregory.clement@...e-electrons.com>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: "linux-gpio\@vger.kernel.org" <linux-gpio@...r.kernel.org>,
Jason Cooper <jason@...edaemon.net>,
Andrew Lunn <andrew@...n.ch>,
Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
"linux-arm-kernel\@lists.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
Rob Herring <robh+dt@...nel.org>,
"devicetree\@vger.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>,
Nadav Haklai <nadavh@...vell.com>,
Victor Gu <xigu@...vell.com>, Marcin Wojtas <mw@...ihalf.com>,
Wilson Ding <dingwei@...vell.com>,
Hua Jing <jinghua@...vell.com>,
Neta Zur Hershkovits <neta@...vell.com>
Subject: Re: [PATCH v2 3/7] pinctrl: armada-37xx: Add pin controller support for Armada 37xx
Hi Linus,
On lun., mars 27 2017, Linus Walleij <linus.walleij@...aro.org> wrote:
> On Tue, Mar 21, 2017 at 7:28 PM, Gregory CLEMENT
> <gregory.clement@...e-electrons.com> wrote:
>
>> The Armada 37xx SoC come with 2 pin controllers: one on the south
>> bridge (managing 28 pins) and one on the north bridge (managing 36 pins).
>>
>> At the hardware level the controller configure the pins by group and not
>> pin by pin. This constraint is reflected in the design of the driver:
>> only the group related functions are implemented.
>
> Interesting!
>
>> +static int armada_37xx_pmx_direction_input(struct armada_37xx_pinctrl *info,
>> + unsigned int offset)
>> +{
>> + unsigned int reg = OUTPUT_EN;
>> + unsigned int mask;
>> +
>> + if (offset >= GPIO_PER_REG) {
>> + offset -= GPIO_PER_REG;
>> + reg += sizeof(u32);
>> + }
>> + mask = BIT(offset);
>> +
>> + return regmap_update_bits(info->regmap, reg, mask, 0);
>> +}
>> +
>> +
>> +
>
> A bit of excess whitespace, OK nitpicking.
>
As I need to send a v4, I will fix it in the same time.
> Then this stuff:
>
>> +static int armada_37xx_add_function(struct armada_37xx_pmx_func *funcs,
>> + int *funcsize, const char *name)
>> +{
>> + int i = 0;
>> +
>> + if (*funcsize <= 0)
>> + return -EOVERFLOW;
>> +
>> + while (funcs->ngroups) {
>> + /* function already there */
>> + if (strcmp(funcs->name, name) == 0) {
>> + funcs->ngroups++;
>> +
>> + return -EEXIST;
>> + }
>> + funcs++;
>> + i++;
>> + }
>> +
>> + /* append new unique function */
>> + funcs->name = name;
>> + funcs->ngroups = 1;
>> + (*funcsize)--;
>> +
>> + return 0;
>> +}
>> +
>> +static int armada_37xx_fill_group(struct armada_37xx_pinctrl *info, int base)
>> +{
>> + int n, num = 0, funcsize = info->data->nr_pins;
>> +
>> + for (n = 0; n < info->ngroups; n++) {
>> + struct armada_37xx_pin_group *grp = &info->groups[n];
>> + int i, j, f;
>> +
>> + grp->pins = devm_kzalloc(info->dev,
>> + (grp->npins + grp->extra_npins) *
>> + sizeof(*grp->pins), GFP_KERNEL);
>> + if (!grp->pins)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < grp->npins; i++)
>> + grp->pins[i] = grp->start_pin + base + i;
>> +
>> + for (j = 0; j < grp->extra_npins; j++)
>> + grp->pins[i+j] = grp->extra_pin + base + j;
>> +
>> + for (f = 0; f < NB_FUNCS; f++) {
>> + int ret;
>> + /* check for unique functions and count groups */
>> + ret = armada_37xx_add_function(info->funcs, &funcsize,
>> + grp->funcs[f]);
>> + if (ret == -EOVERFLOW)
>> + dev_err(info->dev,
>> + "More functions than pins(%d)\n",
>> + info->data->nr_pins);
>> + if (ret < 0)
>> + continue;
>> + num++;
>> + }
>> + }
>> +
>> + info->nfuncs = num;
>> +
>> + return 0;
>> +}
>> +
>> +static int armada_37xx_fill_func(struct armada_37xx_pinctrl *info)
>> +{
>> + struct armada_37xx_pmx_func *funcs = info->funcs;
>> + int n;
>> +
>> + for (n = 0; n < info->nfuncs; n++) {
>> + const char *name = funcs[n].name;
>> + const char **groups;
>> + int g;
>> +
>> + funcs[n].groups = devm_kzalloc(info->dev, funcs[n].ngroups *
>> + sizeof(*(funcs[n].groups)),
>> + GFP_KERNEL);
>> + if (!funcs[n].groups)
>> + return -ENOMEM;
>> +
>> + groups = funcs[n].groups;
>> +
>> + for (g = 0; g < info->ngroups; g++) {
>> + struct armada_37xx_pin_group *gp = &info->groups[g];
>> + int f;
>> +
>> + for (f = 0; f < NB_FUNCS; f++) {
>> + if (strcmp(gp->funcs[f], name) == 0) {
>> + *groups = gp->name;
>> + groups++;
>> + }
>> + }
>> + }
>> + }
>> + return 0;
>> +}
>
> I would be happy if you add kerneldoc to these functions and explain
> what they do. Because I don't get it. I guess they are filling in the data
> structures but yeah. Hard to follow.
OK
>
>> + match = of_match_node(armada_37xx_pinctrl_of_match, np);
>> + info->data = (struct armada_37xx_pin_data *)match->data;
>
> Use of_device_get_match_data()
OK
>
>
>> +static struct platform_driver armada_37xx_pinctrl_driver = {
>> + .driver = {
>> + .name = "armada-37xx-pinctrl",
>> + .of_match_table = armada_37xx_pinctrl_of_match,
>> + },
>> + .probe = armada_37xx_pinctrl_probe,
>> +};
>> +
>> +builtin_platform_driver(armada_37xx_pinctrl_driver);
>
> It almost looks like your could use builting_platform_driver_probe() actually,
> and tag the costly initfunctions with __init so they get dicarded
> after probe.
Sure, I will do it
Thanks,
Gregory
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
Powered by blists - more mailing lists