On 2020-04-01 2:00 AM, Chanwoo Choi wrote:
> Hi Leonard,
>
> On 3/26/20 11:16 AM, Leonard Crestez wrote:
>> There is no single device which can represent the imx interconnect.
>> Instead of adding a virtual one just make the main &noc act as the
>> global interconnect provider.
>>
>> The imx interconnect provider driver will scale the NOC and DDRC based
>> on bandwidth request. More scalable nodes can be added in the future,
>> for example for audio/display/vpu/gpu NICs.
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@xxxxxxx>
>> ---
>> drivers/devfreq/imx-bus.c | 39 +++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 39 insertions(+)
>>
>> diff --git a/drivers/devfreq/imx-bus.c b/drivers/devfreq/imx-bus.c
>> index 285e0f1ae6b1..e9b13e43bf0a 100644
>> --- a/drivers/devfreq/imx-bus.c
>> +++ b/drivers/devfreq/imx-bus.c
>> @@ -15,10 +15,11 @@
>> struct imx_bus {
>> struct devfreq_dev_profile profile;
>> struct devfreq *devfreq;
>> struct clk *clk;
>> struct devfreq_passive_data passive_data;
>> + struct platform_device *icc_pdev;
>> };
>>
>> static int imx_bus_target(struct device *dev,
>> unsigned long *freq, u32 flags)
>> {
>> @@ -60,11 +61,42 @@ static int imx_bus_get_dev_status(struct device *dev,
>> return 0;
>> }
>>
>> static void imx_bus_exit(struct device *dev)
>> {
>> + struct imx_bus *priv = dev_get_drvdata(dev);
>> +
>> dev_pm_opp_of_remove_table(dev);
>> + platform_device_unregister(priv->icc_pdev);
>> +}
>> +
>> +/* imx_bus_init_icc() - register matching icc provider if required */
>
> Better to add following comments without 'imx_bus_init_icc() -' comment.
> /* Register matching icc provider if required */
This form looks like standard kernel-doc
>> +static int imx_bus_init_icc(struct device *dev)
>> +{
>> + struct imx_bus *priv = dev_get_drvdata(dev);
>> + const char *icc_driver_name;
>> +
>> + if (!of_get_property(dev->of_node, "#interconnect-cells", 0))
>> + return 0;
>> + if (!IS_ENABLED(CONFIG_INTERCONNECT_IMX)) {
>> + dev_warn(dev, "imx interconnect drivers disabled\n");
>> + return 0;
>> + }
>> +
>> + icc_driver_name = of_device_get_match_data(dev);
>> + if (!icc_driver_name)
>> + return 0;
>
> Recommend to add the error or warning message.
Makes sense, imx8m-nic shouldn't have #interconnect-cells so if this
condition is reach an error should be printed.
>> +
>> + priv->icc_pdev = platform_device_register_data(
>> + dev, icc_driver_name, -1, NULL, 0);
>> + if (IS_ERR(priv->icc_pdev)) {
>> + dev_err(dev, "failed to register icc provider %s: %ld\n",
>> + icc_driver_name, PTR_ERR(priv->devfreq));
>> + return PTR_ERR(priv->devfreq);
>> + }
>> +
>> + return 0;
>> }
>>
>> static int imx_bus_probe(struct platform_device *pdev)
>> {
>> struct device *dev = &pdev->dev;
>> @@ -112,18 +144,25 @@ static int imx_bus_probe(struct platform_device *pdev)
>> ret = PTR_ERR(priv->devfreq);
>> dev_err(dev, "failed to add devfreq device: %d\n", ret);
>> goto err;
>> }
>>
>> + ret = imx_bus_init_icc(dev);
>> + if (ret)
>
> Recommend to add the error message.
imx_bus_init_icc already prints errors. Printing again here would always
result in a second message.
>> + goto err;
>> +
>> return 0;
>>
>> err:
>> dev_pm_opp_of_remove_table(dev);
>> return ret;
>> }
>>
>> static const struct of_device_id imx_bus_of_match[] = {
>> + { .compatible = "fsl,imx8mq-noc", .data = "imx8mq-interconnect", },
>> + { .compatible = "fsl,imx8mm-noc", .data = "imx8mm-interconnect", },
>> + { .compatible = "fsl,imx8mn-noc", .data = "imx8mn-interconnect", },
>> { .compatible = "fsl,imx8m-noc", },
>> { .compatible = "fsl,imx8m-nic", },
>> { /* sentinel */ },
>> };
>> MODULE_DEVICE_TABLE(of, imx_bus_of_match);
>>
