Re: [PATCH 1/7] slimbus: qcom-ngd-ctrl: Fix up platform_driver registration

[Bjorn Andersson]

On Tue, Mar 10, 2026 at 01:03:09PM +0530, Mukesh Ojha wrote:
> On Mon, Mar 09, 2026 at 11:09:02PM -0500, Bjorn Andersson wrote:
> > Device drivers should not invoke platform_driver_register()/unregister()
> > in their probe and remove paths. They should further not rely on
> > platform_driver_unregister() as their only means of "deleting" their
> > child devices.
> > 
> > Introduce a helper to unregister the child device and move the
> > platform_driver_register()/unregister() to module_init()/exit().
> > 
> > Fixes: 917809e2280b ("slimbus: ngd: Add qcom SLIMBus NGD driver")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxxxxxxxx>
> 
> Reviewed-by: Mukesh Ojha <mukesh.ojha@xxxxxxxxxxxxxxxx>
> 
> > ---
> >  drivers/slimbus/qcom-ngd-ctrl.c | 36 +++++++++++++++++++++++++++++++++---
> >  1 file changed, 33 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/slimbus/qcom-ngd-ctrl.c b/drivers/slimbus/qcom-ngd-ctrl.c
> > index 9aa7218b4e8d2b350835626839371ed6e19860e2..c69656a0ef1766d5a9df40bdf37bae8f64789fab 100644
> > --- a/drivers/slimbus/qcom-ngd-ctrl.c
> > +++ b/drivers/slimbus/qcom-ngd-ctrl.c
> > @@ -1562,6 +1562,13 @@ static int of_qcom_slim_ngd_register(struct device *parent,
> >  	return -ENODEV;
> >  }
> >  
> > +static void qcom_slim_ngd_unregister(struct qcom_slim_ngd_ctrl *ctrl)
> > +{
> > +	struct qcom_slim_ngd *ngd = ctrl->ngd;
> > +
> > +	platform_device_del(ngd->pdev);
> 
> First, it surprised me why only once, then I saw there is return 0 in
> for_each_available_child_of_node_scoped() loop..
> 

Yeah, I'm not sure about that one. If I read the binding correctly it
allows for an arbitrary number of child devices, but the implementation
will only consider the first active child, and silently ignore any
others.

I don't know if it's a valid usecase to have multiple NGDs on a
controller thought?

Regards,
Bjorn

> > +}
> > +
> >  static int qcom_slim_ngd_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> > @@ -1664,7 +1671,6 @@ static int qcom_slim_ngd_ctrl_probe(struct platform_device *pdev)
> >  		goto err_pdr_lookup;
> >  	}
> >  
> > -	platform_driver_register(&qcom_slim_ngd_driver);
> >  	return of_qcom_slim_ngd_register(dev, ctrl);
> >  
> >  err_pdr_alloc:
> > @@ -1678,7 +1684,9 @@ static int qcom_slim_ngd_ctrl_probe(struct platform_device *pdev)
> >  
> >  static void qcom_slim_ngd_ctrl_remove(struct platform_device *pdev)
> >  {
> > -	platform_driver_unregister(&qcom_slim_ngd_driver);
> > +	struct qcom_slim_ngd_ctrl *ctrl = platform_get_drvdata(pdev);
> > +
> > +	qcom_slim_ngd_unregister(ctrl);
> >  }
> >  
> >  static void qcom_slim_ngd_remove(struct platform_device *pdev)
> > @@ -1754,6 +1762,28 @@ static struct platform_driver qcom_slim_ngd_driver = {
> >  	},
> >  };
> >  
> > -module_platform_driver(qcom_slim_ngd_ctrl_driver);
> > +static int qcom_slim_ngd_init(void)
> > +{
> > +	int ret;
> > +
> > +	ret = platform_driver_register(&qcom_slim_ngd_ctrl_driver);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = platform_driver_register(&qcom_slim_ngd_driver);
> > +	if (ret)
> > +		platform_driver_unregister(&qcom_slim_ngd_ctrl_driver);
> > +
> > +	return ret;
> > +}
> > +
> > +static void qcom_slim_ngd_exit(void)
> > +{
> > +	platform_driver_unregister(&qcom_slim_ngd_driver);
> > +	platform_driver_unregister(&qcom_slim_ngd_ctrl_driver);
> > +}
> > +
> > +module_init(qcom_slim_ngd_init);
> > +module_exit(qcom_slim_ngd_exit);
> >  MODULE_LICENSE("GPL v2");
> >  MODULE_DESCRIPTION("Qualcomm SLIMBus NGD controller");
> > 
> > -- 
> > 2.51.0
> > 
> 
> -- 
> -Mukesh Ojha