On September 14, 2014 18:26, Jonathan Cameron wrote:
> >>>> +}
> >>>> +
> >>>> +static inline int da9150_gpadc_vbus_21v_voltage_now(int raw_val)
> >>>> +{
> >>>> + /* Convert to uV */
> >>>> + return (((21 * ((raw_val * 1000) + 500)) / 1024) * 1000);
> >>>> +}
> >>>> +
> >>>> +static inline int da9150_gpadc_vsys_6v_voltage_now(int raw_val)
> >>>> +{
> >>>> + /* Convert to uV */
> >>>> + return (((3 * ((raw_val * 1000) + 500)) / 512) * 1000);
> >>>> +}
> >>>> +
> >>>> +static inline int da9150_gpadc_tjunc_temp(int raw_val)
> >>>> +{
> >>>> + /* Convert to 0.1 degrees C */
> >>>
> >>> IIO wants mille degrees C
> >> Exactly. Please check all of these correspond to the base units required
> >> by the documentation. Most of these might be better handled as raw
> >> with scale and offset provided, letting the maths be doing in userspace using
> >> floating point.
> >
> > Some are needed by the charger driver. The intention of adding the conversions
> > here was so this knowledge was not needed in the charger driver. Seems wrong to
> > move the calculations out of the IIO driver. Most of these are not simple linear
> > so scale and offset will not work, unless I'm missing something.
> Umm. They all appear to be simple linear conversions to me - be it written a form
> that slightly obscures that fact. But I'm not that fussed if you really want to
> do them in kernel.
I looked at the voltage calculations and how I could use the scale & offset
features already provided. The problem is that the offset value is a fraction
if added to the raw value first before scale (as would be done in the inkern.c
code), so I could not see a way to get an accurate enough result without doing
the calculation here. I will however remove the function for tjunc_temp as this
can be done using scale and offset, so will update accordingly.
> >>>> +{
> >>>> + struct da9150_gpadc *gpadc = iio_priv(indio_dev);
> >>>> + int ret;
> >>>> +
> >>>> + if ((chan->channel < DA9150_GPADC_CHAN_GPIOA) ||
> >>>> + (chan->channel > DA9150_GPADC_CHAN_TJUNC_OVP))
> >>>> + return -EINVAL;
> >>>> +
> >>>> + switch (mask) {
> >>>> + case IIO_CHAN_INFO_RAW:
> >>>> + case IIO_CHAN_INFO_PROCESSED:
> >> Would be cleaner to have each of these functions use ret for return code
> >> including IIO_VAL_INT and pass val as an arguement. Would get rid
> >> of the fiddly handling at the end of this function and allow direct
> >> returns from each of the case statements.
> >>
> >> Also, are the base units of all of these channels really giving us
> >> an integer result? E.g. mV, mA etc? Not impossible, but seems unlikely!
> >
> > Ok, I can make that change. Wouldn't say what was there now is fiddly, but am
> > happy to update.
> >
> > In terms of base units, they are actually in V or A, rather than say mV or mA,
> > according to the datasheet, but as there's no floating point support in the
> > kernel the sensible option was to provide them in smaller units and provide
> > greater accuracy.
> No it really isn't. There is no floating point in kernel, so we simulate the
> effect using the IIO_VAL types and a pair of 32bit integers. It's not ideal
> but it does allow us to cover any plausible range. The reason other drivers need to
> match it is that we need a clean ABI to userspace. Hence they must all be the same.
> This is why we try to move the conversions into userspace where possible. It can
> be done in kernel but is admittedly rather fiddly!
Of course. Will make sure the correct types are provided to keep in-line with
this. Thanks.
��.n��������+%������w��{.n�����{��(��)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
