On 12/05/15 17:45, Dmitry Eremin-Solenikov wrote:
> Hello,
>
> 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
>>>
>
> [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]
>
>
--
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
