Hi Linux gurus,
Attached is a patch for the QTI DirecTEMP USB thermometer &
thermometer/hygrometer sensors. This patch is based on linux-2.6.32.
Functionality has been verified against both hardware variants listed in
the driver (0x0002 and 0x0006), as both monolithic and modular.
When the QTI DirecTEMP sensor is connected to a system, the directemp
driver adds appropriate sysfs entries for the sensor type(s) supported.
Examples:
# PID 0x0002 (temp only)
/sys/bus/usb/devices/.../temp
# PID 0x0006 (temp + relative humidity)
/sys/bus/usb/devices/.../temp
/sys/bus/usb/devices/.../rh
Using a standard "cat" will display the value.
An additional "raw" sysfs entry has been added to aid in debugging. If
used, an "echo" will send the data specified in the string to the USB
device, and print back the results. It is not recommended for customer
use except by expert users.
And with that description out of the way, I look forward to your review
of the driver. Please provide any feedback that you may have.
Thanks in advance,
Chris
On Thu, 4 Feb 2010, Chris Verges wrote:
> Hi Linux gurus,
>
> Attached is a patch for the QTI DirecTEMP USB thermometer &
> thermometer/hygrometer sensors. This patch is based on linux-2.6.32.
> Functionality has been verified against both hardware variants listed in
> the driver (0x0002 and 0x0006), as both monolithic and modular.
>
> When the QTI DirecTEMP sensor is connected to a system, the directemp
> driver adds appropriate sysfs entries for the sensor type(s) supported.
> Examples:
>
> # PID 0x0002 (temp only)
> /sys/bus/usb/devices/.../temp
>
> # PID 0x0006 (temp + relative humidity)
> /sys/bus/usb/devices/.../temp
> /sys/bus/usb/devices/.../rh
>
> Using a standard "cat" will display the value.
This isn't a big deal... but it's more common for drivers to provide
regular device files for I/O than to rely on sysfs attributes. You end
up with more control and flexibility.
Alan Stern
On Thu, Feb 04, 2010 at 08:47:05PM -0800, Chris Verges wrote:
> Hi Linux gurus,
>
> Attached is a patch for the QTI DirecTEMP USB thermometer &
> thermometer/hygrometer sensors. This patch is based on linux-2.6.32.
> Functionality has been verified against both hardware variants listed in
> the driver (0x0002 and 0x0006), as both monolithic and modular.
>
> When the QTI DirecTEMP sensor is connected to a system, the directemp
> driver adds appropriate sysfs entries for the sensor type(s) supported.
> Examples:
>
> # PID 0x0002 (temp only)
> /sys/bus/usb/devices/.../temp
>
> # PID 0x0006 (temp + relative humidity)
> /sys/bus/usb/devices/.../temp
> /sys/bus/usb/devices/.../rh
>
> Using a standard "cat" will display the value.
>
> An additional "raw" sysfs entry has been added to aid in debugging. If
> used, an "echo" will send the data specified in the string to the USB
> device, and print back the results. It is not recommended for customer
> use except by expert users.
Is a kernel driver really needed for this device? You could do this all
in userspace with libusb/usbfs and not need a driver, right?
That's what we do for other USB thermometers today.
thanks,
greg k-h
> Is a kernel driver really needed for this device? You could do this
all
> in userspace with libusb/usbfs and not need a driver, right?
>
> That's what we do for other USB thermometers today.
Hi Greg,
I assumed that there were few to no USB thermometers in use with Linux.
Looking at the kernel, there seems to be Cypress USB thermometer driver
listed, but it's for a hobby project that isn't actually a product.
Based on the cytherm driver, I figured support was to be added to the
kernel.
I'd be happy to read up on libusb/usbfs. Thanks for introducing it.
For anyone who wants DirecTEMP support before a port to libusb/usbfs is
done (no ETA), feel free to use this driver.
Thanks,
Chris
On Thu, 4 Feb 2010 20:47:05 -0800
"Chris Verges" <[email protected]> wrote:
> Hi Linux gurus,
>
> Attached is a patch for the QTI DirecTEMP USB thermometer &
> thermometer/hygrometer sensors. This patch is based on linux-2.6.32.
> Functionality has been verified against both hardware variants listed in
> the driver (0x0002 and 0x0006), as both monolithic and modular.
>
> When the QTI DirecTEMP sensor is connected to a system, the directemp
> driver adds appropriate sysfs entries for the sensor type(s) supported.
> Examples:
>
> # PID 0x0002 (temp only)
> /sys/bus/usb/devices/.../temp
>
> # PID 0x0006 (temp + relative humidity)
> /sys/bus/usb/devices/.../temp
> /sys/bus/usb/devices/.../rh
>
> Using a standard "cat" will display the value.
Seems a bit strange that a device driver's only interface is via sysfs.
Once upon a time people used /dev. Ho hum.
It would be useful if the changelog were to describe what the contents
of the sysfs files look like. That's an important part of the user
interface and the user interface is the most important part of the
driver, because we can't change that.
afacit the `temp' file displays something like "44 C", yes? If so,
that's a bit awkward for software parsing. It'd be clearer to rename
`temp' to `temperature_in_centigrade" or simply "centigrade" and then
display "44", and remove the units from the output.
Ditto the "rh" file, which presently contains "42%".
> An additional "raw" sysfs entry has been added to aid in debugging. If
> used, an "echo" will send the data specified in the string to the USB
> device, and print back the results. It is not recommended for customer
> use except by expert users.
>
> And with that description out of the way, I look forward to your review
> of the driver. Please provide any feedback that you may have.
>
Have a review-by-patch:
- `pid' is a well-understood term for a process ID. Rename it.
- clean up some memsets
- Change the `raw' file's permissions to S_IWUSR. We shouldn't permit
unprivileged users to cause a printk spew into the logs.
--- a/drivers/usb/misc/directemp.c~usb-qti-directemp-usb-thermometer-hygrometer-driver-support-fix
+++ a/drivers/usb/misc/directemp.c
@@ -40,7 +40,7 @@
struct usb_directemp {
struct usb_device *udev;
struct usb_interface *interface;
- uint16_t pid;
+ uint16_t product_id;
uint8_t read_temp;
};
@@ -114,7 +114,7 @@ static ssize_t show_temp(struct device *
/* Read twice due to a buffer issue on the DirecTEMP */
for (i = 0; i < DIRECTEMP_MAX_RETRIES; i++) {
- memset(data, 0, (DIRECTEMP_MSG_SIZE + 1) * sizeof(char));
+ memset(data, 0, sizeof(data));
data[0] = directemp->read_temp;
err = show_helper(dev, attr, data, data, DIRECTEMP_MSG_SIZE);
if (err < 0)
@@ -156,7 +156,7 @@ static ssize_t show_rh(struct device *de
/* Read twice due to a buffer issue on the DirecTEMP */
for (i = 0; i < DIRECTEMP_MAX_RETRIES; i++) {
- memset(data, 0, (DIRECTEMP_MSG_SIZE + 1) * sizeof(char));
+ memset(data, 0, sizeof(data));
data[0] = DIRECTEMP_HUMIDITY;
err = show_helper(dev, attr, data, data, DIRECTEMP_MSG_SIZE);
if (err < 0)
@@ -199,7 +199,7 @@ static ssize_t set_raw(struct device *de
if (count > DIRECTEMP_MSG_SIZE)
count = DIRECTEMP_MSG_SIZE;
- memset(data, 0, (DIRECTEMP_MSG_SIZE + 1) * sizeof(char));
+ memset(data, 0, sizeof(data));
memcpy(data, buf, count);
printk("Request:\n");
@@ -219,7 +219,7 @@ static ssize_t set_raw(struct device *de
return 0;
}
-static DEVICE_ATTR(raw, S_IWUGO, NULL, set_raw);
+static DEVICE_ATTR(raw, S_IWUSR, NULL, set_raw);
static int directemp_probe(struct usb_interface *interface,
const struct usb_device_id *id)
@@ -234,10 +234,10 @@ static int directemp_probe(struct usb_in
dev->udev = usb_get_dev(udev);
usb_set_intfdata(interface, dev);
- dev->pid = id->idProduct;
+ dev->product_id = id->idProduct;
dev_dbg(&interface->dev, "Product: %s (ID 0x%04X)\n",
- udev->product, dev->pid);
+ udev->product, dev->product_id);
dev_dbg(&interface->dev, "Manufacturer: %s\n",
udev->manufacturer);
dev_dbg(&interface->dev, "Serial Number: %s\n",
@@ -253,7 +253,7 @@ static int directemp_probe(struct usb_in
if (err)
goto exit_free_sysfs;
- switch (dev->pid) {
+ switch (dev->product_id) {
case 0x0002:
dev->read_temp = '2';
break;
_
> *snip*
Hi Andrew,
Thanks for the technical review. I'll make the changes and send back a
new patch. Would you like a patch against your -mm tree or a fresh
patch against linux-2.6.32?
Chris
On Fri, 5 Feb 2010 13:32:27 -0800
"Chris Verges" <[email protected]> wrote:
> > *snip*
>
> Hi Andrew,
>
> Thanks for the technical review. I'll make the changes and send back a
> new patch. Would you like a patch against your -mm tree or a fresh
> patch against linux-2.6.32?
I guess a new patch would be best - #1 was a bit of a mess. Please
give the patch a good title (I chose "usb: QTI DirecTEMP USB
thermometer/hygrometer driver support") and if possible, include it
within the body of the email so more people are likely to look at it.
If your email client can't do that correctly, use a text/plain
attachment.
Thanks.
On Fri, Feb 05, 2010 at 01:46:00PM -0800, Andrew Morton wrote:
> On Fri, 5 Feb 2010 13:32:27 -0800
> "Chris Verges" <[email protected]> wrote:
>
> > > *snip*
> >
> > Hi Andrew,
> >
> > Thanks for the technical review. I'll make the changes and send back a
> > new patch. Would you like a patch against your -mm tree or a fresh
> > patch against linux-2.6.32?
>
> I guess a new patch would be best - #1 was a bit of a mess. Please
> give the patch a good title (I chose "usb: QTI DirecTEMP USB
> thermometer/hygrometer driver support") and if possible, include it
> within the body of the email so more people are likely to look at it.
> If your email client can't do that correctly, use a text/plain
> attachment.
Sorry, but no, this driver will not be accepted, as it can be done just
fine from userspace instead of a kernel driver, as discussed before.
We have removed drivers from the kernel for this very reason, we don't
want to add new ones in with the same problem.
thanks,
greg k-h
> Sorry, but no, this driver will not be accepted, as it can be done
just
> fine from userspace instead of a kernel driver, as discussed before.
Hi Greg,
Sounds good. I'll still be sending out an updated patch for anyone who
is interested in a kernel driver. They're welcome to patch in the
driver themselves.
I may be missing some key piece of information about libusb and usbfs,
but it seems like it pushes a lot of the protocol communication off to
the user app. So if there are several user apps that want to use the
same USB device, they either need a userland library or to re-implement
functionality; is that correct?
What I may be missing is the rationale behind pushing these drivers into
userland libraries and having yet another entity in the FOSS world that
is responsible for managing them. The kernel seems like an obvious
clearinghouse for software/hardware interactions. Yes, there may be
lots of drivers, but at least everyone knows where to go for them. But
like I said before, I may be missing something.
Thanks for the tips about libusb/usbfs!
Chris
On Fri, Feb 05, 2010 at 04:26:42PM -0800, Chris Verges wrote:
> > Sorry, but no, this driver will not be accepted, as it can be done
> just
> > fine from userspace instead of a kernel driver, as discussed before.
>
> Hi Greg,
>
> Sounds good. I'll still be sending out an updated patch for anyone who
> is interested in a kernel driver. They're welcome to patch in the
> driver themselves.
That's great.
> I may be missing some key piece of information about libusb and usbfs,
> but it seems like it pushes a lot of the protocol communication off to
> the user app. So if there are several user apps that want to use the
> same USB device, they either need a userland library or to re-implement
> functionality; is that correct?
Yes, that is true.
> What I may be missing is the rationale behind pushing these drivers into
> userland libraries and having yet another entity in the FOSS world that
> is responsible for managing them. The kernel seems like an obvious
> clearinghouse for software/hardware interactions. Yes, there may be
> lots of drivers, but at least everyone knows where to go for them. But
> like I said before, I may be missing something.
No, you are correct. We want a driver in the kernel when it provides a
common interface to a class of devices (network, tty, video, etc.) For
devices like yours, there is no specific class in the kernel (well,
there is for hardware monitoring devices, but not for generic
thermometers.) So for that, you are going to write a custom userspace
program anyway to be reading the temp value from sysfs, so you might as
well just either use a library to talk to your device, or put it within
the application itself.
Hope this helps explain things,
greg k-h
On Thu, 04 February 2010 "Chris Verges" wrote:
> Attached is a patch for the QTI DirecTEMP USB thermometer &
> thermometer/hygrometer sensors. This patch is based on linux-2.6.32.
> Functionality has been verified against both hardware variants listed
> in the driver (0x0002 and 0x0006), as both monolithic and modular.
>
> When the QTI DirecTEMP sensor is connected to a system, the directemp
> driver adds appropriate sysfs entries for the sensor type(s)
> supported. Examples:
>
> # PID 0x0002 (temp only)
> /sys/bus/usb/devices/.../temp
>
> # PID 0x0006 (temp + relative humidity)
> /sys/bus/usb/devices/.../temp
> /sys/bus/usb/devices/.../rh
>
> Using a standard "cat" will display the value.
Wouldn't it make sense to make use of hardware monitoring interfaces so
lm_sensors could handle the sensor information? (added to CC)
This would also be a good reason to have the driver on kernel side
rather than somewhere in userspace.
Bruno