On 30/04/13(Tue) 17:39, Stuart Henderson wrote: > On 2013/04/30 15:06, Stuart Henderson wrote: > > On 2013/03/31 17:56, SASANO Takayoshi wrote: > > > Hello, > > > > > > I rewrote patched uthum(4) to new ugold(4) driver. > > > Thanks for advice by yuo@ and deraadt@.
Some comments inline. > Index: dev/usb/ugold.c > =================================================================== > [...] > +/* Driver for Microdia's HID base TEMPer Temperature sensor */ > + > +#include <sys/param.h> > +#include <sys/proc.h> > +#include <sys/systm.h> > +#include <sys/kernel.h> > +#include <sys/malloc.h> > +#include <sys/device.h> > +#include <sys/conf.h> > +#include <sys/sensors.h> > + > +#include <dev/usb/usb.h> > +#include <dev/usb/usbhid.h> > +#include <dev/usb/usbdi.h> > +#include <dev/usb/usbdi_util.h> > +#include <dev/usb/usbdevs.h> > +#include <dev/usb/uhidev.h> > +#include <dev/usb/hid.h> Please make sure you include only the required headers, at least proc.h is not needed. > + > +#ifdef USB_DEBUG > +#define UGOLD_DEBUG > +#endif > + > +#ifdef UGOLD_DEBUG > +int ugolddebug = 0; > +#define DPRINTFN(n, x) do { if (ugolddebug > (n)) printf x; } while (0) > +#else > +#define DPRINTFN(n, x) > +#endif > + > +#define DPRINTF(x) DPRINTFN(0, x) If you don't use the *N() variant, just keep it simple a only define the DPRINTF() macro, this will also allow you to remove the ugolddebug variable. > + > +/* Device types */ > +#define UGOLD_TYPE_TEMPER 0 > +#define UGOLD_TYPE_UNKNOWN 0xffff > + > +/* sensors */ > +#define UGOLD_TEMPER_INNER 0 > +#define UGOLD_TEMPER_OUTER 1 > +#define UGOLD_MAX_SENSORS 2 > + > +enum ugold_sensor_type { > + UGOLD_SENSOR_UNKNOWN, > + UGOLD_SENSOR_DS75, > + UGOLD_SENSOR_MAXTYPES, > +}; > + > +static const char * const ugold_sensor_type_s[UGOLD_SENSOR_MAXTYPES] = { > + "unknown", > + "ds75/12bit", > +}; > + > +static uint8_t cmd_led_off[2] = > + { 0x01, 0x00 }; > +static uint8_t cmd_get_offset[8] = > + { 0x01, 0x82, 0x77, 0x01, 0x00, 0x00, 0x00, 0x00 }; > +static uint8_t cmd_init[8] = > + { 0x01, 0x86, 0xff, 0x01, 0x00, 0x00, 0x00, 0x00 }; > +static uint8_t cmd_get_data[8] = > + { 0x01, 0x80, 0x33, 0x01, 0x00, 0x00, 0x00, 0x00 }; > + > +struct ugold_sensor { > + struct ksensor sensor; > + int cal_offset; /* 10mC or m%RH */ > + int attached; > + enum ugold_sensor_type dev_type; > +}; > + > +struct ugold_softc { > + struct uhidev sc_hdev; > + usbd_device_handle sc_udev; > + u_char sc_dying; You don't use sc_dying so you can probably remove it and the *activate() function also. > + uint16_t sc_flag; This field is also unused. > + int sc_device_type; Because you're attaching only one kind of device there's no need for this field. I suggest you to remove it and all the unneeded logic that comes with it. > + int sc_initialized; > + int sc_num_sensors; > + > + /* uhidev parameters */ > + size_t sc_flen; /* feature report length */ > + size_t sc_ilen; /* input report length */ > + size_t sc_olen; /* output report length */ > + > + uint8_t *sc_ibuf; > + > + /* sensor framework */ > + struct ugold_sensor sc_sensor[UGOLD_MAX_SENSORS]; > + struct ksensordev sc_sensordev; > + struct sensor_task *sc_sensortask; > +}; > + > +const struct usb_devno ugold_devs[] = { > + { USB_VENDOR_MICRODIA, USB_PRODUCT_MICRODIA_TEMPER }, > +}; > +#define ugold_lookup(v, p) usb_lookup(ugold_devs, v, p) > + > +int ugold_match(struct device *, void *, void *); > +void ugold_attach(struct device *, struct device *, void *); > +int ugold_detach(struct device *, int); > +int ugold_activate(struct device *, int); > + > +int ugold_issue_cmd(struct ugold_softc *, uint8_t *, int, int); > +int ugold_read_data(struct ugold_softc *); > +int ugold_init_device(struct ugold_softc *); > +void ugold_setup_sensors(struct ugold_softc *); > +int ugold_setup_device(struct ugold_softc *); > + > +void ugold_intr(struct uhidev *, void *, u_int); > +void ugold_refresh(void *); > +void ugold_refresh_temper(struct ugold_softc *); > + > +int ugold_ds75_temp(uint8_t, uint8_t); > +void ugold_print_sensorinfo(struct ugold_softc *, int); > + > +struct cfdriver ugold_cd = { > + NULL, "ugold", DV_DULL > +}; > + > +const struct cfattach ugold_ca = { > + sizeof(struct ugold_softc), > + ugold_match, > + ugold_attach, > + ugold_detach, > + ugold_activate, > +}; > + > +int > +ugold_match(struct device *parent, void *match, void *aux) > +{ > + struct usb_attach_arg *uaa = aux; > + struct uhidev_attach_arg *uha = (struct uhidev_attach_arg *)uaa; > + > + if (ugold_lookup(uha->uaa->vendor, uha->uaa->product) == NULL) > + return UMATCH_NONE; > + > + return (UMATCH_VENDOR_PRODUCT); > +} > + > +void > +ugold_attach(struct device *parent, struct device *self, void *aux) > +{ > + struct ugold_softc *sc = (struct ugold_softc *)self; > + struct usb_attach_arg *uaa = aux; > + struct uhidev_attach_arg *uha = (struct uhidev_attach_arg *)uaa; > + usbd_device_handle dev = uha->parent->sc_udev; > + int i, size, repid, err; > + void *desc; > + > + sc->sc_udev = dev; > + sc->sc_hdev.sc_intr = ugold_intr; > + sc->sc_hdev.sc_parent = uha->parent; > + sc->sc_hdev.sc_report_id = uha->reportid; > + sc->sc_initialized = 0; > + sc->sc_num_sensors = 0; > + > + uhidev_get_report_desc(uha->parent, &desc, &size); > + repid = uha->reportid; > + sc->sc_ilen = hid_report_size(desc, size, hid_input, repid); > + sc->sc_olen = hid_report_size(desc, size, hid_output, repid); > + sc->sc_flen = hid_report_size(desc, size, hid_feature, repid); > + > + printf("\n"); > + > + /* check device type */ > + if (uha->uaa->vendor == USB_VENDOR_MICRODIA && > + uha->uaa->product == USB_PRODUCT_MICRODIA_TEMPER) { > + sc->sc_device_type = UGOLD_TYPE_TEMPER; > + } else { > + sc->sc_device_type = UGOLD_TYPE_UNKNOWN; > + } > + > + if (sc->sc_flen < 8) { > + /* not sensor interface, just attach */ > + return; > + } > + > + /* sensor data comes from interrupt pipe */ > + err = uhidev_open(&sc->sc_hdev); > + if (err) { > + printf("ugold: uhidev_open %d\n", err); > + return; > + } > + sc->sc_ibuf = malloc(sc->sc_ilen, M_USBDEV, M_WAITOK); I'm not a big fan of using the same name "sc_ibuf" for your buffer as the one given to the USB framework, I find it really misleading... But it looks like it's done in a lot of drivers :/ But anyway, I think that in your case you don't need it. Why don't you update directly your sensor values when you receive the data in ugold_intr() instead of copying to a intermediate buffer? > + /* > + * interrupt pipe is opened but data comes after ugold_attach() > + * is finished. simply attach sensors here and the device will be > + * initialized at ugold_refresh(). > + * > + * at this point, the number of sensors is unknown. setup maximum > + * sensors here and detach unused sensor later. > + */ Why don't you initialize the device before opening the interrupt pipe? > + /* setup sensor */ > + strlcpy(sc->sc_sensordev.xname, sc->sc_hdev.sc_dev.dv_xname, > + sizeof(sc->sc_sensordev.xname)); > + ugold_setup_sensors(sc); > + > + /* attach sensors */ > + for (i = 0; i < UGOLD_MAX_SENSORS; i++) { > + sc->sc_sensor[i].sensor.flags |= SENSOR_FINVALID; > + sensor_attach(&sc->sc_sensordev, &sc->sc_sensor[i].sensor); > + sc->sc_sensor[i].attached = 1; > + sc->sc_num_sensors++; > + } > + > + /* 0.1Hz */ > + sc->sc_sensortask = sensor_task_register(sc, ugold_refresh, 6); > + if (sc->sc_sensortask == NULL) { > + printf(", unable to register update task\n"); > + return; > + } > + sensordev_install(&sc->sc_sensordev); > + > + DPRINTF(("ugold_attach: complete\n")); > +} > + > +int > +ugold_detach(struct device *self, int flags) > +{ > + struct ugold_softc *sc = (struct ugold_softc *)self; > + int i, rv = 0; > + > + if (sc->sc_num_sensors > 0) { > + wakeup(&sc->sc_sensortask); > + sensordev_deinstall(&sc->sc_sensordev); > + for (i = 0; i < UGOLD_MAX_SENSORS; i++) { > + if (sc->sc_sensor[i].attached) > + sensor_detach(&sc->sc_sensordev, > + &sc->sc_sensor[i].sensor); > + } > + if (sc->sc_sensortask != NULL) > + sensor_task_unregister(sc->sc_sensortask); > + } > + > + if (sc->sc_ibuf != NULL) { > + free(sc->sc_ibuf, M_USBDEV); > + sc->sc_ibuf = NULL; > + } > + > + return (rv); > +} > +void > +ugold_intr(struct uhidev *addr, void *ibuf, u_int len) > +{ > + struct ugold_softc *sc = (struct ugold_softc *)addr; > + > + if (sc->sc_ibuf == NULL) > + return; > + > + /* receive sensor data */ > + memcpy(sc->sc_ibuf, ibuf, len); > + > + return; > +} > + > +int > +ugold_issue_cmd(struct ugold_softc *sc, uint8_t *cmd, int len, int delay) > +{ > + usb_device_request_t req; > + > + bzero(sc->sc_ibuf, sc->sc_ilen); > + > + req.bmRequestType = UT_WRITE_CLASS_INTERFACE; > + req.bRequest = UR_SET_REPORT; > + USETW(req.wValue, 0x0200); > + USETW(req.wIndex, 0x0001); > + USETW(req.wLength, len); > + if (usbd_do_request(sc->sc_udev, &req, cmd)) > + return EIO; I would suggest you to have a look at uhidev_set_report{,_async}() instead of writing your own version here. > + /* wait if required */ > + if (delay > 0 && > + !tsleep(&sc->sc_sensortask, 0, "ugold", (delay*hz+999)/1000 + 1)) > + return EIO; > + > + return 0; > +} > + > +int > +ugold_init_device(struct ugold_softc *sc) > +{ > + usb_device_request_t req; > + usbd_status error; > + > + /* send SetReport request to another (Keyboard) interface */ > + req.bmRequestType = UT_WRITE_CLASS_INTERFACE; > + req.bRequest = UR_SET_REPORT; > + USETW(req.wValue, 0x0201); > + USETW(req.wIndex, 0x0000); > + USETW(req.wLength, sizeof(cmd_led_off)); > + error = usbd_do_request(sc->sc_udev, &req, cmd_led_off); > + if (error) > + return EIO; Same here, this is likely to be another uhidev_set_report() with a different interface, no? > + /* init process */ > + if (ugold_issue_cmd(sc, cmd_get_offset, sizeof(cmd_get_offset), 200)) > + return EIO; > + > + /* received one interrupt message, it contains offset parameter */ > + sc->sc_num_sensors = sc->sc_ibuf[1]; How can you be sure sc_ibuf contains the data you asked for? > + if (sc->sc_num_sensors > UGOLD_MAX_SENSORS) > + sc->sc_num_sensors = UGOLD_MAX_SENSORS; > + sc->sc_sensor[UGOLD_TEMPER_INNER].cal_offset = > + ((int8_t)sc->sc_ibuf[2] * 25) / 4; > + sc->sc_sensor[UGOLD_TEMPER_OUTER].cal_offset = > + ((int8_t)sc->sc_ibuf[3] * 25) / 4; > + > + if (ugold_issue_cmd(sc, cmd_init, sizeof(cmd_init), 200)) > + return EIO; > + > + /* received two interrupt messages, discard them */ > + > + return 0; > +} > + > +void > +ugold_setup_sensors(struct ugold_softc *sc) > +{ > + int i; > + > + for (i = 0; i < UGOLD_MAX_SENSORS; i++) > + sc->sc_sensor[i].dev_type = UGOLD_SENSOR_UNKNOWN; > + > + switch (sc->sc_device_type) { > + case UGOLD_TYPE_TEMPER: > + sc->sc_sensor[UGOLD_TEMPER_OUTER].dev_type = > + UGOLD_SENSOR_DS75; > + sc->sc_sensor[UGOLD_TEMPER_OUTER].sensor.type = > + SENSOR_TEMP; > + strlcpy(sc->sc_sensor[UGOLD_TEMPER_OUTER].sensor.desc, > + "outer", > + sizeof(sc->sc_sensor[UGOLD_TEMPER_OUTER].sensor.desc)); > + sc->sc_sensor[UGOLD_TEMPER_INNER].dev_type = > + UGOLD_SENSOR_DS75; > + sc->sc_sensor[UGOLD_TEMPER_INNER].sensor.type = > + SENSOR_TEMP; > + strlcpy(sc->sc_sensor[UGOLD_TEMPER_INNER].sensor.desc, > + "inner", > + sizeof(sc->sc_sensor[UGOLD_TEMPER_INNER].sensor.desc)); > + break; > + default: > + /* do nothing */ > + break; > + } > +} > + > +int > +ugold_setup_device(struct ugold_softc *sc) > +{ > + int i; > + > + if (ugold_init_device(sc)) { > + DPRINTF(("ugold: device initialize failed\n")); > + return EIO; > + } > + > + /* detach unused sensors */ > + for (i = 0; i < UGOLD_MAX_SENSORS; i++) { > + if (sc->sc_sensor[i].attached && > + i >= sc->sc_num_sensors) { > + sensor_detach(&sc->sc_sensordev, > + &sc->sc_sensor[i].sensor); > + sc->sc_sensor[i].attached = 0; > + } > + } > + > + for (i = 0; i < sc->sc_num_sensors; i++) { > + if (sc->sc_sensor[i].attached) > + ugold_print_sensorinfo(sc, i); > + } > + > + return 0; > +} > + > +void > +ugold_refresh(void *arg) > +{ > + struct ugold_softc *sc = arg; > + > + /* if device is not initialized, initialize first */ > + if (!sc->sc_initialized) { > + if (ugold_setup_device(sc)) > + return; > + sc->sc_initialized = 1; > + } > + > + switch (sc->sc_device_type) { > + case UGOLD_TYPE_TEMPER: > + ugold_refresh_temper(sc); > + break; > + default: > + break; > + /* NOTREACHED */ > + } > +} > + > +void > +ugold_refresh_temper(struct ugold_softc *sc) > +{ > + int temp; > + > + /* get sensor data */ > + if (ugold_read_data(sc)) { > + DPRINTF(("ugold: data read failed\n")); > + sc->sc_sensor[UGOLD_TEMPER_OUTER].sensor.flags > + |= SENSOR_FINVALID; > + sc->sc_sensor[UGOLD_TEMPER_INNER].sensor.flags > + |= SENSOR_FINVALID; > + return; > + } How can you be sure sc_ibuf contains the data you want to read? > + switch (sc->sc_num_sensors) { > + case 2: > + temp = ugold_ds75_temp(sc->sc_ibuf[4], sc->sc_ibuf[5]); > + temp += sc->sc_sensor[UGOLD_TEMPER_OUTER].cal_offset; > + sc->sc_sensor[UGOLD_TEMPER_OUTER].sensor.value = > + (temp * 10000) + 273150000; > + sc->sc_sensor[UGOLD_TEMPER_OUTER].sensor.flags &= > + ~SENSOR_FINVALID; > + /* FALLTHROUGH */ > + case 1: > + temp = ugold_ds75_temp(sc->sc_ibuf[2], sc->sc_ibuf[3]); > + temp += sc->sc_sensor[UGOLD_TEMPER_INNER].cal_offset; > + sc->sc_sensor[UGOLD_TEMPER_INNER].sensor.value = > + (temp * 10000) + 273150000; > + sc->sc_sensor[UGOLD_TEMPER_INNER].sensor.flags &= > + ~SENSOR_FINVALID; > + break; > + } > +} > + > +/* return C-degree * 100 value */ > +int > +ugold_ds75_temp(uint8_t msb, uint8_t lsb) > +{ > + /* DS75: 12bit precision mode : 0.0625 degrees Celsius ticks */ > + return (msb * 100) + ((lsb >> 4) * 25 / 4); > +} > + > +void > +ugold_print_sensorinfo(struct ugold_softc *sc, int num) > +{ > + struct ugold_sensor *s; > + s = &sc->sc_sensor[num]; > + > + printf("%s: ", sc->sc_hdev.sc_dev.dv_xname); > + switch (s->sensor.type) { > + case SENSOR_TEMP: > + printf("type %s (temperature)", > + ugold_sensor_type_s[s->dev_type]); > + if (s->cal_offset) > + printf(", calibration offset %d.%d degC", > + s->cal_offset / 100, abs(s->cal_offset % 100)); > + break; > + case SENSOR_HUMIDITY: > + printf("type %s (humidity)", > + ugold_sensor_type_s[s->dev_type]); > + if (s->cal_offset) > + printf("calibration offset %d.%d %%RH", > + s->cal_offset / 100, abs(s->cal_offset % 100)); > + break; > + default: > + printf("unknown"); > + } > + printf("\n"); > +}