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

Groups > linux.kernel > #1446570 > unrolled thread

[RFC 0/7] Add support for IR transmitters

Back to article view | Back to linux.kernel

Contents

  [RFC 0/7] Add support for IR transmitters Andi Shyti <andi.shyti@samsung.com> - 2016-07-19 18:00 +0200
    [RFC 2/7] [media] rc-main: split setup and unregister functions Andi Shyti <andi.shyti@samsung.com> - 2016-07-19 18:00 +0200
    [RFC 4/7] [media] rc-ir-raw: do not generate any receiving thread for  raw transmitters Andi Shyti <andi.shyti@samsung.com> - 2016-07-19 18:00 +0200
    [RFC 3/7] [media] rc-core: add support for IR raw transmitters Andi Shyti <andi.shyti@samsung.com> - 2016-07-19 18:00 +0200
      Re: [RFC 3/7] [media] rc-core: add support for IR raw transmitters Sean Young <sean@mess.org> - 2016-07-20 00:20 +0200
        Re: [RFC 3/7] [media] rc-core: add support for IR raw transmitters Andi Shyti <andi.shyti@samsung.com> - 2016-07-21 02:50 +0200

#1446570 — [RFC 0/7] Add support for IR transmitters

Hi,

this is an RFCset that follows this patch:

http://marc.info/?l=linux-kernel&m=146736225606125&w=2

and after Sean's review and recommendations:

http://marc.info/?l=linux-kernel&m=146737935611128&w=2

The main goal is to add support in the rc framework for IR
transmitters, which currently is only supported by lirc but that
is not the preferred way.

with this RFCset I'm trying to gather some opinions as I'm not
really aware of other use cases other than the simple ir
transmitter in the last patch. As it is, the code to me looks
quite forced in order to achieve "my" goal by abusing on the
driver type check.

The last rfc-patch adds support for an IR transmitter driven by
the MOSI line of an SPI controller, it's the case of the Samsung
TM2(e) board which support is going to come soon.

Please let me know if there is anything to improve.

Thanks,
Andi

Andi Shyti (7):
  [media] rc-main: assign driver type during allocation
  [media] rc-main: split setup and unregister functions
  [media] rc-core: add support for IR raw transmitters
  [media] rc-ir-raw: do not generate any receiving thread for raw
    transmitters
  [media] ir-lirc-codec: do not handle any buffer for raw transmitters
  Documentation: bindings: add documentation for ir-spi device driver
  [media] rc: add support for IR LEDs driven through SPI

 Documentation/devicetree/bindings/media/spi-ir.txt |  20 +++
 drivers/media/rc/Kconfig                           |   9 ++
 drivers/media/rc/Makefile                          |   1 +
 drivers/media/rc/ir-lirc-codec.c                   |  30 ++--
 drivers/media/rc/ir-spi.c                          | 133 +++++++++++++++
 drivers/media/rc/rc-ir-raw.c                       |  17 +-
 drivers/media/rc/rc-main.c                         | 179 ++++++++++++---------
 include/media/rc-core.h                            |   3 +-
 8 files changed, 299 insertions(+), 93 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/spi-ir.txt
 create mode 100644 drivers/media/rc/ir-spi.c

-- 
2.8.1

[toc] | [next] | [standalone]

#1446572 — [RFC 2/7] [media] rc-main: split setup and unregister functions

Move the input device allocation, map and protocol handling to
different functions.

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

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 6403674..ac91157 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -1396,16 +1396,12 @@ void rc_free_device(struct rc_dev *dev)
 }
 EXPORT_SYMBOL_GPL(rc_free_device);
 
