Re: [PATCH] [media] rc: ir-spi: add support for IR LEDs connected with SPI
Hi Rob,
> > The ir-spi is a simple device driver which supports the
> > connection between an IR LED and the MOSI line of an SPI device.
>
> Please split the binding from the driver.
OK!
> > +Device tree bindings for IR LED connected through SPI bus which is used as
> > +remote controller.
>
> Do said devices have part numbers? Seems kind of generic and I've never
> seen such device.
No, it doesn't, this is a simple irled driven by the SPI MOSI
line that works as a remote controller.
It looked strange to me as well when I heard of it the first
time, but it will be present in the upcoming tm2/tm2e patchset
(i.e. Samsung Note 4 and Note 4 edge).
> > +Optional properties:
> > + - irled,switch: specifies the gpio switch which enables the irled
>
> Just "switch-gpios"
OK!
> > +controller-data {
>
> This is part of the controller binding? Omit that from the example.
OK!
Thanks,
Andi
Re: [PATCH] [media] rc: ir-spi: add support for IR LEDs connected with SPI
Hi Rob,
> > The ir-spi is a simple device driver which supports the
> > connection between an IR LED and the MOSI line of an SPI device.
>
> Please split the binding from the driver.
OK!
> > +Device tree bindings for IR LED connected through SPI bus which is used as
> > +remote controller.
>
> Do said devices have part numbers? Seems kind of generic and I've never
> seen such device.
No, it doesn't, this is a simple irled driven by the SPI MOSI
line that works as a remote controller.
It looked strange to me as well when I heard of it the first
time, but it will be present in the upcoming tm2/tm2e patchset
(i.e. Samsung Note 4 and Note 4 edge).
> > +Optional properties:
> > + - irled,switch: specifies the gpio switch which enables the irled
>
> Just "switch-gpios"
OK!
> > +controller-data {
>
> This is part of the controller binding? Omit that from the example.
OK!
Thanks,
Andi
Re: [PATCH] [media] rc: ir-spi: add support for IR LEDs connected with SPI
On Fri, Jul 01, 2016 at 05:33:42PM +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.
Please split the binding from the driver.
> 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;
> - set length: sets the data length. This information is
>optional, if the length is set, then userspace should send raw
>data only with that length; while if the length is set to '0',
>then the driver will figure out himself the length of the data
>based on the length of the data written on the character
>device.
>The latter is not recommended, though, as the driver, at
>any write, allocates and deallocates a buffer where the data
>from userspace are stored.
>
> The driver provides three feedback commands:
>
> - get length: reads the length set and (as mentioned), if the
>length is '0' it will be calculated at any write
> - get frequency: the driver reports the frequency. If userpace
>doesn't set the frequency, the driver will use a default value
>of 38000Hz.
>
> The character device is created under /dev/lircX name, where X is
> and ID assigned by the LIRC framework.
>
> Example of usage:
>
> int fd, ret;
> ssize_t n;
> uint32_t val = 0;
>
> fd = open("/dev/lirc0", O_RDWR);
> if (fd < 0) {
> fprintf(stderr, "unable to open the device\n");
> return -1;
> }
>
> /* ioctl set frequency and length parameters */
> val = 6430;
> ret = ioctl(fd, LIRC_SET_LENGTH, );
> if (ret < 0)
> fprintf(stderr, "LIRC_SET_LENGTH failed\n");
> val = 608000;
> ret = ioctl(fd, LIRC_SET_FREQUENCY, );
> if (ret < 0)
> fprintf(stderr, "LIRC_SET_FREQUENCY failed\n");
>
> /* read back length and frequency parameters */
> ret = ioctl(fd, LIRC_GET_LENGTH, );
> if (ret < 0)
> fprintf(stderr, "LIRC_GET_LENGTH failed\n");
> else
> fprintf(stdout, "legnth = %u\n", val);
>
> ret = ioctl(fd, LIRC_GET_FREQUENCY, );
> if (ret < 0)
> fprintf(stderr, "LIRC_GET_FREQUENCY failed\n");
> else
> fprintf(stdout, "frequency = %u\n", val);
>
> /* write data to device */
> n = write(fd, b, 6430);
> if (n < 0) {
> fprintf(stderr, "unable to write to the device\n");
> ret = -1;
> } else if (n != 6430) {
> fprintf(stderr, "failed to write everything, wrote %ld
> instead\n", n);
> ret = -1;
> } else {
> fprintf(stdout, "written all the %ld data\n", n);
> }
>
> close(fd);
>
> The driver supports multi task access, but all the processes
> which hold the driver should use the same length and frequency
> parameters.
>
> Change-Id: I323d7dd4a56d6dcf48f2c695293822eb04bdb85f
> Signed-off-by: Andi Shyti
> ---
> Documentation/devicetree/bindings/media/spi-ir.txt | 24 ++
> drivers/media/rc/Kconfig | 9 +
> drivers/media/rc/Makefile | 1 +
> drivers/media/rc/ir-spi.c | 301
> +
> 4 files changed, 335 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/spi-ir.txt
> create mode 100644 drivers/media/rc/ir-spi.c
>
> diff --git a/Documentation/devicetree/bindings/media/spi-ir.txt
> b/Documentation/devicetree/bindings/media/spi-ir.txt
> new file mode 100644
> index 000..2232d92
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/spi-ir.txt
> @@ -0,0 +1,24 @@
> +Device tree bindings for IR LED connected through SPI bus which is used as
> +remote controller.
Do said devices have part numbers? Seems kind of generic and I've never
seen such device.
> +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
Just "switch-gpios"
> +
> +Example:
> +
> +irled@0 {
> +compatible = "ir-spi";
> +reg = <0x0>;
> +spi-max-frequency = <500>;
> +irled,switch = < 3 0>;
> +
> +controller-data {
This is part of the controller binding? Omit that from the example.
> +samsung,spi-feedback-delay = <0>;
> +
Re: [PATCH] [media] rc: ir-spi: add support for IR LEDs connected with SPI
On Fri, Jul 01, 2016 at 05:33:42PM +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.
Please split the binding from the driver.
> 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;
> - set length: sets the data length. This information is
>optional, if the length is set, then userspace should send raw
>data only with that length; while if the length is set to '0',
>then the driver will figure out himself the length of the data
>based on the length of the data written on the character
>device.
>The latter is not recommended, though, as the driver, at
>any write, allocates and deallocates a buffer where the data
>from userspace are stored.
>
> The driver provides three feedback commands:
>
> - get length: reads the length set and (as mentioned), if the
>length is '0' it will be calculated at any write
> - get frequency: the driver reports the frequency. If userpace
>doesn't set the frequency, the driver will use a default value
>of 38000Hz.
>
> The character device is created under /dev/lircX name, where X is
> and ID assigned by the LIRC framework.
>
> Example of usage:
>
> int fd, ret;
> ssize_t n;
> uint32_t val = 0;
>
> fd = open("/dev/lirc0", O_RDWR);
> if (fd < 0) {
> fprintf(stderr, "unable to open the device\n");
> return -1;
> }
>
> /* ioctl set frequency and length parameters */
> val = 6430;
> ret = ioctl(fd, LIRC_SET_LENGTH, );
> if (ret < 0)
> fprintf(stderr, "LIRC_SET_LENGTH failed\n");
> val = 608000;
> ret = ioctl(fd, LIRC_SET_FREQUENCY, );
> if (ret < 0)
> fprintf(stderr, "LIRC_SET_FREQUENCY failed\n");
>
> /* read back length and frequency parameters */
> ret = ioctl(fd, LIRC_GET_LENGTH, );
> if (ret < 0)
> fprintf(stderr, "LIRC_GET_LENGTH failed\n");
> else
> fprintf(stdout, "legnth = %u\n", val);
>
> ret = ioctl(fd, LIRC_GET_FREQUENCY, );
> if (ret < 0)
> fprintf(stderr, "LIRC_GET_FREQUENCY failed\n");
> else
> fprintf(stdout, "frequency = %u\n", val);
>
> /* write data to device */
> n = write(fd, b, 6430);
> if (n < 0) {
> fprintf(stderr, "unable to write to the device\n");
> ret = -1;
> } else if (n != 6430) {
> fprintf(stderr, "failed to write everything, wrote %ld
> instead\n", n);
> ret = -1;
> } else {
> fprintf(stdout, "written all the %ld data\n", n);
> }
>
> close(fd);
>
> The driver supports multi task access, but all the processes
> which hold the driver should use the same length and frequency
> parameters.
>
> Change-Id: I323d7dd4a56d6dcf48f2c695293822eb04bdb85f
> Signed-off-by: Andi Shyti
> ---
> Documentation/devicetree/bindings/media/spi-ir.txt | 24 ++
> drivers/media/rc/Kconfig | 9 +
> drivers/media/rc/Makefile | 1 +
> drivers/media/rc/ir-spi.c | 301
> +
> 4 files changed, 335 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/spi-ir.txt
> create mode 100644 drivers/media/rc/ir-spi.c
>
> diff --git a/Documentation/devicetree/bindings/media/spi-ir.txt
> b/Documentation/devicetree/bindings/media/spi-ir.txt
> new file mode 100644
> index 000..2232d92
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/spi-ir.txt
> @@ -0,0 +1,24 @@
> +Device tree bindings for IR LED connected through SPI bus which is used as
> +remote controller.
Do said devices have part numbers? Seems kind of generic and I've never
seen such device.
> +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
Just "switch-gpios"
> +
> +Example:
> +
> +irled@0 {
> +compatible = "ir-spi";
> +reg = <0x0>;
> +spi-max-frequency = <500>;
> +irled,switch = < 3 0>;
> +
> +controller-data {
This is part of the controller binding? Omit that from the example.
> +samsung,spi-feedback-delay = <0>;
> +};
> +
Re: [PATCH] [media] rc: ir-spi: add support for IR LEDs connected with SPI
Hi,
[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.7-rc5 next-20160701]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Andi-Shyti/rc-ir-spi-add-support-for-IR-LEDs-connected-with-SPI/20160702-102955
base: git://linuxtv.org/media_tree.git master
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
include/linux/compiler.h:232:8: sparse: attribute 'no_sanitize_address':
unknown attribute
drivers/media/rc/ir-spi.c:156:14: sparse: undefined identifier
'LIRC_SET_LENGTH'
>> drivers/media/rc/ir-spi.c:156:14: sparse: incompatible types for 'case'
>> statement
drivers/media/rc/ir-spi.c:156:14: sparse: Expected constant expression in
case statement
drivers/media/rc/ir-spi.c: In function 'ir_spi_chardev_ioctl':
drivers/media/rc/ir-spi.c:156:7: error: 'LIRC_SET_LENGTH' undeclared (first
use in this function)
case LIRC_SET_LENGTH: {
^~~
drivers/media/rc/ir-spi.c:156:7: note: each undeclared identifier is
reported only once for each function it appears in
vim +/case +156 drivers/media/rc/ir-spi.c
140
141 static long ir_spi_chardev_ioctl(struct file *file, unsigned int cmd,
142 unsigned long arg)
143 {
144 __u32 p;
145 s32 ret;
146 struct ir_spi_data *idata = file->private_data;
147
148 switch (cmd) {
149 case LIRC_GET_FEATURES:
150 return put_user(idata->lirc_driver.features,
151 (__u32 __user *) arg);
152
153 case LIRC_GET_LENGTH:
154 return put_user(idata->xfer.len, (__u32 __user *) arg);
155
> 156 case LIRC_SET_LENGTH: {
157 void *new;
158
159 ret = get_user(p, (__u32 __user *) arg);
160 if (ret)
161 return ret;
162
163 /*
164 * the user is trying to set the same
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Re: [PATCH] [media] rc: ir-spi: add support for IR LEDs connected with SPI
Hi,
[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.7-rc5 next-20160701]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Andi-Shyti/rc-ir-spi-add-support-for-IR-LEDs-connected-with-SPI/20160702-102955
base: git://linuxtv.org/media_tree.git master
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
include/linux/compiler.h:232:8: sparse: attribute 'no_sanitize_address':
unknown attribute
drivers/media/rc/ir-spi.c:156:14: sparse: undefined identifier
'LIRC_SET_LENGTH'
>> drivers/media/rc/ir-spi.c:156:14: sparse: incompatible types for 'case'
>> statement
drivers/media/rc/ir-spi.c:156:14: sparse: Expected constant expression in
case statement
drivers/media/rc/ir-spi.c: In function 'ir_spi_chardev_ioctl':
drivers/media/rc/ir-spi.c:156:7: error: 'LIRC_SET_LENGTH' undeclared (first
use in this function)
case LIRC_SET_LENGTH: {
^~~
drivers/media/rc/ir-spi.c:156:7: note: each undeclared identifier is
reported only once for each function it appears in
vim +/case +156 drivers/media/rc/ir-spi.c
140
141 static long ir_spi_chardev_ioctl(struct file *file, unsigned int cmd,
142 unsigned long arg)
143 {
144 __u32 p;
145 s32 ret;
146 struct ir_spi_data *idata = file->private_data;
147
148 switch (cmd) {
149 case LIRC_GET_FEATURES:
150 return put_user(idata->lirc_driver.features,
151 (__u32 __user *) arg);
152
153 case LIRC_GET_LENGTH:
154 return put_user(idata->xfer.len, (__u32 __user *) arg);
155
> 156 case LIRC_SET_LENGTH: {
157 void *new;
158
159 ret = get_user(p, (__u32 __user *) arg);
160 if (ret)
161 return ret;
162
163 /*
164 * the user is trying to set the same
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Re: [PATCH] [media] rc: ir-spi: add support for IR LEDs connected with SPI
Hi,
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.7-rc5 next-20160701]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Andi-Shyti/rc-ir-spi-add-support-for-IR-LEDs-connected-with-SPI/20160702-102955
base: git://linuxtv.org/media_tree.git master
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/media/rc/ir-spi.c: In function 'ir_spi_chardev_ioctl':
>> drivers/media/rc/ir-spi.c:156:7: error: 'LIRC_SET_LENGTH' undeclared (first
>> use in this function)
case LIRC_SET_LENGTH: {
^~~
drivers/media/rc/ir-spi.c:156:7: note: each undeclared identifier is
reported only once for each function it appears in
vim +/LIRC_SET_LENGTH +156 drivers/media/rc/ir-spi.c
150 return put_user(idata->lirc_driver.features,
151 (__u32 __user *) arg);
152
153 case LIRC_GET_LENGTH:
154 return put_user(idata->xfer.len, (__u32 __user *) arg);
155
> 156 case LIRC_SET_LENGTH: {
157 void *new;
158
159 ret = get_user(p, (__u32 __user *) arg);
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: Binary data
Re: [PATCH] [media] rc: ir-spi: add support for IR LEDs connected with SPI
Hi,
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.7-rc5 next-20160701]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Andi-Shyti/rc-ir-spi-add-support-for-IR-LEDs-connected-with-SPI/20160702-102955
base: git://linuxtv.org/media_tree.git master
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/media/rc/ir-spi.c: In function 'ir_spi_chardev_ioctl':
>> drivers/media/rc/ir-spi.c:156:7: error: 'LIRC_SET_LENGTH' undeclared (first
>> use in this function)
case LIRC_SET_LENGTH: {
^~~
drivers/media/rc/ir-spi.c:156:7: note: each undeclared identifier is
reported only once for each function it appears in
vim +/LIRC_SET_LENGTH +156 drivers/media/rc/ir-spi.c
150 return put_user(idata->lirc_driver.features,
151 (__u32 __user *) arg);
152
153 case LIRC_GET_LENGTH:
154 return put_user(idata->xfer.len, (__u32 __user *) arg);
155
> 156 case LIRC_SET_LENGTH: {
157 void *new;
158
159 ret = get_user(p, (__u32 __user *) arg);
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: Binary data
Re: [PATCH] [media] rc: ir-spi: add support for IR LEDs connected with SPI
Hi Sean, > > > Also I don't see what justifies this new interface. This can be > > > implemented in rc-core in less lines of code and it will be entirely > > > compatible with existing user-space. > > > > Also here I'm getting a bit confused. When I started writing > > this, I didn't even know of the existence of a remote controlling > > framework, but then I run across this: > > > > "LIRC is a package that allows you to decode and send infra-red > > signals of many (but not all) commonly used remote controls. " > > > > taken from lirc.org: my case is exactly falling into this > > description. > > > > Am I missing anything? > > See drivers/staging/media/lirc/TODO: "All drivers should either be > ported to ir-core, or dropped entirely". ir-core has since been renamed > to rc-core; it is uses for non-IR purposes like cec. > > lirc exists as the user-space ABI but not it is not the preferred > framework for kernel space. I missed this part and now what you say makes sense. > I'm happy to help. I will do as you recommend and thanks a lot, appreciated :) Andi
Re: [PATCH] [media] rc: ir-spi: add support for IR LEDs connected with SPI
Hi Sean, > > > Also I don't see what justifies this new interface. This can be > > > implemented in rc-core in less lines of code and it will be entirely > > > compatible with existing user-space. > > > > Also here I'm getting a bit confused. When I started writing > > this, I didn't even know of the existence of a remote controlling > > framework, but then I run across this: > > > > "LIRC is a package that allows you to decode and send infra-red > > signals of many (but not all) commonly used remote controls. " > > > > taken from lirc.org: my case is exactly falling into this > > description. > > > > Am I missing anything? > > See drivers/staging/media/lirc/TODO: "All drivers should either be > ported to ir-core, or dropped entirely". ir-core has since been renamed > to rc-core; it is uses for non-IR purposes like cec. > > lirc exists as the user-space ABI but not it is not the preferred > framework for kernel space. I missed this part and now what you say makes sense. > I'm happy to help. I will do as you recommend and thanks a lot, appreciated :) Andi
Re: [PATCH] [media] rc: ir-spi: add support for IR LEDs connected with SPI
On Fri, Jul 01, 2016 at 09:30:35PM +0900, Andi Shyti wrote:
> Hi Sean,
>
> > > 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;
> > > - set length: sets the data length. This information is
> > >optional, if the length is set, then userspace should send raw
> > >data only with that length; while if the length is set to '0',
> > >then the driver will figure out himself the length of the data
> > >based on the length of the data written on the character
> > >device.
> > >The latter is not recommended, though, as the driver, at
> > >any write, allocates and deallocates a buffer where the data
> > >from userspace are stored.
> > >
> > > The driver provides three feedback commands:
> > >
> > > - get length: reads the length set and (as mentioned), if the
> > >length is '0' it will be calculated at any write
> > > - get frequency: the driver reports the frequency. If userpace
> > >doesn't set the frequency, the driver will use a default value
> > >of 38000Hz.
> >
> > This interface is not compatible with other lirc devices; there is no
> > way of determining whether this is a regular lirc device or this new
> > flavour you've invented.
>
> except of the set length and get length which I'm using a bit
> freely because I am dealing with devices that exchange always the
> same amount of data, so that I don't need (in my case) to
> pre-allocate or overallocate or runtime allocate. I don't
> understand what else I invented :)
Other than the LIRC_{GET,SET}_LENGTH it might very well be compatible;
you're reusing LIRC_GET_LENGTH for a different purpose.
Is the kmalloc() really that costly that it needs to be avoided for
each transmit?
> This is a simple driver which is driving an LED connected through
> SPI and userspace writes raw data in it (LIRC_CAN_SEND_RAW).
And some odd ioctl.
> > Also I don't see what justifies this new interface. This can be
> > implemented in rc-core in less lines of code and it will be entirely
> > compatible with existing user-space.
>
> Also here I'm getting a bit confused. When I started writing
> this, I didn't even know of the existence of a remote controlling
> framework, but then I run across this:
>
> "LIRC is a package that allows you to decode and send infra-red
> signals of many (but not all) commonly used remote controls. "
>
> taken from lirc.org: my case is exactly falling into this
> description.
>
> Am I missing anything?
See drivers/staging/media/lirc/TODO: "All drivers should either be
ported to ir-core, or dropped entirely". ir-core has since been renamed
to rc-core; it is uses for non-IR purposes like cec.
lirc exists as the user-space ABI but not it is not the preferred
framework for kernel space.
There is one problem here. rc-core does not provide very well for
transmit-only hardware, so rc-core needs some modifications. This is
what I suggest to make it work:
1. in include/media/rc-core.h add a new entry to the enum rc_driver_type
called "RC_DRIVER_IR_RAW_TX_ONLY" (or something like that).
2. rc_allocate_device() needs an argument "enum rc_driver_type"; in the
case it would not allocate an input device. All drivers needs to
pass in this argument.
3. rc_register_device() and rc_unregister_device() should not execute
anything with to do with input devices or key maps for tx only
devices.
4. ir_lirc_register() should not set the LIRC_CAN_REC_MODE2 feature
or allocate an input buffer in the case of TX only device.
With these changes all you need to do in ir-spi is:
struct rc_dev *rc = rc_allocate_device(RC_DRIVER_IR_RAW_TX_ONLY);
strcpy(rc->name, "IR SPI");
rc->s_tx_carrier = ir_spi_set_tx_carrier; // write function
rc->tx_ir = ir_spi_tx; // write function
rc->driver_name = "ir-spi";
rc_register_driver(rc);
I'm happy to help.
Sean
Re: [PATCH] [media] rc: ir-spi: add support for IR LEDs connected with SPI
On Fri, Jul 01, 2016 at 09:30:35PM +0900, Andi Shyti wrote:
> Hi Sean,
>
> > > 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;
> > > - set length: sets the data length. This information is
> > >optional, if the length is set, then userspace should send raw
> > >data only with that length; while if the length is set to '0',
> > >then the driver will figure out himself the length of the data
> > >based on the length of the data written on the character
> > >device.
> > >The latter is not recommended, though, as the driver, at
> > >any write, allocates and deallocates a buffer where the data
> > >from userspace are stored.
> > >
> > > The driver provides three feedback commands:
> > >
> > > - get length: reads the length set and (as mentioned), if the
> > >length is '0' it will be calculated at any write
> > > - get frequency: the driver reports the frequency. If userpace
> > >doesn't set the frequency, the driver will use a default value
> > >of 38000Hz.
> >
> > This interface is not compatible with other lirc devices; there is no
> > way of determining whether this is a regular lirc device or this new
> > flavour you've invented.
>
> except of the set length and get length which I'm using a bit
> freely because I am dealing with devices that exchange always the
> same amount of data, so that I don't need (in my case) to
> pre-allocate or overallocate or runtime allocate. I don't
> understand what else I invented :)
Other than the LIRC_{GET,SET}_LENGTH it might very well be compatible;
you're reusing LIRC_GET_LENGTH for a different purpose.
Is the kmalloc() really that costly that it needs to be avoided for
each transmit?
> This is a simple driver which is driving an LED connected through
> SPI and userspace writes raw data in it (LIRC_CAN_SEND_RAW).
And some odd ioctl.
> > Also I don't see what justifies this new interface. This can be
> > implemented in rc-core in less lines of code and it will be entirely
> > compatible with existing user-space.
>
> Also here I'm getting a bit confused. When I started writing
> this, I didn't even know of the existence of a remote controlling
> framework, but then I run across this:
>
> "LIRC is a package that allows you to decode and send infra-red
> signals of many (but not all) commonly used remote controls. "
>
> taken from lirc.org: my case is exactly falling into this
> description.
>
> Am I missing anything?
See drivers/staging/media/lirc/TODO: "All drivers should either be
ported to ir-core, or dropped entirely". ir-core has since been renamed
to rc-core; it is uses for non-IR purposes like cec.
lirc exists as the user-space ABI but not it is not the preferred
framework for kernel space.
There is one problem here. rc-core does not provide very well for
transmit-only hardware, so rc-core needs some modifications. This is
what I suggest to make it work:
1. in include/media/rc-core.h add a new entry to the enum rc_driver_type
called "RC_DRIVER_IR_RAW_TX_ONLY" (or something like that).
2. rc_allocate_device() needs an argument "enum rc_driver_type"; in the
case it would not allocate an input device. All drivers needs to
pass in this argument.
3. rc_register_device() and rc_unregister_device() should not execute
anything with to do with input devices or key maps for tx only
devices.
4. ir_lirc_register() should not set the LIRC_CAN_REC_MODE2 feature
or allocate an input buffer in the case of TX only device.
With these changes all you need to do in ir-spi is:
struct rc_dev *rc = rc_allocate_device(RC_DRIVER_IR_RAW_TX_ONLY);
strcpy(rc->name, "IR SPI");
rc->s_tx_carrier = ir_spi_set_tx_carrier; // write function
rc->tx_ir = ir_spi_tx; // write function
rc->driver_name = "ir-spi";
rc_register_driver(rc);
I'm happy to help.
Sean
Re: [PATCH] [media] rc: ir-spi: add support for IR LEDs connected with SPI
Hi Sean, > > 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; > > - set length: sets the data length. This information is > >optional, if the length is set, then userspace should send raw > >data only with that length; while if the length is set to '0', > >then the driver will figure out himself the length of the data > >based on the length of the data written on the character > >device. > >The latter is not recommended, though, as the driver, at > >any write, allocates and deallocates a buffer where the data > >from userspace are stored. > > > > The driver provides three feedback commands: > > > > - get length: reads the length set and (as mentioned), if the > >length is '0' it will be calculated at any write > > - get frequency: the driver reports the frequency. If userpace > >doesn't set the frequency, the driver will use a default value > >of 38000Hz. > > This interface is not compatible with other lirc devices; there is no > way of determining whether this is a regular lirc device or this new > flavour you've invented. except of the set length and get length which I'm using a bit freely because I am dealing with devices that exchange always the same amount of data, so that I don't need (in my case) to pre-allocate or overallocate or runtime allocate. I don't understand what else I invented :) This is a simple driver which is driving an LED connected through SPI and userspace writes raw data in it (LIRC_CAN_SEND_RAW). > Also I don't see what justifies this new interface. This can be > implemented in rc-core in less lines of code and it will be entirely > compatible with existing user-space. Also here I'm getting a bit confused. When I started writing this, I didn't even know of the existence of a remote controlling framework, but then I run across this: "LIRC is a package that allows you to decode and send infra-red signals of many (but not all) commonly used remote controls. " taken from lirc.org: my case is exactly falling into this description. Am I missing anything? Thanks, Andi
Re: [PATCH] [media] rc: ir-spi: add support for IR LEDs connected with SPI
Hi Sean, > > 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; > > - set length: sets the data length. This information is > >optional, if the length is set, then userspace should send raw > >data only with that length; while if the length is set to '0', > >then the driver will figure out himself the length of the data > >based on the length of the data written on the character > >device. > >The latter is not recommended, though, as the driver, at > >any write, allocates and deallocates a buffer where the data > >from userspace are stored. > > > > The driver provides three feedback commands: > > > > - get length: reads the length set and (as mentioned), if the > >length is '0' it will be calculated at any write > > - get frequency: the driver reports the frequency. If userpace > >doesn't set the frequency, the driver will use a default value > >of 38000Hz. > > This interface is not compatible with other lirc devices; there is no > way of determining whether this is a regular lirc device or this new > flavour you've invented. except of the set length and get length which I'm using a bit freely because I am dealing with devices that exchange always the same amount of data, so that I don't need (in my case) to pre-allocate or overallocate or runtime allocate. I don't understand what else I invented :) This is a simple driver which is driving an LED connected through SPI and userspace writes raw data in it (LIRC_CAN_SEND_RAW). > Also I don't see what justifies this new interface. This can be > implemented in rc-core in less lines of code and it will be entirely > compatible with existing user-space. Also here I'm getting a bit confused. When I started writing this, I didn't even know of the existence of a remote controlling framework, but then I run across this: "LIRC is a package that allows you to decode and send infra-red signals of many (but not all) commonly used remote controls. " taken from lirc.org: my case is exactly falling into this description. Am I missing anything? Thanks, Andi
Re: [PATCH] [media] rc: ir-spi: add support for IR LEDs connected with SPI
On Fri, Jul 01, 2016 at 05:33:42PM +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; > - set length: sets the data length. This information is >optional, if the length is set, then userspace should send raw >data only with that length; while if the length is set to '0', >then the driver will figure out himself the length of the data >based on the length of the data written on the character >device. >The latter is not recommended, though, as the driver, at >any write, allocates and deallocates a buffer where the data >from userspace are stored. > > The driver provides three feedback commands: > > - get length: reads the length set and (as mentioned), if the >length is '0' it will be calculated at any write > - get frequency: the driver reports the frequency. If userpace >doesn't set the frequency, the driver will use a default value >of 38000Hz. This interface is not compatible with other lirc devices; there is no way of determining whether this is a regular lirc device or this new flavour you've invented. Also I don't see what justifies this new interface. This can be implemented in rc-core in less lines of code and it will be entirely compatible with existing user-space. Sean
Re: [PATCH] [media] rc: ir-spi: add support for IR LEDs connected with SPI
On Fri, Jul 01, 2016 at 05:33:42PM +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; > - set length: sets the data length. This information is >optional, if the length is set, then userspace should send raw >data only with that length; while if the length is set to '0', >then the driver will figure out himself the length of the data >based on the length of the data written on the character >device. >The latter is not recommended, though, as the driver, at >any write, allocates and deallocates a buffer where the data >from userspace are stored. > > The driver provides three feedback commands: > > - get length: reads the length set and (as mentioned), if the >length is '0' it will be calculated at any write > - get frequency: the driver reports the frequency. If userpace >doesn't set the frequency, the driver will use a default value >of 38000Hz. This interface is not compatible with other lirc devices; there is no way of determining whether this is a regular lirc device or this new flavour you've invented. Also I don't see what justifies this new interface. This can be implemented in rc-core in less lines of code and it will be entirely compatible with existing user-space. Sean
