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

[Sascha Hauer <s.hauer@pengutronix.de>]

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 |