Re: [PATCH v4 1/2] iio: dac: Add AD5758 support

[Popa, Stefan Serban]

On Du, 2018-07-01 at 00:26 +0300, Andy Shevchenko wrote:
> On Fri, Jun 29, 2018 at 11:38 AM, Stefan Popa <stefan.popa@xxxxxxxxxx> wrote:
> > 
> > The AD5758 is a single channel DAC with 16-bit precision which uses the
> > SPI interface that operates at clock rates up to 50MHz.
> > 
> > The output can be configured as voltage or current and is available on a
> > single terminal.
> > 
> > Datasheet:
> > http://www.analog.com/media/en/technical-documentation/data-sheets/ad5758.pdf
> Thanks for an update.
> Few comments below.
> 
> > 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> Perhaps keep them ordered?
> 
> > 
> > +
> > +#include 
> > +#include 
> > +
> > 
> > +#include 
> ASM? Hmm...
> 
> > 
> > +static int cmpfunc(const void *a, const void *b)
> > +{
> > +ÂÂÂÂÂÂÂreturn (*(int *)a - *(int *)b);
> Surrounding parens are not needed.
> 
> > 
> > +}
> > +
> > 
> > +static int ad5758_find_closest_match(const int *array,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂunsigned int size, int val)
> > +{
> > +ÂÂÂÂÂÂÂint i;
> > +
> > +ÂÂÂÂÂÂÂfor (i = 0; i < size; i++) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (val <= array[i])
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn i;
> > +ÂÂÂÂÂÂÂ}
> > +
> > +ÂÂÂÂÂÂÂreturn size - 1;
> > +}
> Isn't it what bsearch() covers as well?
> 

bsearch() looks for an item in an array and returns NULL if it is not found.
This function returns the index of the closest value to val even if there isÂ
no exact match.

> > 
> > +ÂÂÂÂÂÂÂdo {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂret = ad5758_spi_reg_read(st, reg);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (ret < 0)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn ret;
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (!(ret & mask))
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn 0;
> > +
> > 
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂudelay(100);
> If it's not called from atomic context, perhaps switch to usleep_range() ?
> 
> > 
> > +ÂÂÂÂÂÂÂ} while (--timeout);
> 
> > 
> > +static int ad5758_soft_reset(struct ad5758_state *st)
> > +{
> > +ÂÂÂÂÂÂÂint ret;
> > +
> > +ÂÂÂÂÂÂÂret = ad5758_spi_reg_write(st, AD5758_KEY, AD5758_KEY_CODE_RESET_1);
> > +ÂÂÂÂÂÂÂif (ret < 0)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn ret;
> > +
> > +ÂÂÂÂÂÂÂret = ad5758_spi_reg_write(st, AD5758_KEY, AD5758_KEY_CODE_RESET_2);
> > +
> > 
> > +ÂÂÂÂÂÂÂ/* Perform a software reset and wait 100us */
> > +ÂÂÂÂÂÂÂudelay(100);
> Ditto.
> 
> > 
> > +
> > +ÂÂÂÂÂÂÂreturn ret;
> > +}
> > 
> > +static int ad5758_find_out_range(struct ad5758_state *st,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂconst struct ad5758_range *range,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂunsigned int size,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂint min, int max)
> > +{
> > +ÂÂÂÂÂÂÂint i;
> > +
> > +ÂÂÂÂÂÂÂfor (i = 0; i < size; i++) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif ((min == range[i].min) && (max == range[i].max)) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂst->out_range.reg = range[i].reg;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂst->out_range.min = range[i].min;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂst->out_range.max = range[i].max;
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn 0;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ}
> > +ÂÂÂÂÂÂÂ}
> One more candidate to use bsearch().
> 

I am not sure if bsearch() will work in this case since it requires that the givenÂ
array is sorted. Moreover, bothÂad5758_voltage_range[] andÂad5758_current_range[]Â
contain duplicate elements.

> > 
> > +
> > +ÂÂÂÂÂÂÂreturn -EINVAL;
> > +}
> > 
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂindex = (int *) bsearch(&tmp, ad5758_dc_dc_ilim,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂARRAY_SIZE(ad5758_dc_dc_ilim),
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂsizeof(int), cmpfunc);
> I'm not sure you need that casting.
>