> -----Original Message----- > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> > Sent: Sunday, July 2, 2023 6:04 PM > To: Paller, Kim Seer <KimSeer.Paller@xxxxxxxxxx> > Cc: jic23@xxxxxxxxxx; lars@xxxxxxxxxx; lgirdwood@xxxxxxxxx; > broonie@xxxxxxxxxx; Hennerich, Michael <Michael.Hennerich@xxxxxxxxxx>; > andy.shevchenko@xxxxxxxxx; robh@xxxxxxxxxx; > krzysztof.kozlowski@xxxxxxxxxx; conor+dt@xxxxxxxxxx; linux- > iio@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; > kernel test robot <lkp@xxxxxxxxx> > 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@xxxxxxxxxx> wrote: > > > The MAX14001 is configurable, isolated 10-bit ADCs for multi-range > > binary inputs. > > > > Signed-off-by: Kim Seer Paller <kimseer.paller@xxxxxxxxxx> > > Reported-by: kernel test robot <lkp@xxxxxxxxx> > > Closes: > > https://urldefense.com/v3/__https://lore.kernel.org/oe-kbuild-all/2023 > > 06211545.7b6CdqsL- > lkp@xxxxxxxxx/__;!!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
