Message-ID: <20250518183852.7e9afdac@jic23-huawei>
Date: Sun, 18 May 2025 18:38:52 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Waqar Hameed <waqar.hameed@...s.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, <kernel@...s.com>,
 <linux-kernel@...r.kernel.org>, <linux-iio@...r.kernel.org>
Subject: Re: [PATCH 3/3] iio: Add driver for Nicera D3-323-AA PIR sensor


> >> +#define D3323AA_DRV_NAME "d3323aa"  
> >
> > Put that inline where used.  A define like this both implies that various values
> > must be the same when they need not be and means that we have to go find the
> > define to fine out what they are set to.  Just setting the strings directly
> > tends to end up more readable.  
> 
> Sure, we can do that. (There are a bunch of IIO-drivers doing this, so I
> just thought that was the "convention".)

I'm sometimes in less fussy mood.  One day I might just clean those up
so there is nothing to copy into new drivers!

>
> >> +	/* Input clock or output detection signal (Vout). */  
> >
> > I'd rename. Vout kind of suggests a variable voltage. This seems to just
> > be a level signal.  
> 
> >> +	struct gpio_desc *gpiod_clk_vout;  
> 
> Yeah, it's a weird pin with multiple use-cases... I just named it
> according to what the datasheet calls it. What about
> `gpiod_clk_detection`?

That sounds like it's detecting a clock.  Hmm.  
gpiod_clkin_detectout maybe?

> >> +static int d3323aa_write_settings(struct iio_dev *indio_dev,
> >> +				  const unsigned long *regmap)  
> >
> > Rename regmap. regmap means some specific stuff in the kernel register_bm or something
> > like that avoids that potential confusion.
> >
> > However, it would be much easier to read this driver if only this
> > function and the read one knew about the bitmap stuff.  Inside the reset
> > of the driver just store and pass around a structure with the various fields.
> > Then in here use that to build up the bitmap locally and write to the device.
> > The opposite in read which decodes the bitmap into those fields.
> >
> > That will make for a more standard and easier to review driver.  
> 
> Hm, maybe it would be easier to understand it if we remove the bitmap
> from `struct d3323aa_data` and instead only store the relevant state
> variables. We still would need to have the `regmap_lock` but to instead
> protect all of the variables.
> 
> Let's go for that then! I actually don't have any strong arguments (or
> opinions) against it.

Just don't use the term regmap and that all sounds fine as it won't
cause confusion against the stuff in drivers/base/regmap/


> >> +static irqreturn_t d3323aa_irq_detection(int irq, void *dev_id)
> >> +{
> >> +	struct iio_dev *indio_dev = dev_id;
> >> +	struct d3323aa_data *data = iio_priv(indio_dev);
> >> +	enum iio_event_direction dir;
> >> +	int val;
> >> +
> >> +	val = gpiod_get_value(data->gpiod_clk_vout);  
> >
> > Ideally I'd like a setup where we can wire the interrupt side of this
> > to one pin and the gpio needed for writing to another.  In practice
> > they may well be the same pin but that does introduced a bunch of races
> > and dependency on what the interrupt controller does when an irq
> > is disabled.
> >
> > Using one pin as an irq and as a data line is prone to some nasty
> > corner cases though it works on some SoCs.  
> 
> Hm, that's an interesting idea. However, as you say there might then be
> other issues to take care of.
> 
> On the other hand, the scope of the clock bit bang is quite small (only in
> `*read/write_settings()`). So I guess we just leave it then?

I'd give a very high chance that if the driver sees much use you will get
bug reports as we've seen for ADCs that do similar multiple uses of the
same line (one of which is as an interrupt).

Here we will probably just see spurious interrupts which will be mostly
harmless.  So fine as it stands.

Powered by blists - more mailing lists