Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 10 Aug 2023 18:36:36 +0300
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     Nuno =?iso-8859-1?Q?S=E1?= <noname.nuno@gmail.com>
Cc:     Dumitru Ceclan <mitrutzceclan@gmail.com>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Michael Hennerich <Michael.Hennerich@analog.com>,
        Jonathan Cameron <jic23@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Arnd Bergmann <arnd@arndb.de>,
        Cosmin Tanislav <demonsingur@gmail.com>,
        Alexander Sverdlin <alexander.sverdlin@gmail.com>,
        Hugo Villeneuve <hvilleneuve@dimonoff.com>,
        Okan Sahin <okan.sahin@analog.com>,
        Niklas Schnelle <schnelle@linux.ibm.com>,
        ChiYuan Huang <cy_huang@richtek.com>,
        Ramona Bolboaca <ramona.bolboaca@analog.com>,
        Ibrahim Tilki <Ibrahim.Tilki@analog.com>,
        ChiaEn Wu <chiaen_wu@richtek.com>,
        William Breathitt Gray <william.gray@linaro.org>,
        Lee Jones <lee@kernel.org>, Haibo Chen <haibo.chen@nxp.com>,
        Mike Looijmans <mike.looijmans@topic.nl>,
        Leonard =?iso-8859-1?Q?G=F6hrs?= <l.goehrs@pengutronix.de>,
        Ceclan Dumitru <dumitru.ceclan@analog.com>,
        linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] iio: adc: ad717x: add AD717X driver
Message-ID: <ZNUEBDsMg6UfeOtl@smile.fi.intel.com>
References: <20230810093322.593259-1-mitrutzceclan@gmail.com>
 <20230810093322.593259-2-mitrutzceclan@gmail.com>
 <34f5e2118a4714048231e6ee9a8f244248616bd0.camel@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <34f5e2118a4714048231e6ee9a8f244248616bd0.camel@gmail.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

On Thu, Aug 10, 2023 at 01:57:02PM +0200, Nuno S? wrote:
> On Thu, 2023-08-10 at 12:33 +0300, Dumitru Ceclan wrote:

...

> Is ad717x_gpio_cleanup() being used anywhere? Moreover I would maybe just get rid of
> the #ifdef wrapper and just select GPIOLIB. How often will it be disabled anyways?

The agreement is that users are depend on and not selecting GPIOLIB.
Any news in these agreement terms?

...

> > +???????id &= AD717X_ID_MASK;
> > +???????if (id != st->info->id)
> > +???????????????dev_warn(&st->sd.spi->dev, "Unexpected device id: %x, expected:
> > %x\n",
> > +?????????????????????????????????????????? id, st->info->id);
> > +
> 
> Shouldn't we error out?

It seems a new way of thinking about unsupported CHIP ID. Dunno if hw vendors
won't ever do a dirty trick that new ID must be programmed differently and
otherwise burn hardware to a smoke...

I'm with you here, unknown chips mustn't be supported.

...

> > +???????????????????????????????*val = -(1 << (chan->scan_type.realbits - 1));
> 
> nit: I don't expect the driver to really be updated with more devices (it's
> like this for a long time) but the above is not very extensible... Imagine we
> add a device with 32bit channels? We would enter shady waters If I'm not
> missing anything.

Also 1 << 31 is UB in accordance with C standard.

...

> > +???????st->info = device_get_match_data(&spi->dev);
> > +???????if (!st->info)
> > +???????????????return -ENODEV;
> > +
> 
> spi_get_device_match_data() (not really sure if this is still applicable
> since some work related to this is being done for i2c - and eventually in spi
> I guess).

Still applicable.

-- 
With Best Regards,
Andy Shevchenko