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

Groups > linux.kernel > #1446565 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC 7/7] [media] rc: add support for IR LEDs driven through SPI Andi Shyti <andi.shyti@samsung.com> - 2016-07-19 18:00 +0200
    Re: [RFC 7/7] [media] rc: add support for IR LEDs driven through SPI Sean Young <sean@mess.org> - 2016-07-20 01:20 +0200
      Re: [RFC 7/7] [media] rc: add support for IR LEDs driven through SPI Andi Shyti <andi.shyti@samsung.com> - 2016-07-21 03:10 +0200
        Re: [RFC 7/7] [media] rc: add support for IR LEDs driven through SPI Sean Young <sean@mess.org> - 2016-07-21 12:30 +0200
          Re: [RFC 7/7] [media] rc: add support for IR LEDs driven through SPI Andi Shyti <andi@etezian.org> - 2016-07-21 17:00 +0200

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

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

[toc] | [next] | [standalone]

#1446779

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

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

#1447562

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

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

#1447769

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

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

#1447942

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web