Hi Sean,
On Fri, Jul 07, 2017 at 10:51:59AM +0100, Sean Young wrote:
> This is a simple bit-banging GPIO IR TX driver.
thanks a lot for the driver, this is highly appreciated!
I tested the patch series on a RPi2, against RPi downstream kernel
4.13-rc1, and noticed an issue: the polarity of the gpio seems
to be reversed.
Other than the polarity issue the driver seems to work fine -
at least on the scope screen. Didn't have an IR transmitter to
do some real tests yet.
I've configured the gpio as active high:
gpio_ir_tx: gpio-ir-transmitter {
compatible = "gpio-ir-tx";
gpios = <&gpio 18 0>;
};
However, when loading the gpio-ir-tx driver the gpio pin changed
to 3.3V. I did some tests with ir-ctl -S, idle state of the signal
was 3.3V, active state 0V.
I think it's better to use the descriptor based gpio functions
instead of the legacy number based ones. That'll simplify the
driver and it can delegate polarity handling to gpiod.
Proposed changes and comments are inline. I've also included
the patch against your patch that I've been testing with at the
end of the message.
> diff --git a/drivers/media/rc/gpio-ir-tx.c b/drivers/media/rc/gpio-ir-tx.c
> new file mode 100644
> index 0000000..7a5371d
> --- /dev/null
> +++ b/drivers/media/rc/gpio-ir-tx.c
> @@ -0,0 +1,189 @@
> +/*
> + * Copyright (C) 2017 Sean Young <sean@xxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/gpio.h>
use linux/gpio/consumer.h instead of linux/gpio.h
> +#include <linux/delay.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
of_gpio.h can be dropped
> +#include <linux/platform_device.h>
> +#include <media/rc-core.h>
> +
> +#define DRIVER_NAME "gpio-ir-tx"
> +#define DEVICE_NAME "GPIO Bit Banging IR Transmitter"
> +
> +struct gpio_ir {
> + int gpio_nr;
> + bool active_low;
Replace these 2 fields with
struct gpio_desc *gpio;
> + unsigned int carrier;
> + unsigned int duty_cycle;
> + /* we need a spinlock to hold the cpu while transmitting */
> + spinlock_t lock;
> +};
> +
> +static const struct of_device_id gpio_ir_tx_of_match[] = {
> + { .compatible = "gpio-ir-tx", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, gpio_ir_tx_of_match);
> +
> +static int gpio_ir_tx_set_duty_cycle(struct rc_dev *dev, u32 duty_cycle)
> +{
> + struct gpio_ir *gpio_ir = dev->priv;
> +
> + gpio_ir->duty_cycle = duty_cycle;
> +
> + return 0;
> +}
> +
> +static int gpio_ir_tx_set_carrier(struct rc_dev *dev, u32 carrier)
> +{
> + struct gpio_ir *gpio_ir = dev->priv;
> +
> + if (!carrier)
> + return -EINVAL;
> +
> + gpio_ir->carrier = carrier;
> +
> + return 0;
> +}
> +
> +static int gpio_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
> + unsigned int count)
> +{
> + struct gpio_ir *gpio_ir = dev->priv;
> + unsigned long flags;
> + ktime_t edge;
> + /*
> + * delta should never exceed 0.5 seconds (IR_MAX_DURATION) and on
> + * m68k ndelay(s64) does not compile; so use s32 rather than s64.
> + */
> + s32 delta;
> + int i;
> + unsigned int pulse, space;
> +
> + /* Ensure the dividend fits into 32 bit */
> + pulse = DIV_ROUND_CLOSEST(gpio_ir->duty_cycle * (NSEC_PER_SEC / 100),
> + gpio_ir->carrier);
> + space = DIV_ROUND_CLOSEST((100 - gpio_ir->duty_cycle) *
> + (NSEC_PER_SEC / 100), gpio_ir->carrier);
> +
> + spin_lock_irqsave(&gpio_ir->lock, flags);
> +
> + edge = ktime_get();
> +
> + for (i = 0; i < count; i++) {
> + if (i % 2) {
> + // space
> + edge = ktime_add_us(edge, txbuf[i]);
> + delta = ktime_us_delta(edge, ktime_get());
> + if (delta > 10) {
> + spin_unlock_irqrestore(&gpio_ir->lock, flags);
> + usleep_range(delta, delta + 10);
> + spin_lock_irqsave(&gpio_ir->lock, flags);
> + } else if (delta > 0) {
> + udelay(delta);
> + }
> + } else {
> + // pulse
> + ktime_t last = ktime_add_us(edge, txbuf[i]);
> +
> + while (ktime_get() < last) {
> + gpio_set_value(gpio_ir->gpio_nr,
> + gpio_ir->active_low);
gpiod_set_value(gpio_ir->gpio, 1);
> + edge += pulse;
> + delta = edge - ktime_get();
> + if (delta > 0)
> + ndelay(delta);
> + gpio_set_value(gpio_ir->gpio_nr,
> + !gpio_ir->active_low);
gpiod_set_value(gpio_ir->gpio, 0);
> + edge += space;
> + delta = edge - ktime_get();
> + if (delta > 0)
> + ndelay(delta);
> + }
> +
> + edge = last;
> + }
> + }
> +
> + spin_unlock_irqrestore(&gpio_ir->lock, flags);
> +
> + return count;
> +}
> +
> +static int gpio_ir_tx_probe(struct platform_device *pdev)
> +{
> + struct gpio_ir *gpio_ir;
> + struct rc_dev *rcdev;
> + enum of_gpio_flags flags;
> + int rc, gpio;
Flags can be dropped, gpio as well.
> +
> + gpio = of_get_gpio_flags(pdev->dev.of_node, 0, &flags);
> + if (gpio < 0) {
> + if (gpio != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "Failed to get gpio flags (%d)\n",
> + gpio);
> + return -EINVAL;
> + }
Drop this and move getting the gpio a bit down so we don't need
a temp variable.
> +
> + gpio_ir = devm_kmalloc(&pdev->dev, sizeof(*gpio_ir), GFP_KERNEL);
> + if (!gpio_ir)
> + return -ENOMEM;
> +
> + rcdev = devm_rc_allocate_device(&pdev->dev, RC_DRIVER_IR_RAW_TX);
> + if (!rcdev)
> + return -ENOMEM;
get the gpio here, configure it to output with logical low (idle)
level and store it in gpio_ir:
gpio_ir->gpio = devm_gpiod_get(&pdev->dev, NULL, GPIOD_OUT_LOW);
if (IS_ERR(gpio_ir->gpio)) {
if (PTR_ERR(gpio_ir->gpio) != -EPROBE_DEFER)
dev_err(&pdev->dev, "Failed to get gpio (%ld)\n",
PTR_ERR(gpio_ir->gpio));
return PTR_ERR(gpio_ir->gpio);
}
> +
> + rcdev->priv = gpio_ir;
> + rcdev->driver_name = DRIVER_NAME;
> + rcdev->device_name = DEVICE_NAME;
> + rcdev->tx_ir = gpio_ir_tx;
> + rcdev->s_tx_duty_cycle = gpio_ir_tx_set_duty_cycle;
> + rcdev->s_tx_carrier = gpio_ir_tx_set_carrier;
> +
> + gpio_ir->gpio_nr = gpio;
> + gpio_ir->active_low = (flags & OF_GPIO_ACTIVE_LOW) != 0;
drop gpio_nr and active_low
> + gpio_ir->carrier = 38000;
> + gpio_ir->duty_cycle = 50;
> + spin_lock_init(&gpio_ir->lock);
> +
> + rc = devm_gpio_request(&pdev->dev, gpio, "gpio-ir-tx");
> + if (rc < 0)
> + return rc;
> +
> + rc = gpio_direction_output(gpio, !gpio_ir->active_low);
> + if (rc < 0)
> + return rc;
drop devm_gpio_request and gpio_direction_output, that is already
handled by devm_gpiod_get.
> +
> + rc = devm_rc_register_device(&pdev->dev, rcdev);
> + if (rc < 0)
> + dev_err(&pdev->dev, "failed to register rc device\n");
> +
> + return rc;
> +}
> +
> +static struct platform_driver gpio_ir_tx_driver = {
> + .probe = gpio_ir_tx_probe,
> + .driver = {
> + .name = DRIVER_NAME,
> + .of_match_table = of_match_ptr(gpio_ir_tx_of_match),
> + },
> +};
> +module_platform_driver(gpio_ir_tx_driver);
> +
> +MODULE_DESCRIPTION("GPIO Bit Banging IR Transmitter");
> +MODULE_AUTHOR("Sean Young <sean@xxxxxxxx>");
> +MODULE_LICENSE("GPL");
> --
> 2.9.4
so long,
Hias
---
diff --git a/drivers/media/rc/gpio-ir-tx.c b/drivers/media/rc/gpio-ir-tx.c
index 7a5371dbb360..ca6834d09467 100644
--- a/drivers/media/rc/gpio-ir-tx.c
+++ b/drivers/media/rc/gpio-ir-tx.c
@@ -13,11 +13,10 @@
#include <linux/kernel.h>
#include <linux/module.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
#include <linux/delay.h>
#include <linux/slab.h>
#include <linux/of.h>
-#include <linux/of_gpio.h>
#include <linux/platform_device.h>
#include <media/rc-core.h>
@@ -25,8 +24,7 @@
#define DEVICE_NAME "GPIO Bit Banging IR Transmitter"
struct gpio_ir {
- int gpio_nr;
- bool active_low;
+ struct gpio_desc *gpio;
unsigned int carrier;
unsigned int duty_cycle;
/* we need a spinlock to hold the cpu while transmitting */
@@ -101,14 +99,12 @@ static int gpio_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
ktime_t last = ktime_add_us(edge, txbuf[i]);
while (ktime_get() < last) {
- gpio_set_value(gpio_ir->gpio_nr,
- gpio_ir->active_low);
+ gpiod_set_value(gpio_ir->gpio, 1);
edge += pulse;
delta = edge - ktime_get();
if (delta > 0)
ndelay(delta);
- gpio_set_value(gpio_ir->gpio_nr,
- !gpio_ir->active_low);
+ gpiod_set_value(gpio_ir->gpio, 0);
edge += space;
delta = edge - ktime_get();
if (delta > 0)
@@ -128,16 +124,7 @@ static int gpio_ir_tx_probe(struct platform_device *pdev)
{
struct gpio_ir *gpio_ir;
struct rc_dev *rcdev;
- enum of_gpio_flags flags;
- int rc, gpio;
-
- gpio = of_get_gpio_flags(pdev->dev.of_node, 0, &flags);
- if (gpio < 0) {
- if (gpio != -EPROBE_DEFER)
- dev_err(&pdev->dev, "Failed to get gpio flags (%d)\n",
- gpio);
- return -EINVAL;
- }
+ int rc;
gpio_ir = devm_kmalloc(&pdev->dev, sizeof(*gpio_ir), GFP_KERNEL);
if (!gpio_ir)
@@ -147,6 +134,14 @@ static int gpio_ir_tx_probe(struct platform_device *pdev)
if (!rcdev)
return -ENOMEM;
+ gpio_ir->gpio = devm_gpiod_get(&pdev->dev, NULL, GPIOD_OUT_LOW);
+ if (IS_ERR(gpio_ir->gpio)) {
+ if (PTR_ERR(gpio_ir->gpio) != -EPROBE_DEFER)
+ dev_err(&pdev->dev, "Failed to get gpio (%ld)\n",
+ PTR_ERR(gpio_ir->gpio));
+ return PTR_ERR(gpio_ir->gpio);
+ }
+
rcdev->priv = gpio_ir;
rcdev->driver_name = DRIVER_NAME;
rcdev->device_name = DEVICE_NAME;
@@ -154,20 +149,10 @@ static int gpio_ir_tx_probe(struct platform_device *pdev)
rcdev->s_tx_duty_cycle = gpio_ir_tx_set_duty_cycle;
rcdev->s_tx_carrier = gpio_ir_tx_set_carrier;
- gpio_ir->gpio_nr = gpio;
- gpio_ir->active_low = (flags & OF_GPIO_ACTIVE_LOW) != 0;
gpio_ir->carrier = 38000;
gpio_ir->duty_cycle = 50;
spin_lock_init(&gpio_ir->lock);
- rc = devm_gpio_request(&pdev->dev, gpio, "gpio-ir-tx");
- if (rc < 0)
- return rc;
-
- rc = gpio_direction_output(gpio, !gpio_ir->active_low);
- if (rc < 0)
- return rc;
-
rc = devm_rc_register_device(&pdev->dev, rcdev);
if (rc < 0)
dev_err(&pdev->dev, "failed to register rc device\n");
