| 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
| ||
|
Message-ID: <fcf6daf6afec48b096df14d8b5308db0@analog.com>
Date: Tue, 4 Jul 2023 09:40:56 +0000
From: "Paller, Kim Seer" <KimSeer.Paller@...log.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
CC: "jic23@...nel.org" <jic23@...nel.org>,
"lars@...afoo.de" <lars@...afoo.de>,
"lgirdwood@...il.com" <lgirdwood@...il.com>,
"broonie@...nel.org" <broonie@...nel.org>,
"Hennerich, Michael" <Michael.Hennerich@...log.com>,
"andy.shevchenko@...il.com" <andy.shevchenko@...il.com>,
"robh@...nel.org" <robh@...nel.org>,
"krzysztof.kozlowski@...aro.org" <krzysztof.kozlowski@...aro.org>,
"conor+dt@...nel.org" <conor+dt@...nel.org>,
"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
kernel test robot <lkp@...el.com>
Subject: RE: [PATCH v8 2/2] iio: adc: max14001: New driver
> -----Original Message-----
> From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> Sent: Sunday, July 2, 2023 6:04 PM
> To: Paller, Kim Seer <KimSeer.Paller@...log.com>
> Cc: jic23@...nel.org; lars@...afoo.de; lgirdwood@...il.com;
> broonie@...nel.org; Hennerich, Michael <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
>
> [External]
>
> 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://urldefense.com/v3/__https://lore.kernel.org/oe-kbuild-all/2023
> > 06211545.7b6CdqsL-
> lkp@...el.com/__;!!A3Ni8CS0y2Y!4npD8X6TpKmeLcUf8QqQW
> >
> yEFp_Z1ORKb2dZNpuqfj0ZK74NiCYKQLNWEfKzVmuKTHJO0RW8n01vdXURqBvc
> ueb3V1Sb
> > GQdI$
>
> 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.
Thank you for your feedback. I have tested the modifications based on your
suggestions, taking the le system into account, and it appears that the code is
functioning correctly. Before sending the new patch version, I would like to
confirm if this aligns with your comments.
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 = 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;
*data = cpu_to_be16(bitrev16(st->spi_rx_buffer));
return 0;
}
static int max14001_write(void *context, unsigned int reg_addr, unsigned int data)
{
struct max14001_state *st = context;
st->spi_tx_buffer = cpu_to_be16(bitrev16(
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));
}
> > + 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, ®_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.
I would like to confirm including the 'static inline' keyword for the
max14001_reg_update function.
> > +
> > + 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);
> > +}
>
Best Regards,
Kim Seer Paller
Powered by blists - more mailing lists