On 9/30/21 8:59 PM, Chanwoo Choi wrote:

On 9/30/21 8:37 PM, Samuel Holland wrote:

quoted

On 9/29/21 11:19 PM, Chanwoo Choi wrote:

quoted

Hi Samuel,


On 9/29/21 1:42 PM, Samuel Holland wrote:

quoted

Since commit ea572f816032 ("PM / devfreq: Change return type of
devfreq_set_freq_table()"), all devfreq devices are required to have a
valid freq_table. If freq_table is not provided by the driver, it will
be filled in by set_freq_table() from the OPPs; if that fails,
devfreq_add_device() will return an error.

However, since commit ab8f58ad72c4 ("PM / devfreq: Set min/max_freq when
adding the devfreq device"), devfreq devices are _also_ required to have
an OPP table, even if they provide freq_table. devfreq_add_device()
requires dev_pm_opp_find_freq_ceil() and dev_pm_opp_find_freq_floor() to
return successfully, specifically to initialize scaling_min/max_freq.

Not all drivers need an OPP table. For example, a driver where all
frequencies are determined dynamically could work by filling out only
freq_table. But with the current code it must call dev_pm_opp_add() on
every freq_table entry to probe successfully.

As you commented, if device has no opp table, it should call dev_pm_opp_add().
The devfreq have to use OPP for controlling the frequency/regulator.

Actually, I want that all devfreq driver uses the OPP as default way.

The current code/documentation implies that an OPP table is intended to
be optional. For example:

 * struct devfreq - Device devfreq structure
...
 * @opp_table:  Reference to OPP table of dev.parent, if one exists.

So this should be updated if an OPP table is no longer optional.

Right. Need to update it.

quoted

quoted

Are there any reason why don't use the OPP table?

dev_pm_opp_add() takes a voltage, and assumes the existence of some
voltage regulator, but there is none involved here. The only way to have
an OPP table without regulators is to use a static table in the
devicetree. But that also doesn't make much sense, because the OPPs
aren't actually customizable; they are integer dividers from a fixed
base clock.

You can use OPP for only clock control without regulator. OPP already
provides them. OPP already provides the helpful function which
implement the functions to handle the clock/regulator or power doamin.
It is useful framework to control clock/regulator. 

If the standard framework in Linux kernel, it is best to use
this framework in order to remove the duplicate codes on multiple
device drivers. It is one of advantage of Linux kernel. 

Also, if OPP doesn't support the some requirement of you,
you can contribute and update the OPP.

 And adding a fixed OPP table to each board would be a lot of

quoted

work to replace a trivial loop in the driver. So it seems to be the
wrong abstraction.

I don't understand. As I commented for patch 10, you can add
the OPP entry of the clock without the fixed OPP table in devicetree.

quoted

Using an OPP table adds extra complexity (memory allocations, error
cases), just to duplicate the list of frequencies that already has to
exist in freq_table. And the driver works fine without any of that.

'freq_table' of devfreq was developed before of adding OPP interface to Linux kernel as I knew. Actually, I prefer to use the OPP interface
instead of initializing the freq_table directly by device driver.
I just keep the 'freq_table' for preventing the build/working issue
for older device driver. I think OPP is enough to control frequency/voltage
and it provides the various helper funcitons for user of OPP.

Thanks for the explanation. I will convert the driver to use
dev_pm_opp_add(), and I will drop patches 2 and 4. I think patch 3 is
still worth considering.

Regards,
Samuel