Re: [PATCH 0/1] Add support for TI HDC200x humidity and temperature sensors

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 22 Nov 2019 15:50:43 +0100
Eugene Zaikonnikov <eugene.zaikonnikov@norphonic.com> wrote:

> Jonathan,
> 
> Just got back to fixing most of the issues you pointed out, excepting these:
> 
> On 03.11.2019 13:19, Jonathan Cameron wrote:
> 
> > +  
> >> +static const struct iio_chan_spec hdc200x_channels[] = {
> >> +    {
> >> +        .type = IIO_TEMP,
> >> +        .extend_name = "Temperature",  
> > Any use of extend_name changes the ABI and should be done extremely
> > carefully.  It basically means that generic userspace code cannot
> > use your driver.
> >
> > Here I can't see any advantage in doing so at all so drop it.  
> 
> If I have two temperature channels and provide no extend_name to at least one of them, they fail to register.
This only worked on the assumption that you could use PEAK below so only
have one channel for both.

Alternative would be to index them, but that doesn't make much sense here either.

> 
> >  
> >> +        },
> >> +    },
> >> +    {
> >> +        .type = IIO_TEMP,
> >> +        .extend_name = "Temperature_MAX",  
> > Could we use the IIO_CHAN_INFO_PEAK element for this?  Given
> > the different scale, we'd need to do some work to make it
> > have the same scale as temp.  
> 
> Started looking into this.
> Are there any examples or docs of using IIO_CHAN_INFO_PEAK? Couldn't find much in the IIO subtree.
> Is it used together with INFO_RAW? And temp&max temp channels have different scales already.

ah. I missed they had different scale values.  That's unfortunate.
A simple work around for this might be to just multiply the output in the _MAX
case by 256.  That way the two would have the same scale which is assumed if using
the IIO_RAW and IIO_PEAK together.

> 
> >> +{
> >> +    char tmp = (~mask & data->drdy_config) | val;
> >> +    int ret;
> >> +
> >> +    ret = i2c_smbus_write_byte_data(data->client,
> >> +                    HDC200X_REG_RESET_DRDY_INT_CONF, tmp);
> >> +    if (!ret)
> >> +        data->drdy_config = tmp;
> >> +
> >> +    return ret;
> >> +}
> >> +
> >> +static int hdc200x_get_measurement_word(struct hdc200x_data *data,
> >> +                    struct iio_chan_spec const *chan)  
> > These wrappers add relatively little.  I'd prefer that you just call
> > the i2c calls directly instead.  Less code and ultimately a tiny
> > bit easier to review.  
> 
> Removed wrappers that are called once. Leaving the wrapper called in different places, makes intent more clear IMO.
Hmm. It's always a trade off.  Will see what the result looks
like!

Jonathan

> 
> --
> 
>   Eugene
>