This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: usb: Add Infineon XMM6260 Baseband modem support

This patch adds a platform driver for the Infineon/Intel XMM6260
3G modem. The driver toggles GPIOs and a regulator, drives a pwrseq
provider, and uses the global USB notifier plus rfkill to react to
the modem's HSIC USB enumeration.

A few observations on the commit message and overall placement before
the inline comments:

[Low]
The commit message tail reads:

  The modem is usually connected via the application processor's USB
  line in HSIC mode; however, to work properly, the modem must control
  this line

The sentence stops there with no completion.  Since this is the central
justification for adding a custom driver, can the explanation of what
"control this line" entails and why an existing solution does not work
be added?

[Low]
The driver is placed under drivers/net/usb/ but does not implement a
net_device (no register_netdev / alloc_netdev / netdev_ops, no usbnet
binding, no tx/rx callbacks).  The dmesg in the commit message and the
Kconfig help confirm the modem enumerates as cdc_acm and that data
goes over ttyACM0 with a userspace DHCP client.

Would drivers/usb/misc/ or drivers/power/ be a more natural home, both
for routing the patch to the right maintainers and to avoid this
appearing alongside actual USB net drivers?

[Low]
Could the design be re-expressed as a pure pwrseq provider attached to
the USB device's hub-port DT node, letting the USB core gate
enumeration?  The current driver pulls in pwrseq for part of the work
but keeps regulator + GPIO toggling, an rfkill instance, a global USB
notifier, and an ad-hoc bitmap state machine alongside it.

[Low]
The title says "XMM626X" (suggesting a family), but the USB notifier
hard-codes a single VID:PID 0x1519:0x0020 for XMM6260 only.  Other
626X variants such as XMM6262 will not be matched.  Should the title
be narrowed to XMM6260, or the match table extended?

+	depends on USB_CHIPIDEA && (RFKILL || RFKILL=n)

[Low]
Does the driver actually have a software dependency on USB_CHIPIDEA?
The XMM6260 driver does not call any chipidea-specific API; it only
reacts to USB enumeration via the generic USB notifier and toggles
GPIOs/regulators/pwrseq.  Tying this to USB_CHIPIDEA prevents using
the driver on other HSIC-/USB-capable SoCs even though nothing in the
code is HCD-specific.

[ ... ]

[ ... ]

+/* bits [0:2] */
+enum baseband_xmm_ipc_ap_wake_state {
+	BASEBAND_XMM_IPC_AP_WAKE_IRQ_READY,
+	BASEBAND_XMM_IPC_AP_WAKE_INIT1,
+	BASEBAND_XMM_IPC_AP_WAKE_INIT2,
+	BASEBAND_XMM_IPC_AP_WAKE_L,
+	BASEBAND_XMM_IPC_AP_WAKE_H,
+	BASEBAND_XMM_IPC_AP_WAKE_UNINIT,
+	BASEBAND_XMM_IPC_AP_WAKE_MASK = 7,
+};
+
+#define BASEBAND_XMM_IPC_AP_WAKE_MAX	3
+
+#define BASEBAND_XMM_STATE_POWERED	3 /* Tracks regulator state */
+#define BASEBAND_XMM_STATE_PROTECTED	4 /* Prevents rfkill from access */
+#define BASEBAND_XMM_STATE_PRESENT	5 /* Tracks USB device presence */
+#define BASEBAND_XMM_STATE_POWEROFF	6 /* Prevents poweroff recursive call */
+#define BASEBAND_XMM_STATE_MAX		8

[Low]
The ipc_ap_wake_state enum reaches UNINIT=5 and MASK=7, but
BASEBAND_XMM_IPC_AP_WAKE_MAX=3 caps the encode/decode loops at three
bits.  Bits 3..6 of priv->state are then repurposed as orthogonal
STATE_POWERED / PROTECTED / PRESENT / POWEROFF flags.  Nothing
asserts that the wake-state value fits in three bits, so a future
enum entry past 7, or a typo widening WAKE_MAX, would silently
overwrite the STATE_* flags.  Could a BUILD_BUG_ON or static_assert
guard this invariant?

The L / H / UNINIT / MASK enumerators are also dead today; they only
appear at their declarations.  The block comment above the enum
describes a multi-state protocol with a retry cycle, but the IRQ
handler implements only IRQ_READY and INIT1.  Should the unused
values be removed, or should the handler be completed?

[ ... ]

