Re: [RFC 7/7] [media] rc: add support for IR LEDs driven through SPI
Hi Sean, > > > > + ret = regulator_enable(idata->regulator); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + mutex_lock(>mutex); > > > > + idata->xfer.len = n; > > > > + idata->xfer.tx_buf = buffer; > > > > + mutex_unlock(>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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@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
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(>mutex); > > > + idata->xfer.len = n; > > > + idata->xfer.tx_buf = buffer; > > > + mutex_unlock(>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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@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
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(>mutex); > > + idata->xfer.len = n; > > + idata->xfer.tx_buf = buffer; > > + mutex_unlock(>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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@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
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, );
> 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
> ---
> 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 000..7b6f344
> --- /dev/null
> +++ b/drivers/media/rc/ir-spi.c
> @@ -0,0 +1,133 @@
> +/*
> + * Copyright (c) 2016 Samsung Electronics Co., Ltd.
> + * Author: Andi Shyti
> + *
> + * 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
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#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(>mutex);
> + idata->xfer.len = n;
> + idata->xfer.tx_buf = buffer;
> + mutex_unlock(>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, >xfer, 1);
> + if (ret)
> + dev_err(>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(>mutex);
> + idata->xfer.speed_hz = carrier;
> + mutex_unlock(>mutex);
> +
> + return 0;
> +}
> +
> +static int ir_spi_probe(struct spi_device *spi)
> +{
> +
[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, );
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
---
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 000..7b6f344
--- /dev/null
+++ b/drivers/media/rc/ir-spi.c
@@ -0,0 +1,133 @@
+/*
+ * Copyright (c) 2016 Samsung Electronics Co., Ltd.
+ * Author: Andi Shyti
+ *
+ * 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
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define IR_SPI_DRIVER_NAME "ir-spi"
+
+#define IR_SPI_DEFAULT_FREQUENCY 38000
+#define IR_SPI_BIT_PER_WORD8
+
+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(>mutex);
+ idata->xfer.len = n;
+ idata->xfer.tx_buf = buffer;
+ mutex_unlock(>mutex);
+
+ ret = spi_sync_transfer(idata->spi, >xfer, 1);
+ if (ret)
+ dev_err(>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(>mutex);
+ idata->xfer.speed_hz = carrier;
+ mutex_unlock(>mutex);
+
+ return 0;
+}
+
+static int ir_spi_probe(struct spi_device *spi)
+{
+ int ret;
+ struct ir_spi_data *idata;
+
+ idata = devm_kzalloc(>dev, sizeof(*idata), GFP_KERNEL);
+ if (!idata)
+ return -ENOMEM;
+
+ idata->regulator = devm_regulator_get(>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 =
