diff mbox

[PATCHv3,4/5] cec-gpio: add HDMI CEC GPIO driver

Message ID 20170830161044.26571-5-hverkuil@xs4all.nl (mailing list archive)
State New, archived
Headers show

Commit Message

Hans Verkuil Aug. 30, 2017, 4:10 p.m. UTC
From: Hans Verkuil <hans.verkuil@cisco.com>

Add a simple HDMI CEC GPIO driver that sits on top of the cec-pin framework.

While I have heard of SoCs that use the GPIO pin for CEC (apparently an
early RockChip SoC used that), the main use-case of this driver is to
function as a debugging tool.

By connecting the CEC line to a GPIO pin on a Raspberry Pi 3 for example
it turns it into a CEC debugger and protocol analyzer.

With 'cec-ctl --monitor-pin' the CEC traffic can be analyzed.

But of course it can also be used with any hardware project where the
HDMI CEC pin is hooked up to a pull-up gpio pin.

In addition this has (optional) support for tracing HPD changes if the
HPD is connected to a GPIO.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/Kconfig             |   9 ++
 drivers/media/platform/Makefile            |   2 +
 drivers/media/platform/cec-gpio/Makefile   |   1 +
 drivers/media/platform/cec-gpio/cec-gpio.c | 237 +++++++++++++++++++++++++++++
 4 files changed, 249 insertions(+)
 create mode 100644 drivers/media/platform/cec-gpio/Makefile
 create mode 100644 drivers/media/platform/cec-gpio/cec-gpio.c

Comments

Linus Walleij Aug. 31, 2017, 8:28 a.m. UTC | #1
On Wed, Aug 30, 2017 at 6:10 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:

> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> Add a simple HDMI CEC GPIO driver that sits on top of the cec-pin framework.
>
> While I have heard of SoCs that use the GPIO pin for CEC (apparently an
> early RockChip SoC used that), the main use-case of this driver is to
> function as a debugging tool.
>
> By connecting the CEC line to a GPIO pin on a Raspberry Pi 3 for example
> it turns it into a CEC debugger and protocol analyzer.
>
> With 'cec-ctl --monitor-pin' the CEC traffic can be analyzed.
>
> But of course it can also be used with any hardware project where the
> HDMI CEC pin is hooked up to a pull-up gpio pin.
>
> In addition this has (optional) support for tracing HPD changes if the
> HPD is connected to a GPIO.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

Pretty cool stuff!

> +config CEC_GPIO
> +       tristate "Generic GPIO-based CEC driver"
> +       depends on PREEMPT

depends on GPIOLIB

or

select GPIOLIB

your pick.

> +#include <linux/gpio.h>

This should not be needed in new code.

> +#include <linux/gpio/consumer.h>

This should be enough.

> +#include <linux/of_gpio.h>

Your should not need this either.

