RE: [PATCH v13 04/11] clk: realtek: Introduce common probe() and remove()

[Yu-Chun Lin [林祐君]]

Hi Brian,

> Hi Yu-Chun,
> 
...

> > +int rtk_clk_probe(struct platform_device *pdev, const struct
> > +rtk_clk_desc *desc) {
> > +     struct device *dev = &pdev->dev;
> > +     struct regmap *regmap;
> > +     struct clk_hw *hw;
> > +     int i, ret;
> > +
> > +     regmap = device_node_to_regmap(dev->of_node);
> 
> Looking at the Sashiko comments for this series. device_node_to_regmap() has
> this note in drivers/mfd/syscon.c:
> 
> /**
>  * device_node_to_regmap() - Get or create a regmap for specified device
> node
>  * @np: Device tree node
>  *
>  * Get a regmap for the specified device node. If there's not an existing
>  * regmap, then one is instantiated. This function should not be used if the
>  * device node has a custom regmap driver or has resources (clocks, resets) to
>  * be managed. Use syscon_node_to_regmap() instead for those cases.
>  *
>  * Return: regmap ptr on success, negative error code on failure.
>  */
> 
> Should this be moved to syscon_node_to_regmap()?
> 

You are right that using device_node_to_regmap() is inappropriate here.
However, these clock domains have their own dedicated registers and are not
shared with any other drivers.

I will drop device_node_to_regmap() and initialize the regmap via
devm_regmap_init_mmio() instead. Since they all share the exact same register
format, I will define a shared regmap_config in clk-rtk-common.c.

> > +     if (IS_ERR(regmap))
> > +             return dev_err_probe(dev, PTR_ERR(regmap), "failed to
> > + get regmap\n");
> > +
> > +     for (i = 0; i < desc->num_clks; i++)
> > +             desc->clks[i]->regmap = regmap;
> > +
> > +     for (i = 0; i < desc->clk_data->num; i++) {
> > +             hw = desc->clk_data->hws[i];
> > +             if (!hw)
> > +                     continue;
> > +
> > +             ret = devm_clk_hw_register(dev, hw);
> > +             if (ret)
> > +                     return dev_err_probe(dev, ret, "failed to register
> hw of clk%d\n", i);
> > +     }
> > +
> > +     ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> > +                                       desc->clk_data);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "failed to add clock
> > + provider\n");
> > +
> > +     platform_set_drvdata(pdev, (void *)desc);
> > +
> > +     return rtk_reset_controller_register(dev, desc->aux_name,
> > +regmap); } EXPORT_SYMBOL_NS_GPL(rtk_clk_probe, "CLK_REALTEK");
> > +
> > +void rtk_clk_remove(struct platform_device *pdev) {
> > +     const struct rtk_clk_desc *desc = platform_get_drvdata(pdev);
> > +
> > +     if (!desc)
> > +             return;
> > +
> > +     for (int i = 0; i < desc->num_clks; i++)
> > +             desc->clks[i]->regmap = NULL;
> 
> Looking at the Sashiko comments, is this remove callback really needed?
> 

Will drop.

Best Regards,
Yu-Chun

> Brian