From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-iio-owner@vger.kernel.org>
Received: from saturn.retrosnub.co.uk ([178.18.118.26]:54661 "EHLO
	saturn.retrosnub.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1753773AbbELTRr (ORCPT
	<rfc822;linux-iio@vger.kernel.org>); Tue, 12 May 2015 15:17:47 -0400
Message-ID: <555251D9.7040800@kernel.org>
Date: Tue, 12 May 2015 20:17:45 +0100
From: Jonathan Cameron <jic23@kernel.org>
MIME-Version: 1.0
To: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>,
	Peter Meerwald <pmeerw@pmeerw.net>
CC: Hartmut Knaack <knaack.h@gmx.de>,
	Lars-Peter Clausen <lars@metafoo.de>, linux-iio@vger.kernel.org
Subject: Re: [PATCH v3] iio: add m62332 DAC driver
References: <1430156261-12742-1-git-send-email-dbaryshkov@gmail.com>	<alpine.DEB.2.02.1504272045300.16708@pmeerw.net> <CALT56yMRvg6qK9Z8hokO2hAE3MYixbW1JbLznPf+b1-30SFrRA@mail.gmail.com>
In-Reply-To: <CALT56yMRvg6qK9Z8hokO2hAE3MYixbW1JbLznPf+b1-30SFrRA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Sender: linux-iio-owner@vger.kernel.org
List-Id: linux-iio@vger.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]
> 
>