Re: [PATCH 03/20] [media] lirc.h: remove several unused ioctls

2016-07-12 Thread Mauro Carvalho Chehab 
Em Tue, 12 Jul 2016 14:54:38 +0100
Sean Young  escreveu:

> On Tue, Jul 12, 2016 at 02:35:56PM +0100, Sean Young wrote:
> > On Tue, Jul 12, 2016 at 10:23:00AM -0300, Mauro Carvalho Chehab wrote:  
> > > Em Tue, 12 Jul 2016 14:14:06 +0100
> > > Sean Young  escreveu:  
> > > > On Tue, Jul 12, 2016 at 09:41:57AM -0300, Mauro Carvalho Chehab wrote:  
> > -snip-  
> > > > > -#define LIRC_SET_REC_DUTY_CYCLE_IOW('i', 0x0016, __u32)  
> > > > >   
> > > > 
> > > > Also remove LIRC_CAN_SET_REC_DUTY_CYCLE and 
> > > > LIRC_CAN_SET_REC_DUTY_CYCLE_RANGE.  
> > > 
> > > Removing the "LIRC_CAN" macros can break userspace, as some app could
> > > be using it to print the LIRC features. That's why I opted to keep
> > > them, but to document that those features are unused - this is at
> > > the next patch (04/20).  
> > 
> > How is that different from removing the ioctls? Might as well go the whole
> > hog.  

If someone implemented LIRC_GET_FEATURES and handled all flags, such
program would break, as the API change.

> 
> Ah you meant that if someone later adds a new feature then we might reuse
> an existing bit. Oops, sorry.

Yes, this is another possibility. In such case, the ABI will also 
break, with is more severe than a pure API change.

Removing the ioctl declarations will still work for compiled programs,
as they'll still receive an error code when the ioctl is issued.

> 
> > Also note that LIRC_CAN_SET_REC_DUTY_CYCLE has the same value as
> > LIRC_CAN_MEASURE_CARRIER, so if some userspace program uses this it might
> > end up in the mistaken belief its supports LIRC_CAN_SET_REC_DUTY_CYCLE.  
> 
> So there is an argument for removing LIRC_CAN_SET_REC_DUTY_CYCLE, but
> that should be a separate patch.

Yes. Yet, IMHO, the best would be to put those unused LIRC_CAN into a:

#ifndef __KERNEL

macro block, to:

1) avoid the risk of breaking userspace;
2) be clear that those are deprecated stuff and should not be used on
   newer programs;
3) Reserve the bits for not be used, to avoid possible conflicts.

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

Re: [PATCH 03/20] [media] lirc.h: remove several unused ioctls

2016-07-12 Thread Sean Young 
On Tue, Jul 12, 2016 at 02:35:56PM +0100, Sean Young wrote:
> On Tue, Jul 12, 2016 at 10:23:00AM -0300, Mauro Carvalho Chehab wrote:
> > Em Tue, 12 Jul 2016 14:14:06 +0100
> > Sean Young  escreveu:
> > > On Tue, Jul 12, 2016 at 09:41:57AM -0300, Mauro Carvalho Chehab wrote:
> -snip-
> > > > -#define LIRC_SET_REC_DUTY_CYCLE_IOW('i', 0x0016, __u32)  
> > > 
> > > Also remove LIRC_CAN_SET_REC_DUTY_CYCLE and 
> > > LIRC_CAN_SET_REC_DUTY_CYCLE_RANGE.
> > 
> > Removing the "LIRC_CAN" macros can break userspace, as some app could
> > be using it to print the LIRC features. That's why I opted to keep
> > them, but to document that those features are unused - this is at
> > the next patch (04/20).
> 
> How is that different from removing the ioctls? Might as well go the whole
> hog.

Ah you meant that if someone later adds a new feature then we might reuse
an existing bit. Oops, sorry.

> Also note that LIRC_CAN_SET_REC_DUTY_CYCLE has the same value as
> LIRC_CAN_MEASURE_CARRIER, so if some userspace program uses this it might
> end up in the mistaken belief its supports LIRC_CAN_SET_REC_DUTY_CYCLE.

So there is an argument for removing LIRC_CAN_SET_REC_DUTY_CYCLE, but
that should be a separate patch.


Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/20] [media] lirc.h: remove several unused ioctls

2016-07-12 Thread Sean Young 
On Tue, Jul 12, 2016 at 10:23:00AM -0300, Mauro Carvalho Chehab wrote:
> Em Tue, 12 Jul 2016 14:14:06 +0100
> Sean Young  escreveu:
> > On Tue, Jul 12, 2016 at 09:41:57AM -0300, Mauro Carvalho Chehab wrote:
-snip-
> > > -#define LIRC_SET_REC_DUTY_CYCLE_IOW('i', 0x0016, __u32)  
> > 
> > Also remove LIRC_CAN_SET_REC_DUTY_CYCLE and 
> > LIRC_CAN_SET_REC_DUTY_CYCLE_RANGE.
> 
> Removing the "LIRC_CAN" macros can break userspace, as some app could
> be using it to print the LIRC features. That's why I opted to keep
> them, but to document that those features are unused - this is at
> the next patch (04/20).

