Re: [PATCH v3 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer

[Andy Shevchenko]

On Tue, Apr 20, 2021 at 11:50 AM Tomas Melin <tomas.melin@xxxxxxxxxxx> wrote:
> On 4/19/21 4:55 PM, Andy Shevchenko wrote:
> > On Mon, Apr 19, 2021 at 4:26 PM Tomas Melin <tomas.melin@xxxxxxxxxxx> wrote:

...

> >> +#define SCA3300_MASK_STATUS    GENMASK(8, 0)
> >> +#define SCA3300_MASK_RS_STATUS GENMASK(1, 0)
> > This feels like an orphan. Shouldn't you move it closer to the group
> > of corresponding register / etc definition?
>
> Tried to group these in alphabetical order, but IIUC preference would be
> towards grouping

Yes, alphabetical is about header block, and definition should be
understandable and HW represented.

> according to how they are used? Would this be clearer and acceptable?

1) with some amendments, see below.

> 1)
>
> /* Device mode register */
> #define SCA3300_REG_MODE    0xd
> #define SCA3300_VALUE_SW_RESET    0x20

SCA3300_MODE_SW_RESET

> /* Last register in map */
> #define SCA3300_REG_SELBANK    0x1f
>
> /* Device status and related mask */
> #define SCA3300_REG_STATUS    0x6
> #define SCA3300_MASK_STATUS    GENMASK(8, 0)

SCA3300_STATUS_MASK

and so on (I guess you got the pattern)

> /* Device ID */
> #define SCA3300_REG_WHOAMI    0x10
> #define SCA3300_VALUE_DEVICE_ID    0x51
>
> /* Device return status and mask */
> #define SCA3300_VALUE_RS_ERROR    0x3
> #define SCA3300_MASK_RS_STATUS    GENMASK(1, 0)

...

> >> + * @txbuf: Transmit buffer
> >> + * @rxbuf: Receive buffer
> > Are the buffers subject to DMA? Shouldn't they have the proper alignment?
> Good point, I will add alignment.

Move them to the end of the structure to save few bytes,

...

> >> +       sca_data->txbuf[0] = 0x0 | (SCA3300_REG_STATUS << 2);
> > Seems you ignored my comment. What is this 0x0? What is the meaning of it?
> > Same for all the rest magic numbers in the code.
>
> Sorry, not ignored but will remove this redundant 0x0 for next round.

Maybe it's not redundant after all (I noticed other magic numbers in
the same position)? Please, comment your intention case-by-case.

...

> >> +       for_each_set_bit(bit, indio_dev->active_scan_mask,
> >> +                        indio_dev->masklength) {
> >> +               ret = sca3300_read_reg(data, sca3300_channels[bit].address,
> >> +                                      &val);
> >> +               if (ret) {
> >> +                       dev_err(&data->spi->dev,
> >> +                               "failed to read register, error: %d\n", ret);
> >> +                       goto out;
> > Does it mean interrupt is handled in this case?
> > Perhaps a comment why it's okay to consider so?
>
> IRQ_HANDLED seemed more correct than IRQ_NONE.

Why? Care to explain?

>  Or did You have some
> other option in mind?
>
> How about something like:
>
>      /* handled with errors */

But what if this is the very first interrupt (bit in the loop) that
failed? What about the rest?

>      goto out;
>
> >> +               }
> >> +               data->scan.channels[i++] = val;
> >> +       }

-- 
With Best Regards,
Andy Shevchenko