-int rc_register_device(struct rc_dev *dev)
+static int rc_setup_rx_device(struct rc_dev *dev)
 {
-	static bool raw_init = false; /* raw decoders loaded? */
-	struct rc_map *rc_map;
-	const char *path;
-	int attr = 0;
-	int minor;
 	int rc;
+	struct rc_map *rc_map;
 
-	if (!dev || !dev->map_name)
+	if (!dev->map_name)
 		return -EINVAL;
 
 	rc_map = rc_map_get(dev->map_name);
@@ -1414,6 +1410,19 @@ int rc_register_device(struct rc_dev *dev)
 	if (!rc_map || !rc_map->scan || rc_map->size == 0)
 		return -EINVAL;
 
+	rc = ir_setkeytable(dev, rc_map);
+	if (rc)
+		return rc;
+
+	if (dev->change_protocol) {
+		u64 rc_type = (1ll << rc_map->rc_type);
+
+		rc = dev->change_protocol(dev, &rc_type);
+		if (rc < 0)
+			goto out_table;
+		dev->enabled_protocols = rc_type;
+	}
+
 	set_bit(EV_KEY, dev->input_dev->evbit);
 	set_bit(EV_REP, dev->input_dev->evbit);
 	set_bit(EV_MSC, dev->input_dev->evbit);
@@ -1423,6 +1432,61 @@ int rc_register_device(struct rc_dev *dev)
 	if (dev->close)
 		dev->input_dev->close = ir_close;
 
+	/*
+	 * Default delay of 250ms is too short for some protocols, especially
+	 * since the timeout is currently set to 250ms. Increase it to 500ms,
+	 * to avoid wrong repetition of the keycodes. Note that this must be
+	 * set after the call to input_register_device().
+	 */
+	dev->input_dev->rep[REP_DELAY] = 500;
+
+	/*
+	 * As a repeat event on protocols like RC-5 and NEC take as long as
+	 * 110/114ms, using 33ms as a repeat period is not the right thing
+	 * to do.
+	 */
+	dev->input_dev->rep[REP_PERIOD] = 125;
+
+	/* rc_open will be called here */
+	rc = input_register_device(dev->input_dev);
+	if (rc)
+		goto out_table;
+
+	dev->input_dev->dev.parent = &dev->dev;
+	memcpy(&dev->input_dev->id, &dev->input_id, sizeof(dev->input_id));
+	dev->input_dev->phys = dev->input_phys;
+	dev->input_dev->name = dev->input_name;
+
+	return 0;
+
+out_table:
+	ir_free_table(&dev->rc_map);
+
+	return rc;
+}
+
+static void rc_free_rx_device(struct rc_dev *dev)
+{
+	if (!dev || dev->driver_type == RC_DRIVER_IR_RAW_TX)
+		return;
+
+	ir_free_table(&dev->rc_map);
+
+	input_unregister_device(dev->input_dev);
+	dev->input_dev = NULL;
+}
+
+int rc_register_device(struct rc_dev *dev)
+{
+	static bool raw_init = false; /* raw decoders loaded? */
+	const char *path;
+	int attr = 0;
+	int minor;
+	int rc;
+
+	if (!dev)
+		return -EINVAL;
+
 	minor = ida_simple_get(&rc_ida, 0, RC_DEV_MAX, GFP_KERNEL);
 	if (minor < 0)
 		return minor;
@@ -1446,35 +1510,6 @@ int rc_register_device(struct rc_dev *dev)
 	if (rc)
 		goto out_unlock;
 
-	rc = ir_setkeytable(dev, rc_map);
-	if (rc)
-		goto out_dev;
-
-	dev->input_dev->dev.parent = &dev->dev;
-	memcpy(&dev->input_dev->id, &dev->input_id, sizeof(dev->input_id));
-	dev->input_dev->phys = dev->input_phys;
-	dev->input_dev->name = dev->input_name;
-
-	/*
-	 * Default delay of 250ms is too short for some protocols, especially
-	 * since the timeout is currently set to 250ms. Increase it to 500ms,
-	 * to avoid wrong repetition of the keycodes. Note that this must be
-	 * set after the call to input_register_device().
-	 */
-	dev->input_dev->rep[REP_DELAY] = 500;
-
-	/*
-	 * As a repeat event on protocols like RC-5 and NEC take as long as
-	 * 110/114ms, using 33ms as a repeat period is not the right thing
-	 * to do.
-	 */
-	dev->input_dev->rep[REP_PERIOD] = 125;
-
-	/* rc_open will be called here */
-	rc = input_register_device(dev->input_dev);
-	if (rc)
-		goto out_table;
-
 	path = kobject_get_path(&dev->dev.kobj, GFP_KERNEL);
 	dev_info(&dev->dev, "%s as %s\n",
 		dev->input_name ?: "Unspecified device", path ?: "N/A");
@@ -1487,36 +1522,20 @@ int rc_register_device(struct rc_dev *dev)
 		}
 		rc = ir_raw_event_register(dev);
 		if (rc < 0)
