| Message ID | 1355151119-2489-1-git-send-email-holler@ahsoftware.de |
|---|---|
| State | Superseded |
| Headers | show |
On 12/10/2012 03:51 PM, Alexander Holler wrote: > This driver makes the time from HID sensors (hubs) which are offering > such available like any other RTC does. > > Currently the time can only be read. Setting the time must be done > through sending a report, which currently isn't supported by > hid-sensor-hub. > > It is necessary that all values like year, month etc, are send as > 8bit values (1 byte each) and all of them in 1 report. Also the > spec HUTRR39b doesn't define the range of the year field, we > tread it as 0 - 99 because that's what most RTCs I know about are > offering. > > Signed-off-by: Alexander Holler <holler@ahsoftware.de> Looks pretty good now. But there are still some IIO remnants which should be removed as well. Also the driver should move to drivers/rtc/ since, well, it's a rtc driver not a IIO driver. > diff --git a/drivers/iio/time/hid-sensor-time.c b/drivers/iio/time/hid-sensor-time.c > new file mode 100644 > index 0000000..556bac9 > --- /dev/null > +++ b/drivers/iio/time/hid-sensor-time.c > @@ -0,0 +1,346 @@ [...] > + > +/* Channel definitions */ > +static const struct iio_chan_spec hid_time_channels[TIME_RTC_CHANNEL_MAX] = { > + { > + .info_mask = IIO_CHAN_INFO_RAW, > + .scan_index = CHANNEL_SCAN_INDEX_YEAR, > + .extend_name = "year", > + }, { > + .info_mask = IIO_CHAN_INFO_RAW, > + .scan_index = CHANNEL_SCAN_INDEX_MONTH, > + .extend_name = "month", > + }, { > + .info_mask = IIO_CHAN_INFO_RAW, > + .scan_index = CHANNEL_SCAN_INDEX_DAY, > + .extend_name = "day", > + }, { > + .info_mask = IIO_CHAN_INFO_RAW, > + .scan_index = CHANNEL_SCAN_INDEX_HOUR, > + .extend_name = "hour", > + }, { > + .info_mask = IIO_CHAN_INFO_RAW, > + .scan_index = CHANNEL_SCAN_INDEX_MINUTE, > + .extend_name = "minute", > + }, { > + .info_mask = IIO_CHAN_INFO_RAW, > + .scan_index = CHANNEL_SCAN_INDEX_SECOND, > + .extend_name = "second", > + } > +}; The channel spec is semi unused. You use it to lookup the scan index and the name, but that could easily be implemented without the channel spec. Especially considering that the scan index lookup is only ever done for channel 0, which will always return CHANNEL_SCAN_INDEX_YEAR. > + > +static int hid_time_read_raw(struct hid_time_state *time_state, > + struct iio_chan_spec const *chan, > + int *val, int *val2, > + long mask) > +{ > + int report_id; > + u32 address; > + > + *val = 0; > + *val2 = 0; > + if (mask) > + return -EINVAL; > + report_id = time_state->info[chan->scan_index].report_id; > + address = hid_time_addresses[chan->scan_index]; > + if (report_id >= 0) { > + *val = sensor_hub_input_attr_get_raw_value( > + time_state->common_attributes.hsdev, > + HID_USAGE_SENSOR_TIME, address, report_id); Just directly call sensor_hub_input_attr_get_raw_value in hid_rtc_read_time instead of wrapping it here. > + return IIO_VAL_INT; > + } > + *val = 0; > + return -EINVAL; > +} > + [...] Are the entries in info ordered in the same way as the addresses in hid_time_addresses? If yes you could just use a lookup-table like static const char * const hid_time_attrib_names[] = { "second", ... }; and just use 'i' to look them up. > +static const char *attrib_name(u32 attrib_id) > +{ > + unsigned i = 0; > + static const char unknown[] = "unknown"; > + > + for (; i < TIME_RTC_CHANNEL_MAX; ++i) { > + if (hid_time_addresses[i] == attrib_id) > + return hid_time_channels[i].extend_name; > + } > + return unknown; /* should never happen */ > +} > + > +static int hid_time_parse_report(struct platform_device *pdev, > + struct hid_sensor_hub_device *hsdev, > + unsigned usage_id, > + struct hid_time_state *time_state) > +{ > + int ret, i = 0; > + > + for (; i < TIME_RTC_CHANNEL_MAX; ++i) { I'd put the i = 0 in the for header. > + ret = sensor_hub_input_get_attribute_info(hsdev, > + HID_INPUT_REPORT, usage_id, > + hid_time_addresses[i], &time_state->info[i]); > + if (ret < 0) > + return ret; > + } > + /* Check the (needed) attributes for sanity */ > + ret = time_state->info[0].report_id; /* use ret as temp. */ maybe name it report_id instead of ret > + if (ret < 0) { > + dev_err(&pdev->dev, "bad report ID!\n"); > + return -EINVAL; > + } > + for (i = 0; i < TIME_RTC_CHANNEL_MAX; ++i) { > + if (time_state->info[i].report_id != ret) { > + dev_err(&pdev->dev, > + "not all needed attributes inside the same report!\n"); > + return -EINVAL; > + } [...] > + } > + > + return 0; > +} > + > +static int hid_rtc_read_time(struct device *dev, > + struct rtc_time *tm) > +{ > + int val; > + unsigned long flags; > + struct hid_time_state *time_state = > + platform_get_drvdata(to_platform_device(dev)); > + > + 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. > + /* start a read */ > + if (hid_time_read_raw(time_state, > + &hid_time_channels[0], &val, &val, 0) == -EINVAL) { > + dev_err(dev, "unable to read time!\n"); > + return -EIO; > + } > + /* 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. > + spin_lock_irqsave(&time_state->lock_last_time, flags); > + *tm = time_state->last_time; > + spin_unlock_irqrestore(&time_state->lock_last_time, flags); > + > + return 0; > +} > + > +static const struct rtc_class_ops rtc_ops = { > + .read_time = hid_rtc_read_time, > +}; > + > +static int __devinit hid_time_probe(struct platform_device *pdev) __devinit is going away, it shouldn't be used anymore in new drivers. > +{ > + int ret = 0; > + struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data; > + 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. > + > + if (time_state == NULL) { > + ret = -ENOMEM; > + goto error_ret; > + } > + > + platform_set_drvdata(pdev, time_state); > + > + time_state->common_attributes.hsdev = hsdev; > + time_state->common_attributes.pdev = pdev; > + > + ret = hid_sensor_parse_common_attributes(hsdev, > + HID_USAGE_SENSOR_TIME, > + &time_state->common_attributes); > + if (ret) { > + dev_err(&pdev->dev, "failed to setup common attributes!\n"); > + goto error_free_drvdata; > + } > + > + ret = hid_time_parse_report(pdev, hsdev, /*channels,*/ remove the commented out code > + HID_USAGE_SENSOR_TIME, time_state); > + if (ret) { > + dev_err(&pdev->dev, "failed to setup attributes!\n"); > + goto error_free_drvdata; > + } > + > + time_state->callbacks.send_event = hid_time_proc_event; > + time_state->callbacks.capture_sample = hid_time_capture_sample; > + time_state->callbacks.pdev = pdev; > + ret = sensor_hub_register_callback(hsdev, HID_USAGE_SENSOR_TIME, > + &time_state->callbacks); > + if (ret < 0) { > + dev_err(&pdev->dev, "register callback failed!\n"); > + goto error_free_drvdata; > + } > + > + time_state->rtc = rtc_device_register("hid-sensor-time", > + &pdev->dev, &rtc_ops, THIS_MODULE); Since the driver does not do much if the rtc device could not be registered I think it should be ok to fail probe in case registering the rtc device fails. > + > + if (IS_ERR(time_state->rtc)) { > + ret = PTR_ERR(time_state->rtc); > + dev_err(&pdev->dev, "rtc device register failed!\n"); > + goto error_free_drvdata; > + } > + > + return ret; > + > +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); Same here. > + kfree(time_state); > + > + return 0; > +} > + [...]
Am 10.12.2012 18:05, schrieb Lars-Peter Clausen: > Looks pretty good now. But there are still some IIO remnants which should be > removed as well. Also the driver should move to drivers/rtc/ since, well, > it's a rtc driver not a IIO driver. I think it still should be stick to iio, because that is where all HID sensors currently are found and where the user would expect to find such a driver. >> +/* Channel definitions */ >> +static const struct iio_chan_spec hid_time_channels[TIME_RTC_CHANNEL_MAX] = { And that is imho the last remaining iio-stuff. If necessary I can remove it too. Regards, Alexander
On 12/10/2012 08:45 PM, Alexander Holler wrote: > Am 10.12.2012 18:05, schrieb Lars-Peter Clausen: > >> Looks pretty good now. But there are still some IIO remnants which should be >> removed as well. Also the driver should move to drivers/rtc/ since, well, >> it's a rtc driver not a IIO driver. > > I think it still should be stick to iio, because that is where all HID > sensors currently are found and where the user would expect to find such > a driver. That's because all the current IIO sensor drivers fall in the IIO domain. This one clearly is a RTC driver, so it belongs in drivers/rtc/ > >>> +/* Channel definitions */ >>> +static const struct iio_chan_spec hid_time_channels[TIME_RTC_CHANNEL_MAX] = { > > And that is imho the last remaining iio-stuff. If necessary I can remove > it too. That and the remaining bits of the read_raw callback.
Am 10.12.2012 21:22, schrieb Lars-Peter Clausen: > On 12/10/2012 08:45 PM, Alexander Holler wrote: >> Am 10.12.2012 18:05, schrieb Lars-Peter Clausen: >> >>> Looks pretty good now. But there are still some IIO remnants which should be >>> removed as well. Also the driver should move to drivers/rtc/ since, well, >>> it's a rtc driver not a IIO driver. >> >> I think it still should be stick to iio, because that is where all HID >> sensors currently are found and where the user would expect to find such >> a driver. > > That's because all the current IIO sensor drivers fall in the IIO domain. This > one clearly is a RTC driver, so it belongs in drivers/rtc/ Where nobody will find it if he searches for drivers for his HID sensor. I still see it as HID sensor driver and not a rtc-driver. But ... > >> >>>> +/* Channel definitions */ >>>> +static const struct iio_chan_spec hid_time_channels[TIME_RTC_CHANNEL_MAX] = { >> >> And that is imho the last remaining iio-stuff. If necessary I can remove >> it too. > > That and the remaining bits of the read_raw callback. Ok. I will make a v3. Alexander
On 12/10/2012 10:26 PM, Alexander Holler wrote: > Am 10.12.2012 21:22, schrieb Lars-Peter Clausen: >> On 12/10/2012 08:45 PM, Alexander Holler wrote: >>> Am 10.12.2012 18:05, schrieb Lars-Peter Clausen: >>> >>>> Looks pretty good now. But there are still some IIO remnants which should be >>>> removed as well. Also the driver should move to drivers/rtc/ since, well, >>>> it's a rtc driver not a IIO driver. >>> >>> I think it still should be stick to iio, because that is where all HID >>> sensors currently are found and where the user would expect to find such >>> a driver. >> >> That's because all the current IIO sensor drivers fall in the IIO domain. This >> one clearly is a RTC driver, so it belongs in drivers/rtc/ > > Where nobody will find it if he searches for drivers for his HID sensor. > I still see it as HID sensor driver and not a rtc-driver. > But ... I can understand your position, but drivers are usually grouped by function not by topology. If there is a proper Kconfig help text people should hopefully be find it. > >> >>> >>>>> +/* Channel definitions */ >>>>> +static const struct iio_chan_spec hid_time_channels[TIME_RTC_CHANNEL_MAX] = { >>> >>> And that is imho the last remaining iio-stuff. If necessary I can remove >>> it too. >> >> That and the remaining bits of the read_raw callback. > > Ok. I will make a v3. > Btw. have you seen the other comments I had inline in my response to your v2?
On 12/10/2012 09:39 PM, Lars-Peter Clausen wrote: > On 12/10/2012 10:26 PM, Alexander Holler wrote: >> Am 10.12.2012 21:22, schrieb Lars-Peter Clausen: >>> On 12/10/2012 08:45 PM, Alexander Holler wrote: >>>> Am 10.12.2012 18:05, schrieb Lars-Peter Clausen: >>>> >>>>> Looks pretty good now. But there are still some IIO remnants which should be >>>>> removed as well. Also the driver should move to drivers/rtc/ since, well, >>>>> it's a rtc driver not a IIO driver. >>>> >>>> I think it still should be stick to iio, because that is where all HID >>>> sensors currently are found and where the user would expect to find such >>>> a driver. >>> >>> That's because all the current IIO sensor drivers fall in the IIO domain. This >>> one clearly is a RTC driver, so it belongs in drivers/rtc/ >> >> Where nobody will find it if he searches for drivers for his HID sensor. >> I still see it as HID sensor driver and not a rtc-driver. >> But ... > > I can understand your position, but drivers are usually grouped by function not > by topology. If there is a proper Kconfig help text people should hopefully be > find it. Seconded on this. If it is a pure rtc driver then it definitely belongs in drivers/rtc. Now there might have been ways of doing this as a consumer / provider with the provider being in IIO and the consumer in rtc, but that sounds like it is over compicating things, at least for now. > >> >>> >>>> >>>>>> +/* Channel definitions */ >>>>>> +static const struct iio_chan_spec hid_time_channels[TIME_RTC_CHANNEL_MAX] = { >>>> >>>> And that is imho the last remaining iio-stuff. If necessary I can remove >>>> it too. >>> >>> That and the remaining bits of the read_raw callback. >> >> Ok. I will make a v3. >> > > Btw. have you seen the other comments I had inline in my response to your v2? > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
Am 10.12.2012 18:05, schrieb Lars-Peter Clausen: > On 12/10/2012 03:51 PM, Alexander Holler wrote: > The channel spec is semi unused. You use it to lookup the scan index and the > name, but that could easily be implemented without the channel spec. > Especially considering that the scan index lookup is only ever done for > channel 0, which will always return CHANNEL_SCAN_INDEX_YEAR. Thats what I had in mind for v3. > Are the entries in info ordered in the same way as the addresses in > hid_time_addresses? If yes you could just use a lookup-table like > > static const char * const hid_time_attrib_names[] = { > "second", > ... > }; > > and just use 'i' to look them up. Again, what I had in mind for v3. It would have been better I wouldn't have used one of the existing drivers as template and afterwards removing tons of stuff. ;) >> + 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(). >> + /* 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? > Same here. > >> + kfree(time_state); >> + >> + return 0; >> +} >> + > [...] Regards, Alexander
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.
Am 10.12.2012 22:42, schrieb Jonathan Cameron: > On 12/10/2012 09:39 PM, Lars-Peter Clausen wrote: >> On 12/10/2012 10:26 PM, Alexander Holler wrote: >>> Am 10.12.2012 21:22, schrieb Lars-Peter Clausen: >>>> On 12/10/2012 08:45 PM, Alexander Holler wrote: >>>>> Am 10.12.2012 18:05, schrieb Lars-Peter Clausen: >>>>> >>>>>> Looks pretty good now. But there are still some IIO remnants which should be >>>>>> removed as well. Also the driver should move to drivers/rtc/ since, well, >>>>>> it's a rtc driver not a IIO driver. >>>>> >>>>> I think it still should be stick to iio, because that is where all HID >>>>> sensors currently are found and where the user would expect to find such >>>>> a driver. >>>> >>>> That's because all the current IIO sensor drivers fall in the IIO domain. This >>>> one clearly is a RTC driver, so it belongs in drivers/rtc/ >>> >>> Where nobody will find it if he searches for drivers for his HID sensor. >>> I still see it as HID sensor driver and not a rtc-driver. >>> But ... >> >> I can understand your position, but drivers are usually grouped by function not >> by topology. If there is a proper Kconfig help text people should hopefully be >> find it. > Seconded on this. If it is a pure rtc driver then it definitely belongs in > drivers/rtc. Now there might have been ways of doing this as a consumer / provider > with the provider being in IIO and the consumer in rtc, but that sounds like > it is over compicating things, at least for now. Personally I just want to use it to have HID-USB_RTCs. ;) But in case of HID sensor hubs, the main usage of that driver will be to set the time of such hubs (something I still have to make patches for), and not to read it. So if you think as an HID sensor user, it should belong to iio or wherever those sensor will finally end up (I think they should end up in HID and should be usable by bluetooth devices too), but if you creatively misuse the standard to get a driver for USB-RTCs, as I want, then rtc is the correct place. ;) Because I don't want to do a v4: the driver has an #include "../common/hid-sensors/hid-sensor-attributes.h" so moving it to drivers/rtc/ will make that even more ugly. Suggestions? I don't really care and as I'm currently at the order receiving end ;) I would change it to whatever the maintainers are wanting. Maybe moving that header to include/linux or even integrating it into include/linux/hid-sensor-hubs.h makes sense. Regards, Alexander
Am 10.12.2012 23:36, schrieb Lars-Peter Clausen: > Well, I've been exaggerating a bit, you can call it multiple times, but you Thanks a lot for the explanation(s). >>>> +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); Btw. I wouldn't call that cargo-cult-coding. It's more defensive programming as people might not be sure, if there is something around which still might access the platform data at that point. Ok, there would be a need for a mutex or similiar if that really could happen, but I wouldn't call such practices cargo-cult. ;) Regards, Alexander
On 10/12/12 22:50, Alexander Holler wrote: > Am 10.12.2012 22:42, schrieb Jonathan Cameron: >> On 12/10/2012 09:39 PM, Lars-Peter Clausen wrote: >>> On 12/10/2012 10:26 PM, Alexander Holler wrote: >>>> Am 10.12.2012 21:22, schrieb Lars-Peter Clausen: >>>>> On 12/10/2012 08:45 PM, Alexander Holler wrote: >>>>>> Am 10.12.2012 18:05, schrieb Lars-Peter Clausen: >>>>>> >>>>>>> Looks pretty good now. But there are still some IIO remnants >>>>>>> which should be >>>>>>> removed as well. Also the driver should move to drivers/rtc/ >>>>>>> since, well, >>>>>>> it's a rtc driver not a IIO driver. >>>>>> >>>>>> I think it still should be stick to iio, because that is where all >>>>>> HID >>>>>> sensors currently are found and where the user would expect to >>>>>> find such >>>>>> a driver. >>>>> >>>>> That's because all the current IIO sensor drivers fall in the IIO >>>>> domain. This >>>>> one clearly is a RTC driver, so it belongs in drivers/rtc/ >>>> >>>> Where nobody will find it if he searches for drivers for his HID >>>> sensor. >>>> I still see it as HID sensor driver and not a rtc-driver. >>>> But ... >>> >>> I can understand your position, but drivers are usually grouped by >>> function not >>> by topology. If there is a proper Kconfig help text people should >>> hopefully be >>> find it. >> Seconded on this. If it is a pure rtc driver then it definitely >> belongs in >> drivers/rtc. Now there might have been ways of doing this as a >> consumer / provider >> with the provider being in IIO and the consumer in rtc, but that >> sounds like >> it is over compicating things, at least for now. > > Personally I just want to use it to have HID-USB_RTCs. ;) > > But in case of HID sensor hubs, the main usage of that driver will be to > set the time of such hubs (something I still have to make patches for), > and not to read it. So if you think as an HID sensor user, it should > belong to iio or wherever those sensor will finally end up (I think they > should end up in HID and should be usable by bluetooth devices too), but > if you creatively misuse the standard to get a driver for USB-RTCs, as I > want, then rtc is the correct place. ;) I did wonder what you were up to ;) > > Because I don't want to do a v4: > > the driver has an > > #include "../common/hid-sensors/hid-sensor-attributes.h" > > so moving it to drivers/rtc/ will make that even more ugly. > > Suggestions? I don't really care and as I'm currently at the order > receiving end ;) I would change it to whatever the maintainers are > wanting. Maybe moving that header to include/linux or even integrating > it into include/linux/hid-sensor-hubs.h makes sense. Yes, move the header or merge into existing one as makes sense. I'm not pulling this driver into the IIO tree (unless for some reason Alessandro wants me to and I can't think why he would...). > > Regards, > > Alexander
On 12/11/2012 10:31 AM, Jonathan Cameron wrote: > On 10/12/12 22:50, Alexander Holler wrote: >> Am 10.12.2012 22:42, schrieb Jonathan Cameron: >>> On 12/10/2012 09:39 PM, Lars-Peter Clausen wrote: >>>> On 12/10/2012 10:26 PM, Alexander Holler wrote: >>>>> Am 10.12.2012 21:22, schrieb Lars-Peter Clausen: >>>>>> On 12/10/2012 08:45 PM, Alexander Holler wrote: >>>>>>> Am 10.12.2012 18:05, schrieb Lars-Peter Clausen: >>>>>>> >>>>>>>> Looks pretty good now. But there are still some IIO remnants >>>>>>>> which should be >>>>>>>> removed as well. Also the driver should move to drivers/rtc/ >>>>>>>> since, well, >>>>>>>> it's a rtc driver not a IIO driver. >>>>>>> >>>>>>> I think it still should be stick to iio, because that is where all >>>>>>> HID >>>>>>> sensors currently are found and where the user would expect to >>>>>>> find such >>>>>>> a driver. >>>>>> >>>>>> That's because all the current IIO sensor drivers fall in the IIO >>>>>> domain. This >>>>>> one clearly is a RTC driver, so it belongs in drivers/rtc/ >>>>> >>>>> Where nobody will find it if he searches for drivers for his HID >>>>> sensor. >>>>> I still see it as HID sensor driver and not a rtc-driver. >>>>> But ... >>>> >>>> I can understand your position, but drivers are usually grouped by >>>> function not >>>> by topology. If there is a proper Kconfig help text people should >>>> hopefully be >>>> find it. >>> Seconded on this. If it is a pure rtc driver then it definitely >>> belongs in >>> drivers/rtc. Now there might have been ways of doing this as a >>> consumer / provider >>> with the provider being in IIO and the consumer in rtc, but that >>> sounds like >>> it is over compicating things, at least for now. >> >> Personally I just want to use it to have HID-USB_RTCs. ;) >> >> But in case of HID sensor hubs, the main usage of that driver will be to >> set the time of such hubs (something I still have to make patches for), >> and not to read it. So if you think as an HID sensor user, it should >> belong to iio or wherever those sensor will finally end up (I think they >> should end up in HID and should be usable by bluetooth devices too), but >> if you creatively misuse the standard to get a driver for USB-RTCs, as I >> want, then rtc is the correct place. ;) > I did wonder what you were up to ;) >> >> Because I don't want to do a v4: >> >> the driver has an >> >> #include "../common/hid-sensors/hid-sensor-attributes.h" >> >> so moving it to drivers/rtc/ will make that even more ugly. >> > >> Suggestions? I don't really care and as I'm currently at the order >> receiving end ;) I would change it to whatever the maintainers are >> wanting. Maybe moving that header to include/linux or even integrating >> it into include/linux/hid-sensor-hubs.h makes sense. > Yes, move the header or merge into existing one as makes sense. > I'm not pulling this driver into the IIO tree (unless for some > reason Alessandro wants me to and I can't think why he would...). > Alessandro has been pretty quiet for quite some time now. Luckily Andrew Morton usually picks up the stuff for orphaned subsystems. So put him on Cc for v4. - Lars
On Tue, 11 Dec 2012 01:01:29 +0100 Alexander Holler <holler@ahsoftware.de> wrote: > Am 10.12.2012 23:36, schrieb Lars-Peter Clausen: > > Well, I've been exaggerating a bit, you can call it multiple times, but you > > Thanks a lot for the explanation(s). > > >>>> +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); > > Btw. I wouldn't call that cargo-cult-coding. It's more defensive > programming as people might not be sure, if there is something around > which still might access the platform data at that point. Ok, there > would be a need for a mutex or similiar if that really could happen, but > I wouldn't call such practices cargo-cult. ;) Agreed - pointer NULLing catches a lot of mistakes during teardown where timers or interrupts running in parallel get missed. Alan
Am 11.12.2012 10:40, schrieb Lars-Peter Clausen: >> Yes, move the header or merge into existing one as makes sense. >> I'm not pulling this driver into the IIO tree (unless for some >> reason Alessandro wants me to and I can't think why he would...). >> > > Alessandro has been pretty quiet for quite some time now. Luckily Andrew > Morton usually picks up the stuff for orphaned subsystems. So put him on Cc > for v4. Will do it. Thanks a lot for your review. I willl post the whole series (4 patches including the merge of hid-sensor-attributes.h) again, when I've finished v3 of the driver (hopefully this evening), marking some patches as RESEND. So 3 out of those 4 patches will be for iio (as hid-sensor-hub is part of it), and the last one, the rtc driver itself, will be for the rtc subsystem. I don't know if they have to be pulled by different maintainers. ;) Regards, Alexander
On 11/12/12 12:39, Alexander Holler wrote: > Am 11.12.2012 10:40, schrieb Lars-Peter Clausen: > >>> Yes, move the header or merge into existing one as makes sense. >>> I'm not pulling this driver into the IIO tree (unless for some >>> reason Alessandro wants me to and I can't think why he would...). >>> >> >> Alessandro has been pretty quiet for quite some time now. Luckily Andrew >> Morton usually picks up the stuff for orphaned subsystems. So put him >> on Cc >> for v4. > > Will do it. Thanks a lot for your review. > > I willl post the whole series (4 patches including the merge of > hid-sensor-attributes.h) again, when I've finished v3 of the driver > (hopefully this evening), marking some patches as RESEND. So 3 out of > those 4 patches will be for iio (as hid-sensor-hub is part of it), and > the last one, the rtc driver itself, will be for the rtc subsystem. I > don't know if they have to be pulled by different maintainers. ;) > > Regards, > > Alexander We'll see what Andrew says. We can take the lot through IIO (doubt Greg will mind) as long as we have the correct acks to do so (or at least statements of not caring ;)
On Tue, 11 Dec 2012 10:40:01 +0100 Lars-Peter Clausen <lars@metafoo.de> wrote: > > Yes, move the header or merge into existing one as makes sense. > > I'm not pulling this driver into the IIO tree (unless for some > > reason Alessandro wants me to and I can't think why he would...). > > > > Alessandro has been pretty quiet for quite some time now. Luckily Andrew > Morton usually picks up the stuff for orphaned subsystems. So put him on Cc > for v4. Still reading e-mails ;) Andrew monitors the rtc mailing list and picks up the patches that do not need to go in any arch's main tree, which is the suggested way as, with modern chipsets, the RTCs shares hw with many other drivers.
Am 16.12.2012 23:15, schrieb Alessandro Zummo: > On Tue, 11 Dec 2012 10:40:01 +0100 > Lars-Peter Clausen <lars@metafoo.de> wrote: > >>> Yes, move the header or merge into existing one as makes sense. >>> I'm not pulling this driver into the IIO tree (unless for some >>> reason Alessandro wants me to and I can't think why he would...). >>> >> >> Alessandro has been pretty quiet for quite some time now. Luckily Andrew >> Morton usually picks up the stuff for orphaned subsystems. So put him on Cc >> for v4. > > Still reading e-mails ;) Andrew monitors the rtc mailing list and picks up > the patches that do not need to go in any arch's main tree, which is the suggested > way as, with modern chipsets, the RTCs shares hw with many other drivers. Reading just the latest patch (v5) might be faster. ;) It basically doesn't do much. On call of read_time() a so called report with the time values (year, month, ...) is requested from the device, rtc_time is filled with them, done. Regards, Alexander
diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig index fc937ac..78fa3ff 100644 --- a/drivers/iio/Kconfig +++ b/drivers/iio/Kconfig @@ -63,5 +63,6 @@ source "drivers/iio/dac/Kconfig" source "drivers/iio/common/Kconfig" source "drivers/iio/gyro/Kconfig" source "drivers/iio/magnetometer/Kconfig" +source "drivers/iio/time/Kconfig" endif # IIO diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile index 761f2b6..6a6da31 100644 --- a/drivers/iio/Makefile +++ b/drivers/iio/Makefile @@ -19,3 +19,4 @@ obj-y += dac/ obj-y += common/ obj-y += gyro/ obj-y += magnetometer/ +obj-y += time/ diff --git a/drivers/iio/time/Kconfig b/drivers/iio/time/Kconfig new file mode 100644 index 0000000..0ca4682 --- /dev/null +++ b/drivers/iio/time/Kconfig @@ -0,0 +1,14 @@ +# +# Time sensors +# +menu "Time sensors" + +config HID_SENSOR_TIME + depends on HID_SENSOR_HUB + select HID_SENSOR_IIO_COMMON + tristate "HID Time" + help + Say yes here to build support for the HID SENSOR Time. + This drivers makes such sensors available as RTCs. + +endmenu diff --git a/drivers/iio/time/Makefile b/drivers/iio/time/Makefile new file mode 100644 index 0000000..705fe0d --- /dev/null +++ b/drivers/iio/time/Makefile @@ -0,0 +1,5 @@ +# +# Makefile for industrial I/O Time sensor driver +# + +obj-$(CONFIG_HID_SENSOR_TIME) += hid-sensor-time.o diff --git a/drivers/iio/time/hid-sensor-time.c b/drivers/iio/time/hid-sensor-time.c new file mode 100644 index 0000000..556bac9 --- /dev/null +++ b/drivers/iio/time/hid-sensor-time.c @@ -0,0 +1,346 @@ +/* + * HID Sensor Time Driver + * Copyright (c) 2012, Alexander Holler. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along with + * this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA. + * + */ +#include <linux/device.h> +#include <linux/platform_device.h> +#include <linux/module.h> +#include <linux/hid-sensor-hub.h> +#include <linux/iio/iio.h> +#include <linux/rtc.h> +#include "../common/hid-sensors/hid-sensor-attributes.h" + +/* Format: HID-SENSOR-usage_id_in_hex */ +/* Usage ID from spec for Time: 0x2000A0 */ +#define DRIVER_NAME "HID-SENSOR-2000a0" /* must be lowercase */ + +enum hid_time_channel { + CHANNEL_SCAN_INDEX_YEAR, + CHANNEL_SCAN_INDEX_MONTH, + CHANNEL_SCAN_INDEX_DAY, + CHANNEL_SCAN_INDEX_HOUR, + CHANNEL_SCAN_INDEX_MINUTE, + CHANNEL_SCAN_INDEX_SECOND, + TIME_RTC_CHANNEL_MAX, +}; + +struct hid_time_state { + struct hid_sensor_hub_callbacks callbacks; + struct hid_sensor_iio_common common_attributes; + struct hid_sensor_hub_attribute_info info[TIME_RTC_CHANNEL_MAX]; + struct rtc_time last_time; + spinlock_t lock_last_time; + struct completion comp_last_time; + struct rtc_time time_buf; + struct rtc_device *rtc; +}; + +static const u32 hid_time_addresses[TIME_RTC_CHANNEL_MAX] = { + HID_USAGE_SENSOR_TIME_YEAR, + HID_USAGE_SENSOR_TIME_MONTH, + HID_USAGE_SENSOR_TIME_DAY, + HID_USAGE_SENSOR_TIME_HOUR, + HID_USAGE_SENSOR_TIME_MINUTE, + HID_USAGE_SENSOR_TIME_SECOND, +}; + +/* Channel definitions */ +static const struct iio_chan_spec hid_time_channels[TIME_RTC_CHANNEL_MAX] = { + { + .info_mask = IIO_CHAN_INFO_RAW, + .scan_index = CHANNEL_SCAN_INDEX_YEAR, + .extend_name = "year", + }, { + .info_mask = IIO_CHAN_INFO_RAW, + .scan_index = CHANNEL_SCAN_INDEX_MONTH, + .extend_name = "month", + }, { + .info_mask = IIO_CHAN_INFO_RAW, + .scan_index = CHANNEL_SCAN_INDEX_DAY, + .extend_name = "day", + }, { + .info_mask = IIO_CHAN_INFO_RAW, + .scan_index = CHANNEL_SCAN_INDEX_HOUR, + .extend_name = "hour", + }, { + .info_mask = IIO_CHAN_INFO_RAW, + .scan_index = CHANNEL_SCAN_INDEX_MINUTE, + .extend_name = "minute", + }, { + .info_mask = IIO_CHAN_INFO_RAW, + .scan_index = CHANNEL_SCAN_INDEX_SECOND, + .extend_name = "second", + } +}; + +static int hid_time_read_raw(struct hid_time_state *time_state, + struct iio_chan_spec const *chan, + int *val, int *val2, + long mask) +{ + int report_id; + u32 address; + + *val = 0; + *val2 = 0; + if (mask) + return -EINVAL; + report_id = time_state->info[chan->scan_index].report_id; + address = hid_time_addresses[chan->scan_index]; + if (report_id >= 0) { + *val = sensor_hub_input_attr_get_raw_value( + time_state->common_attributes.hsdev, + HID_USAGE_SENSOR_TIME, address, report_id); + return IIO_VAL_INT; + } + *val = 0; + return -EINVAL; +} + +/* Callback handler to send event after all samples are received and captured */ +static int hid_time_proc_event(struct hid_sensor_hub_device *hsdev, + unsigned usage_id, void *priv) +{ + unsigned long flags; + struct hid_time_state *time_state = platform_get_drvdata(priv); + + spin_lock_irqsave(&time_state->lock_last_time, flags); + time_state->last_time = time_state->time_buf; + spin_unlock_irqrestore(&time_state->lock_last_time, flags); + complete(&time_state->comp_last_time); + return 0; +} + +static int hid_time_capture_sample(struct hid_sensor_hub_device *hsdev, + unsigned usage_id, size_t raw_len, + char *raw_data, void *priv) +{ + struct hid_time_state *time_state = platform_get_drvdata(priv); + struct rtc_time *time_buf = &time_state->time_buf; + + switch (usage_id) { + case HID_USAGE_SENSOR_TIME_YEAR: + time_buf->tm_year = *(u8 *)raw_data; + if (time_buf->tm_year < 70) + /* assume we are in 1970...2069 */ + time_buf->tm_year += 100; + break; + case HID_USAGE_SENSOR_TIME_MONTH: + time_buf->tm_mon = --*(u8 *)raw_data; + break; + case HID_USAGE_SENSOR_TIME_DAY: + time_buf->tm_mday = *(u8 *)raw_data; + break; + case HID_USAGE_SENSOR_TIME_HOUR: + time_buf->tm_hour = *(u8 *)raw_data; + break; + case HID_USAGE_SENSOR_TIME_MINUTE: + time_buf->tm_min = *(u8 *)raw_data; + break; + case HID_USAGE_SENSOR_TIME_SECOND: + time_buf->tm_sec = *(u8 *)raw_data; + break; + default: + return -EINVAL; + } + return 0; +} + +/* small helper, haven't found any other way */ +static const char *attrib_name(u32 attrib_id) +{ + unsigned i = 0; + static const char unknown[] = "unknown"; + + for (; i < TIME_RTC_CHANNEL_MAX; ++i) { + if (hid_time_addresses[i] == attrib_id) + return hid_time_channels[i].extend_name; + } + return unknown; /* should never happen */ +} + +static int hid_time_parse_report(struct platform_device *pdev, + struct hid_sensor_hub_device *hsdev, + unsigned usage_id, + struct hid_time_state *time_state) +{ + int ret, i = 0; + + for (; i < TIME_RTC_CHANNEL_MAX; ++i) { + ret = sensor_hub_input_get_attribute_info(hsdev, + HID_INPUT_REPORT, usage_id, + hid_time_addresses[i], &time_state->info[i]); + if (ret < 0) + return ret; + } + /* Check the (needed) attributes for sanity */ + ret = time_state->info[0].report_id; /* use ret as temp. */ + if (ret < 0) { + dev_err(&pdev->dev, "bad report ID!\n"); + return -EINVAL; + } + for (i = 0; i < TIME_RTC_CHANNEL_MAX; ++i) { + if (time_state->info[i].report_id != ret) { + dev_err(&pdev->dev, + "not all needed attributes inside the same report!\n"); + return -EINVAL; + } + if (time_state->info[i].size != 1) { + dev_err(&pdev->dev, + "attribute '%s' not 8 bits wide!\n", + attrib_name(time_state->info[i].attrib_id)); + return -EINVAL; + } + if (time_state->info[i].units != + HID_USAGE_SENSOR_UNITS_NOT_SPECIFIED && + /* allow attribute seconds with unit seconds */ + !(time_state->info[i].attrib_id == + HID_USAGE_SENSOR_TIME_SECOND && + time_state->info[i].units == + HID_USAGE_SENSOR_UNITS_SECOND)) { + dev_err(&pdev->dev, + "attribute '%s' hasn't a unit of type 'none'!\n", + attrib_name(time_state->info[i].attrib_id)); + return -EINVAL; + } + if (time_state->info[i].unit_expo) { + dev_err(&pdev->dev, + "attribute '%s' hasn't a unit exponent of 1!\n", + attrib_name(time_state->info[i].attrib_id)); + return -EINVAL; + } + } + + return 0; +} + +static int hid_rtc_read_time(struct device *dev, + struct rtc_time *tm) +{ + int val; + unsigned long flags; + struct hid_time_state *time_state = + platform_get_drvdata(to_platform_device(dev)); + + init_completion(&time_state->comp_last_time); + /* start a read */ + if (hid_time_read_raw(time_state, + &hid_time_channels[0], &val, &val, 0) == -EINVAL) { + dev_err(dev, "unable to read time!\n"); + return -EIO; + } + /* wait for all values (event) */ + wait_for_completion_interruptible_timeout(&time_state->comp_last_time, + HZ*6); + spin_lock_irqsave(&time_state->lock_last_time, flags); + *tm = time_state->last_time; + spin_unlock_irqrestore(&time_state->lock_last_time, flags); + + return 0; +} + +static const struct rtc_class_ops rtc_ops = { + .read_time = hid_rtc_read_time, +}; + +static int __devinit hid_time_probe(struct platform_device *pdev) +{ + int ret = 0; + struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data; + struct hid_time_state *time_state = + kzalloc(sizeof(struct hid_time_state), GFP_KERNEL); + + if (time_state == NULL) { + ret = -ENOMEM; + goto error_ret; + } + + platform_set_drvdata(pdev, time_state); + + time_state->common_attributes.hsdev = hsdev; + time_state->common_attributes.pdev = pdev; + + ret = hid_sensor_parse_common_attributes(hsdev, + HID_USAGE_SENSOR_TIME, + &time_state->common_attributes); + if (ret) { + dev_err(&pdev->dev, "failed to setup common attributes!\n"); + goto error_free_drvdata; + } + + ret = hid_time_parse_report(pdev, hsdev, /*channels,*/ + HID_USAGE_SENSOR_TIME, time_state); + if (ret) { + dev_err(&pdev->dev, "failed to setup attributes!\n"); + goto error_free_drvdata; + } + + time_state->callbacks.send_event = hid_time_proc_event; + time_state->callbacks.capture_sample = hid_time_capture_sample; + time_state->callbacks.pdev = pdev; + ret = sensor_hub_register_callback(hsdev, HID_USAGE_SENSOR_TIME, + &time_state->callbacks); + if (ret < 0) { + dev_err(&pdev->dev, "register callback failed!\n"); + goto error_free_drvdata; + } + + time_state->rtc = rtc_device_register("hid-sensor-time", + &pdev->dev, &rtc_ops, THIS_MODULE); + + if (IS_ERR(time_state->rtc)) { + ret = PTR_ERR(time_state->rtc); + dev_err(&pdev->dev, "rtc device register failed!\n"); + goto error_free_drvdata; + } + + return ret; + +error_free_drvdata: + platform_set_drvdata(pdev, NULL); + 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); + kfree(time_state); + + return 0; +} + +static struct platform_driver hid_time_platform_driver = { + .driver = { + .name = DRIVER_NAME, + .owner = THIS_MODULE, + }, + .probe = hid_time_probe, + .remove = hid_time_remove, +}; +module_platform_driver(hid_time_platform_driver); + +MODULE_DESCRIPTION("HID Sensor Time"); +MODULE_AUTHOR("Alexander Holler <holler@ahsoftware.de>"); +MODULE_LICENSE("GPL");
This driver makes the time from HID sensors (hubs) which are offering such available like any other RTC does. Currently the time can only be read. Setting the time must be done through sending a report, which currently isn't supported by hid-sensor-hub. It is necessary that all values like year, month etc, are send as 8bit values (1 byte each) and all of them in 1 report. Also the spec HUTRR39b doesn't define the range of the year field, we tread it as 0 - 99 because that's what most RTCs I know about are offering. Signed-off-by: Alexander Holler <holler@ahsoftware.de> --- drivers/iio/Kconfig | 1 + drivers/iio/Makefile | 1 + drivers/iio/time/Kconfig | 14 ++ drivers/iio/time/Makefile | 5 + drivers/iio/time/hid-sensor-time.c | 346 ++++++++++++++++++++++++++++++++++++ 5 files changed, 367 insertions(+), 0 deletions(-) create mode 100644 drivers/iio/time/Kconfig create mode 100644 drivers/iio/time/Makefile create mode 100644 drivers/iio/time/hid-sensor-time.c