+static void set_ipc_ap_wake(struct baseband_xmm_data *priv,
+			    enum baseband_xmm_ipc_ap_wake_state state)
+{
+	for (int i = 0; i < BASEBAND_XMM_IPC_AP_WAKE_MAX; i++)
+		if (state & BIT(i))
+			set_bit(i, priv->state);
+		else
+			clear_bit(i, priv->state);
+}
+
+static void baseband_xmm_reset(struct baseband_xmm_data *priv)
+{
+	int ret;
+
+	set_bit(BASEBAND_XMM_STATE_PROTECTED, priv->state);
+
+	if (!test_bit(BASEBAND_XMM_STATE_POWERED, priv->state)) {
+		ret = regulator_enable(priv->vbat_supply);
+		if (ret)
+			dev_err(priv->dev,
+				"failed to enable vbat power supply\n");
+
+		set_bit(BASEBAND_XMM_STATE_POWERED, priv->state);
+	}

[High]
On regulator_enable() failure, ret is logged but STATE_POWERED is
still set unconditionally.  When baseband_xmm_poweroff() later
observes STATE_POWERED it will call regulator_disable() without a
matching enable, which trips WARN_ON in the regulator core for a
negative use_count and corrupts the disable count for every other
consumer of vbat.  Should the set_bit() be moved inside an "if (!ret)"
branch, or should the function bail out on enable failure?

+
+	gpiod_set_value_cansleep(priv->enable_gpio, 0);
+	msleep(50);
+
+	gpiod_set_value_cansleep(priv->reset_gpio, 1);
+	msleep(200);
+	gpiod_set_value_cansleep(priv->reset_gpio, 0);
+
+	msleep(50);
+
+	/* falling edge trigger to CP */
+	gpiod_set_value_cansleep(priv->enable_gpio, 1);
+	usleep_range(50, 100);
+	gpiod_set_value_cansleep(priv->enable_gpio, 0);
+	msleep(20);
+}
+
+static void baseband_xmm_poweroff(struct baseband_xmm_data *priv)
+{
+	/*
+	 * The test_bit check prevents poweroff from being recursively
+	 * called during USB device deregistration. USB device
+	 * deregistration can be triggered by the driver by calling this
+	 * function or by some external event. The first case will cause
+	 * a recursive call by the notifier if not handled, while the
+	 * second case requires this call to handle the USB controller
+	 * properly.
+	 */
+	if (test_bit(BASEBAND_XMM_STATE_POWEROFF, priv->state))
+		return;
+
+	set_bit(BASEBAND_XMM_STATE_PROTECTED, priv->state);
+	set_bit(BASEBAND_XMM_STATE_POWEROFF, priv->state);
+
+	pwrseq_power_off(priv->pwrseq);
+	gpiod_set_value_cansleep(priv->reset_gpio, 1);
+
+	if (test_bit(BASEBAND_XMM_STATE_POWERED, priv->state)) {
+		regulator_disable(priv->vbat_supply);
+		clear_bit(BASEBAND_XMM_STATE_POWERED, priv->state);
+	}
+	set_ipc_ap_wake(priv, BASEBAND_XMM_IPC_AP_WAKE_IRQ_READY);
+
+	clear_bit(BASEBAND_XMM_STATE_PROTECTED, priv->state);
+	clear_bit(BASEBAND_XMM_STATE_PRESENT, priv->state);
+	clear_bit(BASEBAND_XMM_STATE_POWEROFF, priv->state);
+}

[High]
Are STATE_PROTECTED / STATE_POWEROFF / STATE_PRESENT being used here
as ad-hoc locks rather than as state?  The pattern is set on entry,
cleared on exit:

    if (test_bit(STATE_POWEROFF, priv->state))
        return;
    set_bit(STATE_PROTECTED, ...);
    set_bit(STATE_POWEROFF, ...);
    ...
    clear_bit(STATE_PROTECTED, ...);
    clear_bit(STATE_PRESENT, ...);
    clear_bit(STATE_POWEROFF, ...);

test_bit() followed by set_bit() is not atomic, so two callers (the
USB_DEVICE_REMOVE notifier on one CPU, the rfkill set_block callback,
the delayed work, or remove() on another) can both observe POWEROFF=0
and both proceed, calling pwrseq_power_off() twice and (when
STATE_POWERED is set) regulator_disable() twice.

The same concern applies to baseband_xmm_set_block(), which reads
STATE_PROTECTED and STATE_PRESENT outside of any lock, and to the
three separate set_bit/clear_bit operations inside set_ipc_ap_wake()
that the IRQ handler reads back via three separate test_bit() calls
(potential torn read across bits 0..2).

The networking subsystem documentation calls this pattern out
explicitly: "any pattern where a flag, atomic variable, or bit
operation appears to guard a section of code rather than express
state ... is a bug".  Could a real mutex (or spinlock for the IRQ
handler reads) be used instead?

