Re: [PATCH v2 1/2] Driver for TI HDC20x0 humidity and temperature sensors

[Eugene Zaikonnikov <eugene.zaikonnikov@norphonic.com>]

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