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

On Wed, 22 Jul 2026 02:19:37 +0600
Siratul Islam <[email protected]> wrote:

> On Tue, 2026-07-21 at 15:48 +0200, Jorijn van der Graaf wrote:
> > The QST QMC6308 is a 3-axis anisotropic magnetoresistive (AMR)
> > magnetometer on I2C, found e.g. in the Fairphone 6. Its register map
> > is not compatible with the QMC5883L's, so add a separate driver rather
> > than extending the QMC5883L driver.
> >   
> ...
> > - Switch the license to GPL-2.0-only: the dual license had been
> >   carried over from qmc5883l.c, which this driver is modeled on,
> >   without a reason of our own (Jonathan).  
> I wrote the QMC5883L driver. My reason to use dual-license was I wanted 
> people to be able to port the driver to Non-GPL OSes like Zephyr, NuttX, BSDs (specially RTOSes, 
> none of them are GPL, and these sensors are widely used with MCUs) with least friction.
> There are stuff that can't be ported (like regmap etc.) but the rest of it could.
> Also, I read the "Linux Device Drivers 3" book where the samples use Dual BSD/GPL.

That's a valid reason but I do expect people to have one rather than cut
and paste.  I suspect a port that way rarely happens because of things
like regmap that you call out. 

> >   
> > +static int qmc6308_probe(struct i2c_client *client)
> > +{  
> ...
> > +
> > +	ret = devm_add_action_or_reset(dev, qmc6308_power_down_action, data);
> > +	if (ret)
> > +		return ret;
> > +
> > +	pm_runtime_use_autosuspend(dev);
> > +	pm_runtime_set_autosuspend_delay(dev, QMC6308_AUTOSUSPEND_DELAY_MS);  
> Add a space here.
This one feels marginal given close relationship of the 3 lines. Still I think
I agree it is a tiny bit better with a blank line where you suggest.

> > +	ret = devm_pm_runtime_enable(dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return devm_iio_device_register(dev, indio_dev);
> > +}
> > +
> > +static int qmc6308_runtime_resume(struct device *dev)
> > +{
> > +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > +	struct qmc6308_data *data = iio_priv(indio_dev);
> > +	unsigned int status;
> > +	int ret;
> > +
> > +	ret = qmc6308_set_mode(data, QMC6308_MODE_NORMAL);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * DRDY may still be set for a sample converted before the last
> > +	 * suspend; reading the status register clears it so the next
> > +	 * measurement waits for fresh data.
> > +	 */
> > +	ret = regmap_read(data->regmap, QMC6308_REG_STATUS, &status);
> > +	if (ret) {
> > +		/* Best effort to leave the chip in a consistent state */
> > +		qmc6308_set_mode(data, QMC6308_MODE_SUSPEND);
> > +	}  
> It's single line so the brackets { } are not needed.
That gets complex when comments like this get involved.  In these
cases I tend to leave it to the author.

So I've left this one alone.

Series applied with tweaks to patch 1 to resolve the feedback from
Krzysztof.  I'll apply it initially to the testing branch of iio.git
but probably push that out for linux-next to pick up (as togreg) within
a few days.  If on balance you want to change the license back to dual
with BSD just send me a patch stating the reason and I'll merge that on top.

thanks,

Jonathan

> > +
> > +	return ret;
> > +}
> > +  
> ...

> > +
> > +MODULE_DESCRIPTION("QST QMC6308 3-Axis Magnetic Sensor driver");
> > +MODULE_AUTHOR("Jorijn van der Graaf <[email protected]>");
> > +MODULE_LICENSE("GPL");  
> 
> --
> Best regards,
> Sirat