Hello,
2015-05-12 22:17 GMT+03:00 Jonathan Cameron <jic23@xxxxxxxxxx>:
> On 12/05/15 17:45, Dmitry Eremin-Solenikov wrote:
>> 2015-04-27 21:55 GMT+03:00 Peter Meerwald <pmeerw@xxxxxxxxxx>:
>>>
>>>> m62332 is a simple 2-channel DAC used on several Sharp Zaurus boards to
>>>> control LCD voltage, backlight and sound. The driver use regulators to
>>>> control the reference voltage and enabling/disabling the DAC.
>>>
>>> some minor comments below
>>
>> Thank you for the review. I had a followup question, see below.
>>
>> I'll have to submit a V4 anyway -- adding IIO_CHAN_INFO_OFFSET
>> to reflect correct values being generated by DAC.
>>
>>>
>>>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@xxxxxxxxx>
>>>> ---
>>>> Changes since v2:
>>>> * Added M62332_CHANNELS to be used instead of magic value 2
>>>> * Changed the probe funciton handles error cases
>>>>
>>
>>>> +
>>>> +static int m62332_write_raw(struct iio_dev *indio_dev,
>>>> + struct iio_chan_spec const *chan, int val, int val2, long mask)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + switch (mask) {
>>>> + case IIO_CHAN_INFO_RAW:
>>>
>>> see write_raw_get_fmt, the default is IIO_VAL_INT_PLUS_MICRO; probably you
>>> want to overwrite that function to return IIO_VAL_INT
>>>
>>> or at least check val2 == 0
>>
>> This is strange. First, according to iio_write_channel_info(),
>> write_raw_get_fmt()
>> should not return IIO_VAL_INT.
>
> Based on the lack of specific handling? (e.g. an entry in the case statement
> int that function).
Because of the function explicitly returning -EINVAL if
write_raw_get_fmt() callback
returns anything except IIO_VAL_INT_PLUS_MICRO or IIO_VAL_INT_PLUS_NANO.
> It will convert the same as IIO_VAL_INT_PLUS_MICRO but the micro bit will be zero.
>
> Or am I missing an indication that it doesn't support IIO_VAL_INT somewhere?
>
>
>> Also none of the DAC write_raw functions that
>> I have checked actually make use of val2 or check that it is 0.
> They should check it strictly speaking - feel free to post a series
> fixing them ;)
>
> Note it is just as valid to leave the type as IIO_VAL_INT_PLUS_MICRO and just check
> that val2==0 if you prefer.
I'm not so sure that it is a good idea. It means that m62332 will have
a behaviour that
differs from behaviour of (some of) the rest of IIO devices.
--
With best wishes
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
