linux-kernel - Re: [PATCH anybus v2 1/5] misc: support the Arcx anybus bridge.

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]

Message-ID: <CAGngYiVorxYY+8e75068i4=PJi1gEvnGkgvBfi0+9H+c1qD-oQ@mail.gmail.com>
Date:   Thu, 1 Nov 2018 13:17:22 -0400
From:   Sven Van Asbroeck <thesven73@...il.com>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     Arnd Bergmann <arnd@...db.de>,
        Sven Van Asbroeck <svendev@...x.com>,
        Lee Jones <lee.jones@...aro.org>, robh+dt@...nel.org,
        mark.rutland@....com,
        Andreas Färber <afaerber@...e.de>,
        treding@...dia.com, David Lechner <david@...hnology.com>,
        noralf@...nnes.org, johan@...nel.org,
        Michal Simek <monstr@...str.eu>, michal.vokac@...ft.com,
        gregkh@...uxfoundation.org, john.garry@...wei.com,
        geert+renesas@...der.be, robin.murphy@....com,
        paul.gortmaker@...driver.com,
        sebastien.bourdelin@...oirfairelinux.com, icenowy@...c.io,
        Stuart Yoder <stuyoder@...il.com>, maxime.ripard@...tlin.com,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>
Subject: Re: [PATCH anybus v2 1/5] misc: support the Arcx anybus bridge.

Hi Linus,

> This is fun :)

It sure is ! It's fascinating to see how the kernel abstractions are designed,
and how code is reviewed here.

>> +static DEVICE_ATTR_RO(version);
>
> Do you need this in userspace really?
>
>> +static DEVICE_ATTR_RO(design_number);
>
> And this?

Unfortunately, I do :(
The application software reads these out and displays them in an UI. It's
important to be able to see these on a running device.
Perhaps there is another kernel abstraction I could use?

> This:
>>
>> +       cd->reset_gpio = of_get_named_gpio(np, "reset-gpios", 0);
>> +       if (!gpio_is_valid(cd->reset_gpio)) {
>> +               dev_err(dev, "reset-gpios not found\n");
>> +               return -EINVAL;
>> +       }
>> +       devm_gpio_request(dev, cd->reset_gpio, NULL);
>> +       gpio_direction_output(cd->reset_gpio, 0);
>
> Should be:
>
> cd->reset_gpiod = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> if (IS_ERR(cd->reset_gpiod))
>     return PTR_ERR(cd->reset_gpiod);

That's actually pretty neat !
Now we have nothing that explicitly refers to the devicetree (except the
device binding, of course). I now see the point you made earlier:
resets, gpios, interrupts etc are associated with "struct device" and
not necessarily with the devicetree.

The rest of your suggestions have already gone in.

I hope you'll have some time later to look at the architecture description
in the bus driver. Looking forward to your feedback and to the next iteration !

Cheers,
Sven