Re: [PATCH v2 3/4] staging: iio: magnetometer: Add QST QMC5883P driver

[Hardik Phalet]

On Sat Apr 11, 2026 at 1:32 AM IST, David Lechner wrote:
> On 4/9/26 4:07 PM, Hardik Phalet wrote:
>
> This is a little bit much to review all in one patch. Could be nice
> to split out power management to a separate patch.
>
> Oversampling/downsampling could be split to a separate patch or two
> as well.
>
Noted David. I will handle it in v3. I think downsampling can be another
patch.

>> +#define QMC5883P_REG_Y_MSB 0x04
>> +#define QMC58
>> 83P_REG_Z_LSB 0x05
>
> Were you manually editing the patch?
No. Maybe it's my email client (AERC). I will make sure the next version
is tight.

>
>> +#define QMC5883P_RSTCTRL_SET_RESET \
>> +	0x00 /* Set and reset on, i.e. the offset of device is renewed */
>> +#define QMC5883P_RSTCTRL_SET_ONLY 0x01 /* Set only on */
>> +#define QMC5883P_RSTCTRL_OFF 0x02 /* Set
>> and reset off */
>
> Or maybe you mail client mangled the patch? These wraps are
> happening in many places.
>
Yes. Exactly this.

>> +	if (regval != QMC5883P_CHIP_ID)
>> +		return dev_err_probe(data->dev, -ENODEV,
>
> We don't consider ID match an error. It has happened too many times
> that there is a compatible part with a different ID. This can just
> be dev_info() and return success.
>
Noted. Will handle in v3.

>> +static int qmc5883p_chip_init(struct qmc5883p_data *data)
>> +{
>> +	int ret;
>> +
>> +	ret = regmap_field_write(data->rf.sftrst, 1);
>> +	if (ret)
>> +		return ret;
>> +
>> +	usleep_range(1000, 2000);
>
> Use fsleep() instead and add a comment explaining why this specific duration
> was selected.
>
Noted, will handle in v3.

>> +	ret = regmap_field_write(data->rf.odr, QMC5883P_DEFAULT_ODR);
>> +	if (ret)
>> +		return ret;
>
> Since we just reset the chip, why do we need to set everything to a
> new default instead of using the chip's default? If there is a good
> reason, add a comment, otherwise we can leave this out.
>
These were simply more aligned with my usecase, so more of a development
artifact. Chip defaults are sufficient. I will leave this out. 

>> +
>> +	return regmap_field_write(data->rf.mode, QMC5883P_MODE_NORMAL);
>
> Does this start sampling? Seems like it could be out of place here.
>
Yes, normal mode starts sampling data. Your point makes sense, I will
remove it in the next version. Incorrect mental model, I apologise.

>> +	ret = regmap_read_poll_timeout(data->regmap, QMC5883P_REG_STATUS,
>> +				       status, status & QMC5883P_STATUS_DRDY,
>> +				       QMC5883P_DRDY_POLL_US, 150000);
>
> Numbers with lots of 0s are easier to read as 150 * (MICRO / MILLI).
>
My bad, will handle in v3.

>> +static IIO_DEVICE_ATTR(downsampling_ratio, 0644, downsampling_ratio_show,
>> +		       downsampling_ratio_store, 0);
>> +static IIO_CONST_ATTR(downsampling_ratio_available, "1 2 4 8");
>
> As mentioned in the cover letter, we'd like to know more about what
> this actually does. If there is a good reason it doesn't fit with
> any existing filter attribute, then we'll need a patch to document
> the sysfs ABI as well.
>
In the device datasheet, OSR2("Down sampling ratio") is mentioned like this:
"Another filter is added for better noise performance; the depth can be
adjusted through OSR2". OSR2's defintion is called "down sampling ratio"
in a table. Nowhere else. I didn't know what attribute to map it to in
this case.

> Could save some dupilcation by making a macro that takes X/Y/Z
> as parameter. e.g.
>
> #define QMC5883P_CHAN(ch) \
> 	...				\
> 	.channel2 = IIO_MOD_##ch,	\
> 	...				\
> 	.address = AXIS_##ch,		\
> 	...
>
Noted. Will do this in v3.

>> +	ret = devm_regulator_get_enable_optional(dev, "vdd");
>> +	if (ret && ret != -ENODEV)
>> +		return dev_err_probe(dev, ret,
>> +				"failed to get vdd regulator\n");
>> +
>> +	/* Datasheet specifies up to 50 ms supply ramp + 250 us POR time. */
>> +	fsleep(50000);
>> +
>> +	i2c_set_clientdata(client, indio_dev);
>> +
>> +	ret = qmc5883p_rf_init(data);
>
> Would be more logical to move this up right after regmap is declared.
>
Makes sense, I will move it up in v3.

>> +	ret = devm_iio_device_register(dev, indio_dev);
>
> Usually, we just return directly here. This pretty much doesn't ever fail.
>
Okay. Will remove the dev_err_probe() in v3. 

>> +	ret = regmap_field_write(data->rf.mode, QMC5883P_MODE_NORMAL);
>> +	if (ret)
>> +		return ret;
>> +
>> +	usleep_range(10000, 11000);
>
> Again, fsleep() and comment.
>
Noted.


>> +static const struct i2c_device_id qmc5883p_id[] = {
>> +	{ "qmc5883p", 0 },
>> +	{},
>
> IIO style for this is:
>
> 	{ }
>
> space between braces and no trailing comma.
>
Noted. Will take it up in v3.

Thanks for the in-depth review David. I really appreciate it.

Regards,
Hardik