+
+static int baseband_xmm_usb_notifier_call(struct notifier_block *nb,
+					  unsigned long action, void *data)
+{
+	struct baseband_xmm_data *priv =
+		container_of(nb, struct baseband_xmm_data, nb);
+	struct usb_device *udev;
+	u16 product, vendor;
+
+	if (action == USB_BUS_ADD || action == USB_BUS_REMOVE)
+		return NOTIFY_OK;
+
+	udev = data;
+	product = le16_to_cpu(udev->descriptor.idProduct);
+	vendor = le16_to_cpu(udev->descriptor.idVendor);
+
+	switch (action) {
+	case USB_DEVICE_ADD:
+		/* Infineon XMM6260 ID 1519:0020 */
+		if (vendor == BASEBAND_VENDOR_ID_COMNEON &&
+		    product == BASEBAND_PRODUCT_ID_XMM6260) {
+			cancel_delayed_work_sync(&priv->modem_work);
+			clear_bit(BASEBAND_XMM_STATE_PROTECTED, priv->state);
+			set_bit(BASEBAND_XMM_STATE_PRESENT, priv->state);
+		}
+		break;

[Low]
Match is purely on VID:PID via the global USB notifier, with no
binding between this platform_device instance and the USB device that
enumerates.  If two XMM6260 modems are present in the system, both
driver instances react to either modem's add/remove.  Would binding a
USB device driver to that VID:PID, or routing the lifecycle through a
pwrseq attached to the hub-port node, give a stable per-instance
association?

+
+	case USB_DEVICE_REMOVE:
+		/* Infineon XMM6260 ID 1519:0020 */
+		if (vendor == BASEBAND_VENDOR_ID_COMNEON &&
+		    product == BASEBAND_PRODUCT_ID_XMM6260)
+			baseband_xmm_poweroff(priv);
+		break;
+
+	default:
+		break;
+	}
+
+	return NOTIFY_OK;
+}

[ ... ]

+static irqreturn_t baseband_hostwake_interrupt(int irq, void *dev_id)
+{
+	struct baseband_xmm_data *priv = dev_id;
+	int state = gpiod_get_value(priv->ipc_ap_gpio);

[Medium]
This handler is registered with devm_request_threaded_irq(..., NULL,
&baseband_hostwake_interrupt, IRQF_ONESHOT | irqflags, ...), so it
runs in threaded (sleepable) context.  Should this read use
gpiod_get_value_cansleep() instead of gpiod_get_value()?  If ap-wake
is sourced from a sleeping GPIO controller (I2C/SPI expander, PMIC),
gpiod_get_value() will trip the WARN_ON(desc->gdev->can_sleep) in
gpiolib on every interrupt.  All other GPIO accesses in the driver
already use the *_cansleep variants.

+
+	switch (get_ipc_ap_wake(priv)) {
+	case BASEBAND_XMM_IPC_AP_WAKE_IRQ_READY:
+		if (!state) {
+			set_ipc_ap_wake(priv, BASEBAND_XMM_IPC_AP_WAKE_INIT1);
+			pwrseq_power_on(priv->pwrseq);
+		}
+		break;
+
+	case BASEBAND_XMM_IPC_AP_WAKE_INIT1:
+		if (state) {
+			set_ipc_ap_wake(priv, BASEBAND_XMM_IPC_AP_WAKE_INIT2);
+			schedule_delayed_work(&priv->modem_work,
+					      msecs_to_jiffies(BASEBAND_XMM_INIT_DELAY));
+		}
+		break;
+
+	default:
+		break;
+	}
+
+	return IRQ_HANDLED;
+}

[Medium]
Is the polarity of the IRQ_READY -> INIT1 transition consistent with
the comment above the enum?  The block comment states:

  AP wake interrupt keeps low util CP starts to initiate its HSIC hw.
  AP wake interrupt goes up during CP HSIC init
  (BASEBAND_XMM_IPC_AP_WAKE_INIT1 state)

So entering INIT1 corresponds to a rising edge (line goes high).  But
the handler enters INIT1 only when "!state" (line is low) and that's
where it calls pwrseq_power_on().  Either the comment is describing
the protocol incorrectly or the conditional checks the wrong polarity;
if the comment is accurate, the modem would never power on.  Could
this be clarified?

[Low]
For the non-DT path, irqflags is set to
IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING and the handler decides
which transition occurred by re-reading the GPIO.  The value seen in
the handler is the current level, not necessarily the level that
triggered the IRQ.  A coalesced fast L->H->L sequence (or a noisy
edge) can cause the handler to read a state that does not correspond
to the triggering edge, missing the INIT1 -> INIT2 transition or
jumping over states.  Should each edge be latched (e.g. by remembering
the previous level) instead of re-sampled?

