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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 2 Jun 2025 14:49:29 +0000
<Victor.Duicu@microchip.com> wrote:

> On Fri, 2025-05-30 at 17:53 +0100, Jonathan Cameron wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> > 
> > On Thu, 29 May 2025 12:36:28 +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,
> >   
> Hi Jonathan,
> 
> ...
> 
> >   
> > > +
> > > +What:                /sys/bus/iio/devices/iio:deviceX/in_beta1
> > > +KernelVersion:       6.17
> > > +Contact:     linux-iio@vger.kernel.org
> > > +Description:
> > > +             This attribute controls the value of beta correction
> > > +             for channel 1.  
> > 
> > Is this something that we'd normally expect to manually update? What
> > is
> > it a characteristic of?  If it is expected to the be related to the
> > diodes attached, that's a problem for firmware/dt, not sysfs
> > interfaces.
> >   
> 
> Beta is a characteristic of the diode/transistor placed on the setup.
> Different diodes require different betas.
> So yes, beta value should be in devicetree.
> 
> ...
> > > 
> > > +
> > > +             priv->iio_chan[iio_idx++] = (struct
> > > iio_chan_spec)MCP9982_CHAN(reg_nr, reg_nr,
> > > +                                                     
> > > MCP9982_INT_VALUE_ADDR(reg_nr));  
> > 
> > Seems very likely that the (struct iio_chan_spec) should be in the
> > macro definition.
> >   
> 
> In version 1 the macro used to define the channels was:
> 
> >   
> #define MCP9982_CHAN(index, si, __address) ({ \
> 	struct iio_chan_spec __chan = { \
> 		.type = IIO_TEMP, \
> 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> 		.info_mask_shared_by_all_available =
> BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> 		.info_mask_shared_by_all =
> BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> 		.channel = index, \
> 		.address = __address, \
> 		.scan_index = si, \
> 		.scan_type = { \
> 			.sign = 'u', \
> 			.realbits = 8, \
> 			.storagebits = 8, \
> 			.endianness = IIO_CPU \
> 		}, \
> 		.indexed = 1, \
> 	}; \
> 	__chan; \
> })
> 
> which contains struct iio_chan_spec inside.
> Andy suggested to use compound literal, so I switched to version 2.
> I still use a macro because most of the parameters are common among the
> channels.

I think we can add the (struct iio_chan_spec) to the macro definition. 

> 
> With Kind Regards,
> Victor Duicu