From patchwork Fri Jul 21 14:12:45 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Matthias Reichl X-Patchwork-Id: 9857027 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 38FD560388 for ; Fri, 21 Jul 2017 14:12:54 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 177AC287C6 for ; Fri, 21 Jul 2017 14:12:54 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 0C2E7287D1; Fri, 21 Jul 2017 14:12:54 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 0131A287C6 for ; Fri, 21 Jul 2017 14:12:53 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751748AbdGUOMv (ORCPT ); Fri, 21 Jul 2017 10:12:51 -0400 Received: from mail.horus.com ([78.46.148.228]:51534 "EHLO mail.horus.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750762AbdGUOMs (ORCPT ); Fri, 21 Jul 2017 10:12:48 -0400 Received: from [192.168.1.20] (62-46-133-13.adsl.highway.telekom.at [62.46.133.13]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "E-Mail Matthias Reichl", Issuer "HiassofT CA 2014" (verified OK)) by mail.horus.com (Postfix) with ESMTPS id 1A45A6408D; Fri, 21 Jul 2017 16:12:47 +0200 (CEST) Received: by camel2.lan (Postfix, from userid 1000) id ECC061C7298; Fri, 21 Jul 2017 16:12:45 +0200 (CEST) Date: Fri, 21 Jul 2017 16:12:45 +0200 From: Matthias Reichl To: Sean Young Cc: linux-media@vger.kernel.org Subject: Re: [PATCH v2 3/6] [media] rc: gpio-ir-tx: add new driver Message-ID: <20170721141245.3uv55fqxa557dmnt@camel2.lan> References: <92a66fd9852c3143d5726eb3869d58e28d841c84.1499419624.git.sean@mess.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <92a66fd9852c3143d5726eb3869d58e28d841c84.1499419624.git.sean@mess.org> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 > + * > + * 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 > +#include > +#include use linux/gpio/consumer.h instead of linux/gpio.h > +#include > +#include > +#include > +#include of_gpio.h can be dropped > +#include > +#include > + > +#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 "); > +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 #include -#include +#include #include #include #include -#include #include #include @@ -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");