Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1433737 > unrolled thread

[PATCH 00/15] lirc_dev fixes and beautification

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/15] lirc_dev fixes and beautification Andi Shyti <andi.shyti@samsung.com> - 2016-06-29 15:30 +0200
    [PATCH 15/15] include: lirc: add set length and frequency ioctl options Andi Shyti <andi.shyti@samsung.com> - 2016-06-29 15:30 +0200
      Re: [PATCH 15/15] include: lirc: add set length and frequency ioctl  options Sean Young <sean@mess.org> - 2016-06-30 01:00 +0200
        Re: [PATCH 15/15] include: lirc: add set length and frequency ioctl  options Andi Shyti <andi@etezian.org> - 2016-06-30 01:40 +0200
    [PATCH 11/15] lirc_dev: fix variable constant comparisons Andi Shyti <andi.shyti@samsung.com> - 2016-06-29 15:30 +0200
    [PATCH 07/15] lirc_dev: simplify if statement in lirc_add_to_buf Andi Shyti <andi.shyti@samsung.com> - 2016-06-29 15:30 +0200
    [PATCH 09/15] lirc_dev: merge three if statements in only one Andi Shyti <andi.shyti@samsung.com> - 2016-06-29 15:30 +0200

#1433737 — [PATCH 00/15] lirc_dev fixes and beautification

Hi,

because I wanted to add three ioctl commands in lirc, I ended up
with the patchset below.

This is a collection of fixes, added functionality, coding rework
and trivial coding style fixes.

The first patch is preparatory to the second, which allows the
user to create a lirc driver without receiver buffer, which is
obvious for transmitters. Besides, even though that buffer could
have been used also by transmitters, drivers might have the need
to handle it separately.

The rest of the patches is a series of coding style and code
rework, as I said, some of them are very trivial, but I sent them
anyway because I was on fire.

Patch 14 is a segfault fix, while the last patch adds the
possibility to send to ioctl the set frequency, get frequency and
set length command.

Thanks,
Andi

Andi Shyti (15):
  lirc_dev: place buffer allocation on separate function
  lirc_dev: allow bufferless driver registration
  lirc_dev: remove unnecessary debug prints
  lirc_dev: replace printk with pr_* or dev_*
  lirc_dev: simplify goto paths
  lirc_dev: do not use goto to create loops
  lirc_dev: simplify if statement in lirc_add_to_buf
  lirc_dev: remove double if ... else statement
  lirc_dev: merge three if statements in only one
  lirc_dev: remove CONFIG_COMPAT precompiler check
  lirc_dev: fix variable constant comparisons
  lirc_dev: fix error return value
  lirc_dev: extremely trivial comment style fix
  lirc_dev: fix potential segfault
  include: lirc: add set length and frequency ioctl options

 drivers/media/rc/lirc_dev.c | 297 +++++++++++++++++++++-----------------------
 include/media/lirc_dev.h    |  12 ++
 include/uapi/linux/lirc.h   |   4 +
 3 files changed, 156 insertions(+), 157 deletions(-)

-- 
2.8.1

[toc] | [next] | [standalone]

#1433738 — [PATCH 15/15] include: lirc: add set length and frequency ioctl options

The Lirc framework works mainly with receivers, but there is
nothing that prevents us from using it for transmitters as well.

For that we need to have more control on the device frequency to
set (which is a new concept fro LIRC) and we also need to provide
to userspace, as feedback, the values of the used frequency and
length.

Add the LIRC_SET_LENGTH, LIRC_GET_FREQUENCY and
LIRC_SET_FREQUENCY ioctl commands in order to allow the above
mentioned operations.

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
---
 include/uapi/linux/lirc.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/uapi/linux/lirc.h b/include/uapi/linux/lirc.h
index 4b3ab29..94a0d8c 100644
--- a/include/uapi/linux/lirc.h
+++ b/include/uapi/linux/lirc.h
@@ -106,6 +106,7 @@
 
 /* code length in bits, currently only for LIRC_MODE_LIRCCODE */
 #define LIRC_GET_LENGTH                _IOR('i', 0x0000000f, __u32)
+#define LIRC_SET_LENGTH                _IOW('i', 0x00000010, __u32)
 
 #define LIRC_SET_SEND_MODE             _IOW('i', 0x00000011, __u32)
 #define LIRC_SET_REC_MODE              _IOW('i', 0x00000012, __u32)
