lkml.org 
[lkml]   [2022]   [Sep]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@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 |

\
 
 \ /
  Last update: 2022-09-01 10:24    [W:0.077 / U:0.112 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site