Re: [PATCH 2/2] iio: temperature: ltc2983: Add support for ADT7604

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Tue, May 12, 2026 at 09:37:57AM +0000, Stan, Liviu wrote:
> On Tue, May 12, 2026 Andy Shevchenko wrote:
> > > > >  #define LTC2983_CHAN_START_ADDR(chan) \
> > > > >  			(((chan - 1) * 4) +
> > > > LTC2983_CHAN_ASSIGN_START_REG)
> > > > > -#define LTC2983_CHAN_RES_ADDR(chan) \
> > > > > -			(((chan - 1) * 4) + LTC2983_TEMP_RES_START_REG)
> > > > > +#define LTC2983_CHAN_RES_ADDR(chan, base) \
> > > > > +			((((chan) - 1) * 4) + (base))
> > > >
> > > > For the sake of consistency I would see (base) also to be in the
> > _START_ADDR()
> > > > macro.
> > >
> > > I said I would change this in v2, but on second look, I think it would be better
> > > to keep LTC2983_CHAN_START_ADDR without a (base) parameter. The base
> > > parameter in LTC2983_CHAN_RES_ADDR exists because the ADT7604 adds a
> > > second result register bank, so the base genuinely varies. For channel
> > assignment
> > > there is only one bank, so adding a base parameter would make the macro
> > look
> > > configurable when it isn't and force callers to always pass
> > > LTC2983_CHAN_ASSIGN_START_REG.
> > 
> > Do the names of the definitions _START_ADDR and _RES_ADDR come directly
> > from
> > the datasheet?  Also, given the above explanation I would see rather (bank)
> > than (base) there. With this it makes less attractive for a change that I
> > suggested earlier.
> > 
> > > Happy to change if you still prefer consistency.
> > 
> > With current names they sound like they are semantically tighten, when in
> > practice it's not so. There are options:
> > - move to (bank) and leave as currently done
> > - synchronise them and use (base) in both cases
> > - rename one or the other to be different by the name, so less confusion is
> > added
> > 
> > Your choice needs to be based on the datasheet explanation for these
> > registers.
> 
> The datasheet calls the memory regions "Channel Assignment Data"
> (0x0200-0x024F),  "Temperature Result Memory" (0x0010-0x005F) and
> "Resistance Result Memory" (0x060-0x0AF). Each region is a flat array of
> 4-byte slots, one per channel, so LTC2983_CHAN_ in both macro names
> refers to the offset of a specific channel's slot within the enclosing region.
> But I agree that it can easily create confusion.
> 
> Given that, I think it would be best to rename
> LTC2983_CHAN_START_ADDR(chan) to LTC2983_CHAN_ASSIGN_ADDR(chan),
> which aligns it with the existing LTC2983_CHAN_ASSIGN_START_REG
> constant and LTC2983_CHAN_RES_ADDR(chan, base) to
> LTC2983_RESULT_ADDR(chan, base). I would also keep (base) rather than 
> (bank) for the parameter name, since the macro expects the base address 
> of the memory region.
> 
> So, in the end we would have:
> #define LTC2983_CHAN_ASSIGN_ADDR(chan) \
> 			((((chan) - 1) * 4) + LTC2983_CHAN_ASSIGN_START_REG)
> #define LTC2983_RESULT_ADDR(chan, base) \
> 			((((chan) - 1) * 4) + (base))

This is better. Thanks!
Maybe others can propose even better ones, dunno, but these work for me.

-- 
With Best Regards,
Andy Shevchenko