> +struct cec_gpio {
> +       struct cec_adapter      *adap;
> +       struct device           *dev;
> +       int                     gpio;
> +       int                     hpd_gpio;

Think this should be:

struct gpio_desc *gpio;
struct gpio_desc *hpd_gpio;

> +       int                     irq;
> +       int                     hpd_irq;
> +       bool                    hpd_is_high;
> +       ktime_t                 hpd_ts;
> +       bool                    is_low;
> +       bool                    have_irq;
> +};
> +
> +static bool cec_gpio_read(struct cec_adapter *adap)
> +{
> +       struct cec_gpio *cec = cec_get_drvdata(adap);
> +
> +       if (cec->is_low)
> +               return false;
> +       return gpio_get_value(cec->gpio);

Use descriptor accessors

gpiod_get_value()

> +static void cec_gpio_high(struct cec_adapter *adap)
> +{
> +       struct cec_gpio *cec = cec_get_drvdata(adap);
> +
> +       if (!cec->is_low)
> +               return;
> +       cec->is_low = false;
> +       gpio_direction_input(cec->gpio);

Are you setting the GPIO high by setting it as input?
That sounds like you should be requesting it as
open drain in the DTS file, see
Documentation/gpio/driver.txt
for details about open drain, and use
GPIO_LINE_OPEN_DRAIN
from <dt-bindings/gpio/gpio.h>

> +static void cec_gpio_low(struct cec_adapter *adap)
> +{
> +       struct cec_gpio *cec = cec_get_drvdata(adap);
> +
> +       if (cec->is_low)
> +               return;
> +       if (WARN_ON_ONCE(cec->have_irq))
> +               free_irq(cec->irq, cec);
> +       cec->have_irq = false;
> +       cec->is_low = true;
> +       gpio_direction_output(cec->gpio, 0);

Yeah this looks like you're doing open drain.
gpiod_direction_output() etc.

> +static int cec_gpio_read_hpd(struct cec_adapter *adap)
> +{
> +       struct cec_gpio *cec = cec_get_drvdata(adap);
> +
> +       if (cec->hpd_gpio < 0)
> +               return -ENOTTY;
> +       return gpio_get_value(cec->hpd_gpio);

gpiod_get_value()


> +static int cec_gpio_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       enum of_gpio_flags hpd_gpio_flags;
> +       struct cec_gpio *cec;
> +       int ret;
> +
> +       cec = devm_kzalloc(dev, sizeof(*cec), GFP_KERNEL);
> +       if (!cec)
> +               return -ENOMEM;
> +
> +       cec->gpio = of_get_named_gpio_flags(dev->of_node,
> +                                           "cec-gpio", 0, &hpd_gpio_flags);
> +       if (cec->gpio < 0)
> +               return cec->gpio;
> +
> +       cec->hpd_gpio = of_get_named_gpio_flags(dev->of_node,
> +                                           "hpd-gpio", 0, &hpd_gpio_flags);

This is a proper device so don't use all this trouble.

cec->gpio = gpiod_get(dev, "cec-gpio", GPIOD_IN);

or similar (grep for examples!)

Same for hdp_gpio.

> +       cec->irq = gpio_to_irq(cec->gpio);

gpiod_to_irq()

> +       gpio_direction_input(cec->gpio);

This is not needed with the above IN flag.

But as said above, maybe you are looking for an open drain
output actually.

> +       if (cec->hpd_gpio >= 0) {
> +               cec->hpd_irq = gpio_to_irq(cec->hpd_gpio);
> +               gpio_direction_input(cec->hpd_gpio);

Already done if you pass the right flag. This should be IN for sure.

Use gpiod_to_irq().

Please include me on subsequent posts, I want to try to use
descriptors as much as possible for new drivers.

Yours,
Linus Walleij
diff mbox

Patch

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 7e7cc49b8674..d95b0ee367d6 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -553,6 +553,15 @@  config VIDEO_MESON_AO_CEC
 	  This is a driver for Amlogic Meson SoCs AO CEC interface. It uses the
 	  generic CEC framework interface.
 	  CEC bus is present in the HDMI connector and enables communication
+
+config CEC_GPIO
+	tristate "Generic GPIO-based CEC driver"
+	depends on PREEMPT
+	select CEC_CORE
+	select CEC_PIN
+	---help---
+	  This is a generic GPIO-based CEC driver.
+	  The CEC bus is present in the HDMI connector and enables communication
 	  between compatible devices.
 
 config VIDEO_SAMSUNG_S5P_CEC
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index c1ef946bf032..9bf48f118537 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -26,6 +26,8 @@  obj-$(CONFIG_VIDEO_CODA) 		+= coda/
 
 obj-$(CONFIG_VIDEO_SH_VEU)		+= sh_veu.o
 
+obj-$(CONFIG_CEC_GPIO)			+= cec-gpio/
+
 obj-$(CONFIG_VIDEO_MEM2MEM_DEINTERLACE)	+= m2m-deinterlace.o
 
 obj-$(CONFIG_VIDEO_MUX)			+= video-mux.o
diff --git a/drivers/media/platform/cec-gpio/Makefile b/drivers/media/platform/cec-gpio/Makefile
new file mode 100644
index 000000000000..e82b258afa55
--- /dev/null
+++ b/drivers/media/platform/cec-gpio/Makefile
@@ -0,0 +1 @@ 
+obj-$(CONFIG_CEC_GPIO) += cec-gpio.o
diff --git a/drivers/media/platform/cec-gpio/cec-gpio.c b/drivers/media/platform/cec-gpio/cec-gpio.c
new file mode 100644
index 000000000000..a582cf91bf87
--- /dev/null
+++ b/drivers/media/platform/cec-gpio/cec-gpio.c
@@ -0,0 +1,237 @@ 
+/*
+ * Copyright 2017 Cisco Systems, Inc. and/or its affiliates. All rights reserved.
+ *
+ * This program is free software; you may redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/of_gpio.h>
+#include <media/cec-pin.h>
+
+struct cec_gpio {
+	struct cec_adapter	*adap;
+	struct device		*dev;
+	int			gpio;
+	int			hpd_gpio;
+	int			irq;
+	int			hpd_irq;
+	bool			hpd_is_high;
+	ktime_t			hpd_ts;
+	bool			is_low;
+	bool			have_irq;
+};
+
+static bool cec_gpio_read(struct cec_adapter *adap)
+{
+	struct cec_gpio *cec = cec_get_drvdata(adap);
+
+	if (cec->is_low)
+		return false;
+	return gpio_get_value(cec->gpio);
+}
+
+static void cec_gpio_high(struct cec_adapter *adap)
+{
+	struct cec_gpio *cec = cec_get_drvdata(adap);
+
+	if (!cec->is_low)
+		return;
+	cec->is_low = false;
+	gpio_direction_input(cec->gpio);
+}
+
+static void cec_gpio_low(struct cec_adapter *adap)
+{
+	struct cec_gpio *cec = cec_get_drvdata(adap);
+
+	if (cec->is_low)
+		return;
+	if (WARN_ON_ONCE(cec->have_irq))
+		free_irq(cec->irq, cec);
+	cec->have_irq = false;
+	cec->is_low = true;
+	gpio_direction_output(cec->gpio, 0);
+}
+
+static irqreturn_t cec_hpd_gpio_irq_handler_thread(int irq, void *priv)
+{
+	struct cec_gpio *cec = priv;
+
+	cec_queue_pin_hpd_event(cec->adap, cec->hpd_is_high, cec->hpd_ts);
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t cec_hpd_gpio_irq_handler(int irq, void *priv)
+{
+	struct cec_gpio *cec = priv;
+
+	cec->hpd_ts = ktime_get();
+	cec->hpd_is_high = gpio_get_value(cec->hpd_gpio);
+	return IRQ_WAKE_THREAD;
+}
+
+static irqreturn_t cec_gpio_irq_handler(int irq, void *priv)
+{
+	struct cec_gpio *cec = priv;
+
+	cec_pin_changed(cec->adap, gpio_get_value(cec->gpio));
+	return IRQ_HANDLED;
+}
+
+static bool cec_gpio_enable_irq(struct cec_adapter *adap)
+{
+	struct cec_gpio *cec = cec_get_drvdata(adap);
+
+	if (cec->have_irq)
+		return true;
+
+	if (request_irq(cec->irq, cec_gpio_irq_handler,
+			IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+			adap->name, cec))
+		return false;
+	cec->have_irq = true;
+	return true;
+}
+
+static void cec_gpio_disable_irq(struct cec_adapter *adap)
+{
+	struct cec_gpio *cec = cec_get_drvdata(adap);
+
+	if (cec->have_irq)
+		free_irq(cec->irq, cec);
+	cec->have_irq = false;
+}
+
+static void cec_gpio_status(struct cec_adapter *adap, struct seq_file *file)
+{
+	struct cec_gpio *cec = cec_get_drvdata(adap);
+
+	seq_printf(file, "mode: %s\n", cec->is_low ? "low-drive" : "read");
+	if (cec->have_irq)
+		seq_printf(file, "using irq: %d\n", cec->irq);
+	if (cec->hpd_gpio >= 0)
+		seq_printf(file, "hpd: %s\n",
+			   cec->hpd_is_high ? "high" : "low");
+}
+
+static int cec_gpio_read_hpd(struct cec_adapter *adap)
+{
+	struct cec_gpio *cec = cec_get_drvdata(adap);
+
+	if (cec->hpd_gpio < 0)
+		return -ENOTTY;
+	return gpio_get_value(cec->hpd_gpio);
+}
+
+static void cec_gpio_free(struct cec_adapter *adap)
+{
+	cec_gpio_disable_irq(adap);
+}
+
+static const struct cec_pin_ops cec_gpio_pin_ops = {
+	.read = cec_gpio_read,
+	.low = cec_gpio_low,
+	.high = cec_gpio_high,
+	.enable_irq = cec_gpio_enable_irq,
+	.disable_irq = cec_gpio_disable_irq,
+	.status = cec_gpio_status,
+	.free = cec_gpio_free,
+	.read_hpd = cec_gpio_read_hpd,
+};
+
+static int cec_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	enum of_gpio_flags hpd_gpio_flags;
+	struct cec_gpio *cec;
+	int ret;
+
+	cec = devm_kzalloc(dev, sizeof(*cec), GFP_KERNEL);
+	if (!cec)
+		return -ENOMEM;
+
+	cec->gpio = of_get_named_gpio_flags(dev->of_node,
+					    "cec-gpio", 0, &hpd_gpio_flags);
+	if (cec->gpio < 0)
+		return cec->gpio;
+
+	cec->hpd_gpio = of_get_named_gpio_flags(dev->of_node,
+					    "hpd-gpio", 0, &hpd_gpio_flags);
+	cec->irq = gpio_to_irq(cec->gpio);
+	gpio_direction_input(cec->gpio);
+	cec->dev = dev;
+
+	if (cec->hpd_gpio >= 0) {
+		cec->hpd_irq = gpio_to_irq(cec->hpd_gpio);
+		gpio_direction_input(cec->hpd_gpio);
+		if (devm_request_threaded_irq(dev, cec->hpd_irq,
+				cec_hpd_gpio_irq_handler,
+				cec_hpd_gpio_irq_handler_thread,
+				IRQF_ONESHOT |
+				IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
+				"hpd-gpio", cec))
+			cec->hpd_gpio = -1;
+	}
+
+	cec->adap = cec_pin_allocate_adapter(&cec_gpio_pin_ops,
+		cec, pdev->name, CEC_CAP_DEFAULTS | CEC_CAP_PHYS_ADDR |
+				 CEC_CAP_MONITOR_ALL | CEC_CAP_MONITOR_PIN);
+	if (IS_ERR(cec->adap))
+		return PTR_ERR(cec->adap);
+
+	ret = cec_register_adapter(cec->adap, &pdev->dev);
+	if (ret) {
+		cec_delete_adapter(cec->adap);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, cec);
+	return 0;
+}
+
+static int cec_gpio_remove(struct platform_device *pdev)
+{
+	struct cec_gpio *cec = platform_get_drvdata(pdev);
+
+	cec_unregister_adapter(cec->adap);
+	return 0;
+}
+
+static const struct of_device_id cec_gpio_match[] = {
+	{
+		.compatible	= "cec-gpio",
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, cec_gpio_match);
+
+static struct platform_driver cec_gpio_pdrv = {
+	.probe	= cec_gpio_probe,
+	.remove = cec_gpio_remove,
+	.driver = {
+		.name		= "cec-gpio",
+		.of_match_table	= cec_gpio_match,
+	},
+};
+
+module_platform_driver(cec_gpio_pdrv);
+
+MODULE_AUTHOR("Hans Verkuil <hans.verkuil@cisco.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("CEC GPIO driver");