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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 27 May 2025 16:48:30 +0200
Waqar Hameed <waqar.hameed@axis.com> wrote:

> On Sun, May 25, 2025 at 10:30 +0100 Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Tue, 20 May 2025 13:27:54 +0200
> > Waqar Hameed <waqar.hameed@axis.com> wrote:
> >  
> >> On Sun, May 18, 2025 at 18:38 +0100 Jonathan Cameron <jic23@kernel.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!  
> 
> I think if you use it in multiple places, it should definitively be a
> macro definition. I just sent some patches for those that only used it
> once (I didn't include those with `KBUILD_MODNAME`. We can discuss if we
> should also address those in that thread).

I would disagree slightly.  If it is used in multiple places because there
is some inherent reason they should have same string then I absolutely agree.
If it's just because it's a convenient string that is used twice in places
that could have had different string then not so much.

> 
> However, there are a bunch of drivers that _only_ use a macro definition
> in `.name` and `indio_dev->name`, including this one. That _is_ more
> than one place, so we should actually leave it? Or do you still think we
> should have the same string literal in both places, as you originally
> commented above?

I'd prefer that for new code, but it is a less clear cut case than the ones
you have tidied up, so not worth the churn of tidying up unless people
are otherwise working on the relevant drivers.

Jonathan

>