Hi,
Two patches to allow switching of reporting speed of wacom bluetooth
tablet from userspace through sysfs.
kind reagrds,
Przemo
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.
On Thu, 18 Mar 2010, Bastien Nocera wrote:
> > Two patches to allow switching of reporting speed of wacom bluetooth
> > tablet from userspace through sysfs.
>
> Signed-off-by: Bastien Nocera <[email protected]>
Applied, thanks.
--
Jiri Kosina
SUSE Labs, Novell Inc.
Dnia 2010-03-18, czw o godzinie 15:37 +0000, Bastien Nocera pisze:
> 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.
Thanks for your comments,
Przemo
On Thu, 2010-03-18 at 15:25 +0000, Przemo Firszt wrote:
> Hi,
> Two patches to allow switching of reporting speed of wacom bluetooth
> tablet from userspace through sysfs.
Signed-off-by: Bastien Nocera <[email protected]>
Cheers
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?
> +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.
> + 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.
Did you test whether speed switching works after the device has been
started up?
Cheers
Patch 2/2
--
Przemo
On Thu, 22 Apr 2010, Przemo Firszt wrote:
> > 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?
> Hi Jiri,
> Sorry for the delayed reply. See attached patch for ABI entry.
Applied.
> Is Kernel Version: 2.6.35 OK?
Yup.
Thanks,
--
Jiri Kosina
SUSE Labs, Novell Inc.
Dnia 2010-03-22, pon o godzinie 09:48 +0100, Jiri Kosina pisze:
[..]
> 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?
Hi Jiri,
Sorry for the delayed reply. See attached patch for ABI entry.
Is Kernel Version: 2.6.35 OK?
Thanks,
Przemo