On 01.12.2019 13:36, Jonathan Cameron wrote:
>
>> Heater out has been converted to IIO_CHAN_INFO_ENABLE, hope it is idiomatic use.
> Hmm. This is one of those cases where we are probably better off matching
> existing drivers even if they are a bit illogical.
>
> The enable element is mainly used for counting type sensors (start counting
> steps etc) where there is a clear difference between it being on and taking
> a measurement.
OK, will revert that to prior.
> +static const struct iio_chan_spec hdc2010_channels[] = {
>> + {
>> + .type = IIO_TEMP,
>> + .address = HDC2010_REG_TEMP_LOW,
>> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |
>> + BIT(IIO_CHAN_INFO_SCALE),
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> + },
>> + {
>> + .type = IIO_TEMP,
>> + .address = HDC2010_REG_TEMP_MAX,
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PEAK),
> Not sure I like this approach of a separate channel. The intent of
> my previous review as to suggest we used a single channel. Here
> we are really just adding one to get an address. Whilst it works
> today, this sort of unusual structure can make it harder to refactor
> core elements of the code in the future.
>
> I'd rather see a bit of indirection where address actually gives
> an enum value from which the data and _MAX registers can be
> established via a lookup in an associated array.
I see what you mean now. Was taking the name of .address field literally, but if anything goes there, sure.
While we are at it, there are four r/w threshold registers on the device for rh/temp. Should one implement them in the future, are they going to be also mixed into these somehow or can they be own event channels?
--
Eugene
