Re: [PATCH v2 2/4] devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()

[Bartlomiej Zolnierkiewicz]

On 7/16/19 1:26 PM, Chanwoo Choi wrote:
> Hi,
> 
> On 19. 7. 16. ìí 7:59, Bartlomiej Zolnierkiewicz wrote:
>>
>> On 7/16/19 12:33 PM, Chanwoo Choi wrote:
>>> Hi Bartlomiej,
>>>
>>> On 19. 7. 16. ìí 7:13, Bartlomiej Zolnierkiewicz wrote:
>>>>
>>>> Hi Chanwoo,
>>>>
>>>> On 7/16/19 5:56 AM, Chanwoo Choi wrote:
>>>>> Hi Kamil,
>>>>>
>>>>> Looks good to me. But, this patch has some issue.
>>>>> I added the detailed reviews.
>>>>>
>>>>> I recommend that you make the separate patches as following
>>>>> in order to clarify the role of which apply the dev_pm_opp_* function.
>>>>>
>>>>> First patch,
>>>>> Need to consolidate the following two function into one function.
>>>>> because the original exynos-bus.c has the problem that the regulator
>>>>> of parent devfreq device have to be enabled before enabling the clock.
>>>>> This issue did not happen because bootloader enables the bus-related
>>>>> regulators before kernel booting.
>>>>> - exynos_bus_parse_of()
>>>>> - exynos_bus_parent_parse_of()
>>>>>> Second patch,
>>>>> Apply dev_pm_opp_set_regulators() and dev_pm_opp_set_rate()
>>>>>
>>>>>
>>>>> On 19. 7. 15. ìí 9:04, Kamil Konieczny wrote:
>>>>>> 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 <k.konieczny@xxxxxxxxxxxxxxxxxxx>
>>>>>> ---
>>>>>>  drivers/devfreq/exynos-bus.c | 172 ++++++++++++++---------------------
>>>>>>  1 file changed, 66 insertions(+), 106 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>>>>> index 486cc5b422f1..7fc4f76bd848 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,25 @@ 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 */
>>>>>> +	/*
>>>>>> +	 * New frequency for bus may not be exactly matched to opp, adjust
>>>>>> +	 * *freq to correct value.
>>>>>> +	 */
>>>>>
>>>>> You better to change this comment with following styles
>>>>> to keep the consistency:
>>>>>
>>>>> 	/* 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;
>>>>>
>>>>> Have to print the error log if ret has minus error value.
>>>>
>>>> dev_pm_opp_set_rate() should print the error message on all
>>>> errors so wouldn't printing the error log also here be superfluous?
>>>>
>>>> [ Please also note that the other user of dev_pm_opp_set_rate()
>>>>   (cpufreq-dt cpufreq driver) doesn't do this. ]
>>>
>>> OK. Thanks for the explanation. 
>>>
>>>>
>>>>> Modify it as following:
>>>>>
>>>>> 	if (ret < 0) {
>>>>> 		dev_err(dev, "failed to set bus rate\n");
>>>>> 		goto err:
>>>>> 	}
>>>>> 	bus->curr_freq = *freq;
>>>>>
>>>>> err:
>>>>> 	mutex_unlock(&bus->lock);
>>>>> 	
>>>>> 	return ret;
>>>>>
>>>>>>  
>>>>>> -	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;
>>>>>> @@ -194,10 +162,11 @@ static void exynos_bus_exit(struct device *dev)
>>>>>>  	if (ret < 0)
>>>>>>  		dev_warn(dev, "failed to disable the devfreq-event devices\n");
>>>>>>  
>>>>>> -	if (bus->regulator)
>>>>>> -		regulator_disable(bus->regulator);
>>>>>> +	if (bus->opp_table)
>>>>>> +		dev_pm_opp_put_regulators(bus->opp_table);
>>>>>
>>>>> Have to disable regulator after disabling the clock
>>>>> to prevent the h/w fault.
>>>>>
>>>>> I think that you should call them with following sequence:
>>>>>
>>>>> 	clk_disable_unprepare(bus->clk);
>>>>> 	if (bus->opp_table)
>>>>> 		dev_pm_opp_put_regulators(bus->opp_table);
>>>>> 	dev_pm_opp_of_remove_table(dev);
>>>>>
>>>>>>  
>>>>>>  	dev_pm_opp_of_remove_table(dev);
>>>>>> +
>>>>>>  	clk_disable_unprepare(bus->clk);
>>>>>>  }
>>>>>>  
>>>>>> @@ -209,39 +178,26 @@ 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 */
>>>>>> +	/*
>>>>>> +	 * New frequency for bus may not be exactly matched to opp, adjust
>>>>>> +	 * *freq to correct value.
>>>>>> +	 */
>>>>>
>>>>> You better to change this comment with following styles
>>>>> to keep the consistency:
>>>>>
>>>>> 	/* 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;
>>>>>
>>>>> ditto. Have to print the error log, check above comment.
>>>>>
>>>>>>  
>>>>>> -	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 +215,7 @@ 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;
>>>>>> -	}
>>>>>> +	int i, count, size;
>>>>>>  
>>>>>>  	/*
>>>>>>  	 * Get the devfreq-event devices to get the current utilization of
>>>>>> @@ -281,24 +224,20 @@ 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;
>>>>>>  
>>>>>>  	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;
>>>>>>  	}
>>>>>>  
>>>>>>  	/*
>>>>>> @@ -314,22 +253,15 @@ 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 device_node *np,
>>>>>> -			      struct exynos_bus *bus)
>>>>>> +			      struct exynos_bus *bus, bool passive)
>>>>>>  {
>>>>>>  	struct device *dev = bus->dev;
>>>>>> +	struct opp_table *opp_table;
>>>>>> +	const char *vdd = "vdd";
>>>>>>  	struct dev_pm_opp *opp;
>>>>>>  	unsigned long rate;
>>>>>>  	int ret;
>>>>>> @@ -347,11 +279,22 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>>>  		return ret;
>>>>>>  	}
>>>>>>  
>>>>>> +	if (!passive) {
>>>>>> +		opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
>>>>>> +		if (IS_ERR(opp_table)) {
>>>>>> +			ret = PTR_ERR(opp_table);
>>>>>> +			dev_err(dev, "failed to set regulators %d\n", ret);
>>>>>> +			goto err_clk;/
>>>>>> +		}
>>>>>> +
>>>>>> +		bus->opp_table = opp_table;
>>>>>> +	}
>>>>>
>>>>> This driver has exynos_bus_parent_parse_of() function for parent devfreq device.
>>>>> dev_pm_opp_set_regulators() have to be called in exynos_bus_parent_parse_of()
>>>>> because the regulator is only used by parent devfreq device.
>>>>
>>>> exynos_bus_parse_of() is called for all devfreq devices (including
>>>> parent) and (as you've noticed) the regulator should be enabled before
>>>> enabling clock (which is done in exynos_bus_parse_of()) so adding
>>>> extra argument to exynos_bus_parse_of() (like it is done currently in
>>>> the patch) 
>>>
>>> I think that this patch has still the problem about call sequence
>>> between clock and regulator as following:
>>
>> Yes, this should be fixed (though the wrong sequence between regulator
>> and clock handling is not introduced by the patchset itself and is present
>> in the original driver code).
>>
>>> 273         ret = clk_prepare_enable(bus->clk);                                     
>>> 274         if (ret < 0) {                                                          
>>> 275                 dev_err(dev, "failed to get enable clock\n");                   
>>> 276                 return ret;                                                     
>>> 277         }                                                                       
>>> 278                                                                                 
>>> 279         if (!passive) {                                                         
>>> 280                 opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);            
>>> 281                 if (IS_ERR(opp_table)) {                                        
>>> 282                         ret = PTR_ERR(opp_table);                               
>>> 283                         dev_err(dev, "failed to set regulators %d\n", ret);     
>>> 284                         goto err_clk;                                           
>>> 285                 }                                                               
>>> 286                                                                                 
>>> 287                 bus->opp_table = opp_table;                                     
>>> 288         }                   
>>>
>>> makes it possible to do the setup correctly without the need
>>>> of merging both functions into one huge function (which would be more
>>>> difficult to follow than two simpler functions IMHO). Is that approach
>>>> acceptable or do you prefer one big function?
>>>
>>> Actually, I don't force to make one function for both
>>> exynos_bus_parse_of() and exynos_bus_parent_parse_of().
>>>
>>> If we just keep this code, dev_pm_opp_set_regulators()
>>> should be handled in exynos_bus_parent_parse_of()
>>> because only parent devfreq device controls the regulator.
>>
>> Could your please explain rationale for this requirement (besides
>> function name)?
> 
> OK. I hope to satisfy the following requirements:
> 
> 1. Fix the sequence problem between clock and regulator for enabling them.
> 2. dev_pm_opp_set_regulator() have to be handled in exynos_bus_parent_parse_of()
>    instead of exynos_bus_parse_of() for only parent devfreq device.
> 3. exynos_bus_parse_of() have to handle the only common properties
>    of both parent devfreq device and passive devfreq device.
> 
>>
>> The patch adds 'bool passive' argument (which is set to false for
>> parent devfreq device and true for child devfreq device) to
>> exynos_bus_parse_of() (which is called for *all* devfreq devices
> 
> As I menteiond, exynos_bus_parse_of have to handle the only common
> properties of both parent device and passive device. 
> 
> I gathered the properties for parent device into exynos_bus_parent_parse_of()
> This way using 'bool passive' argument is not proper in exynos_bus_parse_of().
> 
> 
>> and is called before exynos_bus_parent_parse_of()) and there is
>> no hard requirement to call dev_pm_opp_set_regulators() in
>> exynos_bus_parent_parse_of() so after only changing the ordering
>> between regulator and clock handling the setup code should be
>> correct.
>>
>> [ Please note that this patch moves parent/child detection before
>>   exynos_bus_parse_of() call. ]
>>
>>> In order to keep the two functions, maybe have to change
>>> the call the sequence between exynos_bus_parse_of() and
>>> exynos_bus_parent_parse_of().
>>
>> Doesn't seem to be needed, care to explain it more?
> 
> In order to fix the sequence problem between clock and regulator
> with dev_pm_opp_set_regualtor() and want to keep two functions
> (exynos_bus_parent_parse_of() and exynos_bus_parse_of()),
> have to change the call order as following and then modify
> the exception handling code when error happen.
> 
> 	node = of_parse_phandle(dev->of_node, "devfreq", 0);                    
> 	if (node) {                                                             
> 		of_node_put(node);                                              
> 		passive = true
> 	}
> 
> 	if (!passive)	
> 		exynos_bus_parent_parse_of()
> 			dev_pm_opp_set_regulator
> 
> 	exynos_bus_parse_of()

OK. This seems like a solution.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

>>
>>> Once again, I don't force any fixed method. I want to fix them
>>> with correct way.
>>>
>>>>
>>>>>> +
>>>>>>  	/* Get the freq and voltage from OPP table to scale the bus freq */
>>>>>>  	ret = dev_pm_opp_of_add_table(dev);
>>>>>>  	if (ret < 0) {
>>>>>>  		dev_err(dev, "failed to get OPP table\n");
>>>>>> -		goto err_clk;
>>>>>> +		goto err_regulator;
>>>>>>  	}
>>>>>>  
>>>>>>  	rate = clk_get_rate(bus->clk);
>>>>>> @@ -362,6 +305,7 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>>>  		ret = PTR_ERR(opp);
>>>>>>  		goto err_opp;
>>>>>>  	}
>>>>>> +
>>>>>>  	bus->curr_freq = dev_pm_opp_get_freq(opp);
>>>>>>  	dev_pm_opp_put(opp);
>>>>>>  
>>>>>> @@ -369,6 +313,13 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>>>  
>>>>>>  err_opp:
>>>>>>  	dev_pm_opp_of_remove_table(dev);
>>>>>> +
>>>>>> +err_regulator:
>>>>>> +	if (bus->opp_table) {
>>>>>> +		dev_pm_opp_put_regulators(bus->opp_table);
>>>>>> +		bus->opp_table = NULL;
>>>>>> +	}
>>>>>
>>>>> As I mentioned above, it it wrong to call dev_pm_opp_put_regulators()
>>>>> after removing the opp_table by dev_pm_opp_of_remove_table().
>>>>>
>>>>>> +
>>>>>>  err_clk:
>>>>>>  	clk_disable_unprepare(bus->clk);
>>>>>>  
>>>>>> @@ -386,6 +337,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>>>  	struct exynos_bus *bus;
>>>>>>  	int ret, max_state;
>>>>>>  	unsigned long min_freq, max_freq;
>>>>>> +	bool passive = false;
>>>>>>  
>>>>>>  	if (!np) {
>>>>>>  		dev_err(dev, "failed to find devicetree node\n");
>>>>>> @@ -395,12 +347,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>>>  	bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
>>>>>>  	if (!bus)
>>>>>>  		return -ENOMEM;
>>>>>> +
>>>>>>  	mutex_init(&bus->lock);
>>>>>>  	bus->dev = &pdev->dev;
>>>>>>  	platform_set_drvdata(pdev, bus);
>>>>>> +	node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>>>>> +	if (node) {
>>>>>> +		of_node_put(node);
>>>>>> +		passive = true;
>>>>>> +	}
>>>>>>  
>>>>>>  	/* Parse the device-tree to get the resource information */
>>>>>> -	ret = exynos_bus_parse_of(np, bus);
>>>>>> +	ret = exynos_bus_parse_of(np, bus, passive);
>>>>>>  	if (ret < 0)
>>>>>>  		return ret;
>>>>>>  
>>>>>> @@ -410,13 +368,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>>>  		goto err;
>>>>>>  	}
>>>>>>  
>>>>>> -	node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>>>>> -	if (node) {
>>>>>> -		of_node_put(node);
>>>>>> +	if (passive)
>>>>>>  		goto passive;
>>>>>> -	} else {
>>>>>> -		ret = exynos_bus_parent_parse_of(np, bus);
>>>>>> -	}
>>>>>> +
>>>>>> +	ret = exynos_bus_parent_parse_of(np, bus);
>>>>>>  
>>>>>
>>>>> Remove unneeded blank line.
>>>>>
>>>>>>  	if (ret < 0)
>>>>>>  		goto err;
>>>>>> @@ -509,6 +464,11 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>>>  
>>>>>>  err:
>>>>>>  	dev_pm_opp_of_remove_table(dev);
>>>>>> +	if (bus->opp_table) {
>>>>>> +		dev_pm_opp_put_regulators(bus->opp_table);
>>>>>> +		bus->opp_table = NULL;
>>>>>> +	}
>>>>>> +
>>>>>
>>>>> ditto.
>>>>> Have to disable regulator after disabling the clock
>>>>> to prevent the h/w fault.
>>>>>
>>>>> I think that you should call them with following sequence:
>>>>>
>>>>> 	clk_disable_unprepare(bus->clk);
>>>>> 	if (bus->opp_table)
>>>>> 		dev_pm_opp_put_regulators(bus->opp_table);
>>>>> 	dev_pm_opp_of_remove_table(dev);
>>>>>
>>>>>>  	clk_disable_unprepare(bus->clk);
>>>>>>  
>>>>>>  	return ret;
>>>>
>>>> Best regards,
>>>> --
>>>> Bartlomiej Zolnierkiewicz
>>>> Samsung R&D Institute Poland
>>>> Samsung Electronics
>>
>> Best regards,
>> --
>> Bartlomiej Zolnierkiewicz
>> Samsung R&D Institute Poland
>> Samsung Electronics