On 13 May 2015 20:31:36 BST, Dmitry Eremin-Solenikov <dbaryshkov@xxxxxxxxx> wrote:
>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.
Gah! Sorry, selective eye sight on my part. We clearly want to add at least the into version to that switch statement.
>
>> 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.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
--
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
