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]
Message-ID: <20241020125437.72c1de38@jic23-huawei>
Date: Sun, 20 Oct 2024 12:54:37 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Ramona Alexandra Nechita <ramona.nechita@...log.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Cosmin Tanislav
 <cosmin.tanislav@...log.com>, Michael Hennerich
 <Michael.Hennerich@...log.com>, Alexandru Ardelean
 <alexandru.ardelean@...log.com>, Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Nuno Sa
 <nuno.sa@...log.com>, David Lechner <dlechner@...libre.com>, George Mois
 <george.mois@...log.com>, Ana-Maria Cusco <ana-maria.cusco@...log.com>,
 <linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <devicetree@...r.kernel.org>
Subject: Re: [PATCH v7 3/3] drivers: iio: adc: add support for ad777x family

On Mon, 14 Oct 2024 17:32:00 +0300
Ramona Alexandra Nechita <ramona.nechita@...log.com> wrote:

> Add support for AD7770, AD7771, AD7779 ADCs. The device is capable of
> sending out data both on DOUT lines interface,as on the SDO line.
> The driver currently implements only the SDO data streaming mode. SPI
> communication is used alternatively for accessing registers and streaming
> data. Register accesses are protected by crc8.
> 
> Signed-off-by: Ramona Alexandra Nechita <ramona.nechita@...log.com>

A few comments inline.  I also tweaked white space in a few places
whilst applying.  Target in IIO is still sub 80 chars whenever it
doesn't hurt readability.

Also, you had unusual formatting for some of the macros. Avoid mixing
tabs then spaces for indentation of the \ 

series applied to the togreg branch of iio.git and initially pushed
out as testing so 0-day can take a look.

Thanks,

Jonathan

> diff --git a/drivers/iio/adc/ad7779.c b/drivers/iio/adc/ad7779.c
> new file mode 100644
> index 000000000000..5904cc669f3a
> --- /dev/null
> +++ b/drivers/iio/adc/ad7779.c
> @@ -0,0 +1,909 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * AD7770, AD7771, AD7779 ADC
> + *
> + * Copyright 2023-2024 Analog Devices Inc.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitmap.h>
> +#include <linux/clk.h>
> +#include <linux/crc8.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/math.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/units.h>
> +
> +#include <asm/unaligned.h>

This moved, I'll fix up.

> +	{								       \
> +		.type = IIO_VOLTAGE,					       \
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE)  |	       \
> +				      BIT(IIO_CHAN_INFO_CALIBBIAS),	       \
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),       \

It's not technically an ABI issue, but ususual to have an ADC driver that provides
no direct readings via sysfs and also provides no indication of channel scaling.

A quick glance at the datasheet suggests there is a PGA in the path, so perhaps
you plan to add scaling later.  Raw reads of single channels would I think
have to just select from the bulk channel read back so not that trivial to implement.
Maybe worth doing longer term though as that is a useful debug interface if nothing
else.

Anyhow, I'm fine with taking the driver with the current feature set
I was just a bit surprised at which features were implemented.

> +		.address = (index),					       \
> +		.indexed = 1,						       \
> +		.channel = (index),					       \
> +		.scan_index = (index),					       \
> +		.ext_info = (_ext_info),				       \
> +		.scan_type = {						       \
> +			.sign = 's',					       \
> +			.realbits = 24,					       \
> +			.storagebits = 32,				       \
> +			.endianness = IIO_BE,				   \
> +		},								\
> +	}
> +
> +#define AD777x_CHAN_NO_FILTER_S(index)					       \
> +	AD777x_CHAN_S(index, NULL)
> +
> +#define AD777x_CHAN_FILTER_S(index)					       \
> +	AD777x_CHAN_S(index, ad7779_ext_filter)
> +static const struct iio_chan_spec ad7779_channels[] = {
> +	AD777x_CHAN_NO_FILTER_S(0),
> +	AD777x_CHAN_NO_FILTER_S(1),
> +	AD777x_CHAN_NO_FILTER_S(2),
> +	AD777x_CHAN_NO_FILTER_S(3),
> +	AD777x_CHAN_NO_FILTER_S(4),
> +	AD777x_CHAN_NO_FILTER_S(5),
> +	AD777x_CHAN_NO_FILTER_S(6),
> +	AD777x_CHAN_NO_FILTER_S(7),
> +	IIO_CHAN_SOFT_TIMESTAMP(8),
> +};
> +
> +static const struct iio_chan_spec ad7779_channels_filter[] = {
> +	AD777x_CHAN_FILTER_S(0),
> +	AD777x_CHAN_FILTER_S(1),
> +	AD777x_CHAN_FILTER_S(2),
> +	AD777x_CHAN_FILTER_S(3),
> +	AD777x_CHAN_FILTER_S(4),
> +	AD777x_CHAN_FILTER_S(5),
> +	AD777x_CHAN_FILTER_S(6),
> +	AD777x_CHAN_FILTER_S(7),
> +	IIO_CHAN_SOFT_TIMESTAMP(8),
> +};

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