[RFC 0/7] Add support for IR transmitters

[RFC 0/7] Add support for IR transmitters

[Andi Shyti]

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

[RFC 1/7] [media] rc-main: assign driver type during allocation

[Andi Shyti]

The driver type can be assigned immediately when an RC device
requests to the framework to allocate the device.

This is an 'enum rc_driver_type' data type and specifies whether
the device is a raw receiver or scancode receiver. The type will
be given as parameter to the rc_allocate_device device.

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

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 7dfc7c2..6403674 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -1346,7 +1346,7 @@ static struct device_type rc_dev_type = {
 	.uevent		= rc_dev_uevent,
 };
 
-struct rc_dev *rc_allocate_device(void)
+struct rc_dev *rc_allocate_device(enum rc_driver_type type)
 {
 	struct rc_dev *dev;
 
@@ -1373,6 +1373,8 @@ struct rc_dev *rc_allocate_device(void)
 	dev->dev.class = &rc_class;
 	device_initialize(&dev->dev);
 
+	dev->driver_type = type;
+
 	__module_get(THIS_MODULE);
 	return dev;
 }
diff --git a/include/media/rc-core.h b/include/media/rc-core.h
index b6586a9..c6bf1ef 100644
--- a/include/media/rc-core.h
+++ b/include/media/rc-core.h
@@ -185,7 +185,7 @@ struct rc_dev {
  * Remote Controller, at sys/class/rc.
  */
 
-struct rc_dev *rc_allocate_device(void);
+struct rc_dev *rc_allocate_device(enum rc_driver_type);
 void rc_free_device(struct rc_dev *dev);
 int rc_register_device(struct rc_dev *dev);
 void rc_unregister_device(struct rc_dev *dev);
-- 
2.8.1

Re: [RFC 1/7] [media] rc-main: assign driver type during allocation

[Sean Young]

On Wed, Jul 20, 2016 at 12:56:52AM +0900, Andi Shyti wrote:
> The driver type can be assigned immediately when an RC device
> requests to the framework to allocate the device.
> 
> This is an 'enum rc_driver_type' data type and specifies whether
> the device is a raw receiver or scancode receiver. The type will
> be given as parameter to the rc_allocate_device device.

This patch is good, but it does unfortunately break all the other
rc-core drivers, as now rc_allocate_device() needs argument. All
drivers will need a simple change in this patch.

Also note that there lots of issues that checkpatch.pl would pick
in these series.

> 
> Suggested-by: Sean Young <sean@mess.org>
> Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
> ---
>  drivers/media/rc/rc-main.c | 4 +++-
>  include/media/rc-core.h    | 2 +-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index 7dfc7c2..6403674 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -1346,7 +1346,7 @@ static struct device_type rc_dev_type = {
>  	.uevent		= rc_dev_uevent,
>  };
>  
> -struct rc_dev *rc_allocate_device(void)
> +struct rc_dev *rc_allocate_device(enum rc_driver_type type)
>  {
>  	struct rc_dev *dev;
>  
> @@ -1373,6 +1373,8 @@ struct rc_dev *rc_allocate_device(void)
>  	dev->dev.class = &rc_class;
>  	device_initialize(&dev->dev);
>  
> +	dev->driver_type = type;
> +
>  	__module_get(THIS_MODULE);
>  	return dev;
>  }
> diff --git a/include/media/rc-core.h b/include/media/rc-core.h
> index b6586a9..c6bf1ef 100644
> --- a/include/media/rc-core.h
> +++ b/include/media/rc-core.h
> @@ -185,7 +185,7 @@ struct rc_dev {
>   * Remote Controller, at sys/class/rc.
>   */
>  
> -struct rc_dev *rc_allocate_device(void);
> +struct rc_dev *rc_allocate_device(enum rc_driver_type);
>  void rc_free_device(struct rc_dev *dev);
>  int rc_register_device(struct rc_dev *dev);
>  void rc_unregister_device(struct rc_dev *dev);
> -- 
> 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

Re: [RFC 1/7] [media] rc-main: assign driver type during allocation

[Andi Shyti]

Hi Sean,

> > The driver type can be assigned immediately when an RC device
> > requests to the framework to allocate the device.
> > 
> > This is an 'enum rc_driver_type' data type and specifies whether
> > the device is a raw receiver or scancode receiver. The type will
> > be given as parameter to the rc_allocate_device device.
> 
> This patch is good, but it does unfortunately break all the other
> rc-core drivers, as now rc_allocate_device() needs argument. All
> drivers will need a simple change in this patch.

Yes, but for being an RFC I didn't took care of fixing
everything.

> Also note that there lots of issues that checkpatch.pl would pick
> in these series.

Some of the issues are coming from the code as it was and I
preferred to not change it. The last patch has some that need to
be fixed in the patchset.

Thanks,
Andi

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

[Andi Shyti]

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

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

[Andi Shyti]

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

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

[Sean Young]

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.

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

[Andi Shyti]

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;

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

[Andi Shyti]

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

[RFC 5/7] [media] ir-lirc-codec: do not handle any buffer for raw transmitters

[Andi Shyti]

Raw transmitters receive the data which need to be sent to
receivers from userspace as stream of bits, they don't require
any handling from the lirc framework.

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
---
 drivers/media/rc/ir-lirc-codec.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index 5effc65..80e94b6 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -121,17 +121,6 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, const char __user *buf,
 	if (!lirc)
 		return -EFAULT;
 
-	if (n < sizeof(unsigned) || n % sizeof(unsigned))
-		return -EINVAL;
-
-	count = n / sizeof(unsigned);
-	if (count > LIRCBUF_SIZE || count % 2 == 0)
-		return -EINVAL;
-
-	txbuf = memdup_user(buf, n);
-	if (IS_ERR(txbuf))
-		return PTR_ERR(txbuf);
-
 	dev = lirc->dev;
 	if (!dev) {
 		ret = -EFAULT;
@@ -143,6 +132,25 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, const char __user *buf,
 		goto out;
 	}
 
+	if (dev->driver_type == RC_DRIVER_IR_RAW_TX) {
+		txbuf = memdup_user(buf, n);
+		if (IS_ERR(txbuf))
+			return PTR_ERR(txbuf);
+
+		return dev->tx_ir(dev, txbuf, n);
+	}
+
+	if (n < sizeof(unsigned) || n % sizeof(unsigned))
+		return -EINVAL;
+
+	count = n / sizeof(unsigned);
+	if (count > LIRCBUF_SIZE || count % 2 == 0)
+		return -EINVAL;
+
+	txbuf = memdup_user(buf, n);
+	if (IS_ERR(txbuf))
+		return PTR_ERR(txbuf);
+
 	for (i = 0; i < count; i++) {
 		if (txbuf[i] > IR_MAX_DURATION / 1000 - duration || !txbuf[i]) {
 			ret = -EINVAL;
-- 
2.8.1

Re: [RFC 5/7] [media] ir-lirc-codec: do not handle any buffer for raw transmitters

[Sean Young]

On Wed, Jul 20, 2016 at 12:56:56AM +0900, Andi Shyti wrote:
> Raw transmitters receive the data which need to be sent to
> receivers from userspace as stream of bits, they don't require
> any handling from the lirc framework.

No drivers of type RC_DRIVER_IR_RAW_TX should handle tx just like any
other device, so data should be provided as an array of u32 alternating
pulse-space. If your device does not handle input like that then convert
it into that format in the driver. Every other driver has to do some
sort of conversion of that kind.

Thanks

Sean


> 
> Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
> ---
>  drivers/media/rc/ir-lirc-codec.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
> index 5effc65..80e94b6 100644
> --- a/drivers/media/rc/ir-lirc-codec.c
> +++ b/drivers/media/rc/ir-lirc-codec.c
> @@ -121,17 +121,6 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, const char __user *buf,
>  	if (!lirc)
>  		return -EFAULT;
>  
> -	if (n < sizeof(unsigned) || n % sizeof(unsigned))
> -		return -EINVAL;
> -
> -	count = n / sizeof(unsigned);
> -	if (count > LIRCBUF_SIZE || count % 2 == 0)
> -		return -EINVAL;
> -
> -	txbuf = memdup_user(buf, n);
> -	if (IS_ERR(txbuf))
> -		return PTR_ERR(txbuf);
> -
>  	dev = lirc->dev;
>  	if (!dev) {
>  		ret = -EFAULT;
> @@ -143,6 +132,25 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, const char __user *buf,
>  		goto out;
>  	}
>  
> +	if (dev->driver_type == RC_DRIVER_IR_RAW_TX) {
> +		txbuf = memdup_user(buf, n);
> +		if (IS_ERR(txbuf))
> +			return PTR_ERR(txbuf);
> +
> +		return dev->tx_ir(dev, txbuf, n);
> +	}
> +
> +	if (n < sizeof(unsigned) || n % sizeof(unsigned))
> +		return -EINVAL;
> +
> +	count = n / sizeof(unsigned);
> +	if (count > LIRCBUF_SIZE || count % 2 == 0)
> +		return -EINVAL;
> +
> +	txbuf = memdup_user(buf, n);
> +	if (IS_ERR(txbuf))
> +		return PTR_ERR(txbuf);
> +
>  	for (i = 0; i < count; i++) {
>  		if (txbuf[i] > IR_MAX_DURATION / 1000 - duration || !txbuf[i]) {
>  			ret = -EINVAL;
> -- 
> 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

Re: [RFC 5/7] [media] ir-lirc-codec: do not handle any buffer for raw transmitters

[Andi Shyti]

Hi Sean,

> > Raw transmitters receive the data which need to be sent to
> > receivers from userspace as stream of bits, they don't require
> > any handling from the lirc framework.
> 
> No drivers of type RC_DRIVER_IR_RAW_TX should handle tx just like any
> other device, so data should be provided as an array of u32 alternating
> pulse-space. If your device does not handle input like that then convert
> it into that format in the driver. Every other driver has to do some
> sort of conversion of that kind.

I don't see anything wrong here, that's how it works for example
in Tizen or in Android for the boards I'm on: userspace sends a
stream of bits that are then submitted to the IR as they are.

If I change it to only pulse-space domain, then I wouldn't
provide support for those platforms. Eventually I can add a new
protocol.

Andi

Re: [RFC 5/7] [media] ir-lirc-codec: do not handle any buffer for raw transmitters

[Sean Young]

Hi Andi,

On Thu, Jul 21, 2016 at 09:48:12AM +0900, Andi Shyti wrote:
> > > Raw transmitters receive the data which need to be sent to
> > > receivers from userspace as stream of bits, they don't require
> > > any handling from the lirc framework.
> > 
> > No drivers of type RC_DRIVER_IR_RAW_TX should handle tx just like any
> > other device, so data should be provided as an array of u32 alternating
> > pulse-space. If your device does not handle input like that then convert
> > it into that format in the driver. Every other driver has to do some
> > sort of conversion of that kind.
> 
> I don't see anything wrong here, that's how it works for example
> in Tizen or in Android for the boards I'm on: userspace sends a
> stream of bits that are then submitted to the IR as they are.

This introduces a new, incompatible api with no way of detecting it.

It's not a good format. For example the leading pulse (9ms) for nec ir
with a carrier of 38000 will be 342 bits. With the pulse-space format
it will be 32 bits.

Doing the conversion in kernel space will be cheap.

> If I change it to only pulse-space domain, then I wouldn't
> provide support for those platforms. Eventually I can add a new
> protocol.

But this is forcing an new, incompatible api onto the rest of us. 

This is the code in tizen:

https://build.tizen.org/package/rdiff?linkrev=base&package=device-manager-plugin-exynos5433&project=Tizen%3AIVI&rev=2

If this patch was merged as-is tizen would have to be changed anyway
to use different ioctls. If that is true, can it switch to use 
pulse-space format in the same change? If LIRC_GET_FREQUENCY fails then
it would be a main-line kernel, else the existent driver.

I could not find the code in android. It might be useful to see so we
can find a solution that works for everyone.


Sean

[RFC 6/7] Documentation: bindings: add documentation for ir-spi device driver

[Andi Shyti]

Document the ir-spi driver's binding which is a IR led driven
through the SPI line.

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
---
 Documentation/devicetree/bindings/media/spi-ir.txt | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/spi-ir.txt

diff --git a/Documentation/devicetree/bindings/media/spi-ir.txt b/Documentation/devicetree/bindings/media/spi-ir.txt
new file mode 100644
index 0000000..532da68
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/spi-ir.txt
@@ -0,0 +1,20 @@
+Device tree bindings for IR LED connected through SPI bus which is used as
+remote controller.
+
+The IR LED switch is connected to the MOSI line of the SPI device and the data
+are delivered thourgh that.
+
+Required properties:
+	- compatible: should be "ir-spi"
+
+Optional properties:
+	- irled,switch: specifies the gpio switch which enables the irled
+
+Example:
+
+        irled@0 {
+                compatible = "ir-spi";
+                reg = <0x0>;
+                spi-max-frequency = <5000000>;
+                irled,switch = <&gpr3 3 0>;
+        };
-- 
2.8.1

[RFC 7/7] [media] rc: add support for IR LEDs driven through SPI

[Andi Shyti]

The ir-spi is a simple device driver which supports the
connection between an IR LED and the MOSI line of an SPI device.

The driver, indeed, uses the SPI framework to stream the raw data
provided by userspace through a character device. The chardev is
handled by the LIRC framework and its functionality basically
provides:

 - raw write: data to be sent to the SPI and then streamed to the
   MOSI line;
 - set frequency: sets the frequency whith which the data should
   be sent;

The character device is created under /dev/lircX name, where X is
and ID assigned by the LIRC framework.

Example of usage:

        fd = open("/dev/lirc0", O_RDWR);
        if (fd < 0)
                return -1;

        val = 608000;
        ret = ioctl(fd, LIRC_SET_SEND_CARRIER, &val);
        if (ret < 0)
                return -1;

        n = write(fd, buffer, BUF_LEN);
        if (n < 0 || n != BUF_LEN)
                ret = -1;

        close(fd);

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
---
 drivers/media/rc/Kconfig  |   9 ++++
 drivers/media/rc/Makefile |   1 +
 drivers/media/rc/ir-spi.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 143 insertions(+)
 create mode 100644 drivers/media/rc/ir-spi.c

diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
index bd4d685..dacaa29 100644
--- a/drivers/media/rc/Kconfig
+++ b/drivers/media/rc/Kconfig
@@ -261,6 +261,15 @@ config IR_REDRAT3
 	   To compile this driver as a module, choose M here: the
 	   module will be called redrat3.
 
+config IR_SPI
+	tristate "SPI connected IR LED"
+	depends on SPI && LIRC
+	---help---
+	  Say Y if you want to use an IR LED connected through SPI bus.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called ir-spi.
+
 config IR_STREAMZAP
 	tristate "Streamzap PC Remote IR Receiver"
 	depends on USB_ARCH_HAS_HCD
diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile
index 379a5c0..1417c8d 100644
--- a/drivers/media/rc/Makefile
+++ b/drivers/media/rc/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_IR_NUVOTON) += nuvoton-cir.o
 obj-$(CONFIG_IR_ENE) += ene_ir.o
 obj-$(CONFIG_IR_REDRAT3) += redrat3.o
 obj-$(CONFIG_IR_RX51) += ir-rx51.o
+obj-$(CONFIG_IR_SPI) += ir-spi.o
 obj-$(CONFIG_IR_STREAMZAP) += streamzap.o
 obj-$(CONFIG_IR_WINBOND_CIR) += winbond-cir.o
 obj-$(CONFIG_RC_LOOPBACK) += rc-loopback.o
diff --git a/drivers/media/rc/ir-spi.c b/drivers/media/rc/ir-spi.c
new file mode 100644
index 0000000..7b6f344
--- /dev/null
+++ b/drivers/media/rc/ir-spi.c
@@ -0,0 +1,133 @@
+/*
+ * Copyright (c) 2016 Samsung Electronics Co., Ltd.
+ * Author: Andi Shyti <andi.shyti@samsung.it>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * SPI driven IR LED device driver
+ */
+
+#include <linux/fs.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_gpio.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <media/rc-core.h>
+
+#define IR_SPI_DRIVER_NAME		"ir-spi"
+
+#define IR_SPI_DEFAULT_FREQUENCY	38000
+#define IR_SPI_BIT_PER_WORD		    8
+
+struct ir_spi_data {
+	struct rc_dev *rc;
+	struct spi_device *spi;
+	struct spi_transfer xfer;
+	struct mutex mutex;
+	struct regulator *regulator;
+};
+
+static int ir_spi_tx(struct rc_dev *dev, unsigned *buffer, unsigned n)
+{
+	int ret;
+	struct ir_spi_data *idata = (struct ir_spi_data *) dev->priv;
+
+	ret = regulator_enable(idata->regulator);
+	if (ret)
+		return ret;
+
+	mutex_lock(&idata->mutex);
+	idata->xfer.len = n;
+	idata->xfer.tx_buf = buffer;
+	mutex_unlock(&idata->mutex);
+
+	ret = spi_sync_transfer(idata->spi, &idata->xfer, 1);
+	if (ret)
+		dev_err(&idata->spi->dev, "unable to deliver the signal\n");
+
+	regulator_disable(idata->regulator);
+
+	return ret;
+}
+
+static int ir_spi_set_tx_carrier(struct rc_dev *dev, u32 carrier)
+{
+	struct ir_spi_data *idata = (struct ir_spi_data *) dev->priv;
+
+	if (!carrier)
+		return -EINVAL;
+
+	mutex_lock(&idata->mutex);
+	idata->xfer.speed_hz = carrier;
+	mutex_unlock(&idata->mutex);
+
+	return 0;
+}
+
+static int ir_spi_probe(struct spi_device *spi)
+{
+	int ret;
+	struct ir_spi_data *idata;
+
+	idata = devm_kzalloc(&spi->dev, sizeof(*idata), GFP_KERNEL);
+	if (!idata)
+		return -ENOMEM;
+
+	idata->regulator = devm_regulator_get(&spi->dev, "irda_regulator");
+	if (IS_ERR(idata->regulator))
+		return PTR_ERR(idata->regulator);
+
+	idata->rc = rc_allocate_device(RC_DRIVER_IR_RAW_TX);
+	if (!idata->rc)
+		return -ENOMEM;
+
+	idata->rc->s_tx_carrier = ir_spi_set_tx_carrier;
+	idata->rc->tx_ir = ir_spi_tx;
+	idata->rc->driver_name = IR_SPI_DRIVER_NAME;
+	idata->rc->priv = idata;
+
+        ret = rc_register_device(idata->rc);
+	if (ret)
+		return ret;
+
+	mutex_init(&idata->mutex);
+
+	idata->spi = spi;
+
+	idata->xfer.bits_per_word = IR_SPI_BIT_PER_WORD;
+	idata->xfer.speed_hz = IR_SPI_DEFAULT_FREQUENCY;
+
+	return 0;
+}
+
+static int ir_spi_remove(struct spi_device *spi)
+{
+	struct ir_spi_data *idata = spi_get_drvdata(spi);
+
+	rc_unregister_device(idata->rc);
+
+	return 0;
+}
+
+static const struct of_device_id ir_spi_of_match[] = {
+	{ .compatible = "ir-spi" },
+	{},
+};
+
+static struct spi_driver ir_spi_driver = {
+	.probe = ir_spi_probe,
+	.remove = ir_spi_remove,
+	.driver = {
+		.name = IR_SPI_DRIVER_NAME,
+		.of_match_table = ir_spi_of_match,
+	},
+};
+
+module_spi_driver(ir_spi_driver);
+
+MODULE_AUTHOR("Andi Shyti <andi.shyti@samsung.com>");
+MODULE_DESCRIPTION("SPI IR LED");
+MODULE_LICENSE("GPL v2");
-- 
2.8.1

Re: [RFC 7/7] [media] rc: add support for IR LEDs driven through SPI

[Sean Young]

On Wed, Jul 20, 2016 at 12:56:58AM +0900, Andi Shyti wrote:
> The ir-spi is a simple device driver which supports the
> connection between an IR LED and the MOSI line of an SPI device.
> 
> The driver, indeed, uses the SPI framework to stream the raw data
> provided by userspace through a character device. The chardev is
> handled by the LIRC framework and its functionality basically
> provides:
> 
>  - raw write: data to be sent to the SPI and then streamed to the
>    MOSI line;
>  - set frequency: sets the frequency whith which the data should
>    be sent;
> 
> The character device is created under /dev/lircX name, where X is
> and ID assigned by the LIRC framework.
> 
> Example of usage:
> 
>         fd = open("/dev/lirc0", O_RDWR);
>         if (fd < 0)
>                 return -1;
> 
>         val = 608000;
>         ret = ioctl(fd, LIRC_SET_SEND_CARRIER, &val);
>         if (ret < 0)
>                 return -1;
> 
>         n = write(fd, buffer, BUF_LEN);
>         if (n < 0 || n != BUF_LEN)
>                 ret = -1;
> 
>         close(fd);
> 
> Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
> ---
>  drivers/media/rc/Kconfig  |   9 ++++
>  drivers/media/rc/Makefile |   1 +
>  drivers/media/rc/ir-spi.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 143 insertions(+)
>  create mode 100644 drivers/media/rc/ir-spi.c
> 
> diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
> index bd4d685..dacaa29 100644
> --- a/drivers/media/rc/Kconfig
> +++ b/drivers/media/rc/Kconfig
> @@ -261,6 +261,15 @@ config IR_REDRAT3
>  	   To compile this driver as a module, choose M here: the
>  	   module will be called redrat3.
>  
> +config IR_SPI
> +	tristate "SPI connected IR LED"
> +	depends on SPI && LIRC
> +	---help---
> +	  Say Y if you want to use an IR LED connected through SPI bus.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called ir-spi.
> +
>  config IR_STREAMZAP
>  	tristate "Streamzap PC Remote IR Receiver"
>  	depends on USB_ARCH_HAS_HCD
> diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile
> index 379a5c0..1417c8d 100644
> --- a/drivers/media/rc/Makefile
> +++ b/drivers/media/rc/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_IR_NUVOTON) += nuvoton-cir.o
>  obj-$(CONFIG_IR_ENE) += ene_ir.o
>  obj-$(CONFIG_IR_REDRAT3) += redrat3.o
>  obj-$(CONFIG_IR_RX51) += ir-rx51.o
> +obj-$(CONFIG_IR_SPI) += ir-spi.o
>  obj-$(CONFIG_IR_STREAMZAP) += streamzap.o
>  obj-$(CONFIG_IR_WINBOND_CIR) += winbond-cir.o
>  obj-$(CONFIG_RC_LOOPBACK) += rc-loopback.o
> diff --git a/drivers/media/rc/ir-spi.c b/drivers/media/rc/ir-spi.c
> new file mode 100644
> index 0000000..7b6f344
> --- /dev/null
> +++ b/drivers/media/rc/ir-spi.c
> @@ -0,0 +1,133 @@
> +/*
> + * Copyright (c) 2016 Samsung Electronics Co., Ltd.
> + * Author: Andi Shyti <andi.shyti@samsung.it>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * SPI driven IR LED device driver
> + */
> +
> +#include <linux/fs.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of_gpio.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <media/rc-core.h>
> +
> +#define IR_SPI_DRIVER_NAME		"ir-spi"
> +
> +#define IR_SPI_DEFAULT_FREQUENCY	38000
> +#define IR_SPI_BIT_PER_WORD		    8
> +
> +struct ir_spi_data {
> +	struct rc_dev *rc;
> +	struct spi_device *spi;
> +	struct spi_transfer xfer;
> +	struct mutex mutex;
> +	struct regulator *regulator;
> +};
> +
> +static int ir_spi_tx(struct rc_dev *dev, unsigned *buffer, unsigned n)
> +{
> +	int ret;
> +	struct ir_spi_data *idata = (struct ir_spi_data *) dev->priv;

No cast needed.

> +
> +	ret = regulator_enable(idata->regulator);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&idata->mutex);
> +	idata->xfer.len = n;
> +	idata->xfer.tx_buf = buffer;
> +	mutex_unlock(&idata->mutex);

I'm not convinced the locking works here. You want to guard against 
someone modifying xfer while you are sending (so in spi_sync_transfer), 
which this locking is not doing. You could declare a 
local "struct spi_transfer xfer" and avoid the mutex altogether.

As discussed here you should be converting from pulse-space also.

> +
> +	ret = spi_sync_transfer(idata->spi, &idata->xfer, 1);
> +	if (ret)
> +		dev_err(&idata->spi->dev, "unable to deliver the signal\n");
> +
> +	regulator_disable(idata->regulator);
> +
> +	return ret;
> +}
> +
> +static int ir_spi_set_tx_carrier(struct rc_dev *dev, u32 carrier)
> +{
> +	struct ir_spi_data *idata = (struct ir_spi_data *) dev->priv;

No cast needed here.

> +
> +	if (!carrier)
> +		return -EINVAL;
> +
> +	mutex_lock(&idata->mutex);
> +	idata->xfer.speed_hz = carrier;
> +	mutex_unlock(&idata->mutex);
> +
> +	return 0;
> +}
> +
> +static int ir_spi_probe(struct spi_device *spi)
> +{
> +	int ret;
> +	struct ir_spi_data *idata;
> +
> +	idata = devm_kzalloc(&spi->dev, sizeof(*idata), GFP_KERNEL);
> +	if (!idata)
> +		return -ENOMEM;
> +
> +	idata->regulator = devm_regulator_get(&spi->dev, "irda_regulator");
> +	if (IS_ERR(idata->regulator))
> +		return PTR_ERR(idata->regulator);
> +
> +	idata->rc = rc_allocate_device(RC_DRIVER_IR_RAW_TX);
> +	if (!idata->rc)
> +		return -ENOMEM;
> +
> +	idata->rc->s_tx_carrier = ir_spi_set_tx_carrier;
> +	idata->rc->tx_ir = ir_spi_tx;
> +	idata->rc->driver_name = IR_SPI_DRIVER_NAME;
> +	idata->rc->priv = idata;
> +
> +        ret = rc_register_device(idata->rc);
> +	if (ret)
> +		return ret;

You could really call rc_register_device() once the rc_dev device is 
ready, so after the mutex_init() etc. In practise I don't think it 
matters, since udev has to create the device and then someone has to 
do a open and either write or ioctl(LIRC_SET_SEND_CARRIER). Surely the 
following code has executed by then.

> +
> +	mutex_init(&idata->mutex);
> +
> +	idata->spi = spi;
> +
> +	idata->xfer.bits_per_word = IR_SPI_BIT_PER_WORD;
> +	idata->xfer.speed_hz = IR_SPI_DEFAULT_FREQUENCY;
> +
> +	return 0;
> +}
> +
> +static int ir_spi_remove(struct spi_device *spi)
> +{
> +	struct ir_spi_data *idata = spi_get_drvdata(spi);
> +
> +	rc_unregister_device(idata->rc);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ir_spi_of_match[] = {
> +	{ .compatible = "ir-spi" },
> +	{},
> +};
> +
> +static struct spi_driver ir_spi_driver = {
> +	.probe = ir_spi_probe,
> +	.remove = ir_spi_remove,
> +	.driver = {
> +		.name = IR_SPI_DRIVER_NAME,
> +		.of_match_table = ir_spi_of_match,
> +	},
> +};
> +
> +module_spi_driver(ir_spi_driver);
> +
> +MODULE_AUTHOR("Andi Shyti <andi.shyti@samsung.com>");
> +MODULE_DESCRIPTION("SPI IR LED");
> +MODULE_LICENSE("GPL v2");
> -- 
> 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

Re: [RFC 7/7] [media] rc: add support for IR LEDs driven through SPI

[Andi Shyti]

Hi Sean,

> > +	int ret;
> > +	struct ir_spi_data *idata = (struct ir_spi_data *) dev->priv;
> 
> No cast needed.

yes, thanks.

> > +	ret = regulator_enable(idata->regulator);
> > +	if (ret)
> > +		return ret;
> > +
> > +	mutex_lock(&idata->mutex);
> > +	idata->xfer.len = n;
> > +	idata->xfer.tx_buf = buffer;
> > +	mutex_unlock(&idata->mutex);
> 
> I'm not convinced the locking works here. You want to guard against 
> someone modifying xfer while you are sending (so in spi_sync_transfer), 
> which this locking is not doing. You could declare a 
> local "struct spi_transfer xfer" and avoid the mutex altogether.

I cannot declare xfer locally because the spi framework needs
a statically allocated xfer, so that either I dynamically
allocate it in the function or I declare it global in idata.

With the mutex I would like to prevent different tasks to change
the value at the same time, it's an easy case, it shouldn't make
much difference.

There are checkpatch issues, in the next patchset I will fix
them.

Thanks a lot for your review,
Andi

Re: [RFC 7/7] [media] rc: add support for IR LEDs driven through SPI

[Sean Young]

Hi Andi,

On Thu, Jul 21, 2016 at 10:09:26AM +0900, Andi Shyti wrote:
> > > +	ret = regulator_enable(idata->regulator);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	mutex_lock(&idata->mutex);
> > > +	idata->xfer.len = n;
> > > +	idata->xfer.tx_buf = buffer;
> > > +	mutex_unlock(&idata->mutex);
> > 
> > I'm not convinced the locking works here. You want to guard against 
> > someone modifying xfer while you are sending (so in spi_sync_transfer), 
> > which this locking is not doing. You could declare a 
> > local "struct spi_transfer xfer" and avoid the mutex altogether.
> 
> I cannot declare xfer locally because the spi framework needs
> a statically allocated xfer, so that either I dynamically
> allocate it in the function or I declare it global in idata.

It can be stack allocated for sync transfers. You might want to lock
the spi bus.

> With the mutex I would like to prevent different tasks to change
> the value at the same time, it's an easy case, it shouldn't make
> much difference.

That's cargo-cult locking. It does not achieve anything.


Sean

Re: [RFC 7/7] [media] rc: add support for IR LEDs driven through SPI

[Andi Shyti]

Hi Sean,

> > > > +	ret = regulator_enable(idata->regulator);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	mutex_lock(&idata->mutex);
> > > > +	idata->xfer.len = n;
> > > > +	idata->xfer.tx_buf = buffer;
> > > > +	mutex_unlock(&idata->mutex);
> > > 
> > > I'm not convinced the locking works here. You want to guard against 
> > > someone modifying xfer while you are sending (so in spi_sync_transfer), 
> > > which this locking is not doing. You could declare a 
> > > local "struct spi_transfer xfer" and avoid the mutex altogether.
> > 
> > I cannot declare xfer locally because the spi framework needs
> > a statically allocated xfer, so that either I dynamically
> > allocate it in the function or I declare it global in idata.
> 
> It can be stack allocated for sync transfers. You might want to lock
> the spi bus.

no, actually it's just dirty data and laziness, a memset to 0
fixes it :)

> > With the mutex I would like to prevent different tasks to change
> > the value at the same time, it's an easy case, it shouldn't make
> > much difference.
> 
> That's cargo-cult locking. It does not achieve anything.

yes, as I said, it's not a big thing, I can remove the mutex.

Thanks,
Andi