Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 1 Sep 2022 10:20:17 +0200
From:   Sascha Hauer <s.hauer@pengutronix.de>
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
Cc:     "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Geert Uytterhoeven <geert@linux-m68k.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        Bartosz Golaszewski <brgl@bgdev.pl>,
        Sascha Hauer <kernel@pengutronix.de>
Subject: Re: [PATCH v2 1/2] gpio: Add gpio latch driver
Message-ID: <20220901082017.GP24324@pengutronix.de>
References: <20220831055811.1936613-1-s.hauer@pengutronix.de>
 <20220831055811.1936613-2-s.hauer@pengutronix.de>
 <CAHp75Vd6LCv1wcxV58Q3Pa=eBRdEK6XusbfeDQtm8+R0hAAyWg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHp75Vd6LCv1wcxV58Q3Pa=eBRdEK6XusbfeDQtm8+R0hAAyWg@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

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