Re: [PATCH v5 2/2] iio: temperature: add support for MCP998X

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 24 Sep 2025 12:47:31 +0000
<Victor.Duicu@microchip.com> wrote:

> On Sat, 2025-09-20 at 11:55 +0100, Jonathan Cameron wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> >   
> 
> Hi Jonathan,
> 
> > On Thu, 18 Sep 2025 14:19:37 +0300
> > <victor.duicu@microchip.com> wrote:
> >   
> > > From: Victor Duicu <victor.duicu@microchip.com>
> > > 
> > > This is the driver for Microchip MCP998X/33 and MCP998XD/33D
> > > Multichannel
> > > Automotive Temperature Monitor Family.
> > > 
> > > Signed-off-by: Victor Duicu <victor.duicu@microchip.com>  
> > Hi Victor,
> > 
> > Various minor comments inline.
> > Given the build warnings I didn't elect to just tidy these up whilst
> > applying.
> > Seemed like there was slightly too high a risk of me messing it up!
> > Also we have lots of time as IIO is closed for this cycle now.
> > 
> > Jonathan
> >   
> ...
> 
> > 
> >   
> > > +/**
> > > + * Bit flags and their meaning  
> > 
> > As below. I don't think it is worth encoding these in a bitmap. Just
> > use
> > 5 bools to represent the state.
> >   
> > > + * @RECD34_ENABLE:           state of Resistance Error
> > > Correction(REC) on channels 3 and 4
> > > + * @RECD12_ENABLE:           state of Resistance Error
> > > Correction(REC) on channels 1 and 2
> > > + * @APDD_ENABLE:             state of anti-parallel diode mode
> > > + * @RUN_STATE:                       chip is in run state,
> > > otherwise is in standby state
> > > + * @WAIT_BEFORE_READ:                whether we need to wait a
> > > delay before reading a new value
> > > + */
> > > +#define RECD34_ENABLE                                0
> > > +#define RECD12_ENABLE                                1
> > > +#define APDD_ENABLE                          2
> > > +#define RUN_STATE                            3
> > > +#define WAIT_BEFORE_READ                     4
> > > +#define USE_PREVIOUS_FREQ                    5
> > > +  
> 
> Considering that I am planning to add new features to this driver,
> I think that it would be useful to keep the flags.

Unless there are many of these I think the loss of readability vs
the likely limited extra space used by bools still makes flags a
less than ideal approach.  For reasons of DMA safe buffer allocations
on many architectures there is a lot of padding in the iio_dev / iio_priv
allocation, so you can always check if it makes any difference at all
on the allocated data

> 
> > > +#define MCP9982_CHAN(index, si, __address)
> > > (                                         \  
> > Why the outer set of ()?  
> 
> Without the outer () compiler returns error "Macros with complex values
> should be enclosed in parentheses."

Hmm. I'd ignore that as an unhelpful warning in this case.

> 
> > > +     (struct iio_chan_spec)
> > > {                                                        \
> > > +             .type =
> > > IIO_TEMP,                                                       \
> > > +             .info_mask_separate =
> > > BIT(IIO_CHAN_INFO_RAW),                           \
> > > +             .info_mask_shared_by_all_available =
> > > BIT(IIO_CHAN_INFO_SAMP_FREQ) |     \
> > > +            
> > > BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),                    
> 
> Kind Regards,
> Victor