Re: [PATCH V2] iio: pressure: hp03: Add Hope RF HP03 sensor support

[Marek Vasut <marex@denx.de>]

On 04/06/2016 08:04 AM, Peter Meerwald-Stadler wrote:
>> Add support for HopeRF pressure and temperature sensor.
> 
> some minor comments

Thanks

[...]

>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/pressure/hp03.txt
>> @@ -0,0 +1,17 @@
>> +HopeRF HP03 digital pressure/temperature sensors
>> +
>> +Required properties:
>> +- compatible: must be "hoperf,hp03"
>> +- xclr-gpio:  must be device tree identifier of the XCLR pin .
> 
> remove space before . 
> indentation seems wrong

OK, fixed now.

>> +              The XCLR pin is a reset of the ADC in the chip,
>> +	      it must be pulled HI before the conversion and
>> +	      readout of the value from the ADC registers and
>> +	      pulled LO afterward.
>> +
>> +Example:

[...]

>> +static int hp03_read_raw(struct iio_dev *indio_dev,
>> +			 struct iio_chan_spec const *chan,
>> +			 int *val, int *val2, long mask)
>> +{
>> +	struct hp03_priv *priv = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	mutex_lock(&priv->lock);
>> +	ret = hp03_update_temp_pressure(priv);
>> +	mutex_unlock(&priv->lock);
>> +
>> +	if (ret)
>> +		return ret;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		switch (chan->type) {
>> +		case IIO_PRESSURE:
>> +			*val = priv->pressure / 100;
>> +			*val2 = (priv->pressure % 100) * 10000;
>> +			ret = IIO_VAL_INT_PLUS_MICRO;
> 
> maybe return directly, here and below

Good point. Looking at the code, I think I will also need to change the
code like the snippet below. The priv->pressure is in 1Pa steps, the
priv->temp is in 0.01C steps. Does it make sense or am I confused ?


        switch (mask) {
        case IIO_CHAN_INFO_RAW:
                switch (chan->type) {
                case IIO_PRESSURE:
                        *val = priv->pressure;
                        return IIO_VAL_INT;
                case IIO_TEMP:
                        *val = priv->temp;
                        return IIO_VAL_INT;
                default:
                        return -EINVAL;
                }
                break;
        case IIO_CHAN_INFO_SCALE:
                switch (chan->type) {
                case IIO_PRESSURE:
                        *val = 1;
                        return IIO_VAL_INT;
                case IIO_TEMP:
                        *val = 0;
                        *val2 = 10000;
                        return IIO_VAL_INT_PLUS_MICRO;
                default:
                        return -EINVAL;
                }
                break;
        default:
                return -EINVAL;
        }

>> +			break;
>> +		case IIO_TEMP:
>> +			*val = priv->temp / 100;
>> +			*val2 = (priv->temp % 100) * 10000;
>> +			ret = IIO_VAL_INT_PLUS_MICRO;
>> +			break;
>> +		default:
>> +			ret = -EINVAL;
>> +			break;
>> +		}
>> +		break;
>> +	case IIO_CHAN_INFO_SCALE:
>> +		switch (chan->type) {
>> +		case IIO_PRESSURE:
>> +		case IIO_TEMP:
>> +			*val = 0;
>> +			*val2 = 10000;
>> +			ret = IIO_VAL_INT_PLUS_MICRO;
>> +			break;
>> +		default:
>> +			ret = -EINVAL;
>> +			break;
>> +		}
>> +		break;
>> +
>> +	default:
>> +		ret = -EINVAL;
>> +		break;
>> +	}
>> +
>> +	return ret;
>> +}

[...]

Best regards,
Marek Vasut