linux-kernel - Re: [PATCH 3/3] iio: Add driver for Nicera D3-323-AA PIR sensor

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: <20250525103019.3773be94@jic23-huawei>
Date: Sun, 25 May 2025 10:30:19 +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

On Tue, 20 May 2025 13:27:54 +0200
Waqar Hameed <waqar.hameed@...s.com> wrote:

> On Sun, May 18, 2025 at 18:38 +0100 Jonathan Cameron <jic23@...nel.org> wrote:
> 
> >> >> +#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!  
> 
> A quick search tells that there are (at least) 105 of those:
> 
>   rgrep -A 30 "\.driver" drivers/iio/ | grep "\.name" | grep -v '"'
>   
> I was just about to write a small Python script to fix those, but just
> wanted to confirm with you before spending more time on this. So if you
> don't want to do this yourself, I can help your here :)

It's probably not worth the churn on the ones that have the string repeated
multiple times.  However, perhaps any that are only using it for .name would
be good to tidy up?  Those are less a case of it being 'taste' vs it being silly
to have a define!

> 
> >>  
> >> >> +	/* 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?  
> 
> No objections.
> 
> [...]
>