-			goto out_input;
-	}
-
-	if (dev->change_protocol) {
-		u64 rc_type = (1ll << rc_map->rc_type);
-		rc = dev->change_protocol(dev, &rc_type);
-		if (rc < 0)
-			goto out_raw;
-		dev->enabled_protocols = rc_type;
+			goto out_rx;
 	}
 
 	/* Allow the RC sysfs nodes to be accessible */
 	atomic_set(&dev->initialized, 1);
 
-	IR_dprintk(1, "Registered rc%u (driver: %s, remote: %s, mode %s)\n",
+	IR_dprintk(1, "Registered rc%u (driver: %s)\n",
 		   dev->minor,
-		   dev->driver_name ? dev->driver_name : "unknown",
-		   rc_map->name ? rc_map->name : "unknown",
-		   dev->driver_type == RC_DRIVER_IR_RAW ? "raw" : "cooked");
+		   dev->driver_name ? dev->driver_name : "unknown");
 
 	return 0;
 
-out_raw:
-	if (dev->driver_type == RC_DRIVER_IR_RAW)
-		ir_raw_event_unregister(dev);
-out_input:
-	input_unregister_device(dev->input_dev);
-	dev->input_dev = NULL;
-out_table:
-	ir_free_table(&dev->rc_map);
+out_rx:
+	rc_free_rx_device(dev);
 out_dev:
 	device_del(&dev->dev);
 out_unlock:
@@ -1535,12 +1554,7 @@ void rc_unregister_device(struct rc_dev *dev)
 	if (dev->driver_type == RC_DRIVER_IR_RAW)
 		ir_raw_event_unregister(dev);
 
-	/* Freeing the table should also call the stop callback */
-	ir_free_table(&dev->rc_map);
-	IR_dprintk(1, "Freed keycode table\n");
-
-	input_unregister_device(dev->input_dev);
-	dev->input_dev = NULL;
+	rc_free_rx_device(dev);
 
 	device_del(&dev->dev);
 
-- 
2.8.1

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

#1446578 — [RFC 4/7] [media] rc-ir-raw: do not generate any receiving thread for raw transmitters

Raw IR transmitters do not need any thread listening for
occurring events. Check the driver type before running the
thread.

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

diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
index 144304c..64ddc3d 100644
--- a/drivers/media/rc/rc-ir-raw.c
+++ b/drivers/media/rc/rc-ir-raw.c
@@ -274,12 +274,19 @@ int ir_raw_event_register(struct rc_dev *dev)
 	INIT_KFIFO(dev->raw->kfifo);
 
 	spin_lock_init(&dev->raw->lock);
-	dev->raw->thread = kthread_run(ir_raw_event_thread, dev->raw,
-				       "rc%u", dev->minor);
 