+
+static int baseband_xmm_probe(struct platform_device *pdev)
+{

[ ... ]

+	bitmap_zero(priv->state, BASEBAND_XMM_STATE_MAX);
+	INIT_DELAYED_WORK(&priv->modem_work, baseband_xmm_work);
+
+	priv->irq = platform_get_irq(pdev, 0);
+	if (priv->irq < 0)
+		return dev_err_probe(dev, priv->irq, "failed to get IRQ\n");
+
+	/*
+	 * Systems using device tree should set up interrupt via DT,
+	 * the rest will use the default edge both interrupt.
+	 */
+	irqflags = dev->of_node ? 0 : IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;
+
+	ret = devm_request_threaded_irq(dev, priv->irq, NULL,
+					&baseband_hostwake_interrupt,
+					IRQF_ONESHOT | irqflags,
+					"modem-hostwake", priv);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to register IRQ %d\n", priv->irq);
+
+	priv->rfkill_dev = rfkill_alloc("xmm-modem", dev, RFKILL_TYPE_WWAN,
+					&baseband_xmm_rfkill_ops, priv);
+	if (!priv->rfkill_dev)
+		return -ENOMEM;
+
+	ret = rfkill_register(priv->rfkill_dev);
+	if (ret) {
+		rfkill_destroy(priv->rfkill_dev);
+		return dev_err_probe(dev, ret,
+				     "failed to register WWAN rfkill\n");
+	}

[High]
Once devm_request_threaded_irq() succeeds, the IRQ is live and the
handler can already step through IRQ_READY -> INIT1 (calling
pwrseq_power_on()) and on the next edge INIT1 -> INIT2, where it does:

    schedule_delayed_work(&priv->modem_work,
                          msecs_to_jiffies(BASEBAND_XMM_INIT_DELAY));

If rfkill_alloc() returns NULL or rfkill_register() fails, probe
returns an error.  devres releases the IRQ but does not cancel the
queued delayed work, and the devm_kzalloc()'d priv is then freed.
Five seconds later baseband_xmm_work() runs and dereferences priv
(priv->pwrseq, priv->state, priv->reset_gpio, priv->vbat_supply).

Should the error paths after devm_request_threaded_irq() call
cancel_delayed_work_sync(&priv->modem_work), or should the cancel be
registered via devm_add_action_or_reset()?

+
+	priv->nb.notifier_call = baseband_xmm_usb_notifier_call;
+	usb_register_notify(&priv->nb);
+
+	return 0;
+}

[High]
The ordering here is rfkill_register() first, usb_register_notify()
second.  rfkill_register() schedules sync_work, which runs the
set_block callback asynchronously.  Since this driver does not set
rfkill->persistent, the default state is unblocked, so set_block(false)
calls baseband_xmm_reset() -> regulator_enable() + GPIO toggling, and
the IRQ handler can step through IRQ_READY -> INIT1 -> INIT2 and
pwrseq_power_on() the modem, which then enumerates on USB.

If usb_register_notify() has not yet run when USB enumeration
completes, the USB_DEVICE_ADD notification is missed: STATE_PRESENT
is never set, the cancel_delayed_work_sync() in the notifier never
runs, and the 5-second watchdog work then calls baseband_xmm_poweroff()
on a working modem.

Should usb_register_notify() be called before rfkill_register()?

+
+static void baseband_xmm_remove(struct platform_device *pdev)
+{
+	struct baseband_xmm_data *priv = platform_get_drvdata(pdev);
+
+	rfkill_unregister(priv->rfkill_dev);
+	rfkill_destroy(priv->rfkill_dev);
+
+	disable_irq(priv->irq);
+	cancel_delayed_work_sync(&priv->modem_work);
+
+	usb_unregister_notify(&priv->nb);
+	baseband_xmm_poweroff(priv);
+}

[Medium]
Between cancel_delayed_work_sync() and usb_unregister_notify(), a
USB_DEVICE_REMOVE notification can still be in flight on another CPU
and call baseband_xmm_poweroff().  usb_unregister_notify() then waits
for that callback to complete, and immediately afterwards remove()
itself calls baseband_xmm_poweroff().  Combined with the
ad-hoc-locking concern above, this can result in pwrseq_power_off()
running twice and possibly regulator_disable() running twice.

Would unregistering the notifier first (before disable_irq /
cancel_delayed_work_sync) close that window?

[ ... ]

+module_platform_driver(baseband_xmm_driver);
+
+MODULE_AUTHOR("Svyatolsav Ryhel [off-list ref]");

[Low]
The name spelling here (Svyatolsav) does not match the file copyright
header or the patch's Signed-off-by ("Svyatoslav"); the 's' and 'l'
are transposed.

+MODULE_DESCRIPTION("Baseband Infineon XMM6260 driver");
+MODULE_LICENSE("GPL");

-- 
pw-bot: cr