On Thu, 18 Mar 2010, Przemo Firszt wrote:
> > On Thu, 2010-03-18 at 15:26 +0000, Przemo Firszt wrote:
> >
> > > + unsigned char speed;
> >
> > Could you add some comments as to what values represent what speed?
> I changed name from 'speed' to 'high_speed' as per your comment below,
> so I think there is no need (unless you think otherwise).
> >
> > > +static ssize_t wacom_store_speed(struct device *dev,
> > > + struct device_attribute *attr,
> > > + const char *buf, size_t count)
> > > +{
> > > + struct hid_device *hdev = container_of(dev, struct hid_device,
> > > dev);
> > > + int new_speed;
> > > +
> > > + sscanf(buf, "%1d", &new_speed);
> >
> > You should check the retval of sscanf as well.
> Done, see attached patch.
> > > + if (new_speed == 0 || new_speed == 1) {
> > > + wacom_poke(hdev, new_speed);
> > > + return strnlen(buf, PAGE_SIZE);
> > > + } else
> > > + return -EINVAL;
> > > +}
> >
> > >
> > > +static DEVICE_ATTR(speed, S_IRUGO | S_IWUGO,
> > > + wacom_show_speed, wacom_store_speed);
> >
> > If there's only 2 speeds available, and it's actually a boolean, I'd
> > rather you used a name like "high_speed".
> >
> > Furthermore, wdata->speed doesn't look like it's initialised.
> It is initialised by the very first call of wacom_poke from wacom_probe
> function. Can you advise if it's enough?
> >
> > Did you test whether speed switching works after the device has been
> > started up?
> Yes, it works like a charm, both ways.
> [przemo@pldmachine ~]$ echo 0 > /sys/class/hidraw/hidraw1/device/speed
> and lag is gone,
> [przemo@pldmachine ~]$ echo 1 > /sys/class/hidraw/hidraw1/device/speed
> and "rubber band" effect is back.
The patch looks fine to me, so I have applied it to my tree.
Could you also please send a followup patch which would add
Documentation/ABI entry for the speed attribute?
Thanks,
--
Jiri Kosina
SUSE Labs, Novell Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
