Re: [PATCH 3/3 v2] iio: add rtc-driver for HID sensors of type time

[Lars-Peter Clausen <lars@metafoo.de>]

On 12/10/2012 11:20 PM, Alexander Holler wrote:
> [...]
>>> +    init_completion(&time_state->comp_last_time);
>>
>> This needs to be INIT_COMPLETION. init_completion must be called exactly
>> once on a completion, which should be from inside probe() in this case.
> 
> Ah, so I've misread http://lwn.net/Articles/23993/ . I've read it as
> init_completion() is usable multiple times (I had the impression
> INIT_COMPLETION got replaced by init_completion().
> 

Well, I've been exaggerating a bit, you can call it multiple times, but you
need to make sure that the completion is not in use at the same time, since
init_completion will completely reinitialize the struct and if it is in use at
the same time you'll get undefined behavior. So it is a good rule of thumb to
just use it once in probe(). INIT_COMPLETION on the other hand will just clear
the done flag, so you know that between your call to INIT_COMPLETION and when
wait_for_completion returns successfully your event has occurred at least once.

>>> +    /* wait for all values (event) */
>>> +   
>>> wait_for_completion_interruptible_timeout(&time_state->comp_last_time,
>>> +                            HZ*6);
>>
>> You should check the return value in case either a timeout happens or the
>> sleep is interrupted.
> 
> Yes, I already wanted to do it, but it seems I've forgotten it.
> 
>>> +    struct hid_time_state *time_state =
>>> +        kzalloc(sizeof(struct hid_time_state), GFP_KERNEL);
>>
>> You could use devm_kzalloc here. By doing so you don't have to take
>> care of
>> freeing it again since it will be auto-freed once the device is removed.
> 
> Thanks. I already searched such and wondered why such didn't exist. Just
> to clarify, if I use devm_kzalloc, I don't have to free time_state here
> 
>>> +error_free_drvdata:
>>> +    platform_set_drvdata(pdev, NULL);
>>
>> Setting the platform data to NULL should not be necessary. Some
>> drivers do
>> this but it's kind of the result of cargo-cult-coding.
>>
>>> +    kfree(time_state);
>>> +error_ret:
>>> +    return ret;
>>> +}
>>> +
>>> +static int __devinit hid_time_remove(struct platform_device *pdev)
>>> +{
>>> +    struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
>>> +    struct hid_time_state *time_state = platform_get_drvdata(pdev);
>>> +
>>> +    if (!IS_ERR(time_state->rtc))
>>> +        rtc_device_unregister(time_state->rtc);
>>> +    sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TIME);
>>> +    platform_set_drvdata(pdev, NULL);
>>
> 
> and here?
> 

yes.