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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Wed, 4 Jul 2018 13:26:03 +0000
From:   "Popa, Stefan Serban" <StefanSerban.Popa@...log.com>
To:     "andy.shevchenko@...il.com" <andy.shevchenko@...il.com>
CC:     "vilhelm.gray@...il.com" <vilhelm.gray@...il.com>,
        "lars@...afoo.de" <lars@...afoo.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "knaack.h@....de" <knaack.h@....de>,
        "rdunlap@...radead.org" <rdunlap@...radead.org>,
        "jic23@...nel.org" <jic23@...nel.org>,
        "Hennerich, Michael" <Michael.Hennerich@...log.com>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "mchehab@...nel.org" <mchehab@...nel.org>,
        "Ismail.Kose@...imintegrated.com" <Ismail.Kose@...imintegrated.com>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "lukas@...ner.de" <lukas@...ner.de>,
        "pmeerw@...erw.net" <pmeerw@...erw.net>,
        "sean.nyekjaer@...vas.dk" <sean.nyekjaer@...vas.dk>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "pombredanne@...b.com" <pombredanne@...b.com>,
        "davem@...emloft.net" <davem@...emloft.net>
Subject: Re: [PATCH v4 1/2] iio: dac: Add AD5758 support

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@...log.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.
>