How is that different from removing the ioctls? Might as well go the whole
hog.

Also note that LIRC_CAN_SET_REC_DUTY_CYCLE has the same value as
LIRC_CAN_MEASURE_CARRIER, so if some userspace program uses this it might
end up in the mistaken belief its supports LIRC_CAN_SET_REC_DUTY_CYCLE.


Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/20] [media] lirc.h: remove several unused ioctls

2016-07-12 Thread Mauro Carvalho Chehab 
Em Tue, 12 Jul 2016 14:14:06 +0100
Sean Young  escreveu:

> On Tue, Jul 12, 2016 at 09:41:57AM -0300, Mauro Carvalho Chehab wrote:
> > While reviewing the documentation gaps on LIRC, it was
> > noticed that several ioctls aren't used by any LIRC drivers
> > (nor at staging or mainstream).
> > 
> > It doesn't make sense to document them, as they're not used
> > anywhere. So, let's remove those from the lirc header.  
> 
> Good to see these go.
> > 
> > Signed-off-by: Mauro Carvalho Chehab 
> > ---
> >  include/uapi/linux/lirc.h | 39 ++-
> >  1 file changed, 2 insertions(+), 37 deletions(-)
> > 
> > diff --git a/include/uapi/linux/lirc.h b/include/uapi/linux/lirc.h
> > index 4b3ab2966b5a..991ab4570b8e 100644
> > --- a/include/uapi/linux/lirc.h
> > +++ b/include/uapi/linux/lirc.h
> > @@ -90,20 +90,11 @@
> >  
> >  #define LIRC_GET_SEND_MODE _IOR('i', 0x0001, __u32)
> >  #define LIRC_GET_REC_MODE  _IOR('i', 0x0002, __u32)
> > -#define LIRC_GET_SEND_CARRIER  _IOR('i', 0x0003, __u32)
> > -#define LIRC_GET_REC_CARRIER   _IOR('i', 0x0004, __u32)
> > -#define LIRC_GET_SEND_DUTY_CYCLE   _IOR('i', 0x0005, __u32)
> > -#define LIRC_GET_REC_DUTY_CYCLE_IOR('i', 0x0006, __u32)
> >  #define LIRC_GET_REC_RESOLUTION_IOR('i', 0x0007, __u32)
> >  
> >  #define LIRC_GET_MIN_TIMEOUT   _IOR('i', 0x0008, __u32)
> >  #define LIRC_GET_MAX_TIMEOUT   _IOR('i', 0x0009, __u32)
> >  
> > -#define LIRC_GET_MIN_FILTER_PULSE  _IOR('i', 0x000a, __u32)
> > -#define LIRC_GET_MAX_FILTER_PULSE  _IOR('i', 0x000b, __u32)
> > -#define LIRC_GET_MIN_FILTER_SPACE  _IOR('i', 0x000c, __u32)
> > -#define LIRC_GET_MAX_FILTER_SPACE  _IOR('i', 0x000d, __u32)
> > -
> >  /* code length in bits, currently only for LIRC_MODE_LIRCCODE */
> >  #define LIRC_GET_LENGTH_IOR('i', 0x000f, __u32)
> >  
> > @@ -113,7 +104,6 @@
> >  #define LIRC_SET_SEND_CARRIER  _IOW('i', 0x0013, __u32)
> >  #define LIRC_SET_REC_CARRIER   _IOW('i', 0x0014, __u32)
> >  #define LIRC_SET_SEND_DUTY_CYCLE   _IOW('i', 0x0015, __u32)
> > -#define LIRC_SET_REC_DUTY_CYCLE_IOW('i', 0x0016, __u32)  
> 
> Also remove LIRC_CAN_SET_REC_DUTY_CYCLE and 
> LIRC_CAN_SET_REC_DUTY_CYCLE_RANGE.

Removing the "LIRC_CAN" macros can break userspace, as some app could
be using it to print the LIRC features. That's why I opted to keep
them, but to document that those features are unused - this is at
the next patch (04/20).

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

Re: [PATCH 03/20] [media] lirc.h: remove several unused ioctls

2016-07-12 Thread Sean Young 
On Tue, Jul 12, 2016 at 09:41:57AM -0300, Mauro Carvalho Chehab wrote:
> While reviewing the documentation gaps on LIRC, it was
> noticed that several ioctls aren't used by any LIRC drivers
> (nor at staging or mainstream).
> 
> It doesn't make sense to document them, as they're not used
> anywhere. So, let's remove those from the lirc header.

