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

[Jonathan Cameron]

On Sat, 13 Jun 2026 14:47:25 +0600
Sirat <email@xxxxxxxx> wrote:

> 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 

It is mainly about readability at the point of use. 

			ret = regmap_update_bits(data->regmap,
						 QMC5883L_OSR_MASK,
						 FIELD_PREP(QMC5883L_OSR_MASK, osr));

makes it clear to the reader that it is a field assignment and that correct
adjustment of bits has been applied (shifting, masking etc)

			ret = regmap_update_bits(data->regmap,
						 QMC5883L_REG_CTRL1,
						 QMC5883L_OSR_MASK, osr); 

means that to check that the right field is being assigned we have to go
find where osr is set and up to the defines just to check that everything
is as expected.

+ the other strong argument is convention. Certainly in IIO and perhaps
more generally we all field values are the content of the field, not the
shifted variant.

As you note, the compile time checks are lost, but I'm not sure they
bring a lot of value in this particular case where it's copy typed stuff
from the datasheet.

> > > +  
> ...
> 
> > > +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.

The array is of the value that are available - so using them to say 'this
one' to me seems fine.  If you were to do the common trick of having those as a
2d array (needs a cast for read_avail) then
static const int qmc5883l_rng_avail[][2] = { // or [2][2] if you prefer
	[QMC5883L_RNG_2G] = { 0, 83333 },
	[QMC5883L_RNG_8G] = { 0, 333333 },
};

At the other place it is looked up it becomes something like:
	*val = qmc5773l_rng_avail[data->range][0]
	*val2 = qmc5773l_rng_avail[data->range][1];

possibly rename it to
static const int qmc5883l_scales[2][];

as it is not used both to express what is available and for use
as a lookup.  It was always the available scales rather than ranges
anyway.


> >  
> ...
> > > +
> > > +     /* 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.

That's fair enough, if very odd given that's a typical path to annoying
races.  Might be worth just testing the hardware to check the default

> >  
> ...
> > > +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