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");
> +}
