Reuse opp core code for setting bus clock and voltage. As a side
effect this allow useage of coupled regulators feature (required
for boards using Exynos5422/5800 SoCs) because dev_pm_opp_set_rate()
uses regulator_set_voltage_triplet() for setting regulator voltage
while the old code used regulator_set_voltage_tol() with fixed
tolerance. This patch also removes no longer needed parsing of DT
property "exynos,voltage-tolerance" (no Exynos devfreq DT node uses
it).
Signed-off-by: Kamil Konieczny <[email protected]>
---
drivers/devfreq/exynos-bus.c | 143 +++++++++--------------------------
1 file changed, 37 insertions(+), 106 deletions(-)
diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index f391044aa39d..c2147b0912a0 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -25,7 +25,6 @@
#include <linux/slab.h>
#define DEFAULT_SATURATION_RATIO 40
-#define DEFAULT_VOLTAGE_TOLERANCE 2
struct exynos_bus {
struct device *dev;
@@ -37,9 +36,9 @@ struct exynos_bus {
unsigned long curr_freq;
- struct regulator *regulator;
+ struct opp_table *opp_table;
+
struct clk *clk;
- unsigned int voltage_tolerance;
unsigned int ratio;
};
@@ -99,56 +98,23 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
{
struct exynos_bus *bus = dev_get_drvdata(dev);
struct dev_pm_opp *new_opp;
- unsigned long old_freq, new_freq, new_volt, tol;
int ret = 0;
- /* Get new opp-bus instance according to new bus clock */
+ /* Get correct frequency for bus. */
new_opp = devfreq_recommended_opp(dev, freq, flags);
if (IS_ERR(new_opp)) {
dev_err(dev, "failed to get recommended opp instance\n");
return PTR_ERR(new_opp);
}
- new_freq = dev_pm_opp_get_freq(new_opp);
- new_volt = dev_pm_opp_get_voltage(new_opp);
dev_pm_opp_put(new_opp);
- old_freq = bus->curr_freq;
-
- if (old_freq == new_freq)
- return 0;
- tol = new_volt * bus->voltage_tolerance / 100;
-
/* Change voltage and frequency according to new OPP level */
mutex_lock(&bus->lock);
+ ret = dev_pm_opp_set_rate(dev, *freq);
+ if (!ret)
+ bus->curr_freq = *freq;
- if (old_freq < new_freq) {
- ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
- if (ret < 0) {
- dev_err(bus->dev, "failed to set voltage\n");
- goto out;
- }
- }
-
- ret = clk_set_rate(bus->clk, new_freq);
- if (ret < 0) {
- dev_err(dev, "failed to change clock of bus\n");
- clk_set_rate(bus->clk, old_freq);
- goto out;
- }
-
- if (old_freq > new_freq) {
- ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
- if (ret < 0) {
- dev_err(bus->dev, "failed to set voltage\n");
- goto out;
- }
- }
- bus->curr_freq = new_freq;
-
- dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
- old_freq, new_freq, clk_get_rate(bus->clk));
-out:
mutex_unlock(&bus->lock);
return ret;
@@ -195,8 +161,8 @@ static void exynos_bus_exit(struct device *dev)
dev_warn(dev, "failed to disable the devfreq-event devices\n");
clk_disable_unprepare(bus->clk);
- if (bus->regulator)
- regulator_disable(bus->regulator);
+ if (bus->opp_table)
+ dev_pm_opp_put_regulators(bus->opp_table);
dev_pm_opp_of_remove_table(dev);
}
@@ -209,39 +175,23 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
{
struct exynos_bus *bus = dev_get_drvdata(dev);
struct dev_pm_opp *new_opp;
- unsigned long old_freq, new_freq;
- int ret = 0;
+ int ret;
- /* Get new opp-bus instance according to new bus clock */
+ /* Get correct frequency for bus. */
new_opp = devfreq_recommended_opp(dev, freq, flags);
if (IS_ERR(new_opp)) {
dev_err(dev, "failed to get recommended opp instance\n");
return PTR_ERR(new_opp);
}
- new_freq = dev_pm_opp_get_freq(new_opp);
dev_pm_opp_put(new_opp);
- old_freq = bus->curr_freq;
-
- if (old_freq == new_freq)
- return 0;
-
/* Change the frequency according to new OPP level */
mutex_lock(&bus->lock);
+ ret = dev_pm_opp_set_rate(dev, *freq);
+ if (!ret)
+ bus->curr_freq = *freq;
- ret = clk_set_rate(bus->clk, new_freq);
- if (ret < 0) {
- dev_err(dev, "failed to set the clock of bus\n");
- goto out;
- }
-
- *freq = new_freq;
- bus->curr_freq = new_freq;
-
- dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
- old_freq, new_freq, clk_get_rate(bus->clk));
-out:
mutex_unlock(&bus->lock);
return ret;
@@ -259,20 +209,9 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
struct exynos_bus *bus)
{
struct device *dev = bus->dev;
- int i, ret, count, size;
-
- /* Get the regulator to provide each bus with the power */
- bus->regulator = devm_regulator_get(dev, "vdd");
- if (IS_ERR(bus->regulator)) {
- dev_err(dev, "failed to get VDD regulator\n");
- return PTR_ERR(bus->regulator);
- }
-
- ret = regulator_enable(bus->regulator);
- if (ret < 0) {
- dev_err(dev, "failed to enable VDD regulator\n");
- return ret;
- }
+ struct opp_table *opp_table;
+ const char *vdd = "vdd";
+ int i, count, size;
/*
* Get the devfreq-event devices to get the current utilization of
@@ -281,26 +220,29 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
count = devfreq_event_get_edev_count(dev);
if (count < 0) {
dev_err(dev, "failed to get the count of devfreq-event dev\n");
- ret = count;
- goto err_regulator;
+ return count;
}
- bus->edev_count = count;
+ bus->edev_count = count;
size = sizeof(*bus->edev) * count;
bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
- if (!bus->edev) {
- ret = -ENOMEM;
- goto err_regulator;
- }
+ if (!bus->edev)
+ return -ENOMEM;
for (i = 0; i < count; i++) {
bus->edev[i] = devfreq_event_get_edev_by_phandle(dev, i);
- if (IS_ERR(bus->edev[i])) {
- ret = -EPROBE_DEFER;
- goto err_regulator;
- }
+ if (IS_ERR(bus->edev[i]))
+ return -EPROBE_DEFER;
+ }
+
+ opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
+ if (IS_ERR(opp_table)) {
+ i = PTR_ERR(opp_table);
+ dev_err(dev, "failed to set regulators %d\n", i);
+ return i;
}
+ bus->opp_table = opp_table;
/*
* Optionally, Get the saturation ratio according to Exynos SoC
* When measuring the utilization of each AXI bus with devfreq-event
@@ -314,16 +256,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
if (of_property_read_u32(np, "exynos,saturation-ratio", &bus->ratio))
bus->ratio = DEFAULT_SATURATION_RATIO;
- if (of_property_read_u32(np, "exynos,voltage-tolerance",
- &bus->voltage_tolerance))
- bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE;
-
return 0;
-
-err_regulator:
- regulator_disable(bus->regulator);
-
- return ret;
}
static int exynos_bus_parse_of(struct exynos_bus *bus)
@@ -414,12 +347,8 @@ static int exynos_bus_probe(struct platform_device *pdev)
/* Parse the device-tree to get the resource information */
ret = exynos_bus_parse_of(bus);
- if (ret < 0) {
- if (!passive)
- regulator_disable(bus->regulator);
-
- return ret;
- }
+ if (ret < 0)
+ goto err_reg;
if (passive)
goto passive;
@@ -512,10 +441,12 @@ static int exynos_bus_probe(struct platform_device *pdev)
err:
clk_disable_unprepare(bus->clk);
- if (!passive)
- regulator_disable(bus->regulator);
-
dev_pm_opp_of_remove_table(dev);
+err_reg:
+ if (bus->opp_table) {
+ dev_pm_opp_put_regulators(bus->opp_table);
+ bus->opp_table = NULL;
+ }
return ret;
}
--
2.22.0
Hi Kamil,
Looks good to me. But, I have some comment. Please check them.
After this patch, exynos_bus_target is perfectly same with
exynos_bus_passive_target. The exynos_bus_passive_target() could be removed.
On 19. 7. 20. 오전 12:05, [email protected] wrote:
> Reuse opp core code for setting bus clock and voltage. As a side
> effect this allow useage of coupled regulators feature (required
s/useage/usage ?
> for boards using Exynos5422/5800 SoCs) because dev_pm_opp_set_rate()
> uses regulator_set_voltage_triplet() for setting regulator voltage
> while the old code used regulator_set_voltage_tol() with fixed
> tolerance. This patch also removes no longer needed parsing of DT
> property "exynos,voltage-tolerance" (no Exynos devfreq DT node uses
> it).
>
> Signed-off-by: Kamil Konieczny <[email protected]>
> ---
> drivers/devfreq/exynos-bus.c | 143 +++++++++--------------------------
> 1 file changed, 37 insertions(+), 106 deletions(-)
>
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index f391044aa39d..c2147b0912a0 100644
> --- a/drivers/devfreq/exynos-bus.c
> +++ b/drivers/devfreq/exynos-bus.c
> @@ -25,7 +25,6 @@
> #include <linux/slab.h>
>
> #define DEFAULT_SATURATION_RATIO 40
> -#define DEFAULT_VOLTAGE_TOLERANCE 2
>
> struct exynos_bus {
> struct device *dev;
> @@ -37,9 +36,9 @@ struct exynos_bus {
>
> unsigned long curr_freq;
>
> - struct regulator *regulator;
> + struct opp_table *opp_table;
> +
> struct clk *clk;
> - unsigned int voltage_tolerance;
> unsigned int ratio;
> };
>
> @@ -99,56 +98,23 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
> {
> struct exynos_bus *bus = dev_get_drvdata(dev);
> struct dev_pm_opp *new_opp;
> - unsigned long old_freq, new_freq, new_volt, tol;
> int ret = 0;
>
> - /* Get new opp-bus instance according to new bus clock */
> + /* Get correct frequency for bus. */
> new_opp = devfreq_recommended_opp(dev, freq, flags);
> if (IS_ERR(new_opp)) {
> dev_err(dev, "failed to get recommended opp instance\n");
> return PTR_ERR(new_opp);
> }
>
> - new_freq = dev_pm_opp_get_freq(new_opp);
> - new_volt = dev_pm_opp_get_voltage(new_opp);
> dev_pm_opp_put(new_opp);
>
> - old_freq = bus->curr_freq;
> -
> - if (old_freq == new_freq)
> - return 0;
> - tol = new_volt * bus->voltage_tolerance / 100;
> -
> /* Change voltage and frequency according to new OPP level */
> mutex_lock(&bus->lock);
> + ret = dev_pm_opp_set_rate(dev, *freq);
> + if (!ret)
> + bus->curr_freq = *freq;
>
> - if (old_freq < new_freq) {
> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
> - if (ret < 0) {
> - dev_err(bus->dev, "failed to set voltage\n");
> - goto out;
> - }
> - }
> -
> - ret = clk_set_rate(bus->clk, new_freq);
> - if (ret < 0) {
> - dev_err(dev, "failed to change clock of bus\n");
> - clk_set_rate(bus->clk, old_freq);
> - goto out;
> - }
> -
> - if (old_freq > new_freq) {
> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
> - if (ret < 0) {
> - dev_err(bus->dev, "failed to set voltage\n");
> - goto out;
> - }
> - }
> - bus->curr_freq = new_freq;
> -
> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
> - old_freq, new_freq, clk_get_rate(bus->clk));
> -out:
> mutex_unlock(&bus->lock);
>
> return ret;
> @@ -195,8 +161,8 @@ static void exynos_bus_exit(struct device *dev)
> dev_warn(dev, "failed to disable the devfreq-event devices\n");
>
> clk_disable_unprepare(bus->clk);
> - if (bus->regulator)
> - regulator_disable(bus->regulator);
> + if (bus->opp_table)
> + dev_pm_opp_put_regulators(bus->opp_table);
>
> dev_pm_opp_of_remove_table(dev);
> }
> @@ -209,39 +175,23 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
> {
> struct exynos_bus *bus = dev_get_drvdata(dev);
> struct dev_pm_opp *new_opp;
> - unsigned long old_freq, new_freq;
> - int ret = 0;
> + int ret;
>
> - /* Get new opp-bus instance according to new bus clock */
> + /* Get correct frequency for bus. */
> new_opp = devfreq_recommended_opp(dev, freq, flags);
> if (IS_ERR(new_opp)) {
> dev_err(dev, "failed to get recommended opp instance\n");
> return PTR_ERR(new_opp);
> }
>
> - new_freq = dev_pm_opp_get_freq(new_opp);
> dev_pm_opp_put(new_opp);
>
> - old_freq = bus->curr_freq;
> -
> - if (old_freq == new_freq)
> - return 0;
> -
> /* Change the frequency according to new OPP level */
> mutex_lock(&bus->lock);
> + ret = dev_pm_opp_set_rate(dev, *freq);
> + if (!ret)
> + bus->curr_freq = *freq;
>
> - ret = clk_set_rate(bus->clk, new_freq);
> - if (ret < 0) {
> - dev_err(dev, "failed to set the clock of bus\n");
> - goto out;
> - }
> -
> - *freq = new_freq;
> - bus->curr_freq = new_freq;
> -
> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
> - old_freq, new_freq, clk_get_rate(bus->clk));
> -out:
> mutex_unlock(&bus->lock);
>
> return ret;
> @@ -259,20 +209,9 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
> struct exynos_bus *bus)
> {
> struct device *dev = bus->dev;
> - int i, ret, count, size;
> -
> - /* Get the regulator to provide each bus with the power */
> - bus->regulator = devm_regulator_get(dev, "vdd");
> - if (IS_ERR(bus->regulator)) {
> - dev_err(dev, "failed to get VDD regulator\n");
> - return PTR_ERR(bus->regulator);
> - }
> -
> - ret = regulator_enable(bus->regulator);
> - if (ret < 0) {
> - dev_err(dev, "failed to enable VDD regulator\n");
> - return ret;
> - }
> + struct opp_table *opp_table;
> + const char *vdd = "vdd";
> + int i, count, size;
>
> /*
> * Get the devfreq-event devices to get the current utilization of
> @@ -281,26 +220,29 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
> count = devfreq_event_get_edev_count(dev);
> if (count < 0) {
> dev_err(dev, "failed to get the count of devfreq-event dev\n");
> - ret = count;
> - goto err_regulator;
> + return count;
> }
> - bus->edev_count = count;
>
> + bus->edev_count = count;
> size = sizeof(*bus->edev) * count;
> bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
> - if (!bus->edev) {
> - ret = -ENOMEM;
> - goto err_regulator;
> - }
> + if (!bus->edev)
> + return -ENOMEM;
>
> for (i = 0; i < count; i++) {
> bus->edev[i] = devfreq_event_get_edev_by_phandle(dev, i);
> - if (IS_ERR(bus->edev[i])) {
> - ret = -EPROBE_DEFER;
> - goto err_regulator;
> - }
> + if (IS_ERR(bus->edev[i]))
> + return -EPROBE_DEFER;
> + }
> +
> + opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
> + if (IS_ERR(opp_table)) {
> + i = PTR_ERR(opp_table);
> + dev_err(dev, "failed to set regulators %d\n", i);
> + return i;
Maybe, you just used the 'i' defined variable instead of adding
new variable. But, I think that you better to add new variable
like 'err' for the readability. Or, jut use the 'PTR_ERR(opp_table)'
directly without any additional variable.
> }
>
> + bus->opp_table = opp_table;
Add blank line.
> /*
> * Optionally, Get the saturation ratio according to Exynos SoC
> * When measuring the utilization of each AXI bus with devfreq-event
> @@ -314,16 +256,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
> if (of_property_read_u32(np, "exynos,saturation-ratio", &bus->ratio))
> bus->ratio = DEFAULT_SATURATION_RATIO;
>
> - if (of_property_read_u32(np, "exynos,voltage-tolerance",
> - &bus->voltage_tolerance))
> - bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE;
> -
> return 0;
> -
> -err_regulator:
> - regulator_disable(bus->regulator);
> -
> - return ret;
> }
>
> static int exynos_bus_parse_of(struct exynos_bus *bus)
> @@ -414,12 +347,8 @@ static int exynos_bus_probe(struct platform_device *pdev)
>
> /* Parse the device-tree to get the resource information */
> ret = exynos_bus_parse_of(bus);
> - if (ret < 0) {
> - if (!passive)
> - regulator_disable(bus->regulator);
> -
> - return ret;
> - }
> + if (ret < 0)
> + goto err_reg;
>
> if (passive)
> goto passive;
> @@ -512,10 +441,12 @@ static int exynos_bus_probe(struct platform_device *pdev)
>
> err:
> clk_disable_unprepare(bus->clk);
> - if (!passive)
> - regulator_disable(bus->regulator);
> -
> dev_pm_opp_of_remove_table(dev);
This function removes the 'opp_table'. But, the below code
uses the 'opp_table' variable by dev_pm_opp_put_regulators().
To disable the regulator, you have to call dev_pm_opp_of_remove_table(dev)
after dev_pm_opp_put_regulators(bus->opp_table).
> +err_reg:
> + if (bus->opp_table) {
> + dev_pm_opp_put_regulators(bus->opp_table);
> + bus->opp_table = NULL;
> + }
>
> return ret;
> }
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
Hi Chanwoo,
On 25.07.2019 12:17, Chanwoo Choi wrote:
> Hi Kamil,
>
> Looks good to me. But, I have some comment. Please check them.
Thank you for review, please see answers below.
> After this patch, exynos_bus_target is perfectly same with
> exynos_bus_passive_target. The exynos_bus_passive_target() could be removed.
Ok, I will make it in separate patch to simplify review process.
> On 19. 7. 20. 오전 12:05, [email protected] wrote:
>> Reuse opp core code for setting bus clock and voltage. As a side
>> effect this allow useage of coupled regulators feature (required
>
> s/useage/usage ?
Good catch, I will change it.
>> for boards using Exynos5422/5800 SoCs) because dev_pm_opp_set_rate()
>> uses regulator_set_voltage_triplet() for setting regulator voltage
>> while the old code used regulator_set_voltage_tol() with fixed
>> tolerance. This patch also removes no longer needed parsing of DT
>> property "exynos,voltage-tolerance" (no Exynos devfreq DT node uses
>> it).
>>
>> Signed-off-by: Kamil Konieczny <[email protected]>
>> ---
>> drivers/devfreq/exynos-bus.c | 143 +++++++++--------------------------
>> 1 file changed, 37 insertions(+), 106 deletions(-)
>>
>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>> index f391044aa39d..c2147b0912a0 100644
>> --- a/drivers/devfreq/exynos-bus.c
>> +++ b/drivers/devfreq/exynos-bus.c
>> @@ -25,7 +25,6 @@
>> #include <linux/slab.h>
>>
>> #define DEFAULT_SATURATION_RATIO 40
>> -#define DEFAULT_VOLTAGE_TOLERANCE 2
>>
>> struct exynos_bus {
>> struct device *dev;
>> @@ -37,9 +36,9 @@ struct exynos_bus {
>>
>> unsigned long curr_freq;
>>
>> - struct regulator *regulator;
>> + struct opp_table *opp_table;
>> +
>> struct clk *clk;
>> - unsigned int voltage_tolerance;
>> unsigned int ratio;
>> };
>>
>> @@ -99,56 +98,23 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
>> {
>> struct exynos_bus *bus = dev_get_drvdata(dev);
>> struct dev_pm_opp *new_opp;
>> - unsigned long old_freq, new_freq, new_volt, tol;
>> int ret = 0;
>>
>> - /* Get new opp-bus instance according to new bus clock */
>> + /* Get correct frequency for bus. */
>> new_opp = devfreq_recommended_opp(dev, freq, flags);
>> if (IS_ERR(new_opp)) {
>> dev_err(dev, "failed to get recommended opp instance\n");
>> return PTR_ERR(new_opp);
>> }
>>
>> - new_freq = dev_pm_opp_get_freq(new_opp);
>> - new_volt = dev_pm_opp_get_voltage(new_opp);
>> dev_pm_opp_put(new_opp);
>>
>> - old_freq = bus->curr_freq;
>> -
>> - if (old_freq == new_freq)
>> - return 0;
>> - tol = new_volt * bus->voltage_tolerance / 100;
>> -
>> /* Change voltage and frequency according to new OPP level */
>> mutex_lock(&bus->lock);
>> + ret = dev_pm_opp_set_rate(dev, *freq);
>> + if (!ret)
>> + bus->curr_freq = *freq;
>>
>> - if (old_freq < new_freq) {
>> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>> - if (ret < 0) {
>> - dev_err(bus->dev, "failed to set voltage\n");
>> - goto out;
>> - }
>> - }
>> -
>> - ret = clk_set_rate(bus->clk, new_freq);
>> - if (ret < 0) {
>> - dev_err(dev, "failed to change clock of bus\n");
>> - clk_set_rate(bus->clk, old_freq);
>> - goto out;
>> - }
>> -
>> - if (old_freq > new_freq) {
>> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>> - if (ret < 0) {
>> - dev_err(bus->dev, "failed to set voltage\n");
>> - goto out;
>> - }
>> - }
>> - bus->curr_freq = new_freq;
>> -
>> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>> - old_freq, new_freq, clk_get_rate(bus->clk));
>> -out:
>> mutex_unlock(&bus->lock);
>>
>> return ret;
>> @@ -195,8 +161,8 @@ static void exynos_bus_exit(struct device *dev)
>> dev_warn(dev, "failed to disable the devfreq-event devices\n");
>>
>> clk_disable_unprepare(bus->clk);
>> - if (bus->regulator)
>> - regulator_disable(bus->regulator);
>> + if (bus->opp_table)
>> + dev_pm_opp_put_regulators(bus->opp_table);
>>
>> dev_pm_opp_of_remove_table(dev);
>> }
>> @@ -209,39 +175,23 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
>> {
>> struct exynos_bus *bus = dev_get_drvdata(dev);
>> struct dev_pm_opp *new_opp;
>> - unsigned long old_freq, new_freq;
>> - int ret = 0;
>> + int ret;
>>
>> - /* Get new opp-bus instance according to new bus clock */
>> + /* Get correct frequency for bus. */
>> new_opp = devfreq_recommended_opp(dev, freq, flags);
>> if (IS_ERR(new_opp)) {
>> dev_err(dev, "failed to get recommended opp instance\n");
>> return PTR_ERR(new_opp);
>> }
>>
>> - new_freq = dev_pm_opp_get_freq(new_opp);
>> dev_pm_opp_put(new_opp);
>>
>> - old_freq = bus->curr_freq;
>> -
>> - if (old_freq == new_freq)
>> - return 0;
>> -
>> /* Change the frequency according to new OPP level */
>> mutex_lock(&bus->lock);
>> + ret = dev_pm_opp_set_rate(dev, *freq);
>> + if (!ret)
>> + bus->curr_freq = *freq;
>>
>> - ret = clk_set_rate(bus->clk, new_freq);
>> - if (ret < 0) {
>> - dev_err(dev, "failed to set the clock of bus\n");
>> - goto out;
>> - }
>> -
>> - *freq = new_freq;
>> - bus->curr_freq = new_freq;
>> -
>> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>> - old_freq, new_freq, clk_get_rate(bus->clk));
>> -out:
>> mutex_unlock(&bus->lock);
>>
>> return ret;
>> @@ -259,20 +209,9 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>> struct exynos_bus *bus)
>> {
>> struct device *dev = bus->dev;
>> - int i, ret, count, size;
>> -
>> - /* Get the regulator to provide each bus with the power */
>> - bus->regulator = devm_regulator_get(dev, "vdd");
>> - if (IS_ERR(bus->regulator)) {
>> - dev_err(dev, "failed to get VDD regulator\n");
>> - return PTR_ERR(bus->regulator);
>> - }
>> -
>> - ret = regulator_enable(bus->regulator);
>> - if (ret < 0) {
>> - dev_err(dev, "failed to enable VDD regulator\n");
>> - return ret;
>> - }
>> + struct opp_table *opp_table;
>> + const char *vdd = "vdd";
>> + int i, count, size;
>>
>> /*
>> * Get the devfreq-event devices to get the current utilization of
>> @@ -281,26 +220,29 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>> count = devfreq_event_get_edev_count(dev);
>> if (count < 0) {
>> dev_err(dev, "failed to get the count of devfreq-event dev\n");
>> - ret = count;
>> - goto err_regulator;
>> + return count;
>> }
>> - bus->edev_count = count;
>>
>> + bus->edev_count = count;
>> size = sizeof(*bus->edev) * count;
>> bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
>> - if (!bus->edev) {
>> - ret = -ENOMEM;
>> - goto err_regulator;
>> - }
>> + if (!bus->edev)
>> + return -ENOMEM;
>>
>> for (i = 0; i < count; i++) {
>> bus->edev[i] = devfreq_event_get_edev_by_phandle(dev, i);
>> - if (IS_ERR(bus->edev[i])) {
>> - ret = -EPROBE_DEFER;
>> - goto err_regulator;
>> - }
>> + if (IS_ERR(bus->edev[i]))
>> + return -EPROBE_DEFER;
>> + }
>> +
>> + opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
>> + if (IS_ERR(opp_table)) {
>> + i = PTR_ERR(opp_table);
>> + dev_err(dev, "failed to set regulators %d\n", i);
>> + return i;
>
> Maybe, you just used the 'i' defined variable instead of adding
> new variable. But, I think that you better to add new variable
> like 'err' for the readability. Or, jut use the 'PTR_ERR(opp_table)'
> directly without any additional variable.
I will remove not related changes, so here I will reuse 'ret' variable.
>> }
>>
>> + bus->opp_table = opp_table;
>
> Add blank line.
OK
>> /*
>> * Optionally, Get the saturation ratio according to Exynos SoC
>> * When measuring the utilization of each AXI bus with devfreq-event
>> @@ -314,16 +256,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>> if (of_property_read_u32(np, "exynos,saturation-ratio", &bus->ratio))
>> bus->ratio = DEFAULT_SATURATION_RATIO;
>>
>> - if (of_property_read_u32(np, "exynos,voltage-tolerance",
>> - &bus->voltage_tolerance))
>> - bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE;
>> -
>> return 0;
>> -
>> -err_regulator:
>> - regulator_disable(bus->regulator);
>> -
>> - return ret;
>> }
>>
>> static int exynos_bus_parse_of(struct exynos_bus *bus)
>> @@ -414,12 +347,8 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>
>> /* Parse the device-tree to get the resource information */
>> ret = exynos_bus_parse_of(bus);
>> - if (ret < 0) {
>> - if (!passive)
>> - regulator_disable(bus->regulator);
>> -
>> - return ret;
>> - }
>> + if (ret < 0)
>> + goto err_reg;
>>
>> if (passive)
>> goto passive;
>> @@ -512,10 +441,12 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>
>> err:
>> clk_disable_unprepare(bus->clk);
>> - if (!passive)
>> - regulator_disable(bus->regulator);
>> -
>> dev_pm_opp_of_remove_table(dev);
>
> This function removes the 'opp_table'. But, the below code
> uses the 'opp_table' variable by dev_pm_opp_put_regulators().
>
> To disable the regulator, you have to call dev_pm_opp_of_remove_table(dev)
> after dev_pm_opp_put_regulators(bus->opp_table).
Regulators should be set before dev_pm_opp_add_table(), so exit sequence
should be in reverse order,
init order:
exynos_bus_parent_parse_of()
dev_pm_opp_set_regulators()
exynos_bus_parse_of()
clk_prepare_enable()
dev_pm_opp_of_add_table()
exit or error order:
dev_pm_opp_of_remove_table()
clk_disable_unprepare()
if (bus->opp_table)
dev_pm_opp_put_regulators(bus->opp_table);
I will send v4 soon.
>> +err_reg:
>> + if (bus->opp_table) {
>> + dev_pm_opp_put_regulators(bus->opp_table);
>> + bus->opp_table = NULL;
>> + }
>>
>> return ret;
>> }
--
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland
2019년 7월 25일 (목) 오후 11:19, Kamil Konieczny
<[email protected]>님이 작성:
>
> Hi Chanwoo,
>
> On 25.07.2019 12:17, Chanwoo Choi wrote:
> > Hi Kamil,
> >
> > Looks good to me. But, I have some comment. Please check them.
>
> Thank you for review, please see answers below.
>
> > After this patch, exynos_bus_target is perfectly same with
> > exynos_bus_passive_target. The exynos_bus_passive_target() could be removed.
>
> Ok, I will make it in separate patch to simplify review process.
I think you can just modify this patch without any separate patch.
> > On 19. 7. 20. 오전 12:05, [email protected] wrote:
> >> Reuse opp core code for setting bus clock and voltage. As a side
> >> effect this allow useage of coupled regulators feature (required
> >
> > s/useage/usage ?
>
> Good catch, I will change it.
>
> >> for boards using Exynos5422/5800 SoCs) because dev_pm_opp_set_rate()
> >> uses regulator_set_voltage_triplet() for setting regulator voltage
> >> while the old code used regulator_set_voltage_tol() with fixed
> >> tolerance. This patch also removes no longer needed parsing of DT
> >> property "exynos,voltage-tolerance" (no Exynos devfreq DT node uses
> >> it).
> >>
> >> Signed-off-by: Kamil Konieczny <[email protected]>
> >> ---
> >> drivers/devfreq/exynos-bus.c | 143 +++++++++--------------------------
> >> 1 file changed, 37 insertions(+), 106 deletions(-)
> >>
> >> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> >> index f391044aa39d..c2147b0912a0 100644
> >> --- a/drivers/devfreq/exynos-bus.c
> >> +++ b/drivers/devfreq/exynos-bus.c
> >> @@ -25,7 +25,6 @@
> >> #include <linux/slab.h>
> >>
> >> #define DEFAULT_SATURATION_RATIO 40
> >> -#define DEFAULT_VOLTAGE_TOLERANCE 2
> >>
> >> struct exynos_bus {
> >> struct device *dev;
> >> @@ -37,9 +36,9 @@ struct exynos_bus {
> >>
> >> unsigned long curr_freq;
> >>
> >> - struct regulator *regulator;
> >> + struct opp_table *opp_table;
> >> +
> >> struct clk *clk;
> >> - unsigned int voltage_tolerance;
> >> unsigned int ratio;
> >> };
> >>
> >> @@ -99,56 +98,23 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
> >> {
> >> struct exynos_bus *bus = dev_get_drvdata(dev);
> >> struct dev_pm_opp *new_opp;
> >> - unsigned long old_freq, new_freq, new_volt, tol;
> >> int ret = 0;
> >>
> >> - /* Get new opp-bus instance according to new bus clock */
> >> + /* Get correct frequency for bus. */
> >> new_opp = devfreq_recommended_opp(dev, freq, flags);
> >> if (IS_ERR(new_opp)) {
> >> dev_err(dev, "failed to get recommended opp instance\n");
> >> return PTR_ERR(new_opp);
> >> }
> >>
> >> - new_freq = dev_pm_opp_get_freq(new_opp);
> >> - new_volt = dev_pm_opp_get_voltage(new_opp);
> >> dev_pm_opp_put(new_opp);
> >>
> >> - old_freq = bus->curr_freq;
> >> -
> >> - if (old_freq == new_freq)
> >> - return 0;
> >> - tol = new_volt * bus->voltage_tolerance / 100;
> >> -
> >> /* Change voltage and frequency according to new OPP level */
> >> mutex_lock(&bus->lock);
> >> + ret = dev_pm_opp_set_rate(dev, *freq);
> >> + if (!ret)
> >> + bus->curr_freq = *freq;
> >>
> >> - if (old_freq < new_freq) {
> >> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
> >> - if (ret < 0) {
> >> - dev_err(bus->dev, "failed to set voltage\n");
> >> - goto out;
> >> - }
> >> - }
> >> -
> >> - ret = clk_set_rate(bus->clk, new_freq);
> >> - if (ret < 0) {
> >> - dev_err(dev, "failed to change clock of bus\n");
> >> - clk_set_rate(bus->clk, old_freq);
> >> - goto out;
> >> - }
> >> -
> >> - if (old_freq > new_freq) {
> >> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
> >> - if (ret < 0) {
> >> - dev_err(bus->dev, "failed to set voltage\n");
> >> - goto out;
> >> - }
> >> - }
> >> - bus->curr_freq = new_freq;
> >> -
> >> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
> >> - old_freq, new_freq, clk_get_rate(bus->clk));
> >> -out:
> >> mutex_unlock(&bus->lock);
> >>
> >> return ret;
> >> @@ -195,8 +161,8 @@ static void exynos_bus_exit(struct device *dev)
> >> dev_warn(dev, "failed to disable the devfreq-event devices\n");
> >>
> >> clk_disable_unprepare(bus->clk);
> >> - if (bus->regulator)
> >> - regulator_disable(bus->regulator);
> >> + if (bus->opp_table)
> >> + dev_pm_opp_put_regulators(bus->opp_table);
> >>
> >> dev_pm_opp_of_remove_table(dev);
> >> }
> >> @@ -209,39 +175,23 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
> >> {
> >> struct exynos_bus *bus = dev_get_drvdata(dev);
> >> struct dev_pm_opp *new_opp;
> >> - unsigned long old_freq, new_freq;
> >> - int ret = 0;
> >> + int ret;
> >>
> >> - /* Get new opp-bus instance according to new bus clock */
> >> + /* Get correct frequency for bus. */
> >> new_opp = devfreq_recommended_opp(dev, freq, flags);
> >> if (IS_ERR(new_opp)) {
> >> dev_err(dev, "failed to get recommended opp instance\n");
> >> return PTR_ERR(new_opp);
> >> }
> >>
> >> - new_freq = dev_pm_opp_get_freq(new_opp);
> >> dev_pm_opp_put(new_opp);
> >>
> >> - old_freq = bus->curr_freq;
> >> -
> >> - if (old_freq == new_freq)
> >> - return 0;
> >> -
> >> /* Change the frequency according to new OPP level */
> >> mutex_lock(&bus->lock);
> >> + ret = dev_pm_opp_set_rate(dev, *freq);
> >> + if (!ret)
> >> + bus->curr_freq = *freq;
> >>
> >> - ret = clk_set_rate(bus->clk, new_freq);
> >> - if (ret < 0) {
> >> - dev_err(dev, "failed to set the clock of bus\n");
> >> - goto out;
> >> - }
> >> -
> >> - *freq = new_freq;
> >> - bus->curr_freq = new_freq;
> >> -
> >> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
> >> - old_freq, new_freq, clk_get_rate(bus->clk));
> >> -out:
> >> mutex_unlock(&bus->lock);
> >>
> >> return ret;
> >> @@ -259,20 +209,9 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
> >> struct exynos_bus *bus)
> >> {
> >> struct device *dev = bus->dev;
> >> - int i, ret, count, size;
> >> -
> >> - /* Get the regulator to provide each bus with the power */
> >> - bus->regulator = devm_regulator_get(dev, "vdd");
> >> - if (IS_ERR(bus->regulator)) {
> >> - dev_err(dev, "failed to get VDD regulator\n");
> >> - return PTR_ERR(bus->regulator);
> >> - }
> >> -
> >> - ret = regulator_enable(bus->regulator);
> >> - if (ret < 0) {
> >> - dev_err(dev, "failed to enable VDD regulator\n");
> >> - return ret;
> >> - }
> >> + struct opp_table *opp_table;
> >> + const char *vdd = "vdd";
> >> + int i, count, size;
> >>
> >> /*
> >> * Get the devfreq-event devices to get the current utilization of
> >> @@ -281,26 +220,29 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
> >> count = devfreq_event_get_edev_count(dev);
> >> if (count < 0) {
> >> dev_err(dev, "failed to get the count of devfreq-event dev\n");
> >> - ret = count;
> >> - goto err_regulator;
> >> + return count;
> >> }
> >> - bus->edev_count = count;
> >>
> >> + bus->edev_count = count;
> >> size = sizeof(*bus->edev) * count;
> >> bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
> >> - if (!bus->edev) {
> >> - ret = -ENOMEM;
> >> - goto err_regulator;
> >> - }
> >> + if (!bus->edev)
> >> + return -ENOMEM;
> >>
> >> for (i = 0; i < count; i++) {
> >> bus->edev[i] = devfreq_event_get_edev_by_phandle(dev, i);
> >> - if (IS_ERR(bus->edev[i])) {
> >> - ret = -EPROBE_DEFER;
> >> - goto err_regulator;
> >> - }
> >> + if (IS_ERR(bus->edev[i]))
> >> + return -EPROBE_DEFER;
> >> + }
> >> +
> >> + opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
> >> + if (IS_ERR(opp_table)) {
> >> + i = PTR_ERR(opp_table);
> >> + dev_err(dev, "failed to set regulators %d\n", i);
> >> + return i;
> >
> > Maybe, you just used the 'i' defined variable instead of adding
> > new variable. But, I think that you better to add new variable
> > like 'err' for the readability. Or, jut use the 'PTR_ERR(opp_table)'
> > directly without any additional variable.
>
> I will remove not related changes, so here I will reuse 'ret' variable.
>
> >> }
> >>
> >> + bus->opp_table = opp_table;
> >
> > Add blank line.
>
> OK
>
> >> /*
> >> * Optionally, Get the saturation ratio according to Exynos SoC
> >> * When measuring the utilization of each AXI bus with devfreq-event
> >> @@ -314,16 +256,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
> >> if (of_property_read_u32(np, "exynos,saturation-ratio", &bus->ratio))
> >> bus->ratio = DEFAULT_SATURATION_RATIO;
> >>
> >> - if (of_property_read_u32(np, "exynos,voltage-tolerance",
> >> - &bus->voltage_tolerance))
> >> - bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE;
> >> -
> >> return 0;
> >> -
> >> -err_regulator:
> >> - regulator_disable(bus->regulator);
> >> -
> >> - return ret;
> >> }
> >>
> >> static int exynos_bus_parse_of(struct exynos_bus *bus)
> >> @@ -414,12 +347,8 @@ static int exynos_bus_probe(struct platform_device *pdev)
> >>
> >> /* Parse the device-tree to get the resource information */
> >> ret = exynos_bus_parse_of(bus);
> >> - if (ret < 0) {
> >> - if (!passive)
> >> - regulator_disable(bus->regulator);
> >> -
> >> - return ret;
> >> - }
> >> + if (ret < 0)
> >> + goto err_reg;
> >>
> >> if (passive)
> >> goto passive;
> >> @@ -512,10 +441,12 @@ static int exynos_bus_probe(struct platform_device *pdev)
> >>
> >> err:
> >> clk_disable_unprepare(bus->clk);
> >> - if (!passive)
> >> - regulator_disable(bus->regulator);
> >> -
> >> dev_pm_opp_of_remove_table(dev);
> >
> > This function removes the 'opp_table'. But, the below code
> > uses the 'opp_table' variable by dev_pm_opp_put_regulators().
> >
> > To disable the regulator, you have to call dev_pm_opp_of_remove_table(dev)
> > after dev_pm_opp_put_regulators(bus->opp_table).
>
> Regulators should be set before dev_pm_opp_add_table(), so exit sequence
> should be in reverse order,
>
> init order:
>
> exynos_bus_parent_parse_of()
> dev_pm_opp_set_regulators()
> exynos_bus_parse_of()
> clk_prepare_enable()
> dev_pm_opp_of_add_table()
>
> exit or error order:
>
> dev_pm_opp_of_remove_table()
> clk_disable_unprepare()
> if (bus->opp_table)
> dev_pm_opp_put_regulators(bus->opp_table);
dev_pm_opp_of_remove_table() have to be called at the end of exit sequence
after disabling clock and put regulators. Because dev_pm_opp_of_remove_table()
frees the 'opp_table' pointer of device.
clk_disable_unprepare()
if (bus->opp_table)
dev_pm_opp_put_regulators(bus->opp_table);
dev_pm_opp_of_remove_table()
>
> I will send v4 soon.
>
> >> +err_reg:
> >> + if (bus->opp_table) {
> >> + dev_pm_opp_put_regulators(bus->opp_table);
> >> + bus->opp_table = NULL;
> >> + }
> >>
> >> return ret;
> >> }
>
> --
> Best regards,
> Kamil Konieczny
> Samsung R&D Institute Poland
>
--
Best Regards,
Chanwoo Choi
Hi,
On 25.07.2019 16:53, Chanwoo Choi wrote:
> 2019년 7월 25일 (목) 오후 11:19, Kamil Konieczny
> <[email protected]>님이 작성:
>>
>> Hi Chanwoo,
>>
>> On 25.07.2019 12:17, Chanwoo Choi wrote:
>>> Hi Kamil,
>>>
>>> Looks good to me. But, I have some comment. Please check them.
>>
>> Thank you for review, please see answers below.
>>
>>> After this patch, exynos_bus_target is perfectly same with
>>> exynos_bus_passive_target. The exynos_bus_passive_target() could be removed.
>>
>> Ok, I will make it in separate patch to simplify review process.
>
> I think you can just modify this patch without any separate patch.
Do you want me to send v5 with patch 5 squashed in patch 3 ?
>>> On 19. 7. 20. 오전 12:05, [email protected] wrote:
>>>> Reuse opp core code for setting bus clock and voltage. As a side
>>>> effect this allow useage of coupled regulators feature (required
>>>
>>> s/useage/usage ?
>>
>> Good catch, I will change it.
>>
>>>> for boards using Exynos5422/5800 SoCs) because dev_pm_opp_set_rate()
>>>> uses regulator_set_voltage_triplet() for setting regulator voltage
>>>> while the old code used regulator_set_voltage_tol() with fixed
>>>> tolerance. This patch also removes no longer needed parsing of DT
>>>> property "exynos,voltage-tolerance" (no Exynos devfreq DT node uses
>>>> it).
>>>>
>>>> Signed-off-by: Kamil Konieczny <[email protected]>
>>>> ---
>>>> drivers/devfreq/exynos-bus.c | 143 +++++++++--------------------------
>>>> 1 file changed, 37 insertions(+), 106 deletions(-)
>>>>
>>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>>> index f391044aa39d..c2147b0912a0 100644
>>>> --- a/drivers/devfreq/exynos-bus.c
>>>> +++ b/drivers/devfreq/exynos-bus.c
>>>> @@ -25,7 +25,6 @@
>>>> #include <linux/slab.h>
>>>>
>>>> #define DEFAULT_SATURATION_RATIO 40
>>>> -#define DEFAULT_VOLTAGE_TOLERANCE 2
>>>>
>>>> struct exynos_bus {
>>>> struct device *dev;
>>>> @@ -37,9 +36,9 @@ struct exynos_bus {
>>>>
>>>> unsigned long curr_freq;
>>>>
>>>> - struct regulator *regulator;
>>>> + struct opp_table *opp_table;
>>>> +
>>>> struct clk *clk;
>>>> - unsigned int voltage_tolerance;
>>>> unsigned int ratio;
>>>> };
>>>>
>>>> @@ -99,56 +98,23 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
>>>> {
>>>> struct exynos_bus *bus = dev_get_drvdata(dev);
>>>> struct dev_pm_opp *new_opp;
>>>> - unsigned long old_freq, new_freq, new_volt, tol;
>>>> int ret = 0;
>>>>
>>>> - /* Get new opp-bus instance according to new bus clock */
>>>> + /* Get correct frequency for bus. */
>>>> new_opp = devfreq_recommended_opp(dev, freq, flags);
>>>> if (IS_ERR(new_opp)) {
>>>> dev_err(dev, "failed to get recommended opp instance\n");
>>>> return PTR_ERR(new_opp);
>>>> }
>>>>
>>>> - new_freq = dev_pm_opp_get_freq(new_opp);
>>>> - new_volt = dev_pm_opp_get_voltage(new_opp);
>>>> dev_pm_opp_put(new_opp);
>>>>
>>>> - old_freq = bus->curr_freq;
>>>> -
>>>> - if (old_freq == new_freq)
>>>> - return 0;
>>>> - tol = new_volt * bus->voltage_tolerance / 100;
>>>> -
>>>> /* Change voltage and frequency according to new OPP level */
>>>> mutex_lock(&bus->lock);
>>>> + ret = dev_pm_opp_set_rate(dev, *freq);
>>>> + if (!ret)
>>>> + bus->curr_freq = *freq;
>>>>
>>>> - if (old_freq < new_freq) {
>>>> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>>>> - if (ret < 0) {
>>>> - dev_err(bus->dev, "failed to set voltage\n");
>>>> - goto out;
>>>> - }
>>>> - }
>>>> -
>>>> - ret = clk_set_rate(bus->clk, new_freq);
>>>> - if (ret < 0) {
>>>> - dev_err(dev, "failed to change clock of bus\n");
>>>> - clk_set_rate(bus->clk, old_freq);
>>>> - goto out;
>>>> - }
>>>> -
>>>> - if (old_freq > new_freq) {
>>>> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>>>> - if (ret < 0) {
>>>> - dev_err(bus->dev, "failed to set voltage\n");
>>>> - goto out;
>>>> - }
>>>> - }
>>>> - bus->curr_freq = new_freq;
>>>> -
>>>> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>>>> - old_freq, new_freq, clk_get_rate(bus->clk));
>>>> -out:
>>>> mutex_unlock(&bus->lock);
>>>>
>>>> return ret;
>>>> @@ -195,8 +161,8 @@ static void exynos_bus_exit(struct device *dev)
>>>> dev_warn(dev, "failed to disable the devfreq-event devices\n");
>>>>
>>>> clk_disable_unprepare(bus->clk);
>>>> - if (bus->regulator)
>>>> - regulator_disable(bus->regulator);
>>>> + if (bus->opp_table)
>>>> + dev_pm_opp_put_regulators(bus->opp_table);
>>>>
>>>> dev_pm_opp_of_remove_table(dev);
>>>> }
>>>> @@ -209,39 +175,23 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
>>>> {
>>>> struct exynos_bus *bus = dev_get_drvdata(dev);
>>>> struct dev_pm_opp *new_opp;
>>>> - unsigned long old_freq, new_freq;
>>>> - int ret = 0;
>>>> + int ret;
>>>>
>>>> - /* Get new opp-bus instance according to new bus clock */
>>>> + /* Get correct frequency for bus. */
>>>> new_opp = devfreq_recommended_opp(dev, freq, flags);
>>>> if (IS_ERR(new_opp)) {
>>>> dev_err(dev, "failed to get recommended opp instance\n");
>>>> return PTR_ERR(new_opp);
>>>> }
>>>>
>>>> - new_freq = dev_pm_opp_get_freq(new_opp);
>>>> dev_pm_opp_put(new_opp);
>>>>
>>>> - old_freq = bus->curr_freq;
>>>> -
>>>> - if (old_freq == new_freq)
>>>> - return 0;
>>>> -
>>>> /* Change the frequency according to new OPP level */
>>>> mutex_lock(&bus->lock);
>>>> + ret = dev_pm_opp_set_rate(dev, *freq);
>>>> + if (!ret)
>>>> + bus->curr_freq = *freq;
>>>>
>>>> - ret = clk_set_rate(bus->clk, new_freq);
>>>> - if (ret < 0) {
>>>> - dev_err(dev, "failed to set the clock of bus\n");
>>>> - goto out;
>>>> - }
>>>> -
>>>> - *freq = new_freq;
>>>> - bus->curr_freq = new_freq;
>>>> -
>>>> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>>>> - old_freq, new_freq, clk_get_rate(bus->clk));
>>>> -out:
>>>> mutex_unlock(&bus->lock);
>>>>
>>>> return ret;
>>>> @@ -259,20 +209,9 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>> struct exynos_bus *bus)
>>>> {
>>>> struct device *dev = bus->dev;
>>>> - int i, ret, count, size;
>>>> -
>>>> - /* Get the regulator to provide each bus with the power */
>>>> - bus->regulator = devm_regulator_get(dev, "vdd");
>>>> - if (IS_ERR(bus->regulator)) {
>>>> - dev_err(dev, "failed to get VDD regulator\n");
>>>> - return PTR_ERR(bus->regulator);
>>>> - }
>>>> -
>>>> - ret = regulator_enable(bus->regulator);
>>>> - if (ret < 0) {
>>>> - dev_err(dev, "failed to enable VDD regulator\n");
>>>> - return ret;
>>>> - }
>>>> + struct opp_table *opp_table;
>>>> + const char *vdd = "vdd";
>>>> + int i, count, size;
>>>>
>>>> /*
>>>> * Get the devfreq-event devices to get the current utilization of
>>>> @@ -281,26 +220,29 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>> count = devfreq_event_get_edev_count(dev);
>>>> if (count < 0) {
>>>> dev_err(dev, "failed to get the count of devfreq-event dev\n");
>>>> - ret = count;
>>>> - goto err_regulator;
>>>> + return count;
>>>> }
>>>> - bus->edev_count = count;
>>>>
>>>> + bus->edev_count = count;
>>>> size = sizeof(*bus->edev) * count;
>>>> bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
>>>> - if (!bus->edev) {
>>>> - ret = -ENOMEM;
>>>> - goto err_regulator;
>>>> - }
>>>> + if (!bus->edev)
>>>> + return -ENOMEM;
>>>>
>>>> for (i = 0; i < count; i++) {
>>>> bus->edev[i] = devfreq_event_get_edev_by_phandle(dev, i);
>>>> - if (IS_ERR(bus->edev[i])) {
>>>> - ret = -EPROBE_DEFER;
>>>> - goto err_regulator;
>>>> - }
>>>> + if (IS_ERR(bus->edev[i]))
>>>> + return -EPROBE_DEFER;
>>>> + }
>>>> +
>>>> + opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
>>>> + if (IS_ERR(opp_table)) {
>>>> + i = PTR_ERR(opp_table);
>>>> + dev_err(dev, "failed to set regulators %d\n", i);
>>>> + return i;
>>>
>>> Maybe, you just used the 'i' defined variable instead of adding
>>> new variable. But, I think that you better to add new variable
>>> like 'err' for the readability. Or, jut use the 'PTR_ERR(opp_table)'
>>> directly without any additional variable.
>>
>> I will remove not related changes, so here I will reuse 'ret' variable.
>>
>>>> }
>>>>
>>>> + bus->opp_table = opp_table;
>>>
>>> Add blank line.
>>
>> OK
>>
>>>> /*
>>>> * Optionally, Get the saturation ratio according to Exynos SoC
>>>> * When measuring the utilization of each AXI bus with devfreq-event
>>>> @@ -314,16 +256,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>> if (of_property_read_u32(np, "exynos,saturation-ratio", &bus->ratio))
>>>> bus->ratio = DEFAULT_SATURATION_RATIO;
>>>>
>>>> - if (of_property_read_u32(np, "exynos,voltage-tolerance",
>>>> - &bus->voltage_tolerance))
>>>> - bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE;
>>>> -
>>>> return 0;
>>>> -
>>>> -err_regulator:
>>>> - regulator_disable(bus->regulator);
>>>> -
>>>> - return ret;
>>>> }
>>>>
>>>> static int exynos_bus_parse_of(struct exynos_bus *bus)
>>>> @@ -414,12 +347,8 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>
>>>> /* Parse the device-tree to get the resource information */
>>>> ret = exynos_bus_parse_of(bus);
>>>> - if (ret < 0) {
>>>> - if (!passive)
>>>> - regulator_disable(bus->regulator);
>>>> -
>>>> - return ret;
>>>> - }
>>>> + if (ret < 0)
>>>> + goto err_reg;
>>>>
>>>> if (passive)
>>>> goto passive;
>>>> @@ -512,10 +441,12 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>
>>>> err:
>>>> clk_disable_unprepare(bus->clk);
>>>> - if (!passive)
>>>> - regulator_disable(bus->regulator);
>>>> -
>>>> dev_pm_opp_of_remove_table(dev);
>>>
>>> This function removes the 'opp_table'. But, the below code
>>> uses the 'opp_table' variable by dev_pm_opp_put_regulators().
>>>
>>> To disable the regulator, you have to call dev_pm_opp_of_remove_table(dev)
>>> after dev_pm_opp_put_regulators(bus->opp_table).
>>
>> Regulators should be set before dev_pm_opp_add_table(), so exit sequence
>> should be in reverse order,
>>
>> init order:
>>
>> exynos_bus_parent_parse_of()
>> dev_pm_opp_set_regulators()
>> exynos_bus_parse_of()
>> clk_prepare_enable()
>> dev_pm_opp_of_add_table()
>>
>> exit or error order:
>>
>> dev_pm_opp_of_remove_table()
>> clk_disable_unprepare()
>> if (bus->opp_table)
>> dev_pm_opp_put_regulators(bus->opp_table);
>
> dev_pm_opp_of_remove_table() have to be called at the end of exit sequence
> after disabling clock and put regulators. Because dev_pm_opp_of_remove_table()
> frees the 'opp_table' pointer of device.
>
> clk_disable_unprepare()
> if (bus->opp_table)
> dev_pm_opp_put_regulators(bus->opp_table);
> dev_pm_opp_of_remove_table()
The table is reference counted so it will be freed after count go to zero.
>> I will send v4 soon.
>>
>>>> +err_reg:
>>>> + if (bus->opp_table) {
>>>> + dev_pm_opp_put_regulators(bus->opp_table);
>>>> + bus->opp_table = NULL;
>>>> + }
>>>>
>>>> return ret;
>>>> }
--
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland
Hi,
On 19. 7. 26. 오전 12:15, Kamil Konieczny wrote:
> Hi,
>
> On 25.07.2019 16:53, Chanwoo Choi wrote:
>> 2019년 7월 25일 (목) 오후 11:19, Kamil Konieczny
>> <[email protected]>님이 작성:
>>>
>>> Hi Chanwoo,
>>>
>>> On 25.07.2019 12:17, Chanwoo Choi wrote:
>>>> Hi Kamil,
>>>>
>>>> Looks good to me. But, I have some comment. Please check them.
>>>
>>> Thank you for review, please see answers below.
>>>
>>>> After this patch, exynos_bus_target is perfectly same with
>>>> exynos_bus_passive_target. The exynos_bus_passive_target() could be removed.
>>>
>>> Ok, I will make it in separate patch to simplify review process.
>>
>> I think you can just modify this patch without any separate patch.
>
> Do you want me to send v5 with patch 5 squashed in patch 3 ?
Yes. In result, it made two functions same by this patch
So, I think that can combine them without separate patch.
>
>>>> On 19. 7. 20. 오전 12:05, [email protected] wrote:
>>>>> Reuse opp core code for setting bus clock and voltage. As a side
>>>>> effect this allow useage of coupled regulators feature (required
>>>>
>>>> s/useage/usage ?
>>>
>>> Good catch, I will change it.
>>>
>>>>> for boards using Exynos5422/5800 SoCs) because dev_pm_opp_set_rate()
>>>>> uses regulator_set_voltage_triplet() for setting regulator voltage
>>>>> while the old code used regulator_set_voltage_tol() with fixed
>>>>> tolerance. This patch also removes no longer needed parsing of DT
>>>>> property "exynos,voltage-tolerance" (no Exynos devfreq DT node uses
>>>>> it).
>>>>>
>>>>> Signed-off-by: Kamil Konieczny <[email protected]>
>>>>> ---
>>>>> drivers/devfreq/exynos-bus.c | 143 +++++++++--------------------------
>>>>> 1 file changed, 37 insertions(+), 106 deletions(-)
>>>>>
>>>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>>>> index f391044aa39d..c2147b0912a0 100644
>>>>> --- a/drivers/devfreq/exynos-bus.c
>>>>> +++ b/drivers/devfreq/exynos-bus.c
>>>>> @@ -25,7 +25,6 @@
>>>>> #include <linux/slab.h>
>>>>>
>>>>> #define DEFAULT_SATURATION_RATIO 40
>>>>> -#define DEFAULT_VOLTAGE_TOLERANCE 2
>>>>>
>>>>> struct exynos_bus {
>>>>> struct device *dev;
>>>>> @@ -37,9 +36,9 @@ struct exynos_bus {
>>>>>
>>>>> unsigned long curr_freq;
>>>>>
>>>>> - struct regulator *regulator;
>>>>> + struct opp_table *opp_table;
>>>>> +
>>>>> struct clk *clk;
>>>>> - unsigned int voltage_tolerance;
>>>>> unsigned int ratio;
>>>>> };
>>>>>
>>>>> @@ -99,56 +98,23 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
>>>>> {
>>>>> struct exynos_bus *bus = dev_get_drvdata(dev);
>>>>> struct dev_pm_opp *new_opp;
>>>>> - unsigned long old_freq, new_freq, new_volt, tol;
>>>>> int ret = 0;
>>>>>
>>>>> - /* Get new opp-bus instance according to new bus clock */
>>>>> + /* Get correct frequency for bus. */
>>>>> new_opp = devfreq_recommended_opp(dev, freq, flags);
>>>>> if (IS_ERR(new_opp)) {
>>>>> dev_err(dev, "failed to get recommended opp instance\n");
>>>>> return PTR_ERR(new_opp);
>>>>> }
>>>>>
>>>>> - new_freq = dev_pm_opp_get_freq(new_opp);
>>>>> - new_volt = dev_pm_opp_get_voltage(new_opp);
>>>>> dev_pm_opp_put(new_opp);
>>>>>
>>>>> - old_freq = bus->curr_freq;
>>>>> -
>>>>> - if (old_freq == new_freq)
>>>>> - return 0;
>>>>> - tol = new_volt * bus->voltage_tolerance / 100;
>>>>> -
>>>>> /* Change voltage and frequency according to new OPP level */
>>>>> mutex_lock(&bus->lock);
>>>>> + ret = dev_pm_opp_set_rate(dev, *freq);
>>>>> + if (!ret)
>>>>> + bus->curr_freq = *freq;
>>>>>
>>>>> - if (old_freq < new_freq) {
>>>>> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>>>>> - if (ret < 0) {
>>>>> - dev_err(bus->dev, "failed to set voltage\n");
>>>>> - goto out;
>>>>> - }
>>>>> - }
>>>>> -
>>>>> - ret = clk_set_rate(bus->clk, new_freq);
>>>>> - if (ret < 0) {
>>>>> - dev_err(dev, "failed to change clock of bus\n");
>>>>> - clk_set_rate(bus->clk, old_freq);
>>>>> - goto out;
>>>>> - }
>>>>> -
>>>>> - if (old_freq > new_freq) {
>>>>> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>>>>> - if (ret < 0) {
>>>>> - dev_err(bus->dev, "failed to set voltage\n");
>>>>> - goto out;
>>>>> - }
>>>>> - }
>>>>> - bus->curr_freq = new_freq;
>>>>> -
>>>>> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>>>>> - old_freq, new_freq, clk_get_rate(bus->clk));
>>>>> -out:
>>>>> mutex_unlock(&bus->lock);
>>>>>
>>>>> return ret;
>>>>> @@ -195,8 +161,8 @@ static void exynos_bus_exit(struct device *dev)
>>>>> dev_warn(dev, "failed to disable the devfreq-event devices\n");
>>>>>
>>>>> clk_disable_unprepare(bus->clk);
>>>>> - if (bus->regulator)
>>>>> - regulator_disable(bus->regulator);
>>>>> + if (bus->opp_table)
>>>>> + dev_pm_opp_put_regulators(bus->opp_table);
>>>>>
>>>>> dev_pm_opp_of_remove_table(dev);
>>>>> }
>>>>> @@ -209,39 +175,23 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
>>>>> {
>>>>> struct exynos_bus *bus = dev_get_drvdata(dev);
>>>>> struct dev_pm_opp *new_opp;
>>>>> - unsigned long old_freq, new_freq;
>>>>> - int ret = 0;
>>>>> + int ret;
>>>>>
>>>>> - /* Get new opp-bus instance according to new bus clock */
>>>>> + /* Get correct frequency for bus. */
>>>>> new_opp = devfreq_recommended_opp(dev, freq, flags);
>>>>> if (IS_ERR(new_opp)) {
>>>>> dev_err(dev, "failed to get recommended opp instance\n");
>>>>> return PTR_ERR(new_opp);
>>>>> }
>>>>>
>>>>> - new_freq = dev_pm_opp_get_freq(new_opp);
>>>>> dev_pm_opp_put(new_opp);
>>>>>
>>>>> - old_freq = bus->curr_freq;
>>>>> -
>>>>> - if (old_freq == new_freq)
>>>>> - return 0;
>>>>> -
>>>>> /* Change the frequency according to new OPP level */
>>>>> mutex_lock(&bus->lock);
>>>>> + ret = dev_pm_opp_set_rate(dev, *freq);
>>>>> + if (!ret)
>>>>> + bus->curr_freq = *freq;
>>>>>
>>>>> - ret = clk_set_rate(bus->clk, new_freq);
>>>>> - if (ret < 0) {
>>>>> - dev_err(dev, "failed to set the clock of bus\n");
>>>>> - goto out;
>>>>> - }
>>>>> -
>>>>> - *freq = new_freq;
>>>>> - bus->curr_freq = new_freq;
>>>>> -
>>>>> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>>>>> - old_freq, new_freq, clk_get_rate(bus->clk));
>>>>> -out:
>>>>> mutex_unlock(&bus->lock);
>>>>>
>>>>> return ret;
>>>>> @@ -259,20 +209,9 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>>> struct exynos_bus *bus)
>>>>> {
>>>>> struct device *dev = bus->dev;
>>>>> - int i, ret, count, size;
>>>>> -
>>>>> - /* Get the regulator to provide each bus with the power */
>>>>> - bus->regulator = devm_regulator_get(dev, "vdd");
>>>>> - if (IS_ERR(bus->regulator)) {
>>>>> - dev_err(dev, "failed to get VDD regulator\n");
>>>>> - return PTR_ERR(bus->regulator);
>>>>> - }
>>>>> -
>>>>> - ret = regulator_enable(bus->regulator);
>>>>> - if (ret < 0) {
>>>>> - dev_err(dev, "failed to enable VDD regulator\n");
>>>>> - return ret;
>>>>> - }
>>>>> + struct opp_table *opp_table;
>>>>> + const char *vdd = "vdd";
>>>>> + int i, count, size;
>>>>>
>>>>> /*
>>>>> * Get the devfreq-event devices to get the current utilization of
>>>>> @@ -281,26 +220,29 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>>> count = devfreq_event_get_edev_count(dev);
>>>>> if (count < 0) {
>>>>> dev_err(dev, "failed to get the count of devfreq-event dev\n");
>>>>> - ret = count;
>>>>> - goto err_regulator;
>>>>> + return count;
>>>>> }
>>>>> - bus->edev_count = count;
>>>>>
>>>>> + bus->edev_count = count;
>>>>> size = sizeof(*bus->edev) * count;
>>>>> bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
>>>>> - if (!bus->edev) {
>>>>> - ret = -ENOMEM;
>>>>> - goto err_regulator;
>>>>> - }
>>>>> + if (!bus->edev)
>>>>> + return -ENOMEM;
>>>>>
>>>>> for (i = 0; i < count; i++) {
>>>>> bus->edev[i] = devfreq_event_get_edev_by_phandle(dev, i);
>>>>> - if (IS_ERR(bus->edev[i])) {
>>>>> - ret = -EPROBE_DEFER;
>>>>> - goto err_regulator;
>>>>> - }
>>>>> + if (IS_ERR(bus->edev[i]))
>>>>> + return -EPROBE_DEFER;
>>>>> + }
>>>>> +
>>>>> + opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
>>>>> + if (IS_ERR(opp_table)) {
>>>>> + i = PTR_ERR(opp_table);
>>>>> + dev_err(dev, "failed to set regulators %d\n", i);
>>>>> + return i;
>>>>
>>>> Maybe, you just used the 'i' defined variable instead of adding
>>>> new variable. But, I think that you better to add new variable
>>>> like 'err' for the readability. Or, jut use the 'PTR_ERR(opp_table)'
>>>> directly without any additional variable.
>>>
>>> I will remove not related changes, so here I will reuse 'ret' variable.
>>>
>>>>> }
>>>>>
>>>>> + bus->opp_table = opp_table;
>>>>
>>>> Add blank line.
>>>
>>> OK
>>>
>>>>> /*
>>>>> * Optionally, Get the saturation ratio according to Exynos SoC
>>>>> * When measuring the utilization of each AXI bus with devfreq-event
>>>>> @@ -314,16 +256,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>>> if (of_property_read_u32(np, "exynos,saturation-ratio", &bus->ratio))
>>>>> bus->ratio = DEFAULT_SATURATION_RATIO;
>>>>>
>>>>> - if (of_property_read_u32(np, "exynos,voltage-tolerance",
>>>>> - &bus->voltage_tolerance))
>>>>> - bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE;
>>>>> -
>>>>> return 0;
>>>>> -
>>>>> -err_regulator:
>>>>> - regulator_disable(bus->regulator);
>>>>> -
>>>>> - return ret;
>>>>> }
>>>>>
>>>>> static int exynos_bus_parse_of(struct exynos_bus *bus)
>>>>> @@ -414,12 +347,8 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>>
>>>>> /* Parse the device-tree to get the resource information */
>>>>> ret = exynos_bus_parse_of(bus);
>>>>> - if (ret < 0) {
>>>>> - if (!passive)
>>>>> - regulator_disable(bus->regulator);
>>>>> -
>>>>> - return ret;
>>>>> - }
>>>>> + if (ret < 0)
>>>>> + goto err_reg;
>>>>>
>>>>> if (passive)
>>>>> goto passive;
>>>>> @@ -512,10 +441,12 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>>
>>>>> err:
>>>>> clk_disable_unprepare(bus->clk);
>>>>> - if (!passive)
>>>>> - regulator_disable(bus->regulator);
>>>>> -
>>>>> dev_pm_opp_of_remove_table(dev);
>>>>
>>>> This function removes the 'opp_table'. But, the below code
>>>> uses the 'opp_table' variable by dev_pm_opp_put_regulators().
>>>>
>>>> To disable the regulator, you have to call dev_pm_opp_of_remove_table(dev)
>>>> after dev_pm_opp_put_regulators(bus->opp_table).
>>>
>>> Regulators should be set before dev_pm_opp_add_table(), so exit sequence
>>> should be in reverse order,
>>>
>>> init order:
>>>
>>> exynos_bus_parent_parse_of()
>>> dev_pm_opp_set_regulators()
>>> exynos_bus_parse_of()
>>> clk_prepare_enable()
>>> dev_pm_opp_of_add_table()
>>>
>>> exit or error order:
>>>
>>> dev_pm_opp_of_remove_table()
>>> clk_disable_unprepare()
>>> if (bus->opp_table)
>>> dev_pm_opp_put_regulators(bus->opp_table);
>>
>> dev_pm_opp_of_remove_table() have to be called at the end of exit sequence
>> after disabling clock and put regulators. Because dev_pm_opp_of_remove_table()
>> frees the 'opp_table' pointer of device.
>>
>> clk_disable_unprepare()
>> if (bus->opp_table)
>> dev_pm_opp_put_regulators(bus->opp_table);
>> dev_pm_opp_of_remove_table()
>
> The table is reference counted so it will be freed after count go to zero.
You're right. I checked it with OPP code.
>
>>> I will send v4 soon.
>>>
>>>>> +err_reg:
>>>>> + if (bus->opp_table) {
>>>>> + dev_pm_opp_put_regulators(bus->opp_table);
>>>>> + bus->opp_table = NULL;
>>>>> + }
>>>>>
>>>>> return ret;
>>>>> }
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics