linux-kernel - Re: [PATCH v3 2/3] iio: adc: ad4695: Add driver for AD4695 and similar ADCs

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: <d2649e69-4c71-4aa9-88e2-9d3f15549e1b@baylibre.com>
Date: Wed, 26 Jun 2024 09:42:49 -0500
From: David Lechner <dlechner@...libre.com>
To: Nuno Sá <noname.nuno@...il.com>,
 Jonathan Cameron <jic23@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>
Cc: Michael Hennerich <michael.hennerich@...log.com>,
 Nuno Sá <nuno.sa@...log.com>,
 Jonathan Corbet <corbet@....net>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-doc@...r.kernel.org, Ramona Gradinariu <ramona.gradinariu@...log.com>
Subject: Re: [PATCH v3 2/3] iio: adc: ad4695: Add driver for AD4695 and
 similar ADCs

On 6/26/24 6:47 AM, Nuno Sá wrote:
> Hi David,
> 
> minor stuff from me..
> 
> 
> ...
> 
>> +
>> +static int ad4695_write_chn_cfg(struct ad4695_state *st,
>> +				struct ad4695_channel_config *cfg)
>> +{
>> +	u32 mask = 0, val = 0;
>> +
>> +	mask |= AD4695_REG_CONFIG_IN_MODE;
>> +	val |= FIELD_PREP(AD4695_REG_CONFIG_IN_MODE, cfg->bipolar ? 1 : 0);
>> +
> 
> nit: don't need to OR the first assignments and so initializing the variables.

:+1:

> 
>> +	mask |= AD4695_REG_CONFIG_IN_PAIR;
>> +	val |= FIELD_PREP(AD4695_REG_CONFIG_IN_PAIR, cfg->pin_pairing);
>> +
>> +	mask |= AD4695_REG_CONFIG_IN_AINHIGHZ_EN;
>> +	val |= FIELD_PREP(AD4695_REG_CONFIG_IN_AINHIGHZ_EN, cfg->highz_en ? 1
>> : 0);
>> +
>> +	return regmap_update_bits(st->regmap, AD4695_REG_CONFIG_IN(cfg-
>>> channel),
>> +				  mask, val);
>> +}
>> +
>> +/**
>> + * ad4695_read_one_sample - Read a single sample using single-cycle mode
>> + * @st: The AD4695 state
>> + * @address: The address of the channel to read
>> + *
>> + * Upon return, the sample will be stored in the raw_data field of @st.
>> + *
>> + * Context: can sleep, must be called with iio_device_claim_direct held
>> + * Return: 0 on success, a negative error code on failure
>> + */
>> +static int ad4695_read_one_sample(struct ad4695_state *st, unsigned int
>> address)
>> +{
>> +	struct spi_transfer xfer[2] = { };
>> +	int ret;
>> +
>> +	ret = ad4695_set_single_cycle_mode(st, address);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/*
>> +	 * Setting the first channel to the temperature channel isn't
>> supported
>> +	 * in single-cycle mode, so we have to do an extra xfer to read the
>> +	 * temperature.
>> +	 */
>> +	if (address == AD4695_CMD_TEMP_CHAN) {
>> +		/* We aren't reading, so we can make this a short xfer. */
>> +		st->cnv_cmd2 = AD4695_CMD_TEMP_CHAN << 3;
>> +		xfer[0].bits_per_word = 8;
> 
> nit: isn't this the default?

yes (looks like leftover from testing when I was trying 16 instead of 8)

> 
>> +		xfer[0].tx_buf = &st->cnv_cmd2;
>> +		xfer[0].len = 1;
>> +		xfer[0].cs_change = 1;
>> +		xfer[0].cs_change_delay.value = AD4695_T_CONVERT_NS;
>> +		xfer[0].cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
>> +
>> +		/* Then read the result and exit conversion mode. */
>> +		st->cnv_cmd = AD4695_CMD_EXIT_CNV_MODE << 11;
>> +		xfer[1].bits_per_word = 16;
>> +		xfer[1].tx_buf = &st->cnv_cmd;
>> +		xfer[1].rx_buf = &st->raw_data;
>> +		xfer[1].len = 2;
>> +
>> +		return spi_sync_transfer(st->spi, xfer, 2);
>> +	}
>> +

...

>> +
>> +static int ad4695_parse_channel_cfg(struct iio_dev *indio_dev)
>> +{
>> +	struct device *dev = indio_dev->dev.parent;
>> +	struct ad4695_state *st = iio_priv(indio_dev);
> 
> Why not passing in struct ad4695_state directly?

Probably because that is how it was done in the ADI tree driver
I started with. Changing it to two parameters would be fine.

> 
> ...
> 
>>
>> +
>> +	/* Needed for debugfs since it only access registers 1 byte at a
>> time. */
>> +	ret = regmap_set_bits(st->regmap, AD4695_REG_SPI_CONFIG_C,
>> +			      AD4695_REG_SPI_CONFIG_C_MB_STRICT);
>> +	if (ret)
>> +		return ret;
>> +
> 
> Question... do we gain something but not doing the above? Because debugfs is
> optional and always doing it even when it's not present looks unnecessary. 

I haven't got to a place where we need to read or write a 2 byte register
yet, so I'm not sure. My plan is to defer worrying about it until then
and update this if necessary in a future patch when it actually makes a
difference. But for now, this is harmless because we are only reading
and writing single byte registers.

> 
> - Nuno Sá
>