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

[Sirat]

On Fri, Jun 12, 2026 at 7:49 PM Joshua Crofts <joshua.crofts1@xxxxxxxxx> 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>
> > ---
>
> Hi Siratul,
>
> various comments inline. I've probably missed a few things
> as I only took a quick look so feel free to call me out on that!
>
> Josh
>
Thanks for the review Josh!
> > --- /dev/null
> > +++ b/drivers/iio/magnetometer/qmc5883l.c
> > @@ -0,0 +1,512 @@
...
> > +
> > +/* POR completion time max per datasheet */
> > +#define QMC5883L_PORT_US     350
>
> If it's POR completion, why does the macro contain PORT instead?
> > +
PORT is the terminology used in the datasheet for this. (Power –On
–Reset time period (PORT)). So I went with it.
> > +
> > +static const int qmc5883l_rng_avail[] = {
> > +     0, QMC5883L_SCALE_2G,   /* 2G */
>
> These comments are redundant IMO, you're already mentioning
> the value in the macro name.
>
I actually made them macros when I noticed that I needed to use them
in multiple places. The comments are now redundant you are right.
Thanks for noticing.
> > +     0, QMC5883L_SCALE_8G,   /* 8G */
> > +static int qmc5883l_read_raw(struct iio_dev *indio_dev,
> > +                          const struct iio_chan_spec *chan, int *val,
>
> I'd put val on the same line as val2 and mask, more logical separation.
>
These are done by clang-format. But yes this change makes more sense. Thanks!
> > +
---
>
> > +     int ret;
> > +
> > +     indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > +     if (!indio_dev)
> > +             return -ENOMEM;
> > +
> > +     regmap = devm_regmap_init_i2c(client, &qmc5883l_regmap_config);
> > +     if (IS_ERR(regmap)) {
>
> No point in adding brackets if this is a single line if statement
> (checkpatch.pl should warn about this IMO).
>
It didn't warn me for this one for some reason. I'll fix it.
> > +             return dev_err_probe(dev, PTR_ERR(regmap),
> > +                                  "regmap initialization failed\n");
> --
> Kind regards
>
> CJD
>