-	if (IS_ERR(dev->raw->thread)) {
-		rc = PTR_ERR(dev->raw->thread);
-		goto out;
+	/*
+	 * raw transmitters do not need any event registration
+	 * because the event is coming from userspace
+	 */
+	if (dev->driver_type != RC_DRIVER_IR_RAW_TX) {
+		dev->raw->thread = kthread_run(ir_raw_event_thread, dev->raw,
+					       "rc%u", dev->minor);
+
+		if (IS_ERR(dev->raw->thread)) {
+			rc = PTR_ERR(dev->raw->thread);
+			goto out;
+		}
 	}
 
 	mutex_lock(&ir_raw_handler_lock);
-- 
2.8.1

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

#1446579 — [RFC 3/7] [media] rc-core: add support for IR raw transmitters

IR raw transmitter driver type is specified in the enum
rc_driver_type as RC_DRIVER_IR_RAW_TX which includes all those
devices that transmit raw stream of bit to a receiver.

The data are provided by userspace applications, therefore they
don't need any input device allocation, but still they need to be
registered as raw devices.

Suggested-by: Sean Young <sean@mess.org>
Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
---
 drivers/media/rc/rc-main.c | 35 +++++++++++++++++++++++------------
 include/media/rc-core.h    |  1 +
 2 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index ac91157..f555f38 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -1354,20 +1354,24 @@ struct rc_dev *rc_allocate_device(enum rc_driver_type type)
 	if (!dev)
 		return NULL;
 
-	dev->input_dev = input_allocate_device();
-	if (!dev->input_dev) {
-		kfree(dev);
-		return NULL;
-	}
+	if (type != RC_DRIVER_IR_RAW_TX) {
+		dev->input_dev = input_allocate_device();
+		if (!dev->input_dev) {
+			kfree(dev);
+			return NULL;
+		}
 
-	dev->input_dev->getkeycode = ir_getkeycode;
-	dev->input_dev->setkeycode = ir_setkeycode;
-	input_set_drvdata(dev->input_dev, dev);
+		dev->input_dev->getkeycode = ir_getkeycode;
+		dev->input_dev->setkeycode = ir_setkeycode;
+		input_set_drvdata(dev->input_dev, dev);
 
-	spin_lock_init(&dev->rc_map.lock);
-	spin_lock_init(&dev->keylock);
+		setup_timer(&dev->timer_keyup, ir_timer_keyup,
+						(unsigned long)dev);
+
+		spin_lock_init(&dev->rc_map.lock);
+		spin_lock_init(&dev->keylock);
+	}
 	mutex_init(&dev->lock);
-	setup_timer(&dev->timer_keyup, ir_timer_keyup, (unsigned long)dev);
 
 	dev->dev.type = &rc_dev_type;
 	dev->dev.class = &rc_class;
@@ -1515,7 +1519,14 @@ int rc_register_device(struct rc_dev *dev)
 		dev->input_name ?: "Unspecified device", path ?: "N/A");
 	kfree(path);
 
