| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 12 Dec 2012 10:51:24 +0100
From: Lars-Peter Clausen <lars@...afoo.de>
To: Alexander Holler <holler@...oftware.de>
CC: linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
Jonathan Cameron <jic23@....ac.uk>, rtc-linux@...glegroups.com,
Alessandro Zummo <a.zummo@...ertech.it>,
srinivas pandruvada <srinivas.pandruvada@...el.com>,
Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH 4/4 v3] rtc: add rtc-driver for HID sensors of type time
On 12/11/2012 07:21 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. (I've planned to submit patches.)
>
> 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@...oftware.de>
Looks good, but as I wrote during the last review the __devinits need to go.
A few other suggerstions online
> ---
> drivers/rtc/Kconfig | 16 ++
> drivers/rtc/Makefile | 1 +
> drivers/rtc/rtc-hid-sensor-time.c | 284 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 301 insertions(+), 0 deletions(-)
> create mode 100644 drivers/rtc/rtc-hid-sensor-time.c
>
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 19c03ab..7c7b33e 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1144,4 +1144,20 @@ config RTC_DRV_SNVS
> This driver can also be built as a module, if so, the module
> will be called "rtc-snvs".
>
> +comment "HID Sensor RTC drivers"
> +
> +config RTC_DRV_HID_SENSOR_TIME
> + tristate "HID Sensor Time"
> + depends on USB_HID
> + select IIO
> + select HID_SENSOR_HUB
> + select HID_SENSOR_IIO_COMMON
> + help
> + Say yes here to build support for the HID Sensors of type Time.
> + This drivers makes such sensors available as RTCs.
> +
> + If this driver is compiled as a module, it will be named
> + rtc-hid-sensor-time.
The help text should be indented using one tab followed by two spaces
> +
> +
> endif # RTC_CLASS
[...]
> diff --git a/drivers/rtc/rtc-hid-sensor-time.c b/drivers/rtc/rtc-hid-sensor-time.c
> new file mode 100644
> index 0000000..42b7ba1
> --- /dev/null
> +++ b/drivers/rtc/rtc-hid-sensor-time.c
> @@ -0,0 +1,284 @@
[...]
> +
> +/* Channel names for verbose error messages */
> +static const char *hid_time_channel_names[TIME_RTC_CHANNEL_MAX] = {
const char * const
> + "year", "month", "day", "hour", "minute", "second",
> +};
> +
> +/* 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;
What's up with the double minus?
> + 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) {
I would put the i = 0 in the for header.
> + if (hid_time_addresses[i] == attrib_id)
> + return hid_time_channel_names[i];
> + }
> + 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) {
Same here
> + 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. */
As I said last time, report_id instead of ret may be a better name for the
variable
> + 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 __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))
This will never be false, so you can just run unregister unconditionally
> + rtc_device_unregister(time_state->rtc);
> + sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TIME);
> +
> + return 0;
> +}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists