On Mon, 2023-02-20 at 10:39 +0800, Liu Ying wrote:
> Hi Marek,
>
> On Sun, 2023-02-19 at 00:54 +0100, Marek Vasut wrote:
> > This particular block can have DT subnodes describing the LVDS LDB
> > bridge. Instead of misusing simple-bus to scan for those nodes, do
> > the scan within the driver.
> >
> > Fixes: 94e6197dadc9 ("arm64: dts: imx8mp: Add LCDIF2 & LDB nodes")
> > Signed-off-by: Marek Vasut <marex@xxxxxxx>
> > ---
> > Cc: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>
> > Cc: Fabio Estevam <festevam@xxxxxxxxx>
> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx>
> > Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > Cc: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
> > Cc: NXP Linux Team <linux-imx@xxxxxxx>
> > Cc: Paul Elder <paul.elder@xxxxxxxxxxxxxxxx>
> > Cc: Peng Fan <peng.fan@xxxxxxx>
> > Cc: Pengutronix Kernel Team <kernel@xxxxxxxxxxxxxx>
> > Cc: Richard Cochran <richardcochran@xxxxxxxxx>
> > Cc: Richard Zhu <hongxing.zhu@xxxxxxx>
> > Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> > Cc: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> > Cc: Shawn Guo <shawnguo@xxxxxxxxxx>
> > Cc: devicetree@xxxxxxxxxxxxxxx
> > Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > ---
> > drivers/soc/imx/imx8m-blk-ctrl.c | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c
> > b/drivers/soc/imx/imx8m-blk-ctrl.c
> > index 399cb85105a18..ab48f9dff4be5 100644
> > --- a/drivers/soc/imx/imx8m-blk-ctrl.c
> > +++ b/drivers/soc/imx/imx8m-blk-ctrl.c
> > @@ -169,7 +169,9 @@ static int imx8m_blk_ctrl_probe(struct
> > platform_device *pdev)
> > {
> > const struct imx8m_blk_ctrl_data *bc_data;
> > struct device *dev = &pdev->dev;
> > + struct platform_device *child;
> > struct imx8m_blk_ctrl *bc;
> > + struct device_node *np;
> > void __iomem *base;
> > int i, ret;
> >
> > @@ -310,6 +312,15 @@ static int imx8m_blk_ctrl_probe(struct
> > platform_device *pdev)
> >
> > dev_set_drvdata(dev, bc);
>
> nit: I would put the below loop before dev_set_drvdata().
>
> >
> > + for_each_child_of_node(dev->of_node, np) {
>
> Please call for_each_available_child_of_node() to create available
> child devices only.
Just found that of_platform_device_create() checks if the device node
is available or not, so you may keep using for_each_child_of_node().
Regards,
Liu Ying
>
> > + child = of_platform_device_create(np, NULL, dev);
> > + if (!child)
> > + ret = -ENOMEM;
>
> -ENODEV is more appropriate?
>
> > + of_node_put(np);
>
> for_each_child_of_node() and for_each_available_child_node() would do
> of_node_put() for you unless you break/return from the loop.
>
> > + if (ret)
> > + goto cleanup_provider;
>
> Instead of return in case of creating child device failure, I would
> give a warning message and continue to create other child devices if
> any. So I would write:
>
> -------------------------------8<-----------------------------------
> for_each_available_child_of_node(dev->of_node, np) {
> child = of_platform_device_create(np, NULL, dev);
> if (!child)
> dev_warn(dev, "failed to create device for
> %pOF\n", child);
> }
> -------------------------------8<-----------------------------------
>
> Even if return is used, dev_pm_genpd_remove_notifier() should be
> called
> to bailout correctly.
>
> Regards,
> Liu Ying
>
> > + }
> > +
> > return 0;
> >
> > cleanup_provider:
>
>
