Re: [PATCHv5 3/9] devfreq: event: Add exynos-ppmu devfreq-event driver

[Chanwoo Choi <cw00.choi@samsung.com>]

On 12/23/2014 12:40 PM, Varka Bhadram wrote:
> On Tue, Dec 23, 2014 at 8:48 AM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>> This patch adds exynos-ppmu devfreq-event driver to get performance data
>> of each IP for Samsung Exynos SoC. These event from Exynos PPMU provide
>> useful information about the behavior of the SoC that you can use when
>> analyzing system performance, and made visible and can be counted using
>> logic in each IP.
>>
>> This patch is based on existing drivers/devfreq/exynos/exynos-ppmu.c
>>
>> Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
>> Cc: Kyungmin Park <kyungmin.park@samsung.com>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> 
> (...)
>> +static int exynos_ppmu_parse_dt(struct exynos_ppmu_data *data)
>> +{
>> +       struct device *dev = data->dev;
>> +       struct device_node *np = dev->of_node;
>> +       int ret = 0;
>> +
>> +       if (!np) {
>> +               dev_err(dev, "failed to find devicetree node\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       /* Maps the memory mapped IO to control PPMU register */
>> +       data->ppmu.base = of_iomap(np, 0);
>> +       if (IS_ERR_OR_NULL(data->ppmu.base)) {
>> +               dev_err(dev, "failed to map memory region\n");
>> +               return -EINVAL;
> -ENOMEM or -ENXIO is the proper error return value..?

OK. I'll return -ENOMEM value.

> 
>> +       }
>> +
>> +       data->clk_ppmu = devm_clk_get(dev, "ppmu");
>> +       if (IS_ERR(data->clk_ppmu)) {
>> +               data->clk_ppmu = NULL;
>> +               dev_warn(dev, "failed to get PPMU clock\n");
>> +       }
> If PPMU clk get fails..?  return PTR_ERR(data->clk_ppmu)

If Exynos SoC don't register the clock to clock tree, the clock remains on state.
So, I print just warning message.

But, It is not proper. I'll fix it.

>> +
>> +       ret = of_get_devfreq_events(np, data);
>> +       if (ret < 0) {
>> +               dev_err(dev, "failed to parse exynos ppmu dt node\n");
>> +               goto err;
>> +       }
>> +
>> +       return 0;
>> +
>> +err:
>> +       iounmap(data->ppmu.base);
>> +
>> +       return ret;
>> +}
>> +
>> +static int exynos_ppmu_probe(struct platform_device *pdev)
>> +{
>> +       struct exynos_ppmu_data *data;
>> +       struct devfreq_event_dev **edev;
>> +       struct devfreq_event_desc *desc;
>> +       int i, ret = 0, size;
>> +
>> +       data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>> +       if (!data)
>> +               return -ENOMEM;
>> +
>> +       mutex_init(&data->lock);
>> +       data->dev = &pdev->dev;
>> +
>> +       /* Parse dt data to get resource */
>> +       ret = exynos_ppmu_parse_dt(data);
>> +       if (ret < 0) {
>> +               dev_err(&pdev->dev,
>> +                       "failed to parse devicetree for resource\n");
>> +               return ret;
>> +       }
>> +       desc = data->desc;
>> +
>> +       size = sizeof(struct devfreq_event_dev *) * data->num_events;
>> +       data->edev = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
>> +       if (!data->edev) {
>> +               dev_err(&pdev->dev,
>> +                       "failed to allocate memory devfreq-event devices\n");
>> +               return -ENOMEM;
>> +       }
>> +       edev = data->edev;
>> +       platform_set_drvdata(pdev, data);
>> +
>> +       for (i = 0; i < data->num_events; i++) {
>> +               edev[i] = devfreq_event_add_edev(&pdev->dev, &desc[i]);
>> +               if (IS_ERR(edev)) {
>> +                       ret = PTR_ERR(edev);
>> +                       dev_err(&pdev->dev,
>> +                               "failed to add devfreq-event device\n");
>> +                       goto err;
>> +               }
>> +       }
>> +
>> +       clk_prepare_enable(data->clk_ppmu);
>> +
>> +       return 0;
>> +err:
>> +       iounmap(data->ppmu.base);
>> +
>> +       return ret;
>> +}
>> +
>> +static int exynos_ppmu_remove(struct platform_device *pdev)
>> +{
>> +       struct exynos_ppmu_data *data = platform_get_drvdata(pdev);
>> +       int i, ret = 0;
>> +
>> +       for (i = 0; i < data->num_events; i++) {
>> +               ret = devfreq_event_remove_edev(data->edev[i]);
>> +               if (ret < 0) {
>> +                       dev_err(&pdev->dev,
>> +                               "failed to remove devfreq-event device\n");
>> +                       goto err;
>> +               }
>> +       }
>> +
>> +err:
>> +       clk_disable_unprepare(data->clk_ppmu);
>> +       iounmap(data->ppmu.base);
>> +
>> +       return ret;
>> +}
>> +
>> +static struct of_device_id exynos_ppmu_id_match[] = {
>> +       { .compatible = "samsung,exynos-ppmu", },
>> +       { /* sentinel */ },
>> +};
>> +
>> +static struct platform_driver exynos_ppmu_driver = {
>> +       .probe  = exynos_ppmu_probe,
>> +       .remove = exynos_ppmu_remove,
>> +       .driver = {
>> +               .name   = "exynos-ppmu",
>> +               .owner  = THIS_MODULE,
> remove owner field. It will be populate by driver core.

OK. I'll remove it.

Thanks for your review.

Best Regards,
Chanwoo Choi