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

[Jonathan Cameron]

On Thu, 16 Jul 2026 19:06:51 +0000
"Siratul Islam" <siratul.islam@xxxxxxxxx> wrote:

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

This sort of thing tends to be author preference as both show up.
AUTOSUSPEND is clearer in purpose so if anything would be my slight
preference

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

I'd prefer to keep them.  Makes a clear association between the defaults
set (via use of defined registered value) and the value in the userspace
interface.

Jonathan

> >   
> ...
> > 
> > Thanks,
> > Jorijn
> >  
> 
> --
> Best regards,
> Sirat