Re: [PATCH v8 2/3] platform: arm64: dell-xps-ec: new driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] platform: dell-xps-ec: unacknowledged level IRQ in dell_xps_ec_irq_handler()
- [Medium] platform: arm64: dell-xps-ec: stopped temp_work on dell_xps_ec_resume() error
- [Low] platform: arm64: dell-xps-ec: mismatched ID and positional init in dell_xps_ec_i2c_id
--

commit 89371344f2f860e258af233ab4a65e60a8d3f321
Author: Aleksandrs Vinarskis <alex@vinarskis.com>

platform: arm64: dell-xps-ec: new driver

This commit introduces an I2C driver for the Embedded Controller in the
Qualcomm Snapdragon-based Dell XPS 13 9345. It configures the thermistor
profile, periodically reports thermistor temperatures read via IIO channels to
the EC, and notifies the EC across system suspend and resume.

> diff --git a/drivers/platform/arm64/dell-xps-ec.c b/drivers/platform/arm64/dell-xps-ec.c
> new file mode 100644
> index 0000000000000..20cb2a1b7bbd4
> --- /dev/null
> +++ b/drivers/platform/arm64/dell-xps-ec.c
> @@ -0,0 +1,308 @@

[ ... ]

> +static irqreturn_t dell_xps_ec_irq_handler(int irq, void *data)
> +{
> +	struct dell_xps_ec *ec = data;
> +
> +	/*
> +	 * TODO: IRQ is fired on lid-close. Follow Windows example to read out
> +	 *       the thermistor thresholds and potentially fan speeds.
> +	 */
> +	dev_dbg(ec->dev, "IRQ triggered (irq=%d)\n", irq);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int dell_xps_ec_probe(struct i2c_client *client)
> +{
> +	const struct dell_xps_ec_data *data;
> +	struct device *dev = &client->dev;
> +	struct dell_xps_ec *ec;
> +	unsigned int i;
> +	int ret;
> +
> +	data = i2c_get_match_data(client);
> +	if (!data)
> +		return dev_err_probe(dev, -ENODEV, "No match data\n");

[ ... ]

> +	/* Request IRQ for EC events */
> +	ret = devm_request_threaded_irq(dev, client->irq, NULL,
> +					dell_xps_ec_irq_handler,
> +					IRQF_ONESHOT, dev_name(dev), ec);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to request IRQ\n");
> +
> +	return 0;
> +}

[Severity: High]
Can requesting this interrupt before event readout is implemented cause an
interrupt storm when the EC asserts the line (for example, on lid close)?

Both the devicetree binding
(Documentation/devicetree/bindings/embedded-controller/dell,xps13-9345-ec.yaml)
and the board devicetree configure the EC interrupt as level-triggered
active-low:

    interrupts-extended = <&tlmm 66 IRQ_TYPE_LEVEL_LOW>;

In dell_xps_ec_probe(), devm_request_threaded_irq() enables this interrupt
with IRQF_ONESHOT, but dell_xps_ec_irq_handler() only logs a debug message
and returns IRQ_HANDLED without performing an I2C transfer to read or clear
the pending event on the EC.

When the EC asserts the level-low interrupt line, irq_thread_fn() runs
dell_xps_ec_irq_handler() and irq_finalize_oneshot() immediately unmasks the
line while it is still asserted low, causing the interrupt to re-fire in a
tight loop. Because dell_xps_ec_irq_handler() unconditionally returns
IRQ_HANDLED, note_interrupt() treats every firing as handled and spurious IRQ
detection will not disable the line.

Should the driver hold off on requesting the IRQ until the EC event readout is
implemented?

> +
> +/*
> + * Notify EC of suspend
> + *
> + * This will:
> + * - Cut power to display/trackpad/keyboard/touchrow, wake-up source still works.
> + */
> +static int dell_xps_ec_suspend(struct device *dev)
> +{
> +	struct dell_xps_ec *ec = dev_get_drvdata(dev);
> +
> +	int ret;
> +
> +	cancel_delayed_work_sync(&ec->temp_work);
> +
> +	ret = dell_xps_ec_suspend_cmd(ec, true);
> +	if (ret) {
> +		schedule_delayed_work(&ec->temp_work,
> +				      DELL_XPS_EC_TEMP_POLL_JIFFIES);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Notify EC of resume
> + *
> + * This will undo the suspend actions. Without the resume signal, device would
> + * wake up but be forced back into suspend by EC within seconds.
> + */
> +static int dell_xps_ec_resume(struct device *dev)
> +{
> +	struct dell_xps_ec *ec = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = dell_xps_ec_suspend_cmd(ec, false);
> +	if (ret)
> +		return ret;
> +
> +	schedule_delayed_work(&ec->temp_work, DELL_XPS_EC_TEMP_POLL_JIFFIES);
> +
> +	return 0;
> +}

[Severity: Medium]
If dell_xps_ec_suspend_cmd(ec, false) fails in dell_xps_ec_resume(), does this
leave ec->temp_work permanently stopped after the system resumes?

During suspend, dell_xps_ec_suspend() cancels ec->temp_work via
cancel_delayed_work_sync(). On resume, the PM core continues resuming the
system even if a device resume callback returns an error, so returning early
before schedule_delayed_work(&ec->temp_work, DELL_XPS_EC_TEMP_POLL_JIFFIES)
leaves periodic temperature reporting disabled while the system is running.

> +
> +static const struct of_device_id dell_xps_ec_of_match[] = {
> +	{ .compatible = "dell,xps13-9345-ec", .data = &dell_xps13_9345_data },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, dell_xps_ec_of_match);
> +
> +static const struct i2c_device_id dell_xps_ec_i2c_id[] = {
> +	{ "dell-xps-ec", (kernel_ulong_t)&dell_xps13_9345_data },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, dell_xps_ec_i2c_id);

[Severity: Low]
Should dell_xps_ec_i2c_id use the vendor-stripped compatible string
"xps13-9345-ec" and named initializers (.name and .driver_data)?

When an i2c_client without an fwnode (such as via sysfs instantiation using
the OF compatible name "xps13-9345-ec") is matched via
i2c_device_match() -> i2c_of_match_device() -> i2c_of_match_device_sysfs(),
dell_xps_ec_probe() calls i2c_get_match_data(client). Because
device_get_match_data(&client->dev) returns NULL when no fwnode is attached,
i2c_get_match_data() falls back to i2c_match_id(driver->id_table, client),
which fails to match "xps13-9345-ec" against "dell-xps-ec" and causes
dell_xps_ec_probe() to fail with -ENODEV. Using named initializers also
matches the I2C subsystem convention for new struct i2c_device_id entries.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-dell-xps-9345-ec-v8-0-251c2cbfbc6c@vinarskis.com?part=2