Re: [PATCH v5 9/9] power: sequencing: pcie-m2: Create serdev device for WCN7850 bluetooth

[Manivannan Sadhasivam]

On Wed, Feb 25, 2026 at 03:22:50AM -0800, Bartosz Golaszewski wrote:
> On Tue, 24 Feb 2026 06:30:55 +0100, Manivannan Sadhasivam via B4 Relay
> <devnull+manivannan.sadhasivam.oss.qualcomm.com@xxxxxxxxxx> said:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxxxxxxxx>
> >
> > For supporting bluetooth over the non-discoverable UART interface of
> > WCN7850, create the serdev device after enumerating the PCIe interface.
> > This is mandatory since the device ID is only known after the PCIe
> > enumeration and the ID is used for creating the serdev device.
> >
> > Since by default there is no OF or ACPI node for the created serdev,
> > create a dynamic OF 'bluetooth' node with the 'compatible' property and
> > attach it to the serdev device. This will allow the serdev device to bind
> > to the existing bluetooth driver.
> >
> > Tested-by: Hans de Goede <johannes.goede@xxxxxxxxxxxxxxxx> # ThinkPad T14s gen6 (arm64)
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxxxxxxxx>
> > ---
> >
> 
> [snip]
> 
> > -static void pwrseq_pcie_m2_free_regulators(void *data)
> > +static void pwrseq_pcie_m2_free_resources(void *data)
> >  {
> >  	struct pwrseq_pcie_m2_ctx *ctx = data;
> >
> > +	serdev_device_remove(ctx->serdev);
> > +	bus_unregister_notifier(&pci_bus_type, &ctx->nb);
> > +	of_changeset_revert(ctx->ocs);
> > +	of_changeset_destroy(ctx->ocs);
> >  	regulator_bulk_free(ctx->num_vregs, ctx->regs);
> >  }
> >
> > +static int pwrseq_m2_pcie_create_bt_node(struct pwrseq_pcie_m2_ctx *ctx,
> > +					struct device_node *parent)
> > +{
> > +	struct device *dev = ctx->dev;
> > +	struct device_node *np;
> > +	int ret;
> > +
> > +	ctx->ocs = devm_kzalloc(dev, sizeof(*ctx->ocs), GFP_KERNEL);
> > +	if (!ctx->ocs)
> > +		return -ENOMEM;
> > +
> > +	of_changeset_init(ctx->ocs);
> > +
> > +	np = of_changeset_create_node(ctx->ocs, parent, "bluetooth");
> > +	if (!np) {
> > +		dev_err(dev, "Failed to create bluetooth node\n");
> > +		ret = -ENODEV;
> > +		goto err_destroy_changeset;
> > +	}
> > +
> > +	ret = of_changeset_add_prop_string(ctx->ocs, np, "compatible", "qcom,wcn7850-bt");
> > +	if (ret) {
> > +		dev_err(dev, "Failed to add bluetooth compatible: %d\n", ret);
> > +		goto err_destroy_changeset;
> > +	}
> > +
> > +	ret = of_changeset_apply(ctx->ocs);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to apply changeset: %d\n", ret);
> > +		goto err_destroy_changeset;
> > +	}
> > +
> > +	ret = device_add_of_node(&ctx->serdev->dev, np);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to add OF node: %d\n", ret);
> > +		goto err_revert_changeset;
> > +	}
> > +
> > +	return 0;
> > +
> > +err_revert_changeset:
> > +	of_changeset_revert(ctx->ocs);
> > +err_destroy_changeset:
> > +	of_changeset_destroy(ctx->ocs);
> > +
> 
> I would prefer pwrseq_pcie_m2_free_resources() to be split into separate
> devm actions, otherwise it's not much different from simply having the
> .remove() callback. With a split like that you'd avoid having these labels
> here.
> 

We do need these error labels since pwrseq_m2_pcie_create_bt_node() is called
from notifier callback and a failure here doesn't cause the driver to fail. So
the changeset will linger till the driver gets removed.

So I don't see a real need to split pwrseq_pcie_m2_free_resources().

- Mani

-- 
மணிவண்ணன் சதாசிவம்