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

["Popa, Stefan Serban" <StefanSerban.Popa@analog.com>]

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@analog.com> 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.
>