On 02/06, Jonathan Cameron wrote:
> On Tue, 14 Dec 2021 14:12:24 -0300
> Marcelo Schmitt <marcelo.schmitt1@xxxxxxxxx> wrote:
>
> > Minor bit inline.
> >
> > On 12/05, Jonathan Cameron wrote:
> > > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > >
> > > The write and two types of read transfer are sufficiently complex that
> > > they benefit from the clarity of using FIELD_PREP() and FIELD_GET()
> > >
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > Reviewed-by: Marcelo Schmitt <marcelo.schmitt1@xxxxxxxxx>
>
> Those hunks you highlighted in patch 2 now in this patch and I've added a note
> to the description to say a similar arguement applies to the event_handler()
>
> Reply to your comment inline. I didn't understand this one.
> >
> > > ---
> > > drivers/staging/iio/adc/ad7280a.c | 46 ++++++++++++++++++++++++-------
> > > 1 file changed, 36 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c
> > > index 1f7ea5fb1e20..158a792c0bf8 100644
> > > --- a/drivers/staging/iio/adc/ad7280a.c
> > > +++ b/drivers/staging/iio/adc/ad7280a.c
> > > @@ -95,6 +95,23 @@
> > > #define AD7280A_READ_ADDR_MSK GENMASK(7, 2)
> > > #define AD7280A_CNVST_CTRL_REG 0x1D /* D7 to D0, Read/write */
> > >
> > > +/* Transfer fields */
> > > +#define AD7280A_TRANS_WRITE_DEVADDR_MSK GENMASK(31, 27)
> > > +#define AD7280A_TRANS_WRITE_ADDR_MSK GENMASK(26, 21)
> > > +#define AD7280A_TRANS_WRITE_VAL_MSK GENMASK(20, 13)
> > > +#define AD7280A_TRANS_WRITE_ALL_MSK BIT(12)
> > > +#define AD7280A_TRANS_WRITE_CRC_MSK GENMASK(10, 3)
> > > +#define AD7280A_TRANS_WRITE_RES_PATTERN 0x2
> > > +
> > > +/* Layouts differ for channel vs other registers */
> > > +#define AD7280A_TRANS_READ_DEVADDR_MSK GENMASK(31, 27)
> > > +#define AD7280A_TRANS_READ_CONV_CHANADDR_MSK GENMASK(26, 23)
> > > +#define AD7280A_TRANS_READ_CONV_DATA_MSK GENMASK(22, 11)
> > > +#define AD7280A_TRANS_READ_REG_REGADDR_MSK GENMASK(26, 21)
> > > +#define AD7280A_TRANS_READ_REG_DATA_MSK GENMASK(20, 13)
> > > +#define AD7280A_TRANS_READ_WRITE_ACK_MSK BIT(10)
> > > +#define AD7280A_TRANS_READ_CRC_MSK GENMASK(9, 2)
> > > +
> > > /* Magic value used to indicate this special case */
> > > #define AD7280A_ALL_CELLS (0xAD << 16)
> > >
> > > @@ -216,10 +233,16 @@ static int __ad7280_read32(struct ad7280_state *st, unsigned int *val)
> > > static int ad7280_write(struct ad7280_state *st, unsigned int devaddr,
> > > unsigned int addr, bool all, unsigned int val)
> > > {
> > > - unsigned int reg = devaddr << 27 | addr << 21 |
> > > - (val & 0xFF) << 13 | all << 12;
> > > + unsigned int reg = FIELD_PREP(AD7280A_TRANS_WRITE_DEVADDR_MSK, devaddr) |
> > > + FIELD_PREP(AD7280A_TRANS_WRITE_ADDR_MSK, addr) |
> > > + FIELD_PREP(AD7280A_TRANS_WRITE_VAL_MSK, val) |
> > > + FIELD_PREP(AD7280A_TRANS_WRITE_ALL_MSK, all);
> > Does reg get initialized to 0? If not, we should take care of the reserved bit D11
> >
> You have lost me here. We are writing the whole register and this first
> assignment of reg uses = not |= so any bits not set explicitly will be zero.
> We could do a FIELD_PREP(BIT(11), 0) with an appropriate name for the BIT(11) define
> but that would be a bit odd.
>
Oh yeah, I was just so paranoid about setting bit D11 to the point of forgetting
all reg bits would be initialized to zero, even without the direct assignment.
Setting bit D11 explicitly was a silly idea, forget about it.
> > > +
> > > + reg |= FIELD_PREP(AD7280A_TRANS_WRITE_CRC_MSK,
> > > + ad7280_calc_crc8(st->crc_tab, reg >> 11));
> > > + /* Reserved b010 pattern not included crc calc */
> > > + reg |= AD7280A_TRANS_WRITE_RES_PATTERN;
> > >
> > > - reg |= ad7280_calc_crc8(st->crc_tab, reg >> 11) << 3 | 0x2;
> > > st->tx = cpu_to_be32(reg);
> > >
> > > return spi_write(st->spi, &st->tx, sizeof(st->tx));
