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

[<Victor.Duicu@microchip.com>]

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.

> > +#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."

> > +     (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