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. -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