Good to see these go.
> 
> Signed-off-by: Mauro Carvalho Chehab 
> ---
>  include/uapi/linux/lirc.h | 39 ++-
>  1 file changed, 2 insertions(+), 37 deletions(-)
> 
> diff --git a/include/uapi/linux/lirc.h b/include/uapi/linux/lirc.h
> index 4b3ab2966b5a..991ab4570b8e 100644
> --- a/include/uapi/linux/lirc.h
> +++ b/include/uapi/linux/lirc.h
> @@ -90,20 +90,11 @@
>  
>  #define LIRC_GET_SEND_MODE _IOR('i', 0x0001, __u32)
>  #define LIRC_GET_REC_MODE  _IOR('i', 0x0002, __u32)
> -#define LIRC_GET_SEND_CARRIER  _IOR('i', 0x0003, __u32)
> -#define LIRC_GET_REC_CARRIER   _IOR('i', 0x0004, __u32)
> -#define LIRC_GET_SEND_DUTY_CYCLE   _IOR('i', 0x0005, __u32)
> -#define LIRC_GET_REC_DUTY_CYCLE_IOR('i', 0x0006, __u32)
>  #define LIRC_GET_REC_RESOLUTION_IOR('i', 0x0007, __u32)
>  
>  #define LIRC_GET_MIN_TIMEOUT   _IOR('i', 0x0008, __u32)
>  #define LIRC_GET_MAX_TIMEOUT   _IOR('i', 0x0009, __u32)
>  
> -#define LIRC_GET_MIN_FILTER_PULSE  _IOR('i', 0x000a, __u32)
> -#define LIRC_GET_MAX_FILTER_PULSE  _IOR('i', 0x000b, __u32)
> -#define LIRC_GET_MIN_FILTER_SPACE  _IOR('i', 0x000c, __u32)
> -#define LIRC_GET_MAX_FILTER_SPACE  _IOR('i', 0x000d, __u32)
> -
>  /* code length in bits, currently only for LIRC_MODE_LIRCCODE */
>  #define LIRC_GET_LENGTH_IOR('i', 0x000f, __u32)
>  
> @@ -113,7 +104,6 @@
>  #define LIRC_SET_SEND_CARRIER  _IOW('i', 0x0013, __u32)
>  #define LIRC_SET_REC_CARRIER   _IOW('i', 0x0014, __u32)
>  #define LIRC_SET_SEND_DUTY_CYCLE   _IOW('i', 0x0015, __u32)
> -#define LIRC_SET_REC_DUTY_CYCLE_IOW('i', 0x0016, __u32)

Also remove LIRC_CAN_SET_REC_DUTY_CYCLE and 
LIRC_CAN_SET_REC_DUTY_CYCLE_RANGE.

>  #define LIRC_SET_TRANSMITTER_MASK  _IOW('i', 0x0017, __u32)
>  
>  /*
> @@ -127,42 +117,17 @@
>  #define LIRC_SET_REC_TIMEOUT_REPORTS   _IOW('i', 0x0019, __u32)
>  
>  /*
> - * pulses shorter than this are filtered out by hardware (software
> - * emulation in lirc_dev?)
> - */
> -#define LIRC_SET_REC_FILTER_PULSE  _IOW('i', 0x001a, __u32)
> -/*
> - * spaces shorter than this are filtered out by hardware (software
> - * emulation in lirc_dev?)
> - */
> -#define LIRC_SET_REC_FILTER_SPACE  _IOW('i', 0x001b, __u32)
> -/*
> - * if filter cannot be set independently for pulse/space, this should
> - * be used
> - */
> -#define LIRC_SET_REC_FILTER_IOW('i', 0x001c, __u32)

Also remove LIRC_CAN_SET_REC_FILTER.
> -
> -/*
>   * if enabled from the next key press on the driver will send
>   * LIRC_MODE2_FREQUENCY packets
>   */
>  #define LIRC_SET_MEASURE_CARRIER_MODE_IOW('i', 0x001d, __u32)
>  
>  /*
> - * to set a range use
> - * LIRC_SET_REC_DUTY_CYCLE_RANGE/LIRC_SET_REC_CARRIER_RANGE with the
> - * lower bound first and later
> - * LIRC_SET_REC_DUTY_CYCLE/LIRC_SET_REC_CARRIER with the upper bound
> + * to set a range use LIRC_SET_REC_CARRIER_RANGE with the
> + * lower bound first and later LIRC_SET_REC_CARRIER with the upper bound
>   */
> -
> -#define LIRC_SET_REC_DUTY_CYCLE_RANGE  _IOW('i', 0x001e, __u32)
>  #define LIRC_SET_REC_CARRIER_RANGE _IOW('i', 0x001f, __u32)
>  
> -#define LIRC_NOTIFY_DECODE _IO('i', 0x0020)

Also remove LIRC_CAN_NOTIFY_DECODE.

> -
> -#define LIRC_SETUP_START   _IO('i', 0x0021)
> -#define LIRC_SETUP_END _IO('i', 0x0022)
> -
>  #define LIRC_SET_WIDEBAND_RECEIVER _IOW('i', 0x0023, __u32)
>  
>  #endif

Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html