Re: [PATCH] iio: Add support for LTC26xx I2C DAC

[Marc Andre <marc.andre@netline.ch>]

On 13.10.2015 09:09, Marc Andre wrote:
> On 12.10.2015 16:09, Lars-Peter Clausen wrote:
>> On 10/12/2015 02:53 PM, Marc Andre wrote:
>>> On 12.10.2015 12:26, Lars-Peter Clausen wrote:
>>>> On 10/12/2015 10:52 AM, Marc Andre wrote:
>>>>> This driver adds support for the Linear LTC26x6, LTC26x7 and 
>>>>> LTC26x9 I2C
>>>>> DAC chips.
>>>> Those look like they are very much register map compatible to what is
>>>> supported by the AD5064 driver.    It makes sense to add support 
>>>> for them
>>>> there
>>>> instead of having a separate driver.
>>> I see that the interfaces are similar. Not all features of the 
>>> ADxxxx are
>>> supported by the LTC26XX. e.g. it doesn't support the different 
>>> power down
>>> modes. It also doesn't support the configuration register.
>>> Other features available to AD which are not available to the LTC 
>>> are LDAC
>>> by software, RESET and CLEAR commands. Those commands are currently 
>>> not in
>>> use by the driver, but in the future, if those commands are used, a
>>> separation would have to be done to avoid issues with the LTC.
>>>
>>> I first also thought that the address / command location is 
>>> different as the
>>> AD5064 sends 4 bytes and "AD5064_ADDR(x)" is "((x) << 20)". By further
>>> reviewing I see that this only applies to SPI connected devices, 
>>> while I2C
>>> connected devices have 3 bytes as LTC26xx.
>>>
>>> I am quite open. I have the tendency to suggest a separate ltc26xx 
>>> driver,
>>> also because it would allow simpler identification of the driver and
>>> separate evolution. (Someone would need to know that AD4064 is 
>>> compatible to
>>> LTC26xx) I also think large drivers containing many separations between
>>> similar, but not equal devices are more risky to break on changes. 
>>> No one
>>> has all those devices available for test. But I am a new contributor 
>>> to the
>>> Kernel, thus I am interested to listen to the experts... :-)
>> It's a subset, but it is a clean subset. The LTC26xx don't have any 
>> extra
>> functionality not yet supported by the AD5064 driver.
>>
>> And I'm pretty convinced the chip was purposefully designed to be 
>> register
>> map compatible, would be a shame not to make use of that. Means we 
>> only have
>> to fix things in one driver if we find a bug, or e.g. if we want the 
>> feature
>> of being able to update all channels at the same time.
>>
> I started testing with the ad5064 driver, but the driver seams to be 
> quite broken for i2c. Before I fix all of it, I just want to confirm 
> that I didn't get it wrong:
> - ad5064_write_raw() expects the ad5064_write() to return 0 if 
> success. But ad5064_i2c_write() directly returns the return value of 
> i2c_master_send() which is the number of bytes sent if successful. I 
> need to add a wrap to convert the return value of i2c_master_send() to 
> 0 if the correct number of bytes is written.
> - #define AD5064_CHANNEL() sets the shift as follows: .shift = 20 - bits
> ad5064_write then applies this shift before sending it to the i2c/spi 
> command. This assumes that the number of data bits is 20 for all DAC 
> chips. As written yesterday this seams to be only true for the SPI 
> type chips. The I2C type chips use 2 bytes (16 bits).
>
> Do you know if the ad5064 driver has been used for any i2c type device 
> yet? Was it tested?
>
> If I want to implement LTC2617 (and fix the other i2c devices), I will 
> have to do a few patches and also make some changes to structures 
> (i.e. adjust the shift bits based on sensor type). I am ok to do that, 
> but just want to confirm first that this is your intention.
>
> Marc
>
After further thoughts I wonder if would make sense to split of AD5629 
and AD5629 from the AD5064 driver and keep AD5064 driver for SPI based 
devices with 20 data bits and make a new driver for I2C based devices 
with 16 data bits.
I could submit a new driver which combines AD5629 and LTC2617 (and 
similar devices).