Re: [PATCH v4 1/9] devfreq: exynos: Add generic exynos memory bus frequency driver

[MyungJoo Ham]

>   
>  This patch adds the generic exynos bus frequency driver for memory bus
> with DEVFREQ framework. The Samsung Exynos SoCs have the common architecture
> for memory bus between DRAM memory and MMC/sub IP in SoC. This driver can
> support the memory bus frequency driver for Exynos SoCs.
> 
> Each memory bus block has a clock for memory bus speed and frequency
> table which is changed according to the utilization of memory bus on runtime.
> And then each memory bus group has the one more memory bus blocks and
> OPP table (including frequency and voltage), regulator, devfreq-event
> devices.
> 
> There are a little difference about the number of memory bus because each Exynos
> SoC have the different sub-IP and different memory bus speed. In spite of this
> difference among Exynos SoCs, we can support almost Exynos SoC by adding
> unique data of memory bus to devicetree file.
> 
> Cc: Myungjoo Ham <myungjoo.ham@xxxxxxxxxxx>
> Cc: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> Cc: Kukjin Kim <kgene@xxxxxxxxxx>
> Signed-off-by: Chanwoo Choi <cw00.choi@xxxxxxxxxxx>
> ---
>  drivers/devfreq/Kconfig             |  15 +
>  drivers/devfreq/Makefile            |   1 +
>  drivers/devfreq/exynos/Makefile     |   1 +
>  drivers/devfreq/exynos/exynos-bus.c | 598 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 615 insertions(+)
>  create mode 100644 drivers/devfreq/exynos/exynos-bus.c
> 

[]

> +static void exynos_bus_exit(struct device *dev)
> +{
> +	struct exynos_memory_bus *bus = dev_get_drvdata(dev);
> +	int i, ret;
> +
> +	ret = exynos_bus_disable_edev(bus);
> +	if (ret < 0)
> +		dev_warn(dev, "failed to disable the devfreq-event devices\n");
> +
> +	for (i = 0; i < bus->block_count; i++)
> +		clk_disable_unprepare(bus->block[i].clk);
> +
> +	if (regulator_is_enabled(bus->regulator))
> +		regulator_disable(bus->regulator);

This is_enabled check is itchy.

Why do you need this here?
Please let me know what kind of errors here.
(note that this may simply hide errors made by other drivers)

Adding this condition does not introduce additional error, but
could you please let me know why it is here? This is supposed to be
paired with probe.


Except this point (valid if addressed or {explained and understood}),
Acked-by: MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx>


Cheers,
MyungJoo

> +
> +	of_free_opp_table(dev);
> +}
> +
> +static struct devfreq_dev_profile exynos_memory_bus_profile = {
> +	.polling_ms	= 100,
> +	.target		= exynos_bus_target,
> +	.get_dev_status	= exynos_bus_get_dev_status,
> +	.exit		= exynos_bus_exit,
> +};
> +

[]