linux-kernel - Re: [PATCH v8 2/2] iio: adc: max14001: New 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]

Message-ID: <20230702180315.00003dbe@Huawei.com>
Date:   Sun, 2 Jul 2023 18:04:09 +0800
From:   Jonathan Cameron <Jonathan.Cameron@...wei.com>
To:     Kim Seer Paller <kimseer.paller@...log.com>
CC:     <jic23@...nel.org>, <lars@...afoo.de>, <lgirdwood@...il.com>,
        <broonie@...nel.org>, <Michael.Hennerich@...log.com>,
        <andy.shevchenko@...il.com>, <robh@...nel.org>,
        <krzysztof.kozlowski@...aro.org>, <conor+dt@...nel.org>,
        <linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <devicetree@...r.kernel.org>, kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v8 2/2] iio: adc: max14001: New driver

On Thu, 22 Jun 2023 22:32:27 +0800
Kim Seer Paller <kimseer.paller@...log.com> wrote:

> The MAX14001 is configurable, isolated 10-bit ADCs for multi-range
> binary inputs.
> 
> Signed-off-by: Kim Seer Paller <kimseer.paller@...log.com>
> Reported-by: kernel test robot <lkp@...el.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202306211545.7b6CdqsL-lkp@intel.com/

Hi, 

Two outstanding comments that I think I raised in earlier reviews..

Jonathan

> diff --git a/drivers/iio/adc/max14001.c b/drivers/iio/adc/max14001.c
> new file mode 100644
> index 000000000000..a21ebcde71fa
> --- /dev/null
> +++ b/drivers/iio/adc/max14001.c

...

> +static int max14001_read(void *context, unsigned int reg_addr, unsigned int *data)
> +{
> +	struct max14001_state *st = context;
> +	int ret;
> +
> +	struct spi_transfer xfers[] = {
> +		{
> +			.tx_buf = &st->spi_tx_buffer,
> +			.len = sizeof(st->spi_tx_buffer),
> +			.cs_change = 1,
> +		}, {
> +			.rx_buf = &st->spi_rx_buffer,
> +			.len = sizeof(st->spi_rx_buffer),
> +		},
> +	};
> +
> +	/*
> +	 * Convert transmit buffer to big-endian format and reverse transmit
> +	 * buffer to align with the LSB-first input on SDI port.
> +	 */
> +	st->spi_tx_buffer = cpu_to_be16(bitrev16(FIELD_PREP(MAX14001_ADDR_MASK,
> +								reg_addr)));
> +
> +	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Align received data from the receive buffer, reversing and reordering
> +	 * it to match the expected MSB-first format.
> +	 */
> +	*data = (__force u16)(be16_to_cpu(bitrev16(st->spi_rx_buffer))) &
> +							MAX14001_DATA_MASK;
> +
These sequences still confuse me a lot because I'd expect the values in tx
to have the opposite operations applied to those for rx and that's not the
case.

Let's take a le system. 
tx = cpu_to_be16(bitrev16(x))
   = cpu_to_be16((__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8));
   = __bitrev8(x & 0xff) | (__bitrev8(x >> 8) << 8)
or swap all the bits in each byte, but don't swap the bytes.

rx = cpu_to_be16(bitrev16(x))
   = be16_to_cpu(((__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8)_
   = __bitrev8(x & 0xff) | __bitrev(x >> 8)
   
also swap all the bits in each byte, but don't swap the bytes.

So it is the reverse because the bytes swaps unwind themselves somewhat.
For a be system cpu_to_be16 etc are noop.
tx = (__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8)
rx = (__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8)

So in this case swap all 16 bits.

Now, given I'd expected them to be reversed for the tx vs rx case.
E.g.
tx = cpu_to_be16(bitrev16(x))
As above.
For rx, le host
rx = bitrev16(be16_to_cpu(x))
   = __bitrev8((x >> 8) & 0xff) << 8) |  __bitrev8((((x & 0xff) << 8) >> 8)
same as above (if you swap the two terms I think.

For be the be16_to_cpu is a noop again, so it's just bitrev16(x) as expected.

Hence if I've understood this correctly you could reverse the terms so that
it was 'obvious' you were doing the opposite for the tx term vs the rx one
without making the slightest bit of difference....
   
hmm. Might be worth doing simply to avoid questions.


> +	return 0;
> +}
> +static int max14001_reg_update(struct max14001_state *st,
> +				unsigned int reg_addr,
> +				unsigned int mask,
> +				unsigned int val)
> +{
> +	int ret;
> +	unsigned int reg_data;
> +
> +	/* Enable SPI Registers Write */
> +	ret = max14001_write(st, MAX14001_WEN, MAX14001_WRITE_WEN);
> +	if (ret)
> +		return ret;
> +
> +	ret = max14001_read(st, reg_addr, &reg_data);
> +	if (ret)
> +		return ret;
> +
> +	reg_data |= FIELD_PREP(mask, val);

This is still a problem if the compiler happens to fail to figure out
that mask is a compile time constant.  Given it only ever takes one value
I'd suggest either calling the FIELD_PREP at the caller, or just 
pushing all this code inline so that you can put the definition
inline.

> +
> +	ret = max14001_write(st, reg_addr, reg_data);
> +	if (ret)
> +		return ret;
> +
> +	/* Write Verification Register */
> +	ret = max14001_write_verification_reg(st, reg_addr);
> +	if (ret)
> +		return ret;
> +
> +	/* Disable SPI Registers Write */
> +	return max14001_write(st, MAX14001_WEN, 0);
> +}