[{"id":1760737,"web_url":"http://patchwork.ozlabs.org/comment/1760737/","msgid":"","list_archive_url":null,"date":"2017-08-31T08:28:49","subject":"Re: [PATCHv3 4/5] cec-gpio: add HDMI CEC GPIO driver","submitter":{"id":7055,"url":"http://patchwork.ozlabs.org/api/people/7055/","name":"Linus Walleij","email":"linus.walleij@linaro.org"},"content":"On Wed, Aug 30, 2017 at 6:10 PM, Hans Verkuil wrote:\n\n> From: Hans Verkuil \n>\n> Add a simple HDMI CEC GPIO driver that sits on top of the cec-pin framework.\n>\n> While I have heard of SoCs that use the GPIO pin for CEC (apparently an\n> early RockChip SoC used that), the main use-case of this driver is to\n> function as a debugging tool.\n>\n> By connecting the CEC line to a GPIO pin on a Raspberry Pi 3 for example\n> it turns it into a CEC debugger and protocol analyzer.\n>\n> With 'cec-ctl --monitor-pin' the CEC traffic can be analyzed.\n>\n> But of course it can also be used with any hardware project where the\n> HDMI CEC pin is hooked up to a pull-up gpio pin.\n>\n> In addition this has (optional) support for tracing HPD changes if the\n> HPD is connected to a GPIO.\n>\n> Signed-off-by: Hans Verkuil \n\nPretty cool stuff!\n\n> +config CEC_GPIO\n> + tristate \"Generic GPIO-based CEC driver\"\n> + depends on PREEMPT\n\ndepends on GPIOLIB\n\nor\n\nselect GPIOLIB\n\nyour pick.\n\n> +#include \n\nThis should not be needed in new code.\n\n> +#include \n\nThis should be enough.\n\n> +#include \n\nYour should not need this either.\n\n> +struct cec_gpio {\n> + struct cec_adapter *adap;\n> + struct device *dev;\n> + int gpio;\n> + int hpd_gpio;\n\nThink this should be:\n\nstruct gpio_desc *gpio;\nstruct gpio_desc *hpd_gpio;\n\n> + int irq;\n> + int hpd_irq;\n> + bool hpd_is_high;\n> + ktime_t hpd_ts;\n> + bool is_low;\n> + bool have_irq;\n> +};\n> +\n> +static bool cec_gpio_read(struct cec_adapter *adap)\n> +{\n> + struct cec_gpio *cec = cec_get_drvdata(adap);\n> +\n> + if (cec->is_low)\n> + return false;\n> + return gpio_get_value(cec->gpio);\n\nUse descriptor accessors\n\ngpiod_get_value()\n\n> +static void cec_gpio_high(struct cec_adapter *adap)\n> +{\n> + struct cec_gpio *cec = cec_get_drvdata(adap);\n> +\n> + if (!cec->is_low)\n> + return;\n> + cec->is_low = false;\n> + gpio_direction_input(cec->gpio);\n\nAre you setting the GPIO high by setting it as input?\nThat sounds like you should be requesting it as\nopen drain in the DTS file, see\nDocumentation/gpio/driver.txt\nfor details about open drain, and use\nGPIO_LINE_OPEN_DRAIN\nfrom \n\n> +static void cec_gpio_low(struct cec_adapter *adap)\n> +{\n> + struct cec_gpio *cec = cec_get_drvdata(adap);\n> +\n> + if (cec->is_low)\n> + return;\n> + if (WARN_ON_ONCE(cec->have_irq))\n> + free_irq(cec->irq, cec);\n> + cec->have_irq = false;\n> + cec->is_low = true;\n> + gpio_direction_output(cec->gpio, 0);\n\nYeah this looks like you're doing open drain.\ngpiod_direction_output() etc.\n\n> +static int cec_gpio_read_hpd(struct cec_adapter *adap)\n> +{\n> + struct cec_gpio *cec = cec_get_drvdata(adap);\n> +\n> + if (cec->hpd_gpio < 0)\n> + return -ENOTTY;\n> + return gpio_get_value(cec->hpd_gpio);\n\ngpiod_get_value()\n\n\n> +static int cec_gpio_probe(struct platform_device *pdev)\n> +{\n> + struct device *dev = &pdev->dev;\n> + enum of_gpio_flags hpd_gpio_flags;\n> + struct cec_gpio *cec;\n> + int ret;\n> +\n> + cec = devm_kzalloc(dev, sizeof(*cec), GFP_KERNEL);\n> + if (!cec)\n> + return -ENOMEM;\n> +\n> + cec->gpio = of_get_named_gpio_flags(dev->of_node,\n> + \"cec-gpio\", 0, &hpd_gpio_flags);\n> + if (cec->gpio < 0)\n> + return cec->gpio;\n> +\n> + cec->hpd_gpio = of_get_named_gpio_flags(dev->of_node,\n> + \"hpd-gpio\", 0, &hpd_gpio_flags);\n\nThis is a proper device so don't use all this trouble.\n\ncec->gpio = gpiod_get(dev, \"cec-gpio\", GPIOD_IN);\n\nor similar (grep for examples!)\n\nSame for hdp_gpio.\n\n> + cec->irq = gpio_to_irq(cec->gpio);\n\ngpiod_to_irq()\n\n> + gpio_direction_input(cec->gpio);\n\nThis is not needed with the above IN flag.\n\nBut as said above, maybe you are looking for an open drain\noutput actually.\n\n> + if (cec->hpd_gpio >= 0) {\n> + cec->hpd_irq = gpio_to_irq(cec->hpd_gpio);\n> + gpio_direction_input(cec->hpd_gpio);\n\nAlready done if you pass the right flag. This should be IN for sure.\n\nUse gpiod_to_irq().\n\nPlease include me on subsequent posts, I want to try to use\ndescriptors as much as possible for new drivers.\n\nYours,\nLinus Walleij\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=linaro.org header.i=@linaro.org\n\theader.b=\"btMXYIsL\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xjbBG3DKJz9t2M\n\tfor ;\n\tThu, 31 Aug 2017 18:28:54 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751387AbdHaI2w (ORCPT\n\t);\n\tThu, 31 Aug 2017 04:28:52 -0400","from mail-oi0-f42.google.com ([209.85.218.42]:35270 \"EHLO\n\tmail-oi0-f42.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751022AbdHaI2u (ORCPT\n\t); Thu, 31 Aug 2017 04:28:50 -0400","by mail-oi0-f42.google.com with SMTP id k77so283529oib.2\n\tfor ;\n\tThu, 31 Aug 2017 01:28:50 -0700 (PDT)","by 10.157.58.74 with HTTP; Thu, 31 Aug 2017 01:28:49 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=YjGr4WJDhxN3ft4NOLkec6gSSqHxSgO++/IXrodDmuc=;\n\tb=btMXYIsLXpFlkbbqpHtO5ZENxOFn2yVFG0brA31WoSIGJAh6wNmWOwLztUgaF3xXAO\n\tC+PBvrDp4FN+sfswcXODi9QDLmvxGaUS0t1llwKDKuUMZu5nxKqrjbadiiWkCrn5yF0e\n\tvsUSCmX5mjfrsU3nRvdT/jJzgBq3uxqrdI7DE=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=YjGr4WJDhxN3ft4NOLkec6gSSqHxSgO++/IXrodDmuc=;\n\tb=cdFpwSfbd8LjilaBTuw4wzMBwIA1X+comXjIkfiI6a1Ms2mK7C5HG62mbAIytiGQsE\n\tWaEO4nwm3ds4i7SsRSk7iftAzida3GfMnnk2ryXz/VFzzrFM255TWALiIeJajBBmVvk1\n\tRvrZtzee2W5JYcxLFDSpIxKLvHPgyK1YNP1l03Yiia578b5BUV3Hy6xF8yEuBw1OUapp\n\tV/Q3z/0mBw47lU9TPPysDZoQu8Da06qM8b7Wo2eWe9ghqvHxqnGSJBK2qsyTuxHiNVj6\n\tNwqu1ynUOaxo2uHlzWukG9/ASb5JOwlbFF2gSVwyooVW1DHBe/pYfA8NLPI4lGw1UsXP\n\t4bUQ==","X-Gm-Message-State":"AHYfb5hr8qeHk/JSZ8qwdx9g5EFfCzLdb5RmULiaN2U2tlQ3eqMtK69S\n\tXlgW5lDB9HEhRNsjuIcphMwjvnQCvtPFGNdHYQ==","X-Received":"by 10.202.169.145 with SMTP id\n\ts139mr3850726oie.297.1504168130009; \n\tThu, 31 Aug 2017 01:28:50 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<20170830161044.26571-5-hverkuil@xs4all.nl>","References":"<20170830161044.26571-1-hverkuil@xs4all.nl>\n\t<20170830161044.26571-5-hverkuil@xs4all.nl>","From":"Linus Walleij ","Date":"Thu, 31 Aug 2017 10:28:49 +0200","Message-ID":"","Subject":"Re: [PATCHv3 4/5] cec-gpio: add HDMI CEC GPIO driver","To":"Hans Verkuil ","Cc":"\"linux-media@vger.kernel.org\" ,\n\t\"devicetree@vger.kernel.org\" ,\n\tHans Verkuil ,\n\t\"open list:DRM PANEL DRIVERS\" ","Content-Type":"text/plain; charset=\"UTF-8\"","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}}]