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

[Hardik Phalet]

On Thu Apr 9, 2026 at 11:24 PM IST, Luka Gejak wrote:
> On Thu Apr 9, 2026 at 6:24 PM CEST, Hardik Phalet wrote:
>
> While functionally correct, the standard kernel idiom for deserializing
> contiguous byte arrays is to use the unaligned access helpers. Consider
> including <linux/unaligned.h> at the top of the file and using
> get_unaligned_le16(). Also since it returns a u16, you must keep the
> (s16) cast. Something like:
> *x = (s16)get_unaligned_le16(&reg_data[0]);
> *y = (s16)get_unaligned_le16(&reg_data[2]);
> *z = (s16)get_unaligned_le16(&reg_data[4]);
>
> For iio sysfs attributes, the dev pointer belongs to the iio device
> (&indio_dev->dev), not the i2c parent device where runtime pm was
> initialized. Calling pm operations on the iio device will lead to pm
> refcount imbalances and potential kernel warnings. You should use
> data->dev for all pm_runtime_* calls inside custom sysfs callbacks.
> Something like ret = pm_runtime_resume_and_get(data->dev);
>
>
> You should change these to use data->dev as well.
>
> Your dt-binding correctly documents an optional vdd-supply for the
> 2.5V-3.6V rail, but the driver never parses or enables it. If a board
> physically powers down this regulator during system suspend or relies on
> the driver to enable it at boot, the driver will fail. Please add
> devm_regulator_get_optional() and enable the regulator here. Also, you
> should ensure that your system resume callbacks correctly re-apply the
> chip's initialization sequence if power is lost during suspend.
>
> Casting function pointers like (void (*)(void *)) violates strict kcfi
> checks, which will cause a kernel panic on modern architectures
> (like arm64) compiled with cfi enabled. The pm core already provides a
> safe helper for this. Plese drop your custom qmc5883p_runtime_pm_disable
> function and the devm_add_action_or_reset call, and simply use
> ret = devm_pm_runtime_enable(dev);
>
> Because you use devm_iio_device_register(), the iio interface is
> unregistered automatically during the devres unwinding phase, which
> happens after your .remove() callback completes. This creates a race
> window. A concurrent userspace sysfs read/write could trigger a pm
> resume exactly while or after remove() is putting the chip to sleep. To
> fix this, create a custom devm action (via devm_add_action_or_reset) to
> write QMC5883P_MODE_SUSPEND to the chip, and register it before this
> devm_iio_device_register() call. Because devres unwinds in lifo order,
> this guarantees the iio interface is fully unregistered before the
> hardware is suspended.
>
> Once you move the suspend logic into a devm action in probe() (as
> mentioned above), you can delete this qmc5883p_remove() function and the
> .remove hook in the i2c_driver struct entirely.

Thanks for the thorough review.

All the points are clear and I'll address them in v2.

Regards,
Hardik