[PATCH v2 00/15] lirc_dev fixes and beautification

[PATCH v2 00/15] lirc_dev fixes and beautification

[Andi Shyti]

Hi,

After applying Joe's suggestion, the next patches had some
conflicts, therefore I have to send all the 15 patches again.

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.

Changelog: V1->V2

 - As Joe recommended, in patch 4 I added the pr_fmt definition
   and removed all the hardcoded prefixes from the pr_*
   functions.

 - In Patch 15, after Sean's review, I removed the definitions of
   the GET/SET_FREQUENCY, I will use GET/SET_SEND_CARRIER
   instead, even though I find the name a bit confusing.

 - In patch 6 I did a better refactoring

Thanks,
Andi

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

 drivers/media/rc/lirc_dev.c | 301 +++++++++++++++++++++-----------------------
 include/media/lirc_dev.h    |  12 ++
 include/uapi/linux/lirc.h   |   1 +
 3 files changed, 155 insertions(+), 159 deletions(-)

-- 
2.8.1

[PATCH v2 01/15] [media] lirc_dev: place buffer allocation on separate function

[Andi Shyti]

During the driver registration, move the buffer allocation on a
separate function.

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

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 92ae190..5716978 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -203,13 +203,41 @@ err_out:
 	return retval;
 }
 
