linux-kernel - Re: [PATCH v2 1/2] gpio: Add gpio latch driver

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: <20220901082017.GP24324@pengutronix.de>
Date:   Thu, 1 Sep 2022 10:20:17 +0200
From:   Sascha Hauer <s.hauer@...gutronix.de>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        Sascha Hauer <kernel@...gutronix.de>
Subject: Re: [PATCH v2 1/2] gpio: Add gpio latch driver

On Wed, Aug 31, 2022 at 11:50:47PM +0300, Andy Shevchenko wrote:
> On Wed, Aug 31, 2022 at 9:02 AM Sascha Hauer <s.hauer@...gutronix.de> wrote:
> >
> > This driver implements a GPIO multiplexer based on latches connected to
> > other GPIOs. A set of data GPIOs is connected to the data input of
> > multiple latches. The clock input of each latch is driven by another
> > set of GPIOs. With two 8-bit latches 10 GPIOs can be multiplexed into
> > 16 GPIOs. GPOs might be a better term as in fact the multiplexed pins
> > are output only.
> 
> I'm still unsure it shouldn't be a part of the (not yet in upstream)
> driver that I have mentioned before. But let's leave this apart right
> now.

I don't see how this could be done. The before mentioned driver depends
on a gpio-mux which is a binary decoder. This doesn't have a
correspondence in this driver.

> 
> ...
> 
> > +#include <linux/err.h>
> > +#include <linux/module.h>
> 
> > +#include <linux/of_device.h>
> 
> Why?
> It seems you misplaced it instead of mod_devicetable.h.

Ok.

> 
> > +#include <linux/gpio/driver.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/gpio/consumer.h>
> 
> Keep above sorted?
> 
> ...
> 
> > +       struct mutex mutex;
> > +       spinlock_t spinlock;
> 
> Checkpatch usually complains if locks are not commented. Looking at
> the below code, why it's not an (anonymous) union?

checkpatch only complains here when given a --subjective. Anyway,
commenting it is a good thing, and a union can be used here.

> 
> ...
> 
> > +       if (val)
> > +               priv->shadow[latch] |= BIT(offset % priv->n_pins);
> > +       else
> > +               priv->shadow[latch] &= ~BIT(offset % priv->n_pins);
> 
> I believe shadow should be defined as unsigned long * and hence normal
> bit operations can be applied. For example here is assign_bit().

Good point.

> > +static const struct of_device_id gpio_latch_ids[] = {
> > +       {
> > +               .compatible     = "gpio-latch",
> > +       }, {
> > +               /* sentinel */
> > +       }
> 
> You may compress this to the 2 LoCs.

I usually prefer not doing that as it means that we have to reformat it
once we initialize other fields as well, like here for example .data.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |