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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 3 Dec 2019 09:10:49 +0000
Eugene Zaikonnikov <eugene.zaikonnikov@norphonic.com> wrote:

> 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?

Look at the datasheet, that is one high and one low for each of
rh and temp?  Initially I thought you meant 2 in each direction for
each channel which we don't support (there is no means of
encoding it in the event code to userspace).

That can be handled just fine using the event setup for each channel
as one channel can have multiple event specs.

Thanks,

Jonathan



> 
> 
> --
> 
>   Eugene
>