-int lirc_register_driver(struct lirc_driver *d)
+static int lirc_allocate_buffer(struct irctl *ir)
 {
-	struct irctl *ir;
-	int minor;
+	int err;
 	int bytes_in_key;
 	unsigned int chunk_size;
 	unsigned int buffer_size;
+	struct lirc_driver *d = &ir->d;
+
+	bytes_in_key = BITS_TO_LONGS(d->code_length) +
+						(d->code_length % 8 ? 1 : 0);
+	buffer_size = d->buffer_size ? d->buffer_size : BUFLEN / bytes_in_key;
+	chunk_size  = d->chunk_size  ? d->chunk_size  : bytes_in_key;
+
+	if (d->rbuf) {
+		ir->buf = d->rbuf;
+	} else {
+		ir->buf = kmalloc(sizeof(struct lirc_buffer), GFP_KERNEL);
+		if (!ir->buf)
+			return -ENOMEM;
+
+		err = lirc_buffer_init(ir->buf, chunk_size, buffer_size);
+		if (err) {
+			kfree(ir->buf);
+			return err;
+		}
+	}
+	ir->chunk_size = ir->buf->chunk_size;
+
+	return 0;
+}
+
+int lirc_register_driver(struct lirc_driver *d)
+{
+	struct irctl *ir;
+	int minor;
 	int err;
 
 	if (!d) {
@@ -314,26 +342,9 @@ int lirc_register_driver(struct lirc_driver *d)
 	/* some safety check 8-) */
 	d->name[sizeof(d->name)-1] = '\0';
 
-	bytes_in_key = BITS_TO_LONGS(d->code_length) +
-			(d->code_length % 8 ? 1 : 0);
-	buffer_size = d->buffer_size ? d->buffer_size : BUFLEN / bytes_in_key;
-	chunk_size  = d->chunk_size  ? d->chunk_size  : bytes_in_key;
-
-	if (d->rbuf) {
-		ir->buf = d->rbuf;
-	} else {
-		ir->buf = kmalloc(sizeof(struct lirc_buffer), GFP_KERNEL);
-		if (!ir->buf) {
-			err = -ENOMEM;
-			goto out_lock;
-		}
-		err = lirc_buffer_init(ir->buf, chunk_size, buffer_size);
-		if (err) {
-			kfree(ir->buf);
-			goto out_lock;
-		}
-	}
-	ir->chunk_size = ir->buf->chunk_size;
+	err = lirc_allocate_buffer(ir);
+	if (err)
+		goto out_lock;
 
 	if (d->features == 0)
 		d->features = LIRC_CAN_REC_LIRCCODE;
-- 
2.8.1

[PATCH v2 02/15] [media] lirc_dev: allow bufferless driver registration

[Andi Shyti]

Some drivers don't necessarily need to have a FIFO managed buffer
for their transfers. Drivers now should call
lirc_register_bufferless_driver in order to handle the buffer
themselves.

The function works exaclty like lirc_register_driver except of
the buffer allocation.

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
---
 drivers/media/rc/lirc_dev.c | 44 ++++++++++++++++++++++++++++++++++----------
 include/media/lirc_dev.h    | 12 ++++++++++++
 2 files changed, 46 insertions(+), 10 deletions(-)

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 5716978..fa562a3 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -205,12 +205,14 @@ err_out:
 
 static int lirc_allocate_buffer(struct irctl *ir)
 {
-	int err;
+	int err = 0;
 	int bytes_in_key;
 	unsigned int chunk_size;
 	unsigned int buffer_size;
 	struct lirc_driver *d = &ir->d;
 
+	mutex_lock(&lirc_dev_lock);
+
 	bytes_in_key = BITS_TO_LONGS(d->code_length) +
 						(d->code_length % 8 ? 1 : 0);
 	buffer_size = d->buffer_size ? d->buffer_size : BUFLEN / bytes_in_key;
@@ -220,21 +222,26 @@ static int lirc_allocate_buffer(struct irctl *ir)
 		ir->buf = d->rbuf;
 	} else {
 		ir->buf = kmalloc(sizeof(struct lirc_buffer), GFP_KERNEL);
-		if (!ir->buf)
-			return -ENOMEM;
+		if (!ir->buf) {
+			err = -ENOMEM;
+			goto out;
+		}
 
 		err = lirc_buffer_init(ir->buf, chunk_size, buffer_size);
 		if (err) {
 			kfree(ir->buf);
-			return err;
+			goto out;
 		}
 	}
 	ir->chunk_size = ir->buf->chunk_size;
 
-	return 0;
+out:
+	mutex_unlock(&lirc_dev_lock);
+
+	return err;
 }
 
-int lirc_register_driver(struct lirc_driver *d)
+static int lirc_allocate_driver(struct lirc_driver *d)
 {
 	struct irctl *ir;
 	int minor;
@@ -342,10 +349,6 @@ int lirc_register_driver(struct lirc_driver *d)
 	/* some safety check 8-) */
 	d->name[sizeof(d->name)-1] = '\0';
 
-	err = lirc_allocate_buffer(ir);
-	if (err)
-		goto out_lock;
-
 	if (d->features == 0)
 		d->features = LIRC_CAN_REC_LIRCCODE;
 
@@ -385,8 +388,29 @@ out_lock:
 out:
 	return err;
 }
+
+int lirc_register_driver(struct lirc_driver *d)
+{
+	int err, minor;
+
+	minor = lirc_allocate_driver(d);
+	if (minor < 0)
+		return minor;
+
+	err = lirc_allocate_buffer(irctls[minor]);
+	if (err)
+		lirc_unregister_driver(minor);
+
+	return err ? err : minor;
+}
 EXPORT_SYMBOL(lirc_register_driver);
 
+int lirc_register_bufferless_driver(struct lirc_driver *d)
+{
+	return lirc_allocate_driver(d);
+}
+EXPORT_SYMBOL(lirc_register_bufferless_driver);
+
 int lirc_unregister_driver(int minor)
 {
 	struct irctl *ir;
diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
index 0ab59a5..8bed57a 100644
--- a/include/media/lirc_dev.h
+++ b/include/media/lirc_dev.h
@@ -214,6 +214,18 @@ struct lirc_driver {
  */
 extern int lirc_register_driver(struct lirc_driver *d);
 
+/* int lirc_register_bufferless_driver - allocates a lirc bufferless driver
+ * @d: reference to the lirc_driver to initialize
+ *
+ * The difference between lirc_register_driver and
+ * lirc_register_bufferless_driver is that the latter doesn't allocate any
+ * buffer, which means that the driver using the lirc_driver should take care of
+ * it by itself.
+ *
+ * returns 0 on success or a the negative errno number in case of failure.
+ */
+extern int lirc_register_bufferless_driver(struct lirc_driver *d);
+
 /* returns negative value on error or 0 if success
 */
 extern int lirc_unregister_driver(int minor);
-- 
2.8.1

Re: [PATCH v2 02/15] [media] lirc_dev: allow bufferless driver registration

[Sean Young]

On Fri, Jul 01, 2016 at 05:01:25PM +0900, Andi Shyti wrote:
> Some drivers don't necessarily need to have a FIFO managed buffer
> for their transfers. Drivers now should call
> lirc_register_bufferless_driver in order to handle the buffer
> themselves.
> 
> The function works exaclty like lirc_register_driver except of
> the buffer allocation.

Indeed transmit-only devices don't need an input buffer, which is
just a waste of memory. However can't lirc_register_driver() figure
out from the features if the driver is capable of receiving, i.e.

int lirc_register_driver(struct lirc_driver *d)
{
	int err, minor;

	minor = lirc_allocate_driver(d);
	if (minor < 0)
		return minor;

	if (d->features & LIRC_CAN_REC_MODE2) {
		err = lirc_allocate_buffer(irctls[minor]);
		if (err)
			lirc_unregister_driver(minor);
	}

	return err ? err : minor;
}


Sean

> 
> Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
> ---
>  drivers/media/rc/lirc_dev.c | 44 ++++++++++++++++++++++++++++++++++----------
>  include/media/lirc_dev.h    | 12 ++++++++++++
>  2 files changed, 46 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
> index 5716978..fa562a3 100644
> --- a/drivers/media/rc/lirc_dev.c
> +++ b/drivers/media/rc/lirc_dev.c
> @@ -205,12 +205,14 @@ err_out:
>  
>  static int lirc_allocate_buffer(struct irctl *ir)
>  {
> -	int err;
> +	int err = 0;
>  	int bytes_in_key;
>  	unsigned int chunk_size;
>  	unsigned int buffer_size;
>  	struct lirc_driver *d = &ir->d;
>  
> +	mutex_lock(&lirc_dev_lock);
> +
>  	bytes_in_key = BITS_TO_LONGS(d->code_length) +
>  						(d->code_length % 8 ? 1 : 0);
>  	buffer_size = d->buffer_size ? d->buffer_size : BUFLEN / bytes_in_key;
> @@ -220,21 +222,26 @@ static int lirc_allocate_buffer(struct irctl *ir)
>  		ir->buf = d->rbuf;
>  	} else {
>  		ir->buf = kmalloc(sizeof(struct lirc_buffer), GFP_KERNEL);
> -		if (!ir->buf)
> -			return -ENOMEM;
> +		if (!ir->buf) {
> +			err = -ENOMEM;
> +			goto out;
> +		}
>  
>  		err = lirc_buffer_init(ir->buf, chunk_size, buffer_size);
>  		if (err) {
>  			kfree(ir->buf);
> -			return err;
> +			goto out;
>  		}
>  	}
>  	ir->chunk_size = ir->buf->chunk_size;
>  
> -	return 0;
> +out:
> +	mutex_unlock(&lirc_dev_lock);
> +
> +	return err;
>  }
>  
> -int lirc_register_driver(struct lirc_driver *d)
> +static int lirc_allocate_driver(struct lirc_driver *d)
>  {
>  	struct irctl *ir;
>  	int minor;
> @@ -342,10 +349,6 @@ int lirc_register_driver(struct lirc_driver *d)
>  	/* some safety check 8-) */
>  	d->name[sizeof(d->name)-1] = '\0';
>  
> -	err = lirc_allocate_buffer(ir);
> -	if (err)
> -		goto out_lock;
> -
>  	if (d->features == 0)
>  		d->features = LIRC_CAN_REC_LIRCCODE;
>  
> @@ -385,8 +388,29 @@ out_lock:
>  out:
>  	return err;
>  }
> +
> +int lirc_register_driver(struct lirc_driver *d)
> +{
> +	int err, minor;
> +
> +	minor = lirc_allocate_driver(d);
> +	if (minor < 0)
> +		return minor;
> +
> +	err = lirc_allocate_buffer(irctls[minor]);
> +	if (err)
> +		lirc_unregister_driver(minor);
> +
> +	return err ? err : minor;
> +}
>  EXPORT_SYMBOL(lirc_register_driver);
>  
> +int lirc_register_bufferless_driver(struct lirc_driver *d)
> +{
> +	return lirc_allocate_driver(d);
> +}
> +EXPORT_SYMBOL(lirc_register_bufferless_driver);
> +
>  int lirc_unregister_driver(int minor)
>  {
>  	struct irctl *ir;
> diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
> index 0ab59a5..8bed57a 100644
> --- a/include/media/lirc_dev.h
> +++ b/include/media/lirc_dev.h
> @@ -214,6 +214,18 @@ struct lirc_driver {
>   */
>  extern int lirc_register_driver(struct lirc_driver *d);
>  
> +/* int lirc_register_bufferless_driver - allocates a lirc bufferless driver
> + * @d: reference to the lirc_driver to initialize
> + *
> + * The difference between lirc_register_driver and
> + * lirc_register_bufferless_driver is that the latter doesn't allocate any
> + * buffer, which means that the driver using the lirc_driver should take care of
> + * it by itself.
> + *
> + * returns 0 on success or a the negative errno number in case of failure.
> + */
> +extern int lirc_register_bufferless_driver(struct lirc_driver *d);
> +
>  /* returns negative value on error or 0 if success
>  */
>  extern int lirc_unregister_driver(int minor);
> -- 
> 2.8.1

[PATCH v2 03/15] [media] lirc_dev: remove unnecessary debug prints

[Andi Shyti]

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

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index fa562a3..ee997ab 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -80,8 +80,6 @@ static void lirc_irctl_init(struct irctl *ir)
 
 static void lirc_irctl_cleanup(struct irctl *ir)
 {
-	dev_dbg(ir->d.dev, LOGHEAD "cleaning up\n", ir->d.name, ir->d.minor);
-
 	device_destroy(lirc_class, MKDEV(MAJOR(lirc_base_dev), ir->d.minor));
 
 	if (ir->buf != ir->d.rbuf) {
@@ -127,9 +125,6 @@ static int lirc_thread(void *irctl)
 {
 	struct irctl *ir = irctl;
 
-	dev_dbg(ir->d.dev, LOGHEAD "poll thread started\n",
-		ir->d.name, ir->d.minor);
-
 	do {
 		if (ir->open) {
 			if (ir->jiffies_to_wait) {
@@ -146,9 +141,6 @@ static int lirc_thread(void *irctl)
 		}
 	} while (!kthread_should_stop());
 
-	dev_dbg(ir->d.dev, LOGHEAD "poll thread ended\n",
-		ir->d.name, ir->d.minor);
-
 	return 0;
 }
 
@@ -277,8 +269,6 @@ static int lirc_allocate_driver(struct lirc_driver *d)
 		goto out;
 	}
 
-	dev_dbg(d->dev, "lirc_dev: lirc_register_driver: sample_rate: %d\n",
-		d->sample_rate);
 	if (d->sample_rate) {
 		if (2 > d->sample_rate || HZ < d->sample_rate) {
 			dev_err(d->dev, "lirc_dev: lirc_register_driver: "
@@ -525,10 +515,6 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file)
 	}
 
 error:
-	if (ir)
-		dev_dbg(ir->d.dev, LOGHEAD "open result = %d\n",
-			ir->d.name, ir->d.minor, retval);
-
 	mutex_unlock(&lirc_dev_lock);
 
 	nonseekable_open(inode, file);
@@ -550,8 +536,6 @@ int lirc_dev_fop_close(struct inode *inode, struct file *file)
 
 	cdev = ir->cdev;
 
-	dev_dbg(ir->d.dev, LOGHEAD "close called\n", ir->d.name, ir->d.minor);
-
 	ret = mutex_lock_killable(&lirc_dev_lock);
 	WARN_ON(ret);
 
@@ -586,8 +570,6 @@ unsigned int lirc_dev_fop_poll(struct file *file, poll_table *wait)
 		return POLLERR;
 	}
 
-	dev_dbg(ir->d.dev, LOGHEAD "poll called\n", ir->d.name, ir->d.minor);
-
 	if (!ir->attached)
 		return POLLERR;
 
@@ -683,9 +665,6 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		result = -EINVAL;
 	}
 
-	dev_dbg(ir->d.dev, LOGHEAD "ioctl result = %d\n",
-		ir->d.name, ir->d.minor, result);
-
 	mutex_unlock(&ir->irctl_lock);
 
 	return result;
@@ -790,8 +769,6 @@ out_locked:
 
 out_unlocked:
 	kfree(buf);
-	dev_dbg(ir->d.dev, LOGHEAD "read result = %s (%d)\n",
-		ir->d.name, ir->d.minor, ret ? "<fail>" : "<ok>", ret);
 
 	return ret ? ret : written;
 }
@@ -814,8 +791,6 @@ ssize_t lirc_dev_fop_write(struct file *file, const char __user *buffer,
 		return -ENODEV;
 	}
 
-	dev_dbg(ir->d.dev, LOGHEAD "write called\n", ir->d.name, ir->d.minor);
-
 	if (!ir->attached)
 		return -ENODEV;
 
-- 
2.8.1

[PATCH v2 04/15] [media] lirc_dev: replace printk with pr_* or dev_*

[Andi Shyti]

This patch mutes also all the checkpatch warnings related to
printk.

Reword all the printouts so that the string doesn't need to be
split, which fixes the following checkpatch warning:

  WARNING: quoted string split across lines

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

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index ee997ab..212ea77 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -19,6 +19,8 @@
  *
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/sched.h>
@@ -240,59 +242,51 @@ static int lirc_allocate_driver(struct lirc_driver *d)
 	int err;
 
 	if (!d) {
-		printk(KERN_ERR "lirc_dev: lirc_register_driver: "
-		       "driver pointer must be not NULL!\n");
+		pr_err("driver pointer must be not NULL!\n");
 		err = -EBADRQC;
 		goto out;
 	}
 
 	if (!d->dev) {
-		printk(KERN_ERR "%s: dev pointer not filled in!\n", __func__);
+		pr_err("dev pointer not filled in!\n");
 		err = -EINVAL;
 		goto out;
 	}
 
 	if (MAX_IRCTL_DEVICES <= d->minor) {
-		dev_err(d->dev, "lirc_dev: lirc_register_driver: "
-			"\"minor\" must be between 0 and %d (%d)!\n",
-			MAX_IRCTL_DEVICES - 1, d->minor);
+		dev_err(d->dev, "minor must be between 0 and %d!\n",
+						MAX_IRCTL_DEVICES - 1);
 		err = -EBADRQC;
 		goto out;
 	}
 
 	if (1 > d->code_length || (BUFLEN * 8) < d->code_length) {
-		dev_err(d->dev, "lirc_dev: lirc_register_driver: "
-			"code length in bits for minor (%d) "
-			"must be less than %d!\n",
-			d->minor, BUFLEN * 8);
+		dev_err(d->dev, "code length must be less than %d bits\n",
+								BUFLEN * 8);
 		err = -EBADRQC;
 		goto out;
 	}
 
 	if (d->sample_rate) {
 		if (2 > d->sample_rate || HZ < d->sample_rate) {
-			dev_err(d->dev, "lirc_dev: lirc_register_driver: "
-				"sample_rate must be between 2 and %d!\n", HZ);
+			dev_err(d->dev, "invalid %d sample rate\n",
+							d->sample_rate);
 			err = -EBADRQC;
 			goto out;
 		}
 		if (!d->add_to_buf) {
-			dev_err(d->dev, "lirc_dev: lirc_register_driver: "
-				"add_to_buf cannot be NULL when "
-				"sample_rate is set\n");
+			dev_err(d->dev, "add_to_buf not set\n");
 			err = -EBADRQC;
 			goto out;
 		}
 	} else if (!(d->fops && d->fops->read) && !d->rbuf) {
-		dev_err(d->dev, "lirc_dev: lirc_register_driver: "
-			"fops->read and rbuf cannot all be NULL!\n");
+		dev_err(d->dev, "fops->read and rbuf are NULL!\n");
 		err = -EBADRQC;
 		goto out;
 	} else if (!d->rbuf) {
 		if (!(d->fops && d->fops->read && d->fops->poll &&
 		      d->fops->unlocked_ioctl)) {
-			dev_err(d->dev, "lirc_dev: lirc_register_driver: "
-				"neither read, poll nor unlocked_ioctl can be NULL!\n");
+			dev_err(d->dev, "undefined read, poll, ioctl\n");
 			err = -EBADRQC;
 			goto out;
 		}
@@ -308,14 +302,12 @@ static int lirc_allocate_driver(struct lirc_driver *d)
 			if (!irctls[minor])
 				break;
 		if (MAX_IRCTL_DEVICES == minor) {
-			dev_err(d->dev, "lirc_dev: lirc_register_driver: "
-				"no free slots for drivers!\n");
+			dev_err(d->dev, "no free slots for drivers!\n");
 			err = -ENOMEM;
 			goto out_lock;
 		}
 	} else if (irctls[minor]) {
-		dev_err(d->dev, "lirc_dev: lirc_register_driver: "
-			"minor (%d) just registered!\n", minor);
+		dev_err(d->dev, "minor (%d) just registered!\n", minor);
 		err = -EBUSY;
 		goto out_lock;
 	}
@@ -352,9 +344,8 @@ static int lirc_allocate_driver(struct lirc_driver *d)
 		/* try to fire up polling thread */
 		ir->task = kthread_run(lirc_thread, (void *)ir, "lirc_dev");
 		if (IS_ERR(ir->task)) {
-			dev_err(d->dev, "lirc_dev: lirc_register_driver: "
-				"cannot run poll thread for minor = %d\n",
-				d->minor);
+			dev_err(d->dev, "cannot run thread for minor = %d\n",
+								d->minor);
 			err = -ECHILD;
 			goto out_sysfs;
 		}
@@ -407,15 +398,14 @@ int lirc_unregister_driver(int minor)
 	struct cdev *cdev;
 
 	if (minor < 0 || minor >= MAX_IRCTL_DEVICES) {
-		printk(KERN_ERR "lirc_dev: %s: minor (%d) must be between "
-		       "0 and %d!\n", __func__, minor, MAX_IRCTL_DEVICES - 1);
+		pr_err("minor (%d) must be between 0 and %d!\n",
+					minor, MAX_IRCTL_DEVICES - 1);
 		return -EBADRQC;
 	}
 
 	ir = irctls[minor];
 	if (!ir) {
-		printk(KERN_ERR "lirc_dev: %s: failed to get irctl struct "
-		       "for minor %d!\n", __func__, minor);
+		pr_err("failed to get irctl\n");
 		return -ENOENT;
 	}
 
@@ -424,8 +414,8 @@ int lirc_unregister_driver(int minor)
 	mutex_lock(&lirc_dev_lock);
 
 	if (ir->d.minor != minor) {
-		printk(KERN_ERR "lirc_dev: %s: minor (%d) device not "
-		       "registered!\n", __func__, minor);
+		dev_err(ir->d.dev, "lirc_dev: minor %d device not registered\n",
+									minor);
 		mutex_unlock(&lirc_dev_lock);
 		return -ENOENT;
 	}
@@ -467,8 +457,7 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file)
 	int retval = 0;
 
 	if (iminor(inode) >= MAX_IRCTL_DEVICES) {
-		printk(KERN_WARNING "lirc_dev [%d]: open result = -ENODEV\n",
-		       iminor(inode));
+		pr_err("open result for %d is -ENODEV\n", iminor(inode));
 		return -ENODEV;
 	}
 
@@ -530,7 +519,7 @@ int lirc_dev_fop_close(struct inode *inode, struct file *file)
 	int ret;
 
 	if (!ir) {
-		printk(KERN_ERR "%s: called with invalid irctl\n", __func__);
+		pr_err("called with invalid irctl\n");
 		return -EINVAL;
 	}
 
@@ -566,7 +555,7 @@ unsigned int lirc_dev_fop_poll(struct file *file, poll_table *wait)
 	unsigned int ret;
 
 	if (!ir) {
-		printk(KERN_ERR "%s: called with invalid irctl\n", __func__);
+		pr_err("called with invalid irctl\n");
 		return POLLERR;
 	}
 
@@ -597,7 +586,7 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	struct irctl *ir = irctls[iminor(file_inode(file))];
 
 	if (!ir) {
-		printk(KERN_ERR "lirc_dev: %s: no irctl found!\n", __func__);
+		pr_err("no irctl found!\n");
 		return -ENODEV;
 	}
 
@@ -605,7 +594,7 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		ir->d.name, ir->d.minor, cmd);
 
 	if (ir->d.minor == NOPLUG || !ir->attached) {
-		dev_dbg(ir->d.dev, LOGHEAD "ioctl result = -ENODEV\n",
+		dev_err(ir->d.dev, LOGHEAD "ioctl result = -ENODEV\n",
 			ir->d.name, ir->d.minor);
 		return -ENODEV;
 	}
@@ -682,7 +671,7 @@ ssize_t lirc_dev_fop_read(struct file *file,
 	DECLARE_WAITQUEUE(wait, current);
 
 	if (!ir) {
-		printk(KERN_ERR "%s: called with invalid irctl\n", __func__);
+		pr_err("called with invalid irctl\n");
 		return -ENODEV;
 	}
 
@@ -787,7 +776,7 @@ ssize_t lirc_dev_fop_write(struct file *file, const char __user *buffer,
 	struct irctl *ir = irctls[iminor(file_inode(file))];
 
 	if (!ir) {
-		printk(KERN_ERR "%s: called with invalid irctl\n", __func__);
+		pr_err("called with invalid irctl\n");
 		return -ENODEV;
 	}
 
@@ -806,7 +795,7 @@ static int __init lirc_dev_init(void)
 	lirc_class = class_create(THIS_MODULE, "lirc");
 	if (IS_ERR(lirc_class)) {
 		retval = PTR_ERR(lirc_class);
-		printk(KERN_ERR "lirc_dev: class_create failed\n");
+		pr_err("class_create failed\n");
 		goto error;
 	}
 
@@ -814,13 +803,13 @@ static int __init lirc_dev_init(void)
 				     IRCTL_DEV_NAME);
 	if (retval) {
 		class_destroy(lirc_class);
-		printk(KERN_ERR "lirc_dev: alloc_chrdev_region failed\n");
+		pr_err("alloc_chrdev_region failed\n");
 		goto error;
 	}
 
 
-	printk(KERN_INFO "lirc_dev: IR Remote Control driver registered, "
-	       "major %d \n", MAJOR(lirc_base_dev));
+	pr_info("IR Remote Control driver registered, major %d\n",
+						MAJOR(lirc_base_dev));
 
 error:
 	return retval;
@@ -832,7 +821,7 @@ static void __exit lirc_dev_exit(void)
 {
 	class_destroy(lirc_class);
 	unregister_chrdev_region(lirc_base_dev, MAX_IRCTL_DEVICES);
-	printk(KERN_INFO "lirc_dev: module unloaded\n");
+	pr_info("module unloaded\n");
 }
 
 module_init(lirc_dev_init);
-- 
2.8.1

[PATCH v2 05/15] [media] lirc_dev: simplify goto paths

[Andi Shyti]

The code can be rearranged so that some goto paths can be removed

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

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 212ea77..0d988c9 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -243,52 +243,44 @@ static int lirc_allocate_driver(struct lirc_driver *d)
 
 	if (!d) {
 		pr_err("driver pointer must be not NULL!\n");
-		err = -EBADRQC;
-		goto out;
+		return -EBADRQC;
 	}
 
 	if (!d->dev) {
 		pr_err("dev pointer not filled in!\n");
-		err = -EINVAL;
-		goto out;
+		return -EINVAL;
 	}
 
 	if (MAX_IRCTL_DEVICES <= d->minor) {
 		dev_err(d->dev, "minor must be between 0 and %d!\n",
 						MAX_IRCTL_DEVICES - 1);
-		err = -EBADRQC;
-		goto out;
+		return -EBADRQC;
 	}
 
 	if (1 > d->code_length || (BUFLEN * 8) < d->code_length) {
 		dev_err(d->dev, "code length must be less than %d bits\n",
 								BUFLEN * 8);
-		err = -EBADRQC;
-		goto out;
+		return -EBADRQC;
 	}
 
 	if (d->sample_rate) {
 		if (2 > d->sample_rate || HZ < d->sample_rate) {
 			dev_err(d->dev, "invalid %d sample rate\n",
 							d->sample_rate);
-			err = -EBADRQC;
-			goto out;
+			return -EBADRQC;
 		}
 		if (!d->add_to_buf) {
 			dev_err(d->dev, "add_to_buf not set\n");
-			err = -EBADRQC;
-			goto out;
+			return -EBADRQC;
 		}
 	} else if (!(d->fops && d->fops->read) && !d->rbuf) {
 		dev_err(d->dev, "fops->read and rbuf are NULL!\n");
-		err = -EBADRQC;
-		goto out;
+		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");
-			err = -EBADRQC;
-			goto out;
+			return -EBADRQC;
 		}
 	}
 
@@ -366,7 +358,7 @@ out_sysfs:
 	device_destroy(lirc_class, MKDEV(MAJOR(lirc_base_dev), ir->d.minor));
 out_lock:
 	mutex_unlock(&lirc_dev_lock);
-out:
+
 	return err;
 }
 
@@ -794,9 +786,8 @@ static int __init lirc_dev_init(void)
 
 	lirc_class = class_create(THIS_MODULE, "lirc");
 	if (IS_ERR(lirc_class)) {
-		retval = PTR_ERR(lirc_class);
 		pr_err("class_create failed\n");
-		goto error;
+		return PTR_ERR(lirc_class);
 	}
 
 	retval = alloc_chrdev_region(&lirc_base_dev, 0, MAX_IRCTL_DEVICES,
@@ -804,15 +795,14 @@ static int __init lirc_dev_init(void)
 	if (retval) {
 		class_destroy(lirc_class);
 		pr_err("alloc_chrdev_region failed\n");
-		goto error;
+		return retval;
 	}
 
 
 	pr_info("IR Remote Control driver registered, major %d\n",
 						MAJOR(lirc_base_dev));
 
-error:
-	return retval;
+	return 0;
 }
 
 
-- 
2.8.1

[PATCH v2 06/15] [media] lirc_dev: do not use goto to create loops

[Andi Shyti]

... use "do .. while" instead.

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

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 0d988c9..da1777c 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -99,18 +99,16 @@ static int lirc_add_to_buf(struct irctl *ir)
 {
 	if (ir->d.add_to_buf) {
 		int res = -ENODATA;
-		int got_data = 0;
+		int got_data = -1;
 
 		/*
 		 * service the device as long as it is returning
 		 * data and we have space
 		 */
-get_data:
-		res = ir->d.add_to_buf(ir->d.data, ir->buf);
-		if (res == 0) {
+		do {
 			got_data++;
-			goto get_data;
-		}
+			res = ir->d.add_to_buf(ir->d.data, ir->buf);
+		} while (!res);
 
 		if (res == -ENODEV)
 			kthread_stop(ir->task);
-- 
2.8.1

[PATCH v2 07/15] [media] lirc_dev: simplify if statement in lirc_add_to_buf

[Andi Shyti]

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 | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index da1777c..2468849 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -97,26 +97,25 @@ 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 = -1;
+	int res;
+	int got_data = -1;
 
-		/*
-		 * service the device as long as it is returning
-		 * data and we have space
-		 */
-		do {
-			got_data++;
-			res = ir->d.add_to_buf(ir->d.data, ir->buf);
-		} 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 {
+		got_data++;
+		res = ir->d.add_to_buf(ir->d.data, ir->buf);
+	} 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

[PATCH v2 08/15] [media] lirc_dev: remove double if ... else statement

[Andi Shyti]

There are two if ... else which check the same thing in different
part of the code, they can be merged in a single check.

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

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 2468849..2643336 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -310,13 +310,6 @@ static int lirc_allocate_driver(struct lirc_driver *d)
 	irctls[minor] = ir;
 	d->minor = minor;
 
-	if (d->sample_rate) {
-		ir->jiffies_to_wait = HZ / d->sample_rate;
-	} else {
-		/* it means - wait for external event in task queue */
-		ir->jiffies_to_wait = 0;
-	}
-
 	/* some safety check 8-) */
 	d->name[sizeof(d->name)-1] = '\0';
 
@@ -330,6 +323,8 @@ static int lirc_allocate_driver(struct lirc_driver *d)
 		      "lirc%u", ir->d.minor);
 
 	if (d->sample_rate) {
+		ir->jiffies_to_wait = HZ / d->sample_rate;
+
 		/* try to fire up polling thread */
 		ir->task = kthread_run(lirc_thread, (void *)ir, "lirc_dev");
 		if (IS_ERR(ir->task)) {
@@ -338,6 +333,9 @@ static int lirc_allocate_driver(struct lirc_driver *d)
 			err = -ECHILD;
 			goto out_sysfs;
 		}
+	} else {
+		/* it means - wait for external event in task queue */
+		ir->jiffies_to_wait = 0;
 	}
 
 	err = lirc_cdev_add(ir);
-- 
2.8.1

[PATCH v2 09/15] [media] lirc_dev: merge three if statements in only one

[Andi Shyti]

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 2643336..d98a9f1 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -270,15 +270,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

[PATCH v2 10/15] [media] lirc_dev: remove CONFIG_COMPAT precompiler check

[Andi Shyti]

There is no need to check in precompilation whether the ioctl is
compat or unlocked, depending on the configuration it will be
called the correct one.

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

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index d98a9f1..16cca46 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -150,9 +150,7 @@ static const struct file_operations lirc_dev_fops = {
 	.write		= lirc_dev_fop_write,
 	.poll		= lirc_dev_fop_poll,
 	.unlocked_ioctl	= lirc_dev_fop_ioctl,
-#ifdef CONFIG_COMPAT
 	.compat_ioctl	= lirc_dev_fop_ioctl,
-#endif
 	.open		= lirc_dev_fop_open,
 	.release	= lirc_dev_fop_close,
 	.llseek		= noop_llseek,
-- 
2.8.1

Re: [PATCH v2 10/15] [media] lirc_dev: remove CONFIG_COMPAT precompiler check

[Hans Verkuil]

On 07/01/2016 10:01 AM, Andi Shyti wrote:
> There is no need to check in precompilation whether the ioctl is
> compat or unlocked, depending on the configuration it will be
> called the correct one.
> 
> Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
> ---
>  drivers/media/rc/lirc_dev.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
> index d98a9f1..16cca46 100644
> --- a/drivers/media/rc/lirc_dev.c
> +++ b/drivers/media/rc/lirc_dev.c
> @@ -150,9 +150,7 @@ static const struct file_operations lirc_dev_fops = {
>  	.write		= lirc_dev_fop_write,
>  	.poll		= lirc_dev_fop_poll,
>  	.unlocked_ioctl	= lirc_dev_fop_ioctl,
> -#ifdef CONFIG_COMPAT
>  	.compat_ioctl	= lirc_dev_fop_ioctl,
> -#endif

I'd say this compat_ioctl can be dropped completely since there is nothing to do.

Regards,

	Hans

>  	.open		= lirc_dev_fop_open,
>  	.release	= lirc_dev_fop_close,
>  	.llseek		= noop_llseek,
> 

[PATCH v2 11/15] [media] lirc_dev: fix variable constant comparisons

[Andi Shyti]

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 16cca46..689e369 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -246,13 +246,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;
@@ -283,7 +283,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

[PATCH v2 12/15] [media] lirc_dev: fix error return value

[Andi Shyti]

If ioctl is called, it cannot be a case of invalid system call
number (ENOSYS), that is an operation not permitted (EPERM).
Replace ENOSYS with EPERM.

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

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 689e369..99d1f98 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -587,7 +587,7 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		break;
 	case LIRC_GET_REC_MODE:
 		if (!(ir->d.features & LIRC_CAN_REC_MASK)) {
-			result = -ENOSYS;
+			result = -EPERM;
 			break;
 		}
 
@@ -597,7 +597,7 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		break;
 	case LIRC_SET_REC_MODE:
 		if (!(ir->d.features & LIRC_CAN_REC_MASK)) {
-			result = -ENOSYS;
+			result = -EPERM;
 			break;
 		}
 
@@ -615,7 +615,7 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case LIRC_GET_MIN_TIMEOUT:
 		if (!(ir->d.features & LIRC_CAN_SET_REC_TIMEOUT) ||
 		    ir->d.min_timeout == 0) {
-			result = -ENOSYS;
+			result = -EPERM;
 			break;
 		}
 
@@ -624,7 +624,7 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case LIRC_GET_MAX_TIMEOUT:
 		if (!(ir->d.features & LIRC_CAN_SET_REC_TIMEOUT) ||
 		    ir->d.max_timeout == 0) {
-			result = -ENOSYS;
+			result = -EPERM;
 			break;
 		}
 
-- 
2.8.1

Re: [PATCH v2 12/15] [media] lirc_dev: fix error return value

[Hans Verkuil]

On 07/01/2016 10:01 AM, Andi Shyti wrote:
> If ioctl is called, it cannot be a case of invalid system call
> number (ENOSYS), that is an operation not permitted (EPERM).
> Replace ENOSYS with EPERM.

I'd say it is ENOTTY, i.e. this hardware does not support this ioctl.

Regards,

	Hans

> 
> Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
> ---
>  drivers/media/rc/lirc_dev.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
> index 689e369..99d1f98 100644
> --- a/drivers/media/rc/lirc_dev.c
> +++ b/drivers/media/rc/lirc_dev.c
> @@ -587,7 +587,7 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  		break;
>  	case LIRC_GET_REC_MODE:
>  		if (!(ir->d.features & LIRC_CAN_REC_MASK)) {
> -			result = -ENOSYS;
> +			result = -EPERM;
>  			break;
>  		}
>  
> @@ -597,7 +597,7 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  		break;
>  	case LIRC_SET_REC_MODE:
>  		if (!(ir->d.features & LIRC_CAN_REC_MASK)) {
> -			result = -ENOSYS;
> +			result = -EPERM;
>  			break;
>  		}
>  
> @@ -615,7 +615,7 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  	case LIRC_GET_MIN_TIMEOUT:
>  		if (!(ir->d.features & LIRC_CAN_SET_REC_TIMEOUT) ||
>  		    ir->d.min_timeout == 0) {
> -			result = -ENOSYS;
> +			result = -EPERM;
>  			break;
>  		}
>  
> @@ -624,7 +624,7 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  	case LIRC_GET_MAX_TIMEOUT:
>  		if (!(ir->d.features & LIRC_CAN_SET_REC_TIMEOUT) ||
>  		    ir->d.max_timeout == 0) {
> -			result = -ENOSYS;
> +			result = -EPERM;
>  			break;
>  		}
>  
> 

[PATCH v2 13/15] [media] lirc_dev: extremely trivial comment style fix

[Andi Shyti]

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

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 99d1f98..4b3efcf 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -692,7 +692,8 @@ ssize_t lirc_dev_fop_read(struct file *file,
 			/* According to the read(2) man page, 'written' can be
 			 * returned as less than 'length', instead of blocking
 			 * again, returning -EWOULDBLOCK, or returning
-			 * -ERESTARTSYS */
+			 * -ERESTARTSYS
+			 */
 			if (written)
 				break;
 			if (file->f_flags & O_NONBLOCK) {
-- 
2.8.1

[PATCH v2 14/15] [media] lirc_dev: fix potential segfault

[Andi Shyti]

When opening or closing a lirc character device, the framework
provides to the user the possibility to keep track of opening or
closing of the device by calling two functions:

 - set_use_inc() when opening the device
 - set_use_dec() when closing the device

if those are not set by the lirc user, the system segfaults.
Check the pointer value before calling the above functions.

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

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 4b3efcf..634779a 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -413,7 +413,10 @@ int lirc_unregister_driver(int minor)
 			ir->d.name, ir->d.minor);
 		wake_up_interruptible(&ir->buf->wait_poll);
 		mutex_lock(&ir->irctl_lock);
-		ir->d.set_use_dec(ir->d.data);
+
+		if (ir->d.set_use_dec)
+			ir->d.set_use_dec(ir->d.data);
+
 		module_put(cdev->owner);
 		mutex_unlock(&ir->irctl_lock);
 	} else {
@@ -471,7 +474,8 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file)
 	cdev = ir->cdev;
 	if (try_module_get(cdev->owner)) {
 		ir->open++;
-		retval = ir->d.set_use_inc(ir->d.data);
+		if (ir->d.set_use_inc)
+			retval = ir->d.set_use_inc(ir->d.data);
 
 		if (retval) {
 			module_put(cdev->owner);
@@ -512,7 +516,8 @@ int lirc_dev_fop_close(struct inode *inode, struct file *file)
 
 	ir->open--;
 	if (ir->attached) {
-		ir->d.set_use_dec(ir->d.data);
+		if (ir->d.set_use_dec)
+			ir->d.set_use_dec(ir->d.data);
 		module_put(cdev->owner);
 	} else {
 		lirc_irctl_cleanup(ir);
-- 
2.8.1

[PATCH v2 15/15] [media] include: lirc: add LIRC_GET_LENGTH command

[Andi Shyti]

Added the get length command to allow userspace users to check on
the data length.

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

diff --git a/include/uapi/linux/lirc.h b/include/uapi/linux/lirc.h
index 4b3ab29..801e5f8 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)
-- 
2.8.1

Re: [PATCH v2 15/15] [media] include: lirc: add LIRC_GET_LENGTH command

[Sean Young]

On Fri, Jul 01, 2016 at 05:01:38PM +0900, Andi Shyti wrote:
> Added the get length command to allow userspace users to check on
> the data length.

So what does LIRC_GET_LENGTH do? If you want to add an ioctl, it
need justification, documenting in 
Documentatoin/DocBook/media/v4l/lirc_device_interface.xml and there
should be at least one driver using it.

If you want to write a new driver which does IR transmit, it's best
to use the rc-core interface like the existing drivers and do 
not use the lirc interface. Most of the other drivers which implement
IR transmit use rc-core too.


Sean