On Wed, Aug 30, 2017 at 6:10 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>
> 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@xxxxxxxxx>
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