@@ -165,4 +166,7 @@
 
 #define LIRC_SET_WIDEBAND_RECEIVER     _IOW('i', 0x00000023, __u32)
 
+#define LIRC_GET_FREQUENCY             _IOR('i', 0x00000024, __u32)
+#define LIRC_SET_FREQUENCY             _IOW('i', 0x00000025, __u32)
+
 #endif
-- 
2.8.1

[toc] | [prev] | [next] | [standalone]

#1434112 — Re: [PATCH 15/15] include: lirc: add set length and frequency ioctl options

On Wed, Jun 29, 2016 at 10:20:44PM +0900, Andi Shyti wrote:
> The Lirc framework works mainly with receivers, but there is
> nothing that prevents us from using it for transmitters as well.

The lirc interface already provides for transmitting IR.

> For that we need to have more control on the device frequency to
> set (which is a new concept fro LIRC) and we also need to provide
> to userspace, as feedback, the values of the used frequency and
> length.

Please can you elaborate on what exactly you mean by frequency and
length.

The carrier frequency can already be set with LIRC_SET_SEND_CARRIER.

> Add the LIRC_SET_LENGTH, LIRC_GET_FREQUENCY and
> LIRC_SET_FREQUENCY ioctl commands in order to allow the above
> mentioned operations.

You're also adding ioctls without any drivers implementing them
unless I missed something.

> 
> Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
> ---
>  include/uapi/linux/lirc.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/uapi/linux/lirc.h b/include/uapi/linux/lirc.h
> index 4b3ab29..94a0d8c 100644
> --- a/include/uapi/linux/lirc.h
> +++ b/include/uapi/linux/lirc.h
> @@ -106,6 +106,7 @@
>  
>  /* code length in bits, currently only for LIRC_MODE_LIRCCODE */
>  #define LIRC_GET_LENGTH                _IOR('i', 0x0000000f, __u32)
> +#define LIRC_SET_LENGTH                _IOW('i', 0x00000010, __u32)

The LIRC_GET_LENGTH is specific to LIRCCODE encoding. Why are you
adding it here?

>  
>  #define LIRC_SET_SEND_MODE             _IOW('i', 0x00000011, __u32)
>  #define LIRC_SET_REC_MODE              _IOW('i', 0x00000012, __u32)
> @@ -165,4 +166,7 @@
>  
>  #define LIRC_SET_WIDEBAND_RECEIVER     _IOW('i', 0x00000023, __u32)
>  
> +#define LIRC_GET_FREQUENCY             _IOR('i', 0x00000024, __u32)
> +#define LIRC_SET_FREQUENCY             _IOW('i', 0x00000025, __u32)
> +
>  #endif
> -- 
> 2.8.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1434119 — Re: [PATCH 15/15] include: lirc: add set length and frequency ioctl options

Hi Sean,

> > For that we need to have more control on the device frequency to
> > set (which is a new concept fro LIRC) and we also need to provide
> > to userspace, as feedback, the values of the used frequency and
> > length.
> 
> Please can you elaborate on what exactly you mean by frequency and
> length.
> 
> The carrier frequency can already be set with LIRC_SET_SEND_CARRIER.

yes, I mean carrier's frequency. I didn't understand that
LIRC_SET_SEND_CARRIER was related to the frequency.

> > Add the LIRC_SET_LENGTH, LIRC_GET_FREQUENCY and
> > LIRC_SET_FREQUENCY ioctl commands in order to allow the above
> > mentioned operations.
> 
> You're also adding ioctls without any drivers implementing them
> unless I missed something.

You're right; the first idea was to submit also the device
driver, but then I decided to keep it separate from this
patchset.
Anyway, we can drop this one (it's the last of the series) and,
in case it will be needed after the above comment, I will re-send
it with the driver.

Thanks,
Andi

[toc] | [prev] | [next] | [standalone]

#1433739 — [PATCH 11/15] lirc_dev: fix variable constant comparisons

When comparing a variable with a constant, the comparison should
start from the variable and not from the constant. It's also
written in the human DNA.

Swap the terms of comparisons whenever the constant comes first
and fix the following checkpatch warning:

  WARNING: Comparisons should place the constant on the right side of the test

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
---
 drivers/media/rc/lirc_dev.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index c11cfc0..7e5cb85 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -245,13 +245,13 @@ static int lirc_allocate_driver(struct lirc_driver *d)
 		return -EINVAL;
 	}
 
