Re: [PATCH v3] iio: add m62332 DAC driver

[Jonathan Cameron <jic23@kernel.org>]

On 12/05/15 17:45, Dmitry Eremin-Solenikov wrote:
> Hello,
> 
> 2015-04-27 21:55 GMT+03:00 Peter Meerwald <pmeerw@pmeerw.net>:
>>
>>> 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@gmail.com>
>>> ---
>>> Changes since v2:
>>>  * Added M62332_CHANNELS to be used instead of magic value 2
>>>  * Changed the probe funciton handles error cases
>>>
> 
> [skipped]
> 
>>> +
>>> +     if (val)
>>> +             res = regulator_enable(data->vcc);
>>> +     if (res)
>>> +             goto out;
>>
>> I would find it marginally clearer to do
>> if (val) {
>>   res = regulator_enable(..);
>>   if (res)
>>     goto out;
>> }
>> and leave res uninitialized (feel free to ignore)
> 
> Ack
> 
>>
>>> +
>>> +     res = i2c_master_send(client, outbuf, 2);
>>> +     if (res >= 0 && res != 2)
> 
> [skipped]
> 
>>> +
>>> +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).

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.
> 
> [skipped]
> 
>