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

[Siratul Islam]

July 16, 2026 at 8:18 PM, "Jorijn van der Graaf" wrote:

...
> > +#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.
>
I can find atleast 10 drivers in IIO that uses *_SLEEP_DELAY_MS postfix
for autosuspend delay value. So it's not a strong reason not to use it.
> 
...
> > 
> > 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.
> 
I know what the code is doing here. But I'm emphasizing readability. You
are using 'i' with double responsibility. You already have #define
values that you are only ever using as array indecies. Using a switch
here combining with the #define(s) would be more readable and explicit. 

But it is more about preferences and following existing conventions
(i.e., the sibling driver). Jonathan, Andy, or David may have more idea
about the "preferred" way to do this.
> 
...
> 
> Thanks,
> Jorijn
>

--
Best regards,
Sirat