Re: [PATCH 3/3] iio: magnetometer: add driver for QST QMC5883L Sensor

[Sirat]

On Fri, Jun 12, 2026 at 11:02 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Fri, 12 Jun 2026 18:45:27 +0600
> Siratul Islam <email@xxxxxxxx> wrote:
>
> > Add driver for the QST QMC5883L 3-Axis Magnetic Sensor
> > connected via i2c.
> >
> > Signed-off-by: Siratul Islam <email@xxxxxxxx>
>
> Trying to avoid repeating stuff Joshua already covered.
> Various comments inline.
>
> Thanks,
>
> Jonathan
>
> > diff --git a/drivers/iio/magnetometer/qmc5883l.c b/drivers/iio/magnetometer/qmc5883l.c
> > new file mode 100644
> > index 000000000000..055e51570635
> > --- /dev/null
> > +++ b/drivers/iio/magnetometer/qmc5883l.c
...
> > +#define QMC5883L_OSR_512     FIELD_PREP_CONST(QMC5883L_OSR_MASK, 0x00)
> > +#define QMC5883L_OSR_256     FIELD_PREP_CONST(QMC5883L_OSR_MASK, 0x01)
> > +#define QMC5883L_OSR_128     FIELD_PREP_CONST(QMC5883L_OSR_MASK, 0x02)
> > +#define QMC5883L_OSR_64              FIELD_PREP_CONST(QMC5883L_OSR_MASK, 0x03)
> These are used for matching - normally we'd add defines for the filed value and
> then use FIELD_GET() to extract it for matching.
> e.g.
> #define QMC5883L_OSR_512                0x0
> #define QMC5883L_OSR_256                0x1
> rather these.
>
I thought using _CONST would be better for compile time checks and not
needing to use FIELD_GET everywhere. The values 0x01, 0x02 don't have
a lot of meaning in a vacuum. But of course you would have more
experience about it and I'd love to know why we shouldn't use const.
> > +
...

> > +static const int qmc5883l_rng_avail[] = {
> > +     0, QMC5883L_SCALE_2G,   /* 2G */
>
> I'm not sure the defines really help. Perhaps push the value down here
> and then look it up from this array when matching.
>
But then the array will be serving more than 1 purpose? kind of like a
side effect I think.
>
...
> > +
> > +     /* DRDY pin no used in this version of the driver */
> > +     ret = regmap_write(data->regmap, QMC5883L_REG_CTRL2,
> > +                        QMC5883L_INT_DISABLE);
> I don't mind if these sorts of cases go a little over 80 chars as sometimes
> it helps readability.
>
I try to disable clang-format for cases like this. It's an oversight on my side.
>
> Does it really reset with interrupts on?  That's odd.  Mind you the
> INT_ENB sounds like it would be an enable but as you have named it here
> it is actually a disable so all bets are off when it comes to sensible ;)
>
The datasheet says "The interrupt can be disabled by set 0AH[0] = 1"
which I took as it's enabled by default.
I explicitly disable it just to be safe.
>
...
> > +static const struct of_device_id qmc5883l_match[] = {
> > +     { .compatible = "qstcorp,qmc5883l" },
> > +     { },
>
> As below, no comma.
>
I did it so that clang-format doesn't wrap it all in 1 line. I forgot
to remove the comma before sending the patch. Sorry.
> > +};
>

Thanks
Sirat