linux-kernel - Re: [PATCH v4 6/6] iio: adc: Add support for AD4000

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]

Message-ID: <20240620210827.2b46a718@jic23-huawei>
Date: Thu, 20 Jun 2024 21:08:27 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: David Lechner <dlechner@...libre.com>
Cc: Marcelo Schmitt <marcelo.schmitt@...log.com>, broonie@...nel.org,
 lars@...afoo.de, Michael.Hennerich@...log.com, robh+dt@...nel.org,
 krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org, nuno.sa@...log.com,
 marcelo.schmitt1@...il.com, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-spi@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 6/6] iio: adc: Add support for AD4000


> > +struct ad4000_chip_info {
> > +	const char *dev_name;
> > +	struct iio_chan_spec chan_spec;
> > +	struct iio_chan_spec three_w_chan_spec;
> > +};  
> 
> I understand the reason for doing this, but it still seems a bit weird
> to me to have two different sets of specs for the same chip. I guess
> we'll see what Jonathan has to say about this.


It's very common, though for a different reason.

Normally it's for cases where we have events (threshold crossing as similar)
and the interrupt is optional. In those case we have channel specs with
and without the event.  In this case the change is small so maybe
the code to set it up on a copy of the chan spec would be fine.

This is simple though so I'd keep it this way.

> 
> > +
> > +static const struct ad4000_chip_info ad4000_chip_info = {
> > +	.dev_name = "ad4000",
> > +	.chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16, 0),
> > +	.three_w_chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16, 1),
> > +};
> 
> or could just replace all of this this will spi_w8r8() and have
> a one-line function.
Good point. I'd forgotten that existed.  Better still than
spi_write_then_read.

Glad we spotted some of the same things. Sometimes it's weird
and two reviews are entirely unrelated issues throughout!

Jonathan