| Message ID | 20200529163200.18031-4-s.nawrocki@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Exynos: Simple QoS for exynos-bus using interconnect | expand |
Hi Sylwester, On Sat, May 30, 2020 at 1:33 AM Sylwester Nawrocki <s.nawrocki@samsung.com> wrote: > > This patch adds registration of a child platform device for the exynos > interconnect driver. It is assumed that the interconnect provider will > only be needed when #interconnect-cells property is present in the bus > DT node, hence the child device will be created only when such a property > is present. > > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > > Changes for v5: > - new patch. > --- > drivers/devfreq/exynos-bus.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c > index 8fa8eb5..856e37d 100644 > --- a/drivers/devfreq/exynos-bus.c > +++ b/drivers/devfreq/exynos-bus.c > @@ -24,6 +24,7 @@ > > struct exynos_bus { > struct device *dev; > + struct platform_device *icc_pdev; > > struct devfreq *devfreq; > struct devfreq_event_dev **edev; > @@ -156,6 +157,8 @@ static void exynos_bus_exit(struct device *dev) > if (ret < 0) > dev_warn(dev, "failed to disable the devfreq-event devices\n"); > > + platform_device_unregister(bus->icc_pdev); > + > dev_pm_opp_of_remove_table(dev); > clk_disable_unprepare(bus->clk); > if (bus->opp_table) { > @@ -168,6 +171,8 @@ static void exynos_bus_passive_exit(struct device *dev) > { > struct exynos_bus *bus = dev_get_drvdata(dev); > > + platform_device_unregister(bus->icc_pdev); > + > dev_pm_opp_of_remove_table(dev); > clk_disable_unprepare(bus->clk); > } > @@ -431,6 +436,18 @@ static int exynos_bus_probe(struct platform_device *pdev) > if (ret < 0) > goto err; > > + /* Create child platform device for the interconnect provider */ > + if (of_get_property(dev->of_node, "#interconnect-cells", NULL)) { > + bus->icc_pdev = platform_device_register_data( > + dev, "exynos-generic-icc", > + PLATFORM_DEVID_AUTO, NULL, 0); > + > + if (IS_ERR(bus->icc_pdev)) { > + ret = PTR_ERR(bus->icc_pdev); > + goto err; > + } > + } > + > max_state = bus->devfreq->profile->max_state; > min_freq = (bus->devfreq->profile->freq_table[0] / 1000); > max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000); > -- > 2.7.4 > It looks like very similar like the registering the interconnect device of imx-bus.c and I already reviewed and agreed this approach. Acked-by: Chanwoo Choi <cw00.choi@samsung.com> nitpick: IMHO, I think that 'exynos-icc' is proper and simple without 'generic' word. If we need to add new icc compatible int the future, we will add 'exynosXXXX-icc' new compatible. But, I'm not forcing it. just opinion. Anyway, I agree this approach.
Cc: Rob, devicetree ML On 31.05.2020 01:57, Chanwoo Choi wrote: > On Sat, May 30, 2020 at 1:33 AM Sylwester Nawrocki > <s.nawrocki@samsung.com> wrote: >> >> This patch adds registration of a child platform device for the exynos >> interconnect driver. It is assumed that the interconnect provider will >> only be needed when #interconnect-cells property is present in the bus >> DT node, hence the child device will be created only when such a property >> is present. >> >> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> >> >> Changes for v5: >> - new patch. >> --- >> drivers/devfreq/exynos-bus.c | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> >> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c >> index 8fa8eb5..856e37d 100644 >> --- a/drivers/devfreq/exynos-bus.c >> +++ b/drivers/devfreq/exynos-bus.c >> @@ -24,6 +24,7 @@ >> >> struct exynos_bus { >> struct device *dev; >> + struct platform_device *icc_pdev; >> >> struct devfreq *devfreq; >> struct devfreq_event_dev **edev; >> @@ -156,6 +157,8 @@ static void exynos_bus_exit(struct device *dev) >> if (ret < 0) >> dev_warn(dev, "failed to disable the devfreq-event devices\n"); >> >> + platform_device_unregister(bus->icc_pdev); >> + >> dev_pm_opp_of_remove_table(dev); >> clk_disable_unprepare(bus->clk); >> if (bus->opp_table) { >> @@ -168,6 +171,8 @@ static void exynos_bus_passive_exit(struct device *dev) >> { >> struct exynos_bus *bus = dev_get_drvdata(dev); >> >> + platform_device_unregister(bus->icc_pdev); >> + >> dev_pm_opp_of_remove_table(dev); >> clk_disable_unprepare(bus->clk); >> } >> @@ -431,6 +436,18 @@ static int exynos_bus_probe(struct platform_device *pdev) >> if (ret < 0) >> goto err; >> >> + /* Create child platform device for the interconnect provider */ >> + if (of_get_property(dev->of_node, "#interconnect-cells", NULL)) { >> + bus->icc_pdev = platform_device_register_data( >> + dev, "exynos-generic-icc", >> + PLATFORM_DEVID_AUTO, NULL, 0); >> + >> + if (IS_ERR(bus->icc_pdev)) { >> + ret = PTR_ERR(bus->icc_pdev); >> + goto err; >> + } >> + } >> + >> max_state = bus->devfreq->profile->max_state; >> min_freq = (bus->devfreq->profile->freq_table[0] / 1000); >> max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000); >> -- >> 2.7.4 >> > > It looks like very similar like the registering the interconnect > device of imx-bus.c > and I already reviewed and agreed this approach. > > Acked-by: Chanwoo Choi <cw00.choi@samsung.com> > > nitpick: IMHO, I think that 'exynos-icc' is proper and simple without > 'generic' word. > If we need to add new icc compatible int the future, we will add > 'exynosXXXX-icc' new compatible. > But, I'm not forcing it. just opinion. Anyway, I agree this approach. Thanks for review. I will change the name to exynos-icc in next version, as I commented at other patch, it is not part of any DT binding, it is just for device/driver matching between devfreq and interconnect. -- Thanks, Sylwester
Hi Sylwester, On 6/1/20 7:04 PM, Sylwester Nawrocki wrote: > Cc: Rob, devicetree ML > > On 31.05.2020 01:57, Chanwoo Choi wrote: >> On Sat, May 30, 2020 at 1:33 AM Sylwester Nawrocki >> <s.nawrocki@samsung.com> wrote: >>> >>> This patch adds registration of a child platform device for the exynos >>> interconnect driver. It is assumed that the interconnect provider will >>> only be needed when #interconnect-cells property is present in the bus >>> DT node, hence the child device will be created only when such a property >>> is present. >>> >>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> >>> >>> Changes for v5: >>> - new patch. >>> --- >>> drivers/devfreq/exynos-bus.c | 17 +++++++++++++++++ >>> 1 file changed, 17 insertions(+) >>> >>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c >>> index 8fa8eb5..856e37d 100644 >>> --- a/drivers/devfreq/exynos-bus.c >>> +++ b/drivers/devfreq/exynos-bus.c >>> @@ -24,6 +24,7 @@ >>> >>> struct exynos_bus { >>> struct device *dev; >>> + struct platform_device *icc_pdev; >>> >>> struct devfreq *devfreq; >>> struct devfreq_event_dev **edev; >>> @@ -156,6 +157,8 @@ static void exynos_bus_exit(struct device *dev) >>> if (ret < 0) >>> dev_warn(dev, "failed to disable the devfreq-event devices\n"); >>> >>> + platform_device_unregister(bus->icc_pdev); >>> + >>> dev_pm_opp_of_remove_table(dev); >>> clk_disable_unprepare(bus->clk); >>> if (bus->opp_table) { >>> @@ -168,6 +171,8 @@ static void exynos_bus_passive_exit(struct device *dev) >>> { >>> struct exynos_bus *bus = dev_get_drvdata(dev); >>> >>> + platform_device_unregister(bus->icc_pdev); >>> + >>> dev_pm_opp_of_remove_table(dev); >>> clk_disable_unprepare(bus->clk); >>> } >>> @@ -431,6 +436,18 @@ static int exynos_bus_probe(struct platform_device *pdev) >>> if (ret < 0) >>> goto err; >>> >>> + /* Create child platform device for the interconnect provider */ >>> + if (of_get_property(dev->of_node, "#interconnect-cells", NULL)) { >>> + bus->icc_pdev = platform_device_register_data( >>> + dev, "exynos-generic-icc", >>> + PLATFORM_DEVID_AUTO, NULL, 0); >>> + >>> + if (IS_ERR(bus->icc_pdev)) { >>> + ret = PTR_ERR(bus->icc_pdev); >>> + goto err; >>> + } >>> + } >>> + >>> max_state = bus->devfreq->profile->max_state; >>> min_freq = (bus->devfreq->profile->freq_table[0] / 1000); >>> max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000); >>> -- >>> 2.7.4 >>> >> >> It looks like very similar like the registering the interconnect >> device of imx-bus.c >> and I already reviewed and agreed this approach. >> >> Acked-by: Chanwoo Choi <cw00.choi@samsung.com> >> >> nitpick: IMHO, I think that 'exynos-icc' is proper and simple without >> 'generic' word. >> If we need to add new icc compatible int the future, we will add >> 'exynosXXXX-icc' new compatible. >> But, I'm not forcing it. just opinion. Anyway, I agree this approach. > > Thanks for review. I will change the name to exynos-icc in next version, > as I commented at other patch, it is not part of any DT binding, > it is just for device/driver matching between devfreq and interconnect. Thanks. I have not any objection to use either 'exynos-generic-icc' or 'exynos-icc'. It is just my opinion. And on next version, please add linux-pm mailing list to Cc.
diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c index 8fa8eb5..856e37d 100644 --- a/drivers/devfreq/exynos-bus.c +++ b/drivers/devfreq/exynos-bus.c @@ -24,6 +24,7 @@ struct exynos_bus { struct device *dev; + struct platform_device *icc_pdev; struct devfreq *devfreq; struct devfreq_event_dev **edev; @@ -156,6 +157,8 @@ static void exynos_bus_exit(struct device *dev) if (ret < 0) dev_warn(dev, "failed to disable the devfreq-event devices\n"); + platform_device_unregister(bus->icc_pdev); + dev_pm_opp_of_remove_table(dev); clk_disable_unprepare(bus->clk); if (bus->opp_table) { @@ -168,6 +171,8 @@ static void exynos_bus_passive_exit(struct device *dev) { struct exynos_bus *bus = dev_get_drvdata(dev); + platform_device_unregister(bus->icc_pdev); + dev_pm_opp_of_remove_table(dev); clk_disable_unprepare(bus->clk); } @@ -431,6 +436,18 @@ static int exynos_bus_probe(struct platform_device *pdev) if (ret < 0) goto err; + /* Create child platform device for the interconnect provider */ + if (of_get_property(dev->of_node, "#interconnect-cells", NULL)) { + bus->icc_pdev = platform_device_register_data( + dev, "exynos-generic-icc", + PLATFORM_DEVID_AUTO, NULL, 0); + + if (IS_ERR(bus->icc_pdev)) { + ret = PTR_ERR(bus->icc_pdev); + goto err; + } + } + max_state = bus->devfreq->profile->max_state; min_freq = (bus->devfreq->profile->freq_table[0] / 1000); max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
This patch adds registration of a child platform device for the exynos interconnect driver. It is assumed that the interconnect provider will only be needed when #interconnect-cells property is present in the bus DT node, hence the child device will be created only when such a property is present. Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> Changes for v5: - new patch. --- drivers/devfreq/exynos-bus.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)