linux-kernel - Re: [PATCH 2/2] iio: adc: ad717x: add AD717X driver

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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 28 Aug 2023 17:52:06 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Nuno Sá <noname.nuno@...il.com>
Cc:     Dumitru Ceclan <mitrutzceclan@...il.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Michael Hennerich <Michael.Hennerich@...log.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Arnd Bergmann <arnd@...db.de>,
        Cosmin Tanislav <demonsingur@...il.com>,
        Alexander Sverdlin <alexander.sverdlin@...il.com>,
        Hugo Villeneuve <hvilleneuve@...onoff.com>,
        Okan Sahin <okan.sahin@...log.com>,
        Niklas Schnelle <schnelle@...ux.ibm.com>,
        ChiYuan Huang <cy_huang@...htek.com>,
        Ramona Bolboaca <ramona.bolboaca@...log.com>,
        Ibrahim Tilki <Ibrahim.Tilki@...log.com>,
        ChiaEn Wu <chiaen_wu@...htek.com>,
        William Breathitt Gray <william.gray@...aro.org>,
        Lee Jones <lee@...nel.org>, Haibo Chen <haibo.chen@....com>,
        Mike Looijmans <mike.looijmans@...ic.nl>,
        Leonard Göhrs <l.goehrs@...gutronix.de>,
        Ceclan Dumitru <dumitru.ceclan@...log.com>,
        linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] iio: adc: ad717x: add AD717X driver

On Thu, 10 Aug 2023 13:57:02 +0200
Nuno Sá <noname.nuno@...il.com> wrote:

> Hi Dumitru,
> 
> Thanks for taking care of this driver which is out of tree for a long time... Some
> comments below.
Hi.

A few follow ups...


> > +static int ad717x_setup(struct iio_dev *indio_dev)
> > +{
> > +       struct ad717x_state *st = iio_priv(indio_dev);
> > +       unsigned int id;
> > +       u8 *buf;
> > +       int ret;
> > +
> > +       /* reset the serial interface */
> > +       buf = kcalloc(8, sizeof(*buf), GFP_KERNEL);
> > +       if (!buf)
> > +               return -ENOMEM;
> > +
> > +       memset(buf, 0xff, 8);
> > +       ret = spi_write(st->sd.spi, buf, 8);
> > +       kfree(buf);  
> 
> Hmm, why allocating the buffer? I guess one could argue that we'll get DMA safe
> alignment but then maybe use some define instead of the magic 8.
> 

Just use spi_write_then_read() without an read buffer as then it will use
magic bounce buffers for you within the spi subsystem.



> > +static struct spi_driver ad717x_driver = {
> > +       .driver = {
> > +               .name   = "ad717x",  
> 
> I would keep the name as we have out of tree which is ad7173.c. I'm not sure if
> there's any policy in here but I think typically you just name your driver from the
> first supported device and then extend it from there. Since here you are just adding
> more than one device at once, it would be nice if you could keep the name of the
> driver Lars originally developed...

In this case, indeed the one originally developed is a good choice.
Otherwise, pick a supported part.

Wild cards have bitten us far too many times as manufacturers love to
'use up' gaps in their ID space with parts that are either very different
or even worse look the same but have totally different interfaces...


>