On 24 October 2012 06:13, Francesco Lavra <francescolavra...@gmail.com> wrote:
> Hi,
> On 10/23/2012 10:23 AM, Hongbo Zhang wrote:
>> Hi Francesco,
>> I found out more points about this issue.
>>
>> [1]
>> cdev should be ready when get_max_state callback be called, otherwise
>> parameter cdev is useless, imagine there may be cases that
>> get_max_state call back is shared by more than one cooling devices of
>> same kind, like this:
>> common_get_max_state(*cdev, *state)
>> {
>> if (cdev == cdev1) *state = 3;
>> else if (cdev == cdev) *state = 5;
>> else
>> }
>>
>> [2]
>> In my cpufreq cooling case(in fact cdev is not used to calculate
>> max_state), the cooling_cpufreq_list should be ready when
>> get_max_state call back is called. In this patch I defer the binding
>> when registration finished, but this is not perfect now, see this
>> routine in cpufreq_cooling_register:
>>
>> thermal_cooling_device_register;
>> at this time: thermal_bind_work -> get_max_state -> get NULL
>> cooling_cpufreq_list
>> and then: list_add_tail(&cpufreq_dev->node, &cooling_cpufreq_list)
>> This is due to we cannot know exactly when the bind work is executed.
>> (and this can be fixed by moving mutex_lock(&cooling_cpufreq_lock)
>> aheadof thermal_cooling_device_register and other corresponding
>> modifications, but I found another way as below)
>>
>> [3]
>> Root cause of this problem is calling get_max_state in register -> bind
>> routine.
>> Better solution is to add another parameter in cooling device register
>> function, also add a max_state member in struct cdev, like:
>> thermal_cooling_device_register(char *type, void *devdata, const
>> struct thermal_cooling_device_ops *ops, unsigned long max_state)
>> and then in the bind function:
>> if(cdev->max_state)
>> max_state = cdev->max_state;
>> else
>> cdev->get_max_state(cdev, &max_state)
>>
>> It is common sense that the cooling driver should know its cooling
>> max_state(ability) before registration, and it can be offered when
>> register.
>> I think this way doesn't change both thermal and cooling layer much,
>> it is more clear.
>> I will update this patch soon.
>
> I still believe the thermal layer doesn't need any change to work around
> this problem, and I still believe that when a cooling device is being
> registered, all of its ops should be fully functional.
> The problem with the cpufreq cooling device driver is that its callbacks
> use the internal list of devices to retrieve the struct
> cpufreq_cooling_device instance corresponding to a given struct
> thermal_cooling_device. This is not necessary, because the struct
> thermal_cooling_device has a private data pointer (devdata) which in
> this case is exactly a reference to the struct cpufreq_cooling_device
> instance the callbacks are looking for. In fact, I think the
> cooling_cpufreq_list is not necessary at all, and should be removed from
> the cpufreq cooling driver.
> So the 3 callbacks, instead of iterating through the device list, should
> have something like:
> struct cpufreq_cooling_device *cpufreq_device = cdev->devdata;
> That would do the trick.
Hi Francesco,
When I found out this issue, I was hesitating to select the best
solution among several ideas.
It is clear now after talk with you, I will send patch for cpufreq
cooling layer.
Thank you.
>
> --
> Francesco
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
