Re: [PATCH 2/5] Bluetooth: btnxpuart: Add auxiliary driver for PCIe M.2 modules

[Andy Shevchenko]

On Tue, Sep 15, 2026 at 04:24:03PM +0200, Manivannan Sadhasivam via B4 Relay wrote:

> The 88W8987 combo module exposes Bluetooth over UART. Since this module is
> attached over PCIe, it is not described in firmware like devicetree. So it
> is discovered at runtime over PCIe by the power sequencing driver, which
> allocates the UART serdev and creates an auxiliary device carrying that
> transport and the power sequencing target to power up the Bluetooth
> function.
> 
> Add an auxiliary driver that binds to this device. It reuses the serdev
> provided by the producer, brings up the controller through the existing
> UART transport with nxp_register_dev() and drives power through the
> sequencer obtained with pwrseq_get().
> 
> Factor the HCI device setup and teardown shared with the serdev probe into
> helpers so both paths register the same controller.

...

> -static int nxp_serdev_probe(struct serdev_device *serdev)
> +static int nxp_register_dev(struct btnxpuart_dev *nxpdev)
>  {
> +	struct serdev_device *serdev = nxpdev->serdev;

Why not also

	struct device *dev = &serdev->dev;

and use it to make lines even shorter?

...

> -	device_property_read_u32(&nxpdev->serdev->dev, "max-speed",
> +	device_property_read_u32(&serdev->dev, "max-speed",
>  				 &nxpdev->secondary_baudrate);

	device_property_read_u32(dev, "max-speed", &nxpdev->secondary_baudrate);

Now exactly a single line (out of 80 characters).

...

> -	device_property_read_u8_array(&nxpdev->serdev->dev,
> +	device_property_read_u8_array(&serdev->dev,
>  				      "local-bd-address",
>  				      (u8 *)&ba, sizeof(ba));

	device_property_read_u8_array(dev, "local-bd-address",
				      (u8 *)&ba, sizeof(ba));

Now two lines instead of three. But ideally this casting should gone and
instead something like ether_addr_copy() to be used. (The latter is out
of scope here, of course.)

...

> +static int nxp_serdev_probe(struct serdev_device *serdev)
> +{
> +	struct btnxpuart_dev *nxpdev;
> +	int err;
> +
> +	nxpdev = devm_kzalloc(&serdev->dev, sizeof(*nxpdev), GFP_KERNEL);
> +	if (!nxpdev)
> +		return -ENOMEM;

> +	nxpdev->nxp_data = (struct btnxpuart_data *)device_get_match_data(&serdev->dev);

This is bad. The const qualifier is for a reason. Make sure it's kept.
On top the same suggestion as per above, use local 'dev' pointer.

Also Sashiko found a nice issue with the driver data, id est driver_override
mechanism that in some cases may lead to NULL dereferencing. Please, double
check if it's not the case, otherwise check for NULL and return -ENODATA.

> +	nxpdev->serdev = serdev;
> +
> +	nxpdev->pdn = devm_reset_control_get_optional_shared(&serdev->dev, NULL);
> +	if (IS_ERR(nxpdev->pdn))
> +		return PTR_ERR(nxpdev->pdn);
> +
> +	err = devm_regulator_get_enable(&serdev->dev, "vcc");
> +	if (err) {
> +		dev_err(&serdev->dev, "Failed to enable vcc regulator\n");
> +		return err;
> +	}
> +
> +	if (nxp_m2_connector_is_available(&serdev->ctrl->dev)) {
> +		struct pwrseq_desc *pwrseq;
> +
> +		pwrseq = pwrseq_get(&serdev->ctrl->dev, "uart");
> +		if (IS_ERR(pwrseq))
> +			return dev_err_probe(&serdev->dev, PTR_ERR(pwrseq),
> +					     "failed to get pwrseq\n");
> +
> +		nxpdev->pwrseq = pwrseq;
> +		err = pwrseq_enable(pwrseq);
> +		if (err)
> +			goto err_pwrseq_put;
> +	}
> +
> +	err = nxp_register_dev(nxpdev);
> +	if (err)
> +		goto err_pwrseq_put;
> +
> +	return 0;
> +
>  err_pwrseq_put:
>  	if (nxpdev->pwrseq)
>  		pwrseq_put(nxpdev->pwrseq);
>  	return err;
>  }

-- 
With Best Regards,
Andy Shevchenko