Hi Sean,

Em Sun, 10 Jul 2016 17:34:39 +0100
Sean Young <[email protected]> escreveu:

> Signed-off-by: Sean Young <[email protected]>

Please always add a description on your patches.

That's said, I'm not sure about this patch.

I don't like adding private APIs, as different hardware manufacturers tend 
to implement the same features on other hardware. Also, this makes userspace
apps full of per-device hacks, with is not a good idea.

So, why adding those as a redrat3-specific sysfs API? Can't we make them
generic enough and add at the LIRC API instead?

The other patches in this series are OK, and were applied already.

Regards,
Mauro

> ---
>  Documentation/ABI/testing/sysfs-class-rc-redrat3 |  33 ++++
>  drivers/media/rc/redrat3.c                       | 200 
> +++++++++++++++++++++++
>  2 files changed, 233 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-rc-redrat3
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-rc-redrat3 
> b/Documentation/ABI/testing/sysfs-class-rc-redrat3
> new file mode 100644
> index 0000000..33a24fa
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-rc-redrat3
> @@ -0,0 +1,33 @@
> +What:                /sys/class/rc/rcN/length_fuzz
> +Date:                Jul 2016
> +KernelVersion:       4.8
> +Contact:     Mauro Carvalho Chehab <[email protected]>
> +Description:
> +             The redrat3 encodes an IR signal as set of different lengths
> +             and a set of indices into those lengths. Writing this file
> +             sets how much two lengths must differ before they are
> +             considered distinct, the value is specified in microseconds.
> +             Default 5, value 0 to 127.
> +
> +What:                /sys/class/rc/rcN/min_pause
> +Date:                Jul 2016
> +KernelVersion:       4.8
> +Contact:     Mauro Carvalho Chehab <[email protected]>
> +Description:
> +             When receiving a continuous ir stream (for example when a user
> +             is holding a button down on a remote), this specifies the
> +             minimum size of a space when the redrat3 sends a irdata packet
> +             to the host. Specified in miliseconds. Default value 18ms.
> +             The value can be between 2 and 30 inclusive.
> +
> +What:                /sys/class/rc/rcN/periods_measure_carrier
> +Date:                Jul 2016
> +KernelVersion:       4.8
> +Contact:     Mauro Carvalho Chehab <[email protected]>
> +Description:
> +             The carrier frequency is measured during the first pulse of
> +             the IR signal. The larger the number of periods used To
> +             measure, the more accurate the result is likely to be, however
> +             some signals have short initial pulses, so in some case it
> +             may be necessary for applications to reduce this value.
> +             Default 8, value 1 to 255.
> diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
> index 399f44d..8365547 100644
> --- a/drivers/media/rc/redrat3.c
> +++ b/drivers/media/rc/redrat3.c
> @@ -860,6 +860,195 @@ static void redrat3_led_complete(struct urb *urb)
>       atomic_dec(&rr3->flash);
>  }
>  
> +static ssize_t min_pause_store(struct device *dev,
> +                              struct device_attribute *attr,
> +                              const char *buf, size_t len)
> +{
> +     struct rc_dev *rc_dev = to_rc_dev(dev);
> +     struct redrat3_dev *rr3 = rc_dev->priv;
> +     struct usb_device *udev = rr3->udev;
> +     long value;
> +     u8 *pause;
> +     int rc;
> +
> +     rc = kstrtol(buf, 0, &value);
> +     if (rc)
> +             return rc;
> +
> +     if (value < 2 || value > 30)
> +             return -EINVAL;
> +
> +     pause = kmalloc(sizeof(*pause), GFP_KERNEL);
> +     if (!pause)
> +             return -ENOMEM;
> +
> +     *pause = (65536 - (value * 2000)) / 256;
> +     rc = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), RR3_SET_IR_PARAM,
> +                  USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
> +                  RR3_IR_IO_MIN_PAUSE, 0, pause, sizeof(*pause), HZ * 25);
> +     dev_dbg(dev, "set ir parm min pause %d rc 0x%02x\n", *pause, rc);
> +     if (rc == sizeof(*pause))
> +             rc = len;
> +
> +     kfree(pause);
> +
> +     return rc;
> +}
> +
> +static ssize_t min_pause_show(struct device *dev,
> +                             struct device_attribute *attr, char *buf)
> +{
> +     struct rc_dev *rc_dev = to_rc_dev(dev);
> +     struct redrat3_dev *rr3 = rc_dev->priv;
> +     struct usb_device *udev = rr3->udev;
> +     int rc;
> +     u8 *pause;
> +
> +     pause = kmalloc(sizeof(*pause), GFP_KERNEL);
> +     if (!pause)
> +             return -ENOMEM;
> +
> +     rc = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0), RR3_GET_IR_PARAM,
> +                  USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
> +                  RR3_IR_IO_MIN_PAUSE, 0, pause, sizeof(*pause), HZ * 25);
> +     dev_dbg(dev, "get ir parm len pause %d rc 0x%02x\n", *pause, rc);
> +     if (rc == sizeof(*pause))
> +             rc = sprintf(buf, "%d\n", (65536 - 256 * *pause) / 2000);
> +
> +     kfree(pause);
> +
> +     return rc;
> +}
> +
> +static ssize_t length_fuzz_store(struct device *dev,
> +                              struct device_attribute *attr,
> +                              const char *buf, size_t len)
> +{
> +     struct rc_dev *rc_dev = to_rc_dev(dev);
> +     struct redrat3_dev *rr3 = rc_dev->priv;
> +     struct usb_device *udev = rr3->udev;
> +     long value;
> +     u8 *fuzz;
> +     int rc;
> +
> +     rc = kstrtol(buf, 0, &value);
> +     if (rc)
> +             return rc;
> +
> +     if (value < 0 || value > 127)
> +             return -EINVAL;
> +
> +     fuzz = kmalloc(sizeof(*fuzz), GFP_KERNEL);
> +     if (!fuzz)
> +             return -ENOMEM;
> +
> +     *fuzz = value * 2;
> +     rc = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), RR3_SET_IR_PARAM,
> +                  USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
> +                  RR3_IR_IO_LENGTH_FUZZ, 0, fuzz, sizeof(*fuzz), HZ * 25);
> +     dev_dbg(dev, "set ir parm len fuzz %d rc 0x%02x\n", *fuzz, rc);
> +     if (rc == sizeof(*fuzz))
> +             rc = len;
> +
> +     kfree(fuzz);
> +
> +     return rc;
> +}
> +
> +static ssize_t length_fuzz_show(struct device *dev,
> +                             struct device_attribute *attr, char *buf)
> +{
> +     struct rc_dev *rc_dev = to_rc_dev(dev);
> +     struct redrat3_dev *rr3 = rc_dev->priv;
> +     struct usb_device *udev = rr3->udev;
> +     int rc;
> +     u8 *fuzz;
> +
> +     fuzz = kmalloc(sizeof(*fuzz), GFP_KERNEL);
> +     if (!fuzz)
> +             return -ENOMEM;
> +
> +     rc = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0), RR3_GET_IR_PARAM,
> +                  USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
> +                  RR3_IR_IO_LENGTH_FUZZ, 0, fuzz, sizeof(*fuzz), HZ * 25);
> +     dev_dbg(dev, "get ir parm len fuzz %d rc 0x%02x\n", *fuzz, rc);
> +     if (rc == sizeof(*fuzz))
> +             rc = sprintf(buf, "%d\n", *fuzz / 2);
> +
> +     kfree(fuzz);
> +
> +     return rc;
> +}
> +
> +static ssize_t periods_measure_carrier_store(struct device *dev,
> +                              struct device_attribute *attr,
> +                              const char *buf, size_t len)
> +{
> +     struct rc_dev *rc_dev = to_rc_dev(dev);
> +     struct redrat3_dev *rr3 = rc_dev->priv;
> +     struct usb_device *udev = rr3->udev;
> +     long value;
> +     u8 *periods;
> +     int rc;
> +
> +     rc = kstrtol(buf, 0, &value);
> +     if (rc)
> +             return rc;
> +
> +     if (value < 1 || value > 255)
> +             return -EINVAL;
> +
> +     periods = kmalloc(sizeof(*periods), GFP_KERNEL);
> +     if (!periods)
> +             return -ENOMEM;
> +
> +     *periods = value;
> +     rc = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), RR3_SET_IR_PARAM,
> +                          USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
> +                          RR3_IR_IO_LENGTH_FUZZ, 0, periods,
> +                          sizeof(*periods), HZ * 25);
> +     dev_dbg(dev, "set ir parm periods to measure carrier %d rc 0x%02x\n",
> +                                                             *periods, rc);
> +     if (rc == sizeof(*periods))
> +             rc = len;
> +
> +     kfree(periods);
> +
> +     return rc;
> +}
> +
> +static ssize_t periods_measure_carrier_show(struct device *dev,
> +                             struct device_attribute *attr, char *buf)
> +{
> +     struct rc_dev *rc_dev = to_rc_dev(dev);
> +     struct redrat3_dev *rr3 = rc_dev->priv;
> +     struct usb_device *udev = rr3->udev;
> +     u8 *periods;
> +     int rc;
> +
> +     periods = kmalloc(sizeof(*periods), GFP_KERNEL);
> +     if (!periods)
> +             return -ENOMEM;
> +
> +     rc = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0), RR3_GET_IR_PARAM,
> +                          USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
> +                          RR3_IR_IO_LENGTH_FUZZ, 0, periods,
> +                          sizeof(*periods), HZ * 25);
> +     dev_dbg(dev, "get ir parm periods to measure carrier %d rc 0x%02x\n",
> +                                                             *periods, rc);
> +     if (rc == sizeof(*periods))
> +             rc = sprintf(buf, "%d\n", *periods);
> +
> +     kfree(periods);
> +
> +     return rc;
> +}
> +
> +static struct device_attribute redrat3_attrs[] = {
> +     __ATTR_RW(length_fuzz), __ATTR_RW(min_pause),
> +     __ATTR_RW(periods_measure_carrier)
> +};
> +
>  static struct rc_dev *redrat3_init_rc_dev(struct redrat3_dev *rr3)
>  {
>       struct device *dev = rr3->dev;
> @@ -1036,6 +1225,13 @@ static int redrat3_dev_probe(struct usb_interface 
> *intf,
>               retval = -ENOMEM;
>               goto led_free_error;
>       }
> +     for (i = 0; i < ARRAY_SIZE(redrat3_attrs); i++) {
> +             retval = device_create_file(&rr3->rc->dev, &redrat3_attrs[i]);
> +             if (retval) {
> +                     rc_unregister_device(rr3->rc);
> +                     goto led_free_error;
> +             }
> +     }
>  
>       /* we can register the device now, as it is ready */
>       usb_set_intfdata(intf, rr3);
> @@ -1057,10 +1253,14 @@ static void redrat3_dev_disconnect(struct 
> usb_interface *intf)
>  {
>       struct usb_device *udev = interface_to_usbdev(intf);
>       struct redrat3_dev *rr3 = usb_get_intfdata(intf);
> +     int i;
>  
>       if (!rr3)
>               return;
>  
> +     for (i = 0; i < ARRAY_SIZE(redrat3_attrs); i++)
> +             device_remove_file(&rr3->rc->dev, &redrat3_attrs[i]);
> +
>       usb_set_intfdata(intf, NULL);
>       rc_unregister_device(rr3->rc);
>       led_classdev_unregister(&rr3->led);



Thanks,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to