-	if (MAX_IRCTL_DEVICES <= d->minor) {
+	if (d->minor >= MAX_IRCTL_DEVICES) {
 		dev_err(d->dev, "minor must be between 0 and %d!\n",
 						MAX_IRCTL_DEVICES - 1);
 		return -EBADRQC;
 	}
 
-	if (1 > d->code_length || (BUFLEN * 8) < d->code_length) {
+	if (d->code_length < 1 || d->code_length > (BUFLEN * 8)) {
 		dev_err(d->dev, "code length must be less than %d bits\n",
 								BUFLEN * 8);
 		return -EBADRQC;
@@ -282,7 +282,7 @@ static int lirc_allocate_driver(struct lirc_driver *d)
 		for (minor = 0; minor < MAX_IRCTL_DEVICES; minor++)
 			if (!irctls[minor])
 				break;
-		if (MAX_IRCTL_DEVICES == minor) {
+		if (minor == MAX_IRCTL_DEVICES) {
 			dev_err(d->dev, "no free slots for drivers!\n");
 			err = -ENOMEM;
 			goto out_lock;
-- 
2.8.1

[toc] | [prev] | [next] | [standalone]

#1433740 — [PATCH 07/15] lirc_dev: simplify if statement in lirc_add_to_buf

The whole function is inside an 'if' statement
("if (ir->d.add_to_buf)").

Check the opposite of that statement at the beginning and exit,
this way we can have one level less of indentation.

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
---
 drivers/media/rc/lirc_dev.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index cc00b9a..d63ff85 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -95,27 +95,26 @@ static void lirc_irctl_cleanup(struct irctl *ir)
  */
 static int lirc_add_to_buf(struct irctl *ir)
 {
-	if (ir->d.add_to_buf) {
-		int res = -ENODATA;
-		int got_data = 0;
+	int res;
+	int got_data = 0;
 
-		/*
-		 * service the device as long as it is returning
-		 * data and we have space
-		 */
-		do {
-			res = ir->d.add_to_buf(ir->d.data, ir->buf);
-			if (!res)
-				got_data++;
-		} while (!res);
+	if (!ir->d.add_to_buf)
+		return 0;
 
-		if (res == -ENODEV)
-			kthread_stop(ir->task);
+	/*
+	 * service the device as long as it is returning
+	 * data and we have space
+	 */
+	do {
+		res = ir->d.add_to_buf(ir->d.data, ir->buf);
+		if (!res)
+			got_data++;
+	} while (!res);
 
-		return got_data ? 0 : res;
-	}
+	if (res == -ENODEV)
+		kthread_stop(ir->task);
 
-	return 0;
+	return got_data ? 0 : res;
 }
 
 /* main function of the polling thread
-- 
2.8.1

[toc] | [prev] | [next] | [standalone]

#1433741 — [PATCH 09/15] lirc_dev: merge three if statements in only one

The three if statements check the same thing, merge them in only
one statement.

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
---
 drivers/media/rc/lirc_dev.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 7dff92c..0c26609 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -269,15 +269,10 @@ static int lirc_allocate_driver(struct lirc_driver *d)
 			dev_err(d->dev, "add_to_buf not set\n");
 			return -EBADRQC;
 		}
-	} else if (!(d->fops && d->fops->read) && !d->rbuf) {
-		dev_err(d->dev, "fops->read and rbuf are NULL!\n");
+	} else if (!d->rbuf && !(d->fops && d->fops->read &&
+				d->fops->poll && d->fops->unlocked_ioctl)) {
+		dev_err(d->dev, "undefined read, poll, ioctl\n");
 		return -EBADRQC;
-	} else if (!d->rbuf) {
-		if (!(d->fops && d->fops->read && d->fops->poll &&
-		      d->fops->unlocked_ioctl)) {
-			dev_err(d->dev, "undefined read, poll, ioctl\n");
-			return -EBADRQC;
-		}
 	}
 
 	mutex_lock(&lirc_dev_lock);
-- 
2.8.1

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web