Re: [PATCH 2/2] iio: magnetometer: add support for QST QMC6308

[Jorijn van der Graaf]

Hi Sirat,

Thanks for the thorough review. And no worries about the formatting -
it was all still readable; if more than that opening sentence got
dropped, feel free to resend it.

On Thu, 2026-07-16 at 15:52 +0600, Siratul Islam wrote:
> > +#define QMC6308_OSR2_MASK	GENMASK(7, 6)
> This is never used so it's dead code. Are you supposed to use it
> somewhere?

It completes the CTRL1 field layout, but you're right that nothing
uses it. The full-register write in qmc6308_init() already sets OSR2
to 0 (= its power-on default, filter depth 1); v2 will make that
explicit with a FIELD_PREP() in that write so the OSR2 policy is
visible in the code - or I can drop the define instead, if that's
preferred.

> > +/*
> > + * Power-on completion time (datasheet Table 7), also used as a
> > + * conservative bound after soft reset, for which the datasheet
> > + * gives no figure.
> > + */
> > +#define QMC6308_POR_US 250
> Describe what this value is here, and explain in call sites why you
> are using it for something else too.

Sure, will split it that way in v2 (and that covers your later note
at the qmc6308_init() call site too).

> > +#define QMC6308_AUTOSUSPEND_DELAY_MS 500
> You can call it QMC6308_SLEEP_DELAY_MS to align with other values.

I'd prefer to keep AUTOSUSPEND here: it names the mechanism the value
feeds (pm_runtime_set_autosuspend_delay()) and matches yamaha-yas530
and ak8974 in this directory.

> "reading the status register clears DRDY" - This bit of information
> is not describing the mutex. Explain it at a call site. i.e, where
> you read STATUS register to clear DRDY.

Agreed. v2 keeps the mutex comment to what it protects (the cached
range/odr/osr and the atomicity of poll + data read) and moves the
DRDY-clearing note next to the status poll.

> Use reverse x-mas tree order.

The natural initializer order is fixed by the dependencies here (map
comes from data, dev from map), so reverse xmas tree would need the
assignments split from the declarations; I'd rather keep them
together.

> Why not use a switch here instead of depending on 'i'. In it's
> current form, we would need to keep going back to check what the
> scales, odr, osr arrays look like.
> I would suggest following the same pattern as the QMC5883L driver.
>
> The arrays could also be flattened like this if you follow the
> above suggestion.
> +static const int qmc6308_odr_avail[] = { 10, 50, 100, 200 };

The arrays are designated-initializer tables indexed by the register
field code, and the same tables back read_raw(), write_raw() and
read_avail() - so the value <-> field-code mapping lives in one place
and the accepted writes stay in lockstep with the advertised
_available values. Switches would duplicate that mapping per
direction, and the flattened form only works once the tables stop
being indexed by field code. I'd prefer to keep the tables, but if
Jonathan wants the qmc5883l shape for consistency across the two
drivers I'm fine changing it.

> This check won't be needed then.

With the loop kept, that check stays - it is the loop's not-found
case.

> Another issue I noticed, what happens if the update succeeds but the
> "status read to clear" below fails?
> You are returning an incorrect error to the userspace even though
> the scale change took effect.
> I think a better approach here would be a log and continue instead
> of failing.

Good catch. The range change has already taken effect at that point,
so failing the write would mislead userspace - v2 logs the failed
discard and returns success. For the record, the residual cost: if
the bus recovers before the next read, that read can still return one
sample converted at the old range.

> Might as well assign the default values for data->{range, odr, osr}
> together?

Sure, v2 groups all three before the CTRL2 write (which consumes
data->range).

Thanks,
Jorijn