linux-kernel - Re: [PATCH v6 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] [day] [month] [year] [list]

Message-ID: <20230617194318.1a9b0e9c@jic23-huawei>
Date:   Sat, 17 Jun 2023 19:43:28 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Kim Seer Paller <kimseer.paller@...log.com>
Cc:     <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>
Subject: Re: [PATCH v6 2/2] iio: adc: max14001: New driver

On Wed, 14 Jun 2023 08:48:57 +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>

Build tests showed up a set of warnings we need to deal with.

Jonathan


> +
> +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),
> +		},
> +	};
> +
> +	st->spi_tx_buffer = bitrev16(cpu_to_be16(FIELD_PREP(MAX14001_ADDR_MASK,
> +								reg_addr)));

This gives an endian warning with sparse.

drivers/iio/adc/max14001.c:81:29: warning: incorrect type in initializer (different base types)
drivers/iio/adc/max14001.c:81:29:    expected unsigned short [usertype] __x
drivers/iio/adc/max14001.c:81:29:    got restricted __be16 [usertype]
drivers/iio/adc/max14001.c:81:27: warning: incorrect type in assignment (different base types)
drivers/iio/adc/max14001.c:81:27:    expected restricted __be16 [usertype] spi_tx_buffer
drivers/iio/adc/max14001.c:81:27:    got int
drivers/iio/adc/max14001.c:97:29: warning: incorrect type in initializer (different base types)
drivers/iio/adc/max14001.c:97:29:    expected unsigned short [usertype] __x
drivers/iio/adc/max14001.c:97:29:    got restricted __be16 [usertype]
drivers/iio/adc/max14001.c:97:27: warning: incorrect type in assignment (different base types)
drivers/iio/adc/max14001.c:97:27:    expected restricted __be16 [usertype] spi_tx_buffer
drivers/iio/adc/max14001.c:97:27:    got int

Using a forced cast and adding a comment is probably fine to fix this...  Bit reversing
a big endian value is not going to be common enough to warrant anything clever.

> +
> +	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> +	if (ret)
> +		return ret;
> +
> +	*data = bitrev16(be16_to_cpu(st->spi_rx_buffer)) & MAX14001_DATA_MASK;
But it got me looking.  Seems a bit odd that this isn't a direct reverse of what
is done on the tx bfufer.
e.g.
	be16_to_cpu(bitrev16(st->spi_rx_buffer))

I'm struggling to figure out enough of the datasheet to understand this.
Perhaps you could add some comments on the logic?


> +
> +	return 0;
> +}
> +
> +static int max14001_write(void *context, unsigned int reg_addr, unsigned int data)
> +{
> +	struct max14001_state *st = context;
> +
> +	st->spi_tx_buffer = bitrev16(cpu_to_be16(
> +			FIELD_PREP(MAX14001_ADDR_MASK, reg_addr) |
> +			FIELD_PREP(MAX14001_SET_WRITE_BIT, 1) |
> +			FIELD_PREP(MAX14001_DATA_MASK, data)));
> +
> +	return spi_write(st->spi, &st->spi_tx_buffer, sizeof(st->spi_tx_buffer));
> +}