lkml.org 
[lkml]   [2017]   [Oct]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 21/22] cpufreq: add support for CPU DVFS based on SCMI message protocol
From
Date


On 04/10/17 12:30, Arnd Bergmann wrote:
>> +static int scmi_cpufreq_probe(struct platform_device *pdev)
>> +{
>> + int ret;
>> +
>> + handle = devm_scmi_handle_get(&pdev->dev);
>> +
>> + if (IS_ERR_OR_NULL(handle) || !handle->perf_ops)
>> + return -EPROBE_DEFER;
>
> As mentioned before, never create an interface that needs to use
> IS_ERR_OR_NULL(), make it return either NULL on error, or always
> have an error code.
>

Agreed.

>> +
>> +static struct platform_driver scmi_cpufreq_platdrv = {
>> + .driver = {
>> + .name = "scmi-cpufreq",
>> + },
>> + .probe = scmi_cpufreq_probe,
>> + .remove = scmi_cpufreq_remove,
>> +};
>
> You appear to have split this driver into the 'cpufreq' side and
> the 'protocol' handler in a different file. I already commented that
> the way the main scmi driver knows about all the protocols looks
> bad, here we can see another aspect of the same problem.
>
> Rather than manually register a platform_device for the purpose
> of connecting it to the cpufreq driver, there should be a way
> to dynamically register the protocol from the cpufreq driver
> and then have both in the same file.
>

I agree that should be possible. I took this approach for 2 reasons:

1. to avoid all sorts of probe ordering issues
2. we may have system with multiple instances of SCMI. E.g. a system
may have multiple remote processors, each controlling dvfs/power mgmt
of a subset of CPUs/devices controller in OS.

I have to admit that I haven't thought too much in details yet. That's
the main idea behind scmi_handle and restricting access to that only to
sub-nodes in it. I am open to suggestions.

--
Regards,
Sudeep

\
 
 \ /
  Last update: 2017-10-04 17:03    [W:0.126 / U:93.508 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site