-	if (dev->driver_type == RC_DRIVER_IR_RAW) {
+	if (dev->driver_type != RC_DRIVER_IR_RAW_TX) {
+		rc = rc_setup_rx_device(dev);
+		if (rc)
+			goto out_dev;
+	}
+
+	if (dev->driver_type == RC_DRIVER_IR_RAW ||
+				dev->driver_type == RC_DRIVER_IR_RAW_TX) {
 		if (!raw_init) {
 			request_module_nowait("ir-lirc-codec");
 			raw_init = true;
diff --git a/include/media/rc-core.h b/include/media/rc-core.h
index c6bf1ef..77b0893 100644
--- a/include/media/rc-core.h
+++ b/include/media/rc-core.h
@@ -32,6 +32,7 @@ do {								\
 enum rc_driver_type {
 	RC_DRIVER_SCANCODE = 0,	/* Driver or hardware generates a scancode */
 	RC_DRIVER_IR_RAW,	/* Needs a Infra-Red pulse/space decoder */
+	RC_DRIVER_IR_RAW_TX,	/* Device is transmitter, driver handles raw */
 };
 
 /**
-- 
2.8.1

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

#1446750 — Re: [RFC 3/7] [media] rc-core: add support for IR raw transmitters

On Wed, Jul 20, 2016 at 12:56:54AM +0900, Andi Shyti wrote:
> IR raw transmitter driver type is specified in the enum
> rc_driver_type as RC_DRIVER_IR_RAW_TX which includes all those
> devices that transmit raw stream of bit to a receiver.
> 
> The data are provided by userspace applications, therefore they
> don't need any input device allocation, but still they need to be
> registered as raw devices.
> 
> Suggested-by: Sean Young <sean@mess.org>
> Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
> ---
>  drivers/media/rc/rc-main.c | 35 +++++++++++++++++++++++------------
>  include/media/rc-core.h    |  1 +
>  2 files changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index ac91157..f555f38 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -1354,20 +1354,24 @@ struct rc_dev *rc_allocate_device(enum rc_driver_type type)
>  	if (!dev)
>  		return NULL;
>  
> -	dev->input_dev = input_allocate_device();
> -	if (!dev->input_dev) {
> -		kfree(dev);
> -		return NULL;
> -	}
> +	if (type != RC_DRIVER_IR_RAW_TX) {
> +		dev->input_dev = input_allocate_device();
> +		if (!dev->input_dev) {
> +			kfree(dev);
> +			return NULL;
> +		}
>  
> -	dev->input_dev->getkeycode = ir_getkeycode;
> -	dev->input_dev->setkeycode = ir_setkeycode;
> -	input_set_drvdata(dev->input_dev, dev);
> +		dev->input_dev->getkeycode = ir_getkeycode;
> +		dev->input_dev->setkeycode = ir_setkeycode;
> +		input_set_drvdata(dev->input_dev, dev);
>  
> -	spin_lock_init(&dev->rc_map.lock);
> -	spin_lock_init(&dev->keylock);
> +		setup_timer(&dev->timer_keyup, ir_timer_keyup,
> +						(unsigned long)dev);
> +
> +		spin_lock_init(&dev->rc_map.lock);
> +		spin_lock_init(&dev->keylock);
> +	}
>  	mutex_init(&dev->lock);
> -	setup_timer(&dev->timer_keyup, ir_timer_keyup, (unsigned long)dev);
>  
>  	dev->dev.type = &rc_dev_type;
>  	dev->dev.class = &rc_class;
> @@ -1515,7 +1519,14 @@ int rc_register_device(struct rc_dev *dev)
>  		dev->input_name ?: "Unspecified device", path ?: "N/A");
>  	kfree(path);
>  
> -	if (dev->driver_type == RC_DRIVER_IR_RAW) {
> +	if (dev->driver_type != RC_DRIVER_IR_RAW_TX) {
> +		rc = rc_setup_rx_device(dev);
> +		if (rc)
> +			goto out_dev;
> +	}
> +
> +	if (dev->driver_type == RC_DRIVER_IR_RAW ||
> +				dev->driver_type == RC_DRIVER_IR_RAW_TX) {

Here the if is wrong. It should be 
"if (dev->driver_type != RC_DRIVER_IR_RAW_TX)". Note that as result
the decoder thread is not started, so patch 4 won't be needed either.


>  		if (!raw_init) {
>  			request_module_nowait("ir-lirc-codec");
>  			raw_init = true;
> diff --git a/include/media/rc-core.h b/include/media/rc-core.h
> index c6bf1ef..77b0893 100644
> --- a/include/media/rc-core.h
> +++ b/include/media/rc-core.h
> @@ -32,6 +32,7 @@ do {								\
>  enum rc_driver_type {
>  	RC_DRIVER_SCANCODE = 0,	/* Driver or hardware generates a scancode */
>  	RC_DRIVER_IR_RAW,	/* Needs a Infra-Red pulse/space decoder */
> +	RC_DRIVER_IR_RAW_TX,	/* Device is transmitter, driver handles raw */

The comment should really mention the lack of receiver.

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

#1447554 — Re: [RFC 3/7] [media] rc-core: add support for IR raw transmitters

Hi Sean,

> > +	if (dev->driver_type == RC_DRIVER_IR_RAW ||
> > +				dev->driver_type == RC_DRIVER_IR_RAW_TX) {
> 
> Here the if is wrong. It should be 
> "if (dev->driver_type != RC_DRIVER_IR_RAW_TX)". Note that as result
> the decoder thread is not started, so patch 4 won't be needed either.

but I need the ir-lirc-codec as it handles the interface with
userspace and it calls the tx_ir and s_tx_carrier.

if I do "if (dev->driver_type != RC_DRIVER_IR_RAW_TX)" the
lirc-codec is not called and I would need to handle it on my
driver, but then we fall in the first version of the driver.

Thanks,
Andi

> >  		if (!raw_init) {
> >  			request_module_nowait("ir-lirc-codec");
> >  			raw_init = true;

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web