* Re: [PATCH v2 1/5] video: omapdss: Add opa362 driver
@ 2014-11-13 11:51 ` Tomi Valkeinen
0 siblings, 0 replies; 40+ messages in thread
From: Tomi Valkeinen @ 2014-11-13 11:51 UTC (permalink / raw)
To: Marek Belisko
Cc: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak,
bcousson, tony, linux, plagnioj, grant.likely, devicetree,
linux-kernel, linux-omap, linux-arm-kernel, linux-fbdev, hns,
gta04-owner
[-- Attachment #1: Type: text/plain, Size: 10111 bytes --]
On 13/11/14 00:10, Marek Belisko wrote:
> opa362 is amplifier for video and can be connected to the tvout pads
> of the OMAP3. It has one gpio control for enable/disable of the output
> (high impedance).
>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
> drivers/video/fbdev/omap2/displays-new/Kconfig | 6 +
> drivers/video/fbdev/omap2/displays-new/Makefile | 1 +
> .../fbdev/omap2/displays-new/amplifier-opa362.c | 343 +++++++++++++++++++++
I think it would be better to rename this to encoder-opa362.c. It's not
encoder as such, but it falls into the same category.
> include/video/omap-panel-data.h | 12 +
> 4 files changed, 362 insertions(+)
> create mode 100644 drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
>
> diff --git a/drivers/video/fbdev/omap2/displays-new/Kconfig b/drivers/video/fbdev/omap2/displays-new/Kconfig
> index e6cfc38..211b3ec 100644
> --- a/drivers/video/fbdev/omap2/displays-new/Kconfig
> +++ b/drivers/video/fbdev/omap2/displays-new/Kconfig
> @@ -1,6 +1,12 @@
> menu "OMAP Display Device Drivers (new device model)"
> depends on OMAP2_DSS
>
> +config DISPLAY_AMPLIFIER_OPA362
Here also use ENCODER instead.
> + tristate "external analog amplifier with output disable/high-Z (e.g. OPA362)"
> + help
> + Driver to enable an external analog TV amplifier (e.g. OPA362)
> + through a GPIO.
The indentation above seems funny.
The text looks a bit odd. So is this a driver for OPA362, or is this a
generic driver for any similar devices? Most of the names and code makes
me think this is a driver for OPA362, but the text above quite clearly
gives the impression that this is a driver for any analog video amp,
with single enable gpio.
> +
> config DISPLAY_ENCODER_TFP410
> tristate "TFP410 DPI to DVI Encoder"
> help
> diff --git a/drivers/video/fbdev/omap2/displays-new/Makefile b/drivers/video/fbdev/omap2/displays-new/Makefile
> index 0323a8a..b311542 100644
> --- a/drivers/video/fbdev/omap2/displays-new/Makefile
> +++ b/drivers/video/fbdev/omap2/displays-new/Makefile
> @@ -1,3 +1,4 @@
> +obj-$(CONFIG_DISPLAY_AMPLIFIER_OPA362) += amplifier-opa362.o
> obj-$(CONFIG_DISPLAY_ENCODER_TFP410) += encoder-tfp410.o
> obj-$(CONFIG_DISPLAY_ENCODER_TPD12S015) += encoder-tpd12s015.o
> obj-$(CONFIG_DISPLAY_CONNECTOR_DVI) += connector-dvi.o
> diff --git a/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c b/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
> new file mode 100644
> index 0000000..8065a28
> --- /dev/null
> +++ b/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
> @@ -0,0 +1,343 @@
> +/*
> + * OPA362 analog video amplifier with output/power control
> + *
> + * Copyright (C) 2014 Golden Delicious Computers
> + * Author: H. Nikolaus Schaller <hns@goldelico.com>
> + *
> + * based on encoder-tfp410
> + *
> + * Copyright (C) 2013 Texas Instruments
> + * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
> +
> +#include <linux/gpio.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/of_gpio.h>
> +
> +#include <video/omapdss.h>
> +#include <video/omap-panel-data.h>
> +
> +struct panel_drv_data {
> + struct omap_dss_device dssdev;
> + struct omap_dss_device *in;
> +
> + int enable_gpio;
> +
> + struct omap_video_timings timings;
> +};
> +
> +#define to_panel_data(x) container_of(x, struct panel_drv_data, dssdev)
> +
> +static int opa362_connect(struct omap_dss_device *dssdev,
> + struct omap_dss_device *dst)
> +{
> + struct panel_drv_data *ddata = to_panel_data(dssdev);
> + struct omap_dss_device *in = ddata->in;
> + int r;
> +
> + dev_dbg(dssdev->dev, "connect\n");
> +
> + if (omapdss_device_is_connected(dssdev))
> + return -EBUSY;
> +
> + r = in->ops.atv->connect(in, dssdev);
> + if (r)
> + return r;
> +
> + dst->src = dssdev;
> + dssdev->dst = dst;
> +
> + return 0;
> +}
> +
> +static void opa362_disconnect(struct omap_dss_device *dssdev,
> + struct omap_dss_device *dst)
> +{
> + struct panel_drv_data *ddata = to_panel_data(dssdev);
> + struct omap_dss_device *in = ddata->in;
> +
> + dev_dbg(dssdev->dev, "disconnect\n");
> +
> + WARN_ON(!omapdss_device_is_connected(dssdev));
> + if (!omapdss_device_is_connected(dssdev))
> + return;
> +
> + WARN_ON(dst != dssdev->dst);
> + if (dst != dssdev->dst)
> + return;
> +
> + dst->src = NULL;
> + dssdev->dst = NULL;
> +
> + in->ops.atv->disconnect(in, &ddata->dssdev);
> +}
> +
> +static int opa362_enable(struct omap_dss_device *dssdev)
> +{
> + struct panel_drv_data *ddata = to_panel_data(dssdev);
> + struct omap_dss_device *in = ddata->in;
> + int r;
> +
> + dev_dbg(dssdev->dev, "enable\n");
> +
> + if (!omapdss_device_is_connected(dssdev))
> + return -ENODEV;
> +
> + if (omapdss_device_is_enabled(dssdev))
> + return 0;
> +
> + in->ops.atv->set_timings(in, &ddata->timings);
> + /* fixme: should we receive the invert from our consumer, i.e. the connector? */
> + in->ops.atv->invert_vid_out_polarity(in, true);
Well this does seem to be broken. I don't know what the answer to the
question above is, but the code doesn't work properly.
In the opa362_invert_vid_out_polarity function below, you get the invert
boolean from the connector. This you pass to the OMAP venc. However,
above you always override that value in venc with true.
So, either the invert_vid_out_polarity value has to be always true or
false, because _OPA362_ requires it to be true or false, OR you need use
the value from the connector.
Seeing the comment in opa362_invert_vid_out_polarity, my guess is the
latter, and the call above should be removed.
> +
> + r = in->ops.atv->enable(in);
> + if (r)
> + return r;
> +
> + if (gpio_is_valid(ddata->enable_gpio))
> + gpio_set_value_cansleep(ddata->enable_gpio, 1);
> +
> + dssdev->state = OMAP_DSS_DISPLAY_ACTIVE;
> +
> + return 0;
> +}
> +
> +static void opa362_disable(struct omap_dss_device *dssdev)
> +{
> + struct panel_drv_data *ddata = to_panel_data(dssdev);
> + struct omap_dss_device *in = ddata->in;
> +
> + dev_dbg(dssdev->dev, "disable\n");
> +
> + if (!omapdss_device_is_enabled(dssdev))
> + return;
> +
> + if (gpio_is_valid(ddata->enable_gpio))
> + gpio_set_value_cansleep(ddata->enable_gpio, 0);
> +
> + in->ops.atv->disable(in);
> +
> + dssdev->state = OMAP_DSS_DISPLAY_DISABLED;
> +}
> +
> +static void opa362_set_timings(struct omap_dss_device *dssdev,
> + struct omap_video_timings *timings)
> +{
> + struct panel_drv_data *ddata = to_panel_data(dssdev);
> + struct omap_dss_device *in = ddata->in;
> +
> + dev_dbg(dssdev->dev, "set_timings\n");
> +
> + ddata->timings = *timings;
> + dssdev->panel.timings = *timings;
> +
> + in->ops.atv->set_timings(in, timings);
> +}
> +
> +static void opa362_get_timings(struct omap_dss_device *dssdev,
> + struct omap_video_timings *timings)
> +{
> + struct panel_drv_data *ddata = to_panel_data(dssdev);
> +
> + dev_dbg(dssdev->dev, "get_timings\n");
> +
> + *timings = ddata->timings;
> +}
> +
> +static int opa362_check_timings(struct omap_dss_device *dssdev,
> + struct omap_video_timings *timings)
> +{
> + struct panel_drv_data *ddata = to_panel_data(dssdev);
> + struct omap_dss_device *in = ddata->in;
> +
> + dev_dbg(dssdev->dev, "check_timings\n");
> +
> + return in->ops.atv->check_timings(in, timings);
> +}
> +
> +static void opa362_set_type(struct omap_dss_device *dssdev,
> + enum omap_dss_venc_type type)
> +{
> + /* we can only drive a COMPOSITE output */
> + WARN_ON(type != OMAP_DSS_VENC_TYPE_COMPOSITE);
> +
> +}
> +
> +static void opa362_invert_vid_out_polarity(struct omap_dss_device *dssdev,
> + bool invert_polarity)
> +{
> + struct panel_drv_data *ddata = to_panel_data(dssdev);
> + struct omap_dss_device *in = ddata->in;
> +
> + /* OPA362 inverts the polarity */
> + in->ops.atv->invert_vid_out_polarity(in, !invert_polarity);
> +}
> +
> +static const struct omapdss_atv_ops opa362_atv_ops = {
> + .connect = opa362_connect,
> + .disconnect = opa362_disconnect,
> +
> + .enable = opa362_enable,
> + .disable = opa362_disable,
> +
> + .check_timings = opa362_check_timings,
> + .set_timings = opa362_set_timings,
> + .get_timings = opa362_get_timings,
> +
> + .set_type = opa362_set_type,
> + .invert_vid_out_polarity = opa362_invert_vid_out_polarity,
> +};
> +
> +static int opa362_probe_pdata(struct platform_device *pdev)
> +{
> + struct panel_drv_data *ddata = platform_get_drvdata(pdev);
> + struct amplifier_opa362_platform_data *pdata;
> + struct omap_dss_device *dssdev, *in;
> +
> + pdata = dev_get_platdata(&pdev->dev);
> +
> + ddata->enable_gpio = pdata->enable_gpio;
> +
> + in = omap_dss_find_output(pdata->source);
> + if (in == NULL) {
> + dev_err(&pdev->dev, "Failed to find video source\n");
> + return -ENODEV;
> + }
> +
> + ddata->in = in;
> +
> + dssdev = &ddata->dssdev;
> + dssdev->name = pdata->name;
> +
> + return 0;
> +}
We are going to support only DT boot at some point. Thus I think the
whole platform data code should be left out.
> +
> +static int opa362_probe_of(struct platform_device *pdev)
> +{
> + struct panel_drv_data *ddata = platform_get_drvdata(pdev);
> + struct device_node *node = pdev->dev.of_node;
> + struct omap_dss_device *in;
> + int gpio;
> +
> + gpio = of_get_gpio(node, 0);
> +
> + if (gpio_is_valid(gpio) || gpio == -ENOENT) {
> + ddata->enable_gpio = gpio;
> + } else {
> + dev_err(&pdev->dev, "failed to parse enable gpio\n");
> + return gpio;
> + }
You should use the new GPIO API to get full support from the DT data.
For an example, see panel-dpi.c's enable_gpio.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v2 1/5] video: omapdss: Add opa362 driver
@ 2014-11-13 11:51 ` Tomi Valkeinen
0 siblings, 0 replies; 40+ messages in thread
From: Tomi Valkeinen @ 2014-11-13 11:51 UTC (permalink / raw)
To: linux-arm-kernel
On 13/11/14 00:10, Marek Belisko wrote:
> opa362 is amplifier for video and can be connected to the tvout pads
> of the OMAP3. It has one gpio control for enable/disable of the output
> (high impedance).
>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
> drivers/video/fbdev/omap2/displays-new/Kconfig | 6 +
> drivers/video/fbdev/omap2/displays-new/Makefile | 1 +
> .../fbdev/omap2/displays-new/amplifier-opa362.c | 343 +++++++++++++++++++++
I think it would be better to rename this to encoder-opa362.c. It's not
encoder as such, but it falls into the same category.
> include/video/omap-panel-data.h | 12 +
> 4 files changed, 362 insertions(+)
> create mode 100644 drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
>
> diff --git a/drivers/video/fbdev/omap2/displays-new/Kconfig b/drivers/video/fbdev/omap2/displays-new/Kconfig
> index e6cfc38..211b3ec 100644
> --- a/drivers/video/fbdev/omap2/displays-new/Kconfig
> +++ b/drivers/video/fbdev/omap2/displays-new/Kconfig
> @@ -1,6 +1,12 @@
> menu "OMAP Display Device Drivers (new device model)"
> depends on OMAP2_DSS
>
> +config DISPLAY_AMPLIFIER_OPA362
Here also use ENCODER instead.
> + tristate "external analog amplifier with output disable/high-Z (e.g. OPA362)"
> + help
> + Driver to enable an external analog TV amplifier (e.g. OPA362)
> + through a GPIO.
The indentation above seems funny.
The text looks a bit odd. So is this a driver for OPA362, or is this a
generic driver for any similar devices? Most of the names and code makes
me think this is a driver for OPA362, but the text above quite clearly
gives the impression that this is a driver for any analog video amp,
with single enable gpio.
> +
> config DISPLAY_ENCODER_TFP410
> tristate "TFP410 DPI to DVI Encoder"
> help
> diff --git a/drivers/video/fbdev/omap2/displays-new/Makefile b/drivers/video/fbdev/omap2/displays-new/Makefile
> index 0323a8a..b311542 100644
> --- a/drivers/video/fbdev/omap2/displays-new/Makefile
> +++ b/drivers/video/fbdev/omap2/displays-new/Makefile
> @@ -1,3 +1,4 @@
> +obj-$(CONFIG_DISPLAY_AMPLIFIER_OPA362) += amplifier-opa362.o
> obj-$(CONFIG_DISPLAY_ENCODER_TFP410) += encoder-tfp410.o
> obj-$(CONFIG_DISPLAY_ENCODER_TPD12S015) += encoder-tpd12s015.o
> obj-$(CONFIG_DISPLAY_CONNECTOR_DVI) += connector-dvi.o
> diff --git a/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c b/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
> new file mode 100644
> index 0000000..8065a28
> --- /dev/null
> +++ b/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
> @@ -0,0 +1,343 @@
> +/*
> + * OPA362 analog video amplifier with output/power control
> + *
> + * Copyright (C) 2014 Golden Delicious Computers
> + * Author: H. Nikolaus Schaller <hns@goldelico.com>
> + *
> + * based on encoder-tfp410
> + *
> + * Copyright (C) 2013 Texas Instruments
> + * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
> +
> +#include <linux/gpio.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/of_gpio.h>
> +
> +#include <video/omapdss.h>
> +#include <video/omap-panel-data.h>
> +
> +struct panel_drv_data {
> + struct omap_dss_device dssdev;
> + struct omap_dss_device *in;
> +
> + int enable_gpio;
> +
> + struct omap_video_timings timings;
> +};
> +
> +#define to_panel_data(x) container_of(x, struct panel_drv_data, dssdev)
> +
> +static int opa362_connect(struct omap_dss_device *dssdev,
> + struct omap_dss_device *dst)
> +{
> + struct panel_drv_data *ddata = to_panel_data(dssdev);
> + struct omap_dss_device *in = ddata->in;
> + int r;
> +
> + dev_dbg(dssdev->dev, "connect\n");
> +
> + if (omapdss_device_is_connected(dssdev))
> + return -EBUSY;
> +
> + r = in->ops.atv->connect(in, dssdev);
> + if (r)
> + return r;
> +
> + dst->src = dssdev;
> + dssdev->dst = dst;
> +
> + return 0;
> +}
> +
> +static void opa362_disconnect(struct omap_dss_device *dssdev,
> + struct omap_dss_device *dst)
> +{
> + struct panel_drv_data *ddata = to_panel_data(dssdev);
> + struct omap_dss_device *in = ddata->in;
> +
> + dev_dbg(dssdev->dev, "disconnect\n");
> +
> + WARN_ON(!omapdss_device_is_connected(dssdev));
> + if (!omapdss_device_is_connected(dssdev))
> + return;
> +
> + WARN_ON(dst != dssdev->dst);
> + if (dst != dssdev->dst)
> + return;
> +
> + dst->src = NULL;
> + dssdev->dst = NULL;
> +
> + in->ops.atv->disconnect(in, &ddata->dssdev);
> +}
> +
> +static int opa362_enable(struct omap_dss_device *dssdev)
> +{
> + struct panel_drv_data *ddata = to_panel_data(dssdev);
> + struct omap_dss_device *in = ddata->in;
> + int r;
> +
> + dev_dbg(dssdev->dev, "enable\n");
> +
> + if (!omapdss_device_is_connected(dssdev))
> + return -ENODEV;
> +
> + if (omapdss_device_is_enabled(dssdev))
> + return 0;
> +
> + in->ops.atv->set_timings(in, &ddata->timings);
> + /* fixme: should we receive the invert from our consumer, i.e. the connector? */
> + in->ops.atv->invert_vid_out_polarity(in, true);
Well this does seem to be broken. I don't know what the answer to the
question above is, but the code doesn't work properly.
In the opa362_invert_vid_out_polarity function below, you get the invert
boolean from the connector. This you pass to the OMAP venc. However,
above you always override that value in venc with true.
So, either the invert_vid_out_polarity value has to be always true or
false, because _OPA362_ requires it to be true or false, OR you need use
the value from the connector.
Seeing the comment in opa362_invert_vid_out_polarity, my guess is the
latter, and the call above should be removed.
> +
> + r = in->ops.atv->enable(in);
> + if (r)
> + return r;
> +
> + if (gpio_is_valid(ddata->enable_gpio))
> + gpio_set_value_cansleep(ddata->enable_gpio, 1);
> +
> + dssdev->state = OMAP_DSS_DISPLAY_ACTIVE;
> +
> + return 0;
> +}
> +
> +static void opa362_disable(struct omap_dss_device *dssdev)
> +{
> + struct panel_drv_data *ddata = to_panel_data(dssdev);
> + struct omap_dss_device *in = ddata->in;
> +
> + dev_dbg(dssdev->dev, "disable\n");
> +
> + if (!omapdss_device_is_enabled(dssdev))
> + return;
> +
> + if (gpio_is_valid(ddata->enable_gpio))
> + gpio_set_value_cansleep(ddata->enable_gpio, 0);
> +
> + in->ops.atv->disable(in);
> +
> + dssdev->state = OMAP_DSS_DISPLAY_DISABLED;
> +}
> +
> +static void opa362_set_timings(struct omap_dss_device *dssdev,
> + struct omap_video_timings *timings)
> +{
> + struct panel_drv_data *ddata = to_panel_data(dssdev);
> + struct omap_dss_device *in = ddata->in;
> +
> + dev_dbg(dssdev->dev, "set_timings\n");
> +
> + ddata->timings = *timings;
> + dssdev->panel.timings = *timings;
> +
> + in->ops.atv->set_timings(in, timings);
> +}
> +
> +static void opa362_get_timings(struct omap_dss_device *dssdev,
> + struct omap_video_timings *timings)
> +{
> + struct panel_drv_data *ddata = to_panel_data(dssdev);
> +
> + dev_dbg(dssdev->dev, "get_timings\n");
> +
> + *timings = ddata->timings;
> +}
> +
> +static int opa362_check_timings(struct omap_dss_device *dssdev,
> + struct omap_video_timings *timings)
> +{
> + struct panel_drv_data *ddata = to_panel_data(dssdev);
> + struct omap_dss_device *in = ddata->in;
> +
> + dev_dbg(dssdev->dev, "check_timings\n");
> +
> + return in->ops.atv->check_timings(in, timings);
> +}
> +
> +static void opa362_set_type(struct omap_dss_device *dssdev,
> + enum omap_dss_venc_type type)
> +{
> + /* we can only drive a COMPOSITE output */
> + WARN_ON(type != OMAP_DSS_VENC_TYPE_COMPOSITE);
> +
> +}
> +
> +static void opa362_invert_vid_out_polarity(struct omap_dss_device *dssdev,
> + bool invert_polarity)
> +{
> + struct panel_drv_data *ddata = to_panel_data(dssdev);
> + struct omap_dss_device *in = ddata->in;
> +
> + /* OPA362 inverts the polarity */
> + in->ops.atv->invert_vid_out_polarity(in, !invert_polarity);
> +}
> +
> +static const struct omapdss_atv_ops opa362_atv_ops = {
> + .connect = opa362_connect,
> + .disconnect = opa362_disconnect,
> +
> + .enable = opa362_enable,
> + .disable = opa362_disable,
> +
> + .check_timings = opa362_check_timings,
> + .set_timings = opa362_set_timings,
> + .get_timings = opa362_get_timings,
> +
> + .set_type = opa362_set_type,
> + .invert_vid_out_polarity = opa362_invert_vid_out_polarity,
> +};
> +
> +static int opa362_probe_pdata(struct platform_device *pdev)
> +{
> + struct panel_drv_data *ddata = platform_get_drvdata(pdev);
> + struct amplifier_opa362_platform_data *pdata;
> + struct omap_dss_device *dssdev, *in;
> +
> + pdata = dev_get_platdata(&pdev->dev);
> +
> + ddata->enable_gpio = pdata->enable_gpio;
> +
> + in = omap_dss_find_output(pdata->source);
> + if (in == NULL) {
> + dev_err(&pdev->dev, "Failed to find video source\n");
> + return -ENODEV;
> + }
> +
> + ddata->in = in;
> +
> + dssdev = &ddata->dssdev;
> + dssdev->name = pdata->name;
> +
> + return 0;
> +}
We are going to support only DT boot at some point. Thus I think the
whole platform data code should be left out.
> +
> +static int opa362_probe_of(struct platform_device *pdev)
> +{
> + struct panel_drv_data *ddata = platform_get_drvdata(pdev);
> + struct device_node *node = pdev->dev.of_node;
> + struct omap_dss_device *in;
> + int gpio;
> +
> + gpio = of_get_gpio(node, 0);
> +
> + if (gpio_is_valid(gpio) || gpio == -ENOENT) {
> + ddata->enable_gpio = gpio;
> + } else {
> + dev_err(&pdev->dev, "failed to parse enable gpio\n");
> + return gpio;
> + }
You should use the new GPIO API to get full support from the DT data.
For an example, see panel-dpi.c's enable_gpio.
Tomi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141113/86a8daae/attachment-0001.sig>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 1/5] video: omapdss: Add opa362 driver
@ 2014-11-13 11:51 ` Tomi Valkeinen
0 siblings, 0 replies; 40+ messages in thread
From: Tomi Valkeinen @ 2014-11-13 11:51 UTC (permalink / raw)
To: Marek Belisko
Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, pawel.moll-5wv7dgnIgG8,
mark.rutland-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
galak-sgV2jX0FEOL9JmXXK+q4OQ, bcousson-rdvid1DuHRBWk0Htik3J/w,
tony-4v6yS6AI5VpBDgjK7y7TUQ, linux-lFZ/pmaqli7XmaaqVzeoHQ,
plagnioj-sclMFOaUSTBWk0Htik3J/w,
grant.likely-QSEj5FYQhm4dnm+yROfE0A,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-omap-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA, hns-xXXSsgcRVICgSpxsJD1C4w,
gta04-owner-xXXSsgcRVICgSpxsJD1C4w
[-- Attachment #1: Type: text/plain, Size: 10185 bytes --]
On 13/11/14 00:10, Marek Belisko wrote:
> opa362 is amplifier for video and can be connected to the tvout pads
> of the OMAP3. It has one gpio control for enable/disable of the output
> (high impedance).
>
> Signed-off-by: H. Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
> ---
> drivers/video/fbdev/omap2/displays-new/Kconfig | 6 +
> drivers/video/fbdev/omap2/displays-new/Makefile | 1 +
> .../fbdev/omap2/displays-new/amplifier-opa362.c | 343 +++++++++++++++++++++
I think it would be better to rename this to encoder-opa362.c. It's not
encoder as such, but it falls into the same category.
> include/video/omap-panel-data.h | 12 +
> 4 files changed, 362 insertions(+)
> create mode 100644 drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
>
> diff --git a/drivers/video/fbdev/omap2/displays-new/Kconfig b/drivers/video/fbdev/omap2/displays-new/Kconfig
> index e6cfc38..211b3ec 100644
> --- a/drivers/video/fbdev/omap2/displays-new/Kconfig
> +++ b/drivers/video/fbdev/omap2/displays-new/Kconfig
> @@ -1,6 +1,12 @@
> menu "OMAP Display Device Drivers (new device model)"
> depends on OMAP2_DSS
>
> +config DISPLAY_AMPLIFIER_OPA362
Here also use ENCODER instead.
> + tristate "external analog amplifier with output disable/high-Z (e.g. OPA362)"
> + help
> + Driver to enable an external analog TV amplifier (e.g. OPA362)
> + through a GPIO.
The indentation above seems funny.
The text looks a bit odd. So is this a driver for OPA362, or is this a
generic driver for any similar devices? Most of the names and code makes
me think this is a driver for OPA362, but the text above quite clearly
gives the impression that this is a driver for any analog video amp,
with single enable gpio.
> +
> config DISPLAY_ENCODER_TFP410
> tristate "TFP410 DPI to DVI Encoder"
> help
> diff --git a/drivers/video/fbdev/omap2/displays-new/Makefile b/drivers/video/fbdev/omap2/displays-new/Makefile
> index 0323a8a..b311542 100644
> --- a/drivers/video/fbdev/omap2/displays-new/Makefile
> +++ b/drivers/video/fbdev/omap2/displays-new/Makefile
> @@ -1,3 +1,4 @@
> +obj-$(CONFIG_DISPLAY_AMPLIFIER_OPA362) += amplifier-opa362.o
> obj-$(CONFIG_DISPLAY_ENCODER_TFP410) += encoder-tfp410.o
> obj-$(CONFIG_DISPLAY_ENCODER_TPD12S015) += encoder-tpd12s015.o
> obj-$(CONFIG_DISPLAY_CONNECTOR_DVI) += connector-dvi.o
> diff --git a/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c b/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
> new file mode 100644
> index 0000000..8065a28
> --- /dev/null
> +++ b/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
> @@ -0,0 +1,343 @@
> +/*
> + * OPA362 analog video amplifier with output/power control
> + *
> + * Copyright (C) 2014 Golden Delicious Computers
> + * Author: H. Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
> + *
> + * based on encoder-tfp410
> + *
> + * Copyright (C) 2013 Texas Instruments
> + * Author: Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
> +
> +#include <linux/gpio.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/of_gpio.h>
> +
> +#include <video/omapdss.h>
> +#include <video/omap-panel-data.h>
> +
> +struct panel_drv_data {
> + struct omap_dss_device dssdev;
> + struct omap_dss_device *in;
> +
> + int enable_gpio;
> +
> + struct omap_video_timings timings;
> +};
> +
> +#define to_panel_data(x) container_of(x, struct panel_drv_data, dssdev)
> +
> +static int opa362_connect(struct omap_dss_device *dssdev,
> + struct omap_dss_device *dst)
> +{
> + struct panel_drv_data *ddata = to_panel_data(dssdev);
> + struct omap_dss_device *in = ddata->in;
> + int r;
> +
> + dev_dbg(dssdev->dev, "connect\n");
> +
> + if (omapdss_device_is_connected(dssdev))
> + return -EBUSY;
> +
> + r = in->ops.atv->connect(in, dssdev);
> + if (r)
> + return r;
> +
> + dst->src = dssdev;
> + dssdev->dst = dst;
> +
> + return 0;
> +}
> +
> +static void opa362_disconnect(struct omap_dss_device *dssdev,
> + struct omap_dss_device *dst)
> +{
> + struct panel_drv_data *ddata = to_panel_data(dssdev);
> + struct omap_dss_device *in = ddata->in;
> +
> + dev_dbg(dssdev->dev, "disconnect\n");
> +
> + WARN_ON(!omapdss_device_is_connected(dssdev));
> + if (!omapdss_device_is_connected(dssdev))
> + return;
> +
> + WARN_ON(dst != dssdev->dst);
> + if (dst != dssdev->dst)
> + return;
> +
> + dst->src = NULL;
> + dssdev->dst = NULL;
> +
> + in->ops.atv->disconnect(in, &ddata->dssdev);
> +}
> +
> +static int opa362_enable(struct omap_dss_device *dssdev)
> +{
> + struct panel_drv_data *ddata = to_panel_data(dssdev);
> + struct omap_dss_device *in = ddata->in;
> + int r;
> +
> + dev_dbg(dssdev->dev, "enable\n");
> +
> + if (!omapdss_device_is_connected(dssdev))
> + return -ENODEV;
> +
> + if (omapdss_device_is_enabled(dssdev))
> + return 0;
> +
> + in->ops.atv->set_timings(in, &ddata->timings);
> + /* fixme: should we receive the invert from our consumer, i.e. the connector? */
> + in->ops.atv->invert_vid_out_polarity(in, true);
Well this does seem to be broken. I don't know what the answer to the
question above is, but the code doesn't work properly.
In the opa362_invert_vid_out_polarity function below, you get the invert
boolean from the connector. This you pass to the OMAP venc. However,
above you always override that value in venc with true.
So, either the invert_vid_out_polarity value has to be always true or
false, because _OPA362_ requires it to be true or false, OR you need use
the value from the connector.
Seeing the comment in opa362_invert_vid_out_polarity, my guess is the
latter, and the call above should be removed.
> +
> + r = in->ops.atv->enable(in);
> + if (r)
> + return r;
> +
> + if (gpio_is_valid(ddata->enable_gpio))
> + gpio_set_value_cansleep(ddata->enable_gpio, 1);
> +
> + dssdev->state = OMAP_DSS_DISPLAY_ACTIVE;
> +
> + return 0;
> +}
> +
> +static void opa362_disable(struct omap_dss_device *dssdev)
> +{
> + struct panel_drv_data *ddata = to_panel_data(dssdev);
> + struct omap_dss_device *in = ddata->in;
> +
> + dev_dbg(dssdev->dev, "disable\n");
> +
> + if (!omapdss_device_is_enabled(dssdev))
> + return;
> +
> + if (gpio_is_valid(ddata->enable_gpio))
> + gpio_set_value_cansleep(ddata->enable_gpio, 0);
> +
> + in->ops.atv->disable(in);
> +
> + dssdev->state = OMAP_DSS_DISPLAY_DISABLED;
> +}
> +
> +static void opa362_set_timings(struct omap_dss_device *dssdev,
> + struct omap_video_timings *timings)
> +{
> + struct panel_drv_data *ddata = to_panel_data(dssdev);
> + struct omap_dss_device *in = ddata->in;
> +
> + dev_dbg(dssdev->dev, "set_timings\n");
> +
> + ddata->timings = *timings;
> + dssdev->panel.timings = *timings;
> +
> + in->ops.atv->set_timings(in, timings);
> +}
> +
> +static void opa362_get_timings(struct omap_dss_device *dssdev,
> + struct omap_video_timings *timings)
> +{
> + struct panel_drv_data *ddata = to_panel_data(dssdev);
> +
> + dev_dbg(dssdev->dev, "get_timings\n");
> +
> + *timings = ddata->timings;
> +}
> +
> +static int opa362_check_timings(struct omap_dss_device *dssdev,
> + struct omap_video_timings *timings)
> +{
> + struct panel_drv_data *ddata = to_panel_data(dssdev);
> + struct omap_dss_device *in = ddata->in;
> +
> + dev_dbg(dssdev->dev, "check_timings\n");
> +
> + return in->ops.atv->check_timings(in, timings);
> +}
> +
> +static void opa362_set_type(struct omap_dss_device *dssdev,
> + enum omap_dss_venc_type type)
> +{
> + /* we can only drive a COMPOSITE output */
> + WARN_ON(type != OMAP_DSS_VENC_TYPE_COMPOSITE);
> +
> +}
> +
> +static void opa362_invert_vid_out_polarity(struct omap_dss_device *dssdev,
> + bool invert_polarity)
> +{
> + struct panel_drv_data *ddata = to_panel_data(dssdev);
> + struct omap_dss_device *in = ddata->in;
> +
> + /* OPA362 inverts the polarity */
> + in->ops.atv->invert_vid_out_polarity(in, !invert_polarity);
> +}
> +
> +static const struct omapdss_atv_ops opa362_atv_ops = {
> + .connect = opa362_connect,
> + .disconnect = opa362_disconnect,
> +
> + .enable = opa362_enable,
> + .disable = opa362_disable,
> +
> + .check_timings = opa362_check_timings,
> + .set_timings = opa362_set_timings,
> + .get_timings = opa362_get_timings,
> +
> + .set_type = opa362_set_type,
> + .invert_vid_out_polarity = opa362_invert_vid_out_polarity,
> +};
> +
> +static int opa362_probe_pdata(struct platform_device *pdev)
> +{
> + struct panel_drv_data *ddata = platform_get_drvdata(pdev);
> + struct amplifier_opa362_platform_data *pdata;
> + struct omap_dss_device *dssdev, *in;
> +
> + pdata = dev_get_platdata(&pdev->dev);
> +
> + ddata->enable_gpio = pdata->enable_gpio;
> +
> + in = omap_dss_find_output(pdata->source);
> + if (in == NULL) {
> + dev_err(&pdev->dev, "Failed to find video source\n");
> + return -ENODEV;
> + }
> +
> + ddata->in = in;
> +
> + dssdev = &ddata->dssdev;
> + dssdev->name = pdata->name;
> +
> + return 0;
> +}
We are going to support only DT boot at some point. Thus I think the
whole platform data code should be left out.
> +
> +static int opa362_probe_of(struct platform_device *pdev)
> +{
> + struct panel_drv_data *ddata = platform_get_drvdata(pdev);
> + struct device_node *node = pdev->dev.of_node;
> + struct omap_dss_device *in;
> + int gpio;
> +
> + gpio = of_get_gpio(node, 0);
> +
> + if (gpio_is_valid(gpio) || gpio == -ENOENT) {
> + ddata->enable_gpio = gpio;
> + } else {
> + dev_err(&pdev->dev, "failed to parse enable gpio\n");
> + return gpio;
> + }
You should use the new GPIO API to get full support from the DT data.
For an example, see panel-dpi.c's enable_gpio.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 1/5] video: omapdss: Add opa362 driver
@ 2014-11-13 16:25 ` Dr. H. Nikolaus Schaller
0 siblings, 0 replies; 40+ messages in thread
From: Dr. H. Nikolaus Schaller @ 2014-11-13 16:25 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
Am 13.11.2014 um 12:51 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>:
> On 13/11/14 00:10, Marek Belisko wrote:
>> opa362 is amplifier for video and can be connected to the tvout pads
>> of the OMAP3. It has one gpio control for enable/disable of the output
>> (high impedance).
>>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/video/fbdev/omap2/displays-new/Kconfig | 6 +
>> drivers/video/fbdev/omap2/displays-new/Makefile | 1 +
>> .../fbdev/omap2/displays-new/amplifier-opa362.c | 343 +++++++++++++++++++++
>
> I think it would be better to rename this to encoder-opa362.c. It's not
When developing this driver we did simply rename the encoder-tfp410 file,
but thent hough that it does not fit into the „encoder“ category, because we
would expect something digital or digital to analog „encoding“ which it does not.
> encoder as such, but it falls into the same category.
But we can change it.
>
>> include/video/omap-panel-data.h | 12 +
>> 4 files changed, 362 insertions(+)
>> create mode 100644 drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
>>
>> diff --git a/drivers/video/fbdev/omap2/displays-new/Kconfig b/drivers/video/fbdev/omap2/displays-new/Kconfig
>> index e6cfc38..211b3ec 100644
>> --- a/drivers/video/fbdev/omap2/displays-new/Kconfig
>> +++ b/drivers/video/fbdev/omap2/displays-new/Kconfig
>> @@ -1,6 +1,12 @@
>> menu "OMAP Display Device Drivers (new device model)"
>> depends on OMAP2_DSS
>>
>> +config DISPLAY_AMPLIFIER_OPA362
>
> Here also use ENCODER instead.
>
>> + tristate "external analog amplifier with output disable/high-Z (e.g. OPA362)"
>> + help
>> + Driver to enable an external analog TV amplifier (e.g. OPA362)
>> + through a GPIO.
>
> The indentation above seems funny.
>
> The text looks a bit odd. So is this a driver for OPA362, or is this a
> generic driver for any similar devices? Most of the names and code makes
> me think this is a driver for OPA362, but the text above quite clearly
> gives the impression that this is a driver for any analog video amp,
> with single enable gpio.
Hm. We can imagine that there are other devices with similar functionality
and gpio but we have not tested any. So it is indeed better to describe it as
a pure OPA362 driver.
>
>> +
>> config DISPLAY_ENCODER_TFP410
>> tristate "TFP410 DPI to DVI Encoder"
>> help
>> diff --git a/drivers/video/fbdev/omap2/displays-new/Makefile b/drivers/video/fbdev/omap2/displays-new/Makefile
>> index 0323a8a..b311542 100644
>> --- a/drivers/video/fbdev/omap2/displays-new/Makefile
>> +++ b/drivers/video/fbdev/omap2/displays-new/Makefile
>> @@ -1,3 +1,4 @@
>> +obj-$(CONFIG_DISPLAY_AMPLIFIER_OPA362) += amplifier-opa362.o
>> obj-$(CONFIG_DISPLAY_ENCODER_TFP410) += encoder-tfp410.o
>> obj-$(CONFIG_DISPLAY_ENCODER_TPD12S015) += encoder-tpd12s015.o
>> obj-$(CONFIG_DISPLAY_CONNECTOR_DVI) += connector-dvi.o
>> diff --git a/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c b/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
>> new file mode 100644
>> index 0000000..8065a28
>> --- /dev/null
>> +++ b/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
>> @@ -0,0 +1,343 @@
>> +/*
>> + * OPA362 analog video amplifier with output/power control
>> + *
>> + * Copyright (C) 2014 Golden Delicious Computers
>> + * Author: H. Nikolaus Schaller <hns@goldelico.com>
>> + *
>> + * based on encoder-tfp410
>> + *
>> + * Copyright (C) 2013 Texas Instruments
>> + * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + */
>> +
>> +#include <linux/gpio.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/of_gpio.h>
>> +
>> +#include <video/omapdss.h>
>> +#include <video/omap-panel-data.h>
>> +
>> +struct panel_drv_data {
>> + struct omap_dss_device dssdev;
>> + struct omap_dss_device *in;
>> +
>> + int enable_gpio;
>> +
>> + struct omap_video_timings timings;
>> +};
>> +
>> +#define to_panel_data(x) container_of(x, struct panel_drv_data, dssdev)
>> +
>> +static int opa362_connect(struct omap_dss_device *dssdev,
>> + struct omap_dss_device *dst)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> + int r;
>> +
>> + dev_dbg(dssdev->dev, "connect\n");
>> +
>> + if (omapdss_device_is_connected(dssdev))
>> + return -EBUSY;
>> +
>> + r = in->ops.atv->connect(in, dssdev);
>> + if (r)
>> + return r;
>> +
>> + dst->src = dssdev;
>> + dssdev->dst = dst;
>> +
>> + return 0;
>> +}
>> +
>> +static void opa362_disconnect(struct omap_dss_device *dssdev,
>> + struct omap_dss_device *dst)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> +
>> + dev_dbg(dssdev->dev, "disconnect\n");
>> +
>> + WARN_ON(!omapdss_device_is_connected(dssdev));
>> + if (!omapdss_device_is_connected(dssdev))
>> + return;
>> +
>> + WARN_ON(dst != dssdev->dst);
>> + if (dst != dssdev->dst)
>> + return;
>> +
>> + dst->src = NULL;
>> + dssdev->dst = NULL;
>> +
>> + in->ops.atv->disconnect(in, &ddata->dssdev);
>> +}
>> +
>> +static int opa362_enable(struct omap_dss_device *dssdev)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> + int r;
>> +
>> + dev_dbg(dssdev->dev, "enable\n");
>> +
>> + if (!omapdss_device_is_connected(dssdev))
>> + return -ENODEV;
>> +
>> + if (omapdss_device_is_enabled(dssdev))
>> + return 0;
>> +
>> + in->ops.atv->set_timings(in, &ddata->timings);
>> + /* fixme: should we receive the invert from our consumer, i.e. the connector? */
>> + in->ops.atv->invert_vid_out_polarity(in, true);
>
> Well this does seem to be broken. I don't know what the answer to the
> question above is, but the code doesn't work properly.
>
> In the opa362_invert_vid_out_polarity function below, you get the invert
> boolean from the connector. This you pass to the OMAP venc. However,
> above you always override that value in venc with true.
>
> So, either the invert_vid_out_polarity value has to be always true or
> false, because _OPA362_ requires it to be true or false, OR you need use
> the value from the connector.
>
> Seeing the comment in opa362_invert_vid_out_polarity, my guess is the
> latter, and the call above should be removed.
Yes, you are right - this is not systematic.
But the problem is that we can’t ask the connector here what it wants
to see. It might (or might not) call our opa362_invert_vid_out_polarity() later
which we can then forward to overwrite the inital state of this opa362_enable().
What it should be is something like
in->ops.atv->invert_vid_out_polarity(in, ! out->ops.atv->get_expected_vid_output_polarity())
but that does not exist and I am not sure if you want to introduce it.
So I only see the option that we always use
in->ops.atv->invert_vid_out_polarity(in, true);
and do a sanity check in opa362_invert_vid_out_polarity() to block a connector
that is requesting the wrong polarity.
>
>> +
>> + r = in->ops.atv->enable(in);
>> + if (r)
>> + return r;
>> +
>> + if (gpio_is_valid(ddata->enable_gpio))
>> + gpio_set_value_cansleep(ddata->enable_gpio, 1);
>> +
>> + dssdev->state = OMAP_DSS_DISPLAY_ACTIVE;
>> +
>> + return 0;
>> +}
>> +
>> +static void opa362_disable(struct omap_dss_device *dssdev)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> +
>> + dev_dbg(dssdev->dev, "disable\n");
>> +
>> + if (!omapdss_device_is_enabled(dssdev))
>> + return;
>> +
>> + if (gpio_is_valid(ddata->enable_gpio))
>> + gpio_set_value_cansleep(ddata->enable_gpio, 0);
>> +
>> + in->ops.atv->disable(in);
>> +
>> + dssdev->state = OMAP_DSS_DISPLAY_DISABLED;
>> +}
>> +
>> +static void opa362_set_timings(struct omap_dss_device *dssdev,
>> + struct omap_video_timings *timings)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> +
>> + dev_dbg(dssdev->dev, "set_timings\n");
>> +
>> + ddata->timings = *timings;
>> + dssdev->panel.timings = *timings;
>> +
>> + in->ops.atv->set_timings(in, timings);
>> +}
>> +
>> +static void opa362_get_timings(struct omap_dss_device *dssdev,
>> + struct omap_video_timings *timings)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> +
>> + dev_dbg(dssdev->dev, "get_timings\n");
>> +
>> + *timings = ddata->timings;
>> +}
>> +
>> +static int opa362_check_timings(struct omap_dss_device *dssdev,
>> + struct omap_video_timings *timings)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> +
>> + dev_dbg(dssdev->dev, "check_timings\n");
>> +
>> + return in->ops.atv->check_timings(in, timings);
>> +}
>> +
>> +static void opa362_set_type(struct omap_dss_device *dssdev,
>> + enum omap_dss_venc_type type)
>> +{
>> + /* we can only drive a COMPOSITE output */
>> + WARN_ON(type != OMAP_DSS_VENC_TYPE_COMPOSITE);
>> +
>> +}
>> +
>> +static void opa362_invert_vid_out_polarity(struct omap_dss_device *dssdev,
>> + bool invert_polarity)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> +
>> + /* OPA362 inverts the polarity */
>> + in->ops.atv->invert_vid_out_polarity(in, !invert_polarity);
>> +}
>> +
>> +static const struct omapdss_atv_ops opa362_atv_ops = {
>> + .connect = opa362_connect,
>> + .disconnect = opa362_disconnect,
>> +
>> + .enable = opa362_enable,
>> + .disable = opa362_disable,
>> +
>> + .check_timings = opa362_check_timings,
>> + .set_timings = opa362_set_timings,
>> + .get_timings = opa362_get_timings,
>> +
>> + .set_type = opa362_set_type,
>> + .invert_vid_out_polarity = opa362_invert_vid_out_polarity,
>> +};
>> +
>> +static int opa362_probe_pdata(struct platform_device *pdev)
>> +{
>> + struct panel_drv_data *ddata = platform_get_drvdata(pdev);
>> + struct amplifier_opa362_platform_data *pdata;
>> + struct omap_dss_device *dssdev, *in;
>> +
>> + pdata = dev_get_platdata(&pdev->dev);
>> +
>> + ddata->enable_gpio = pdata->enable_gpio;
>> +
>> + in = omap_dss_find_output(pdata->source);
>> + if (in = NULL) {
>> + dev_err(&pdev->dev, "Failed to find video source\n");
>> + return -ENODEV;
>> + }
>> +
>> + ddata->in = in;
>> +
>> + dssdev = &ddata->dssdev;
>> + dssdev->name = pdata->name;
>> +
>> + return 0;
>> +}
>
> We are going to support only DT boot at some point. Thus I think the
> whole platform data code should be left out.
Is there already a decision? I think it should not be done before. And it
does not harm to still have it.
>
>> +
>> +static int opa362_probe_of(struct platform_device *pdev)
>> +{
>> + struct panel_drv_data *ddata = platform_get_drvdata(pdev);
>> + struct device_node *node = pdev->dev.of_node;
>> + struct omap_dss_device *in;
>> + int gpio;
>> +
>> + gpio = of_get_gpio(node, 0);
>> +
>> + if (gpio_is_valid(gpio) || gpio = -ENOENT) {
>> + ddata->enable_gpio = gpio;
>> + } else {
>> + dev_err(&pdev->dev, "failed to parse enable gpio\n");
>> + return gpio;
>> + }
>
> You should use the new GPIO API to get full support from the DT data.
> For an example, see panel-dpi.c's enable_gpio.
Ok.
BR,
Nikolaus
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 1/5] video: omapdss: Add opa362 driver
@ 2014-11-13 16:25 ` Dr. H. Nikolaus Schaller
0 siblings, 0 replies; 40+ messages in thread
From: Dr. H. Nikolaus Schaller @ 2014-11-13 16:25 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Marek Belisko, robh+dt, pawel.moll, mark.rutland, ijc+devicetree,
galak, bcousson, tony, linux, plagnioj, grant.likely, devicetree,
linux-kernel, linux-omap, linux-arm-kernel, linux-fbdev,
gta04-owner
Hi,
Am 13.11.2014 um 12:51 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>:
> On 13/11/14 00:10, Marek Belisko wrote:
>> opa362 is amplifier for video and can be connected to the tvout pads
>> of the OMAP3. It has one gpio control for enable/disable of the output
>> (high impedance).
>>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/video/fbdev/omap2/displays-new/Kconfig | 6 +
>> drivers/video/fbdev/omap2/displays-new/Makefile | 1 +
>> .../fbdev/omap2/displays-new/amplifier-opa362.c | 343 +++++++++++++++++++++
>
> I think it would be better to rename this to encoder-opa362.c. It's not
When developing this driver we did simply rename the encoder-tfp410 file,
but thent hough that it does not fit into the „encoder“ category, because we
would expect something digital or digital to analog „encoding“ which it does not.
> encoder as such, but it falls into the same category.
But we can change it.
>
>> include/video/omap-panel-data.h | 12 +
>> 4 files changed, 362 insertions(+)
>> create mode 100644 drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
>>
>> diff --git a/drivers/video/fbdev/omap2/displays-new/Kconfig b/drivers/video/fbdev/omap2/displays-new/Kconfig
>> index e6cfc38..211b3ec 100644
>> --- a/drivers/video/fbdev/omap2/displays-new/Kconfig
>> +++ b/drivers/video/fbdev/omap2/displays-new/Kconfig
>> @@ -1,6 +1,12 @@
>> menu "OMAP Display Device Drivers (new device model)"
>> depends on OMAP2_DSS
>>
>> +config DISPLAY_AMPLIFIER_OPA362
>
> Here also use ENCODER instead.
>
>> + tristate "external analog amplifier with output disable/high-Z (e.g. OPA362)"
>> + help
>> + Driver to enable an external analog TV amplifier (e.g. OPA362)
>> + through a GPIO.
>
> The indentation above seems funny.
>
> The text looks a bit odd. So is this a driver for OPA362, or is this a
> generic driver for any similar devices? Most of the names and code makes
> me think this is a driver for OPA362, but the text above quite clearly
> gives the impression that this is a driver for any analog video amp,
> with single enable gpio.
Hm. We can imagine that there are other devices with similar functionality
and gpio but we have not tested any. So it is indeed better to describe it as
a pure OPA362 driver.
>
>> +
>> config DISPLAY_ENCODER_TFP410
>> tristate "TFP410 DPI to DVI Encoder"
>> help
>> diff --git a/drivers/video/fbdev/omap2/displays-new/Makefile b/drivers/video/fbdev/omap2/displays-new/Makefile
>> index 0323a8a..b311542 100644
>> --- a/drivers/video/fbdev/omap2/displays-new/Makefile
>> +++ b/drivers/video/fbdev/omap2/displays-new/Makefile
>> @@ -1,3 +1,4 @@
>> +obj-$(CONFIG_DISPLAY_AMPLIFIER_OPA362) += amplifier-opa362.o
>> obj-$(CONFIG_DISPLAY_ENCODER_TFP410) += encoder-tfp410.o
>> obj-$(CONFIG_DISPLAY_ENCODER_TPD12S015) += encoder-tpd12s015.o
>> obj-$(CONFIG_DISPLAY_CONNECTOR_DVI) += connector-dvi.o
>> diff --git a/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c b/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
>> new file mode 100644
>> index 0000000..8065a28
>> --- /dev/null
>> +++ b/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
>> @@ -0,0 +1,343 @@
>> +/*
>> + * OPA362 analog video amplifier with output/power control
>> + *
>> + * Copyright (C) 2014 Golden Delicious Computers
>> + * Author: H. Nikolaus Schaller <hns@goldelico.com>
>> + *
>> + * based on encoder-tfp410
>> + *
>> + * Copyright (C) 2013 Texas Instruments
>> + * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + */
>> +
>> +#include <linux/gpio.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/of_gpio.h>
>> +
>> +#include <video/omapdss.h>
>> +#include <video/omap-panel-data.h>
>> +
>> +struct panel_drv_data {
>> + struct omap_dss_device dssdev;
>> + struct omap_dss_device *in;
>> +
>> + int enable_gpio;
>> +
>> + struct omap_video_timings timings;
>> +};
>> +
>> +#define to_panel_data(x) container_of(x, struct panel_drv_data, dssdev)
>> +
>> +static int opa362_connect(struct omap_dss_device *dssdev,
>> + struct omap_dss_device *dst)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> + int r;
>> +
>> + dev_dbg(dssdev->dev, "connect\n");
>> +
>> + if (omapdss_device_is_connected(dssdev))
>> + return -EBUSY;
>> +
>> + r = in->ops.atv->connect(in, dssdev);
>> + if (r)
>> + return r;
>> +
>> + dst->src = dssdev;
>> + dssdev->dst = dst;
>> +
>> + return 0;
>> +}
>> +
>> +static void opa362_disconnect(struct omap_dss_device *dssdev,
>> + struct omap_dss_device *dst)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> +
>> + dev_dbg(dssdev->dev, "disconnect\n");
>> +
>> + WARN_ON(!omapdss_device_is_connected(dssdev));
>> + if (!omapdss_device_is_connected(dssdev))
>> + return;
>> +
>> + WARN_ON(dst != dssdev->dst);
>> + if (dst != dssdev->dst)
>> + return;
>> +
>> + dst->src = NULL;
>> + dssdev->dst = NULL;
>> +
>> + in->ops.atv->disconnect(in, &ddata->dssdev);
>> +}
>> +
>> +static int opa362_enable(struct omap_dss_device *dssdev)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> + int r;
>> +
>> + dev_dbg(dssdev->dev, "enable\n");
>> +
>> + if (!omapdss_device_is_connected(dssdev))
>> + return -ENODEV;
>> +
>> + if (omapdss_device_is_enabled(dssdev))
>> + return 0;
>> +
>> + in->ops.atv->set_timings(in, &ddata->timings);
>> + /* fixme: should we receive the invert from our consumer, i.e. the connector? */
>> + in->ops.atv->invert_vid_out_polarity(in, true);
>
> Well this does seem to be broken. I don't know what the answer to the
> question above is, but the code doesn't work properly.
>
> In the opa362_invert_vid_out_polarity function below, you get the invert
> boolean from the connector. This you pass to the OMAP venc. However,
> above you always override that value in venc with true.
>
> So, either the invert_vid_out_polarity value has to be always true or
> false, because _OPA362_ requires it to be true or false, OR you need use
> the value from the connector.
>
> Seeing the comment in opa362_invert_vid_out_polarity, my guess is the
> latter, and the call above should be removed.
Yes, you are right - this is not systematic.
But the problem is that we can’t ask the connector here what it wants
to see. It might (or might not) call our opa362_invert_vid_out_polarity() later
which we can then forward to overwrite the inital state of this opa362_enable().
What it should be is something like
in->ops.atv->invert_vid_out_polarity(in, ! out->ops.atv->get_expected_vid_output_polarity())
but that does not exist and I am not sure if you want to introduce it.
So I only see the option that we always use
in->ops.atv->invert_vid_out_polarity(in, true);
and do a sanity check in opa362_invert_vid_out_polarity() to block a connector
that is requesting the wrong polarity.
>
>> +
>> + r = in->ops.atv->enable(in);
>> + if (r)
>> + return r;
>> +
>> + if (gpio_is_valid(ddata->enable_gpio))
>> + gpio_set_value_cansleep(ddata->enable_gpio, 1);
>> +
>> + dssdev->state = OMAP_DSS_DISPLAY_ACTIVE;
>> +
>> + return 0;
>> +}
>> +
>> +static void opa362_disable(struct omap_dss_device *dssdev)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> +
>> + dev_dbg(dssdev->dev, "disable\n");
>> +
>> + if (!omapdss_device_is_enabled(dssdev))
>> + return;
>> +
>> + if (gpio_is_valid(ddata->enable_gpio))
>> + gpio_set_value_cansleep(ddata->enable_gpio, 0);
>> +
>> + in->ops.atv->disable(in);
>> +
>> + dssdev->state = OMAP_DSS_DISPLAY_DISABLED;
>> +}
>> +
>> +static void opa362_set_timings(struct omap_dss_device *dssdev,
>> + struct omap_video_timings *timings)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> +
>> + dev_dbg(dssdev->dev, "set_timings\n");
>> +
>> + ddata->timings = *timings;
>> + dssdev->panel.timings = *timings;
>> +
>> + in->ops.atv->set_timings(in, timings);
>> +}
>> +
>> +static void opa362_get_timings(struct omap_dss_device *dssdev,
>> + struct omap_video_timings *timings)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> +
>> + dev_dbg(dssdev->dev, "get_timings\n");
>> +
>> + *timings = ddata->timings;
>> +}
>> +
>> +static int opa362_check_timings(struct omap_dss_device *dssdev,
>> + struct omap_video_timings *timings)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> +
>> + dev_dbg(dssdev->dev, "check_timings\n");
>> +
>> + return in->ops.atv->check_timings(in, timings);
>> +}
>> +
>> +static void opa362_set_type(struct omap_dss_device *dssdev,
>> + enum omap_dss_venc_type type)
>> +{
>> + /* we can only drive a COMPOSITE output */
>> + WARN_ON(type != OMAP_DSS_VENC_TYPE_COMPOSITE);
>> +
>> +}
>> +
>> +static void opa362_invert_vid_out_polarity(struct omap_dss_device *dssdev,
>> + bool invert_polarity)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> +
>> + /* OPA362 inverts the polarity */
>> + in->ops.atv->invert_vid_out_polarity(in, !invert_polarity);
>> +}
>> +
>> +static const struct omapdss_atv_ops opa362_atv_ops = {
>> + .connect = opa362_connect,
>> + .disconnect = opa362_disconnect,
>> +
>> + .enable = opa362_enable,
>> + .disable = opa362_disable,
>> +
>> + .check_timings = opa362_check_timings,
>> + .set_timings = opa362_set_timings,
>> + .get_timings = opa362_get_timings,
>> +
>> + .set_type = opa362_set_type,
>> + .invert_vid_out_polarity = opa362_invert_vid_out_polarity,
>> +};
>> +
>> +static int opa362_probe_pdata(struct platform_device *pdev)
>> +{
>> + struct panel_drv_data *ddata = platform_get_drvdata(pdev);
>> + struct amplifier_opa362_platform_data *pdata;
>> + struct omap_dss_device *dssdev, *in;
>> +
>> + pdata = dev_get_platdata(&pdev->dev);
>> +
>> + ddata->enable_gpio = pdata->enable_gpio;
>> +
>> + in = omap_dss_find_output(pdata->source);
>> + if (in == NULL) {
>> + dev_err(&pdev->dev, "Failed to find video source\n");
>> + return -ENODEV;
>> + }
>> +
>> + ddata->in = in;
>> +
>> + dssdev = &ddata->dssdev;
>> + dssdev->name = pdata->name;
>> +
>> + return 0;
>> +}
>
> We are going to support only DT boot at some point. Thus I think the
> whole platform data code should be left out.
Is there already a decision? I think it should not be done before. And it
does not harm to still have it.
>
>> +
>> +static int opa362_probe_of(struct platform_device *pdev)
>> +{
>> + struct panel_drv_data *ddata = platform_get_drvdata(pdev);
>> + struct device_node *node = pdev->dev.of_node;
>> + struct omap_dss_device *in;
>> + int gpio;
>> +
>> + gpio = of_get_gpio(node, 0);
>> +
>> + if (gpio_is_valid(gpio) || gpio == -ENOENT) {
>> + ddata->enable_gpio = gpio;
>> + } else {
>> + dev_err(&pdev->dev, "failed to parse enable gpio\n");
>> + return gpio;
>> + }
>
> You should use the new GPIO API to get full support from the DT data.
> For an example, see panel-dpi.c's enable_gpio.
Ok.
BR,
Nikolaus
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v2 1/5] video: omapdss: Add opa362 driver
@ 2014-11-13 16:25 ` Dr. H. Nikolaus Schaller
0 siblings, 0 replies; 40+ messages in thread
From: Dr. H. Nikolaus Schaller @ 2014-11-13 16:25 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
Am 13.11.2014 um 12:51 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>:
> On 13/11/14 00:10, Marek Belisko wrote:
>> opa362 is amplifier for video and can be connected to the tvout pads
>> of the OMAP3. It has one gpio control for enable/disable of the output
>> (high impedance).
>>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/video/fbdev/omap2/displays-new/Kconfig | 6 +
>> drivers/video/fbdev/omap2/displays-new/Makefile | 1 +
>> .../fbdev/omap2/displays-new/amplifier-opa362.c | 343 +++++++++++++++++++++
>
> I think it would be better to rename this to encoder-opa362.c. It's not
When developing this driver we did simply rename the encoder-tfp410 file,
but thent hough that it does not fit into the ?encoder? category, because we
would expect something digital or digital to analog ?encoding? which it does not.
> encoder as such, but it falls into the same category.
But we can change it.
>
>> include/video/omap-panel-data.h | 12 +
>> 4 files changed, 362 insertions(+)
>> create mode 100644 drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
>>
>> diff --git a/drivers/video/fbdev/omap2/displays-new/Kconfig b/drivers/video/fbdev/omap2/displays-new/Kconfig
>> index e6cfc38..211b3ec 100644
>> --- a/drivers/video/fbdev/omap2/displays-new/Kconfig
>> +++ b/drivers/video/fbdev/omap2/displays-new/Kconfig
>> @@ -1,6 +1,12 @@
>> menu "OMAP Display Device Drivers (new device model)"
>> depends on OMAP2_DSS
>>
>> +config DISPLAY_AMPLIFIER_OPA362
>
> Here also use ENCODER instead.
>
>> + tristate "external analog amplifier with output disable/high-Z (e.g. OPA362)"
>> + help
>> + Driver to enable an external analog TV amplifier (e.g. OPA362)
>> + through a GPIO.
>
> The indentation above seems funny.
>
> The text looks a bit odd. So is this a driver for OPA362, or is this a
> generic driver for any similar devices? Most of the names and code makes
> me think this is a driver for OPA362, but the text above quite clearly
> gives the impression that this is a driver for any analog video amp,
> with single enable gpio.
Hm. We can imagine that there are other devices with similar functionality
and gpio but we have not tested any. So it is indeed better to describe it as
a pure OPA362 driver.
>
>> +
>> config DISPLAY_ENCODER_TFP410
>> tristate "TFP410 DPI to DVI Encoder"
>> help
>> diff --git a/drivers/video/fbdev/omap2/displays-new/Makefile b/drivers/video/fbdev/omap2/displays-new/Makefile
>> index 0323a8a..b311542 100644
>> --- a/drivers/video/fbdev/omap2/displays-new/Makefile
>> +++ b/drivers/video/fbdev/omap2/displays-new/Makefile
>> @@ -1,3 +1,4 @@
>> +obj-$(CONFIG_DISPLAY_AMPLIFIER_OPA362) += amplifier-opa362.o
>> obj-$(CONFIG_DISPLAY_ENCODER_TFP410) += encoder-tfp410.o
>> obj-$(CONFIG_DISPLAY_ENCODER_TPD12S015) += encoder-tpd12s015.o
>> obj-$(CONFIG_DISPLAY_CONNECTOR_DVI) += connector-dvi.o
>> diff --git a/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c b/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
>> new file mode 100644
>> index 0000000..8065a28
>> --- /dev/null
>> +++ b/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
>> @@ -0,0 +1,343 @@
>> +/*
>> + * OPA362 analog video amplifier with output/power control
>> + *
>> + * Copyright (C) 2014 Golden Delicious Computers
>> + * Author: H. Nikolaus Schaller <hns@goldelico.com>
>> + *
>> + * based on encoder-tfp410
>> + *
>> + * Copyright (C) 2013 Texas Instruments
>> + * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + */
>> +
>> +#include <linux/gpio.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/of_gpio.h>
>> +
>> +#include <video/omapdss.h>
>> +#include <video/omap-panel-data.h>
>> +
>> +struct panel_drv_data {
>> + struct omap_dss_device dssdev;
>> + struct omap_dss_device *in;
>> +
>> + int enable_gpio;
>> +
>> + struct omap_video_timings timings;
>> +};
>> +
>> +#define to_panel_data(x) container_of(x, struct panel_drv_data, dssdev)
>> +
>> +static int opa362_connect(struct omap_dss_device *dssdev,
>> + struct omap_dss_device *dst)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> + int r;
>> +
>> + dev_dbg(dssdev->dev, "connect\n");
>> +
>> + if (omapdss_device_is_connected(dssdev))
>> + return -EBUSY;
>> +
>> + r = in->ops.atv->connect(in, dssdev);
>> + if (r)
>> + return r;
>> +
>> + dst->src = dssdev;
>> + dssdev->dst = dst;
>> +
>> + return 0;
>> +}
>> +
>> +static void opa362_disconnect(struct omap_dss_device *dssdev,
>> + struct omap_dss_device *dst)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> +
>> + dev_dbg(dssdev->dev, "disconnect\n");
>> +
>> + WARN_ON(!omapdss_device_is_connected(dssdev));
>> + if (!omapdss_device_is_connected(dssdev))
>> + return;
>> +
>> + WARN_ON(dst != dssdev->dst);
>> + if (dst != dssdev->dst)
>> + return;
>> +
>> + dst->src = NULL;
>> + dssdev->dst = NULL;
>> +
>> + in->ops.atv->disconnect(in, &ddata->dssdev);
>> +}
>> +
>> +static int opa362_enable(struct omap_dss_device *dssdev)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> + int r;
>> +
>> + dev_dbg(dssdev->dev, "enable\n");
>> +
>> + if (!omapdss_device_is_connected(dssdev))
>> + return -ENODEV;
>> +
>> + if (omapdss_device_is_enabled(dssdev))
>> + return 0;
>> +
>> + in->ops.atv->set_timings(in, &ddata->timings);
>> + /* fixme: should we receive the invert from our consumer, i.e. the connector? */
>> + in->ops.atv->invert_vid_out_polarity(in, true);
>
> Well this does seem to be broken. I don't know what the answer to the
> question above is, but the code doesn't work properly.
>
> In the opa362_invert_vid_out_polarity function below, you get the invert
> boolean from the connector. This you pass to the OMAP venc. However,
> above you always override that value in venc with true.
>
> So, either the invert_vid_out_polarity value has to be always true or
> false, because _OPA362_ requires it to be true or false, OR you need use
> the value from the connector.
>
> Seeing the comment in opa362_invert_vid_out_polarity, my guess is the
> latter, and the call above should be removed.
Yes, you are right - this is not systematic.
But the problem is that we can?t ask the connector here what it wants
to see. It might (or might not) call our opa362_invert_vid_out_polarity() later
which we can then forward to overwrite the inital state of this opa362_enable().
What it should be is something like
in->ops.atv->invert_vid_out_polarity(in, ! out->ops.atv->get_expected_vid_output_polarity())
but that does not exist and I am not sure if you want to introduce it.
So I only see the option that we always use
in->ops.atv->invert_vid_out_polarity(in, true);
and do a sanity check in opa362_invert_vid_out_polarity() to block a connector
that is requesting the wrong polarity.
>
>> +
>> + r = in->ops.atv->enable(in);
>> + if (r)
>> + return r;
>> +
>> + if (gpio_is_valid(ddata->enable_gpio))
>> + gpio_set_value_cansleep(ddata->enable_gpio, 1);
>> +
>> + dssdev->state = OMAP_DSS_DISPLAY_ACTIVE;
>> +
>> + return 0;
>> +}
>> +
>> +static void opa362_disable(struct omap_dss_device *dssdev)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> +
>> + dev_dbg(dssdev->dev, "disable\n");
>> +
>> + if (!omapdss_device_is_enabled(dssdev))
>> + return;
>> +
>> + if (gpio_is_valid(ddata->enable_gpio))
>> + gpio_set_value_cansleep(ddata->enable_gpio, 0);
>> +
>> + in->ops.atv->disable(in);
>> +
>> + dssdev->state = OMAP_DSS_DISPLAY_DISABLED;
>> +}
>> +
>> +static void opa362_set_timings(struct omap_dss_device *dssdev,
>> + struct omap_video_timings *timings)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> +
>> + dev_dbg(dssdev->dev, "set_timings\n");
>> +
>> + ddata->timings = *timings;
>> + dssdev->panel.timings = *timings;
>> +
>> + in->ops.atv->set_timings(in, timings);
>> +}
>> +
>> +static void opa362_get_timings(struct omap_dss_device *dssdev,
>> + struct omap_video_timings *timings)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> +
>> + dev_dbg(dssdev->dev, "get_timings\n");
>> +
>> + *timings = ddata->timings;
>> +}
>> +
>> +static int opa362_check_timings(struct omap_dss_device *dssdev,
>> + struct omap_video_timings *timings)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> +
>> + dev_dbg(dssdev->dev, "check_timings\n");
>> +
>> + return in->ops.atv->check_timings(in, timings);
>> +}
>> +
>> +static void opa362_set_type(struct omap_dss_device *dssdev,
>> + enum omap_dss_venc_type type)
>> +{
>> + /* we can only drive a COMPOSITE output */
>> + WARN_ON(type != OMAP_DSS_VENC_TYPE_COMPOSITE);
>> +
>> +}
>> +
>> +static void opa362_invert_vid_out_polarity(struct omap_dss_device *dssdev,
>> + bool invert_polarity)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> +
>> + /* OPA362 inverts the polarity */
>> + in->ops.atv->invert_vid_out_polarity(in, !invert_polarity);
>> +}
>> +
>> +static const struct omapdss_atv_ops opa362_atv_ops = {
>> + .connect = opa362_connect,
>> + .disconnect = opa362_disconnect,
>> +
>> + .enable = opa362_enable,
>> + .disable = opa362_disable,
>> +
>> + .check_timings = opa362_check_timings,
>> + .set_timings = opa362_set_timings,
>> + .get_timings = opa362_get_timings,
>> +
>> + .set_type = opa362_set_type,
>> + .invert_vid_out_polarity = opa362_invert_vid_out_polarity,
>> +};
>> +
>> +static int opa362_probe_pdata(struct platform_device *pdev)
>> +{
>> + struct panel_drv_data *ddata = platform_get_drvdata(pdev);
>> + struct amplifier_opa362_platform_data *pdata;
>> + struct omap_dss_device *dssdev, *in;
>> +
>> + pdata = dev_get_platdata(&pdev->dev);
>> +
>> + ddata->enable_gpio = pdata->enable_gpio;
>> +
>> + in = omap_dss_find_output(pdata->source);
>> + if (in == NULL) {
>> + dev_err(&pdev->dev, "Failed to find video source\n");
>> + return -ENODEV;
>> + }
>> +
>> + ddata->in = in;
>> +
>> + dssdev = &ddata->dssdev;
>> + dssdev->name = pdata->name;
>> +
>> + return 0;
>> +}
>
> We are going to support only DT boot at some point. Thus I think the
> whole platform data code should be left out.
Is there already a decision? I think it should not be done before. And it
does not harm to still have it.
>
>> +
>> +static int opa362_probe_of(struct platform_device *pdev)
>> +{
>> + struct panel_drv_data *ddata = platform_get_drvdata(pdev);
>> + struct device_node *node = pdev->dev.of_node;
>> + struct omap_dss_device *in;
>> + int gpio;
>> +
>> + gpio = of_get_gpio(node, 0);
>> +
>> + if (gpio_is_valid(gpio) || gpio == -ENOENT) {
>> + ddata->enable_gpio = gpio;
>> + } else {
>> + dev_err(&pdev->dev, "failed to parse enable gpio\n");
>> + return gpio;
>> + }
>
> You should use the new GPIO API to get full support from the DT data.
> For an example, see panel-dpi.c's enable_gpio.
Ok.
BR,
Nikolaus
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 1/5] video: omapdss: Add opa362 driver
@ 2014-11-13 16:25 ` Dr. H. Nikolaus Schaller
0 siblings, 0 replies; 40+ messages in thread
From: Dr. H. Nikolaus Schaller @ 2014-11-13 16:25 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Marek Belisko, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
pawel.moll-5wv7dgnIgG8, mark.rutland-5wv7dgnIgG8,
ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
galak-sgV2jX0FEOL9JmXXK+q4OQ, bcousson-rdvid1DuHRBWk0Htik3J/w,
tony-4v6yS6AI5VpBDgjK7y7TUQ, linux-lFZ/pmaqli7XmaaqVzeoHQ,
plagnioj-sclMFOaUSTBWk0Htik3J/w,
grant.likely-QSEj5FYQhm4dnm+yROfE0A,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-omap-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
gta04-owner-xXXSsgcRVICgSpxsJD1C4w
Hi,
Am 13.11.2014 um 12:51 schrieb Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org>:
> On 13/11/14 00:10, Marek Belisko wrote:
>> opa362 is amplifier for video and can be connected to the tvout pads
>> of the OMAP3. It has one gpio control for enable/disable of the output
>> (high impedance).
>>
>> Signed-off-by: H. Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
>> ---
>> drivers/video/fbdev/omap2/displays-new/Kconfig | 6 +
>> drivers/video/fbdev/omap2/displays-new/Makefile | 1 +
>> .../fbdev/omap2/displays-new/amplifier-opa362.c | 343 +++++++++++++++++++++
>
> I think it would be better to rename this to encoder-opa362.c. It's not
When developing this driver we did simply rename the encoder-tfp410 file,
but thent hough that it does not fit into the „encoder“ category, because we
would expect something digital or digital to analog „encoding“ which it does not.
> encoder as such, but it falls into the same category.
But we can change it.
>
>> include/video/omap-panel-data.h | 12 +
>> 4 files changed, 362 insertions(+)
>> create mode 100644 drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
>>
>> diff --git a/drivers/video/fbdev/omap2/displays-new/Kconfig b/drivers/video/fbdev/omap2/displays-new/Kconfig
>> index e6cfc38..211b3ec 100644
>> --- a/drivers/video/fbdev/omap2/displays-new/Kconfig
>> +++ b/drivers/video/fbdev/omap2/displays-new/Kconfig
>> @@ -1,6 +1,12 @@
>> menu "OMAP Display Device Drivers (new device model)"
>> depends on OMAP2_DSS
>>
>> +config DISPLAY_AMPLIFIER_OPA362
>
> Here also use ENCODER instead.
>
>> + tristate "external analog amplifier with output disable/high-Z (e.g. OPA362)"
>> + help
>> + Driver to enable an external analog TV amplifier (e.g. OPA362)
>> + through a GPIO.
>
> The indentation above seems funny.
>
> The text looks a bit odd. So is this a driver for OPA362, or is this a
> generic driver for any similar devices? Most of the names and code makes
> me think this is a driver for OPA362, but the text above quite clearly
> gives the impression that this is a driver for any analog video amp,
> with single enable gpio.
Hm. We can imagine that there are other devices with similar functionality
and gpio but we have not tested any. So it is indeed better to describe it as
a pure OPA362 driver.
>
>> +
>> config DISPLAY_ENCODER_TFP410
>> tristate "TFP410 DPI to DVI Encoder"
>> help
>> diff --git a/drivers/video/fbdev/omap2/displays-new/Makefile b/drivers/video/fbdev/omap2/displays-new/Makefile
>> index 0323a8a..b311542 100644
>> --- a/drivers/video/fbdev/omap2/displays-new/Makefile
>> +++ b/drivers/video/fbdev/omap2/displays-new/Makefile
>> @@ -1,3 +1,4 @@
>> +obj-$(CONFIG_DISPLAY_AMPLIFIER_OPA362) += amplifier-opa362.o
>> obj-$(CONFIG_DISPLAY_ENCODER_TFP410) += encoder-tfp410.o
>> obj-$(CONFIG_DISPLAY_ENCODER_TPD12S015) += encoder-tpd12s015.o
>> obj-$(CONFIG_DISPLAY_CONNECTOR_DVI) += connector-dvi.o
>> diff --git a/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c b/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
>> new file mode 100644
>> index 0000000..8065a28
>> --- /dev/null
>> +++ b/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
>> @@ -0,0 +1,343 @@
>> +/*
>> + * OPA362 analog video amplifier with output/power control
>> + *
>> + * Copyright (C) 2014 Golden Delicious Computers
>> + * Author: H. Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
>> + *
>> + * based on encoder-tfp410
>> + *
>> + * Copyright (C) 2013 Texas Instruments
>> + * Author: Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + */
>> +
>> +#include <linux/gpio.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/of_gpio.h>
>> +
>> +#include <video/omapdss.h>
>> +#include <video/omap-panel-data.h>
>> +
>> +struct panel_drv_data {
>> + struct omap_dss_device dssdev;
>> + struct omap_dss_device *in;
>> +
>> + int enable_gpio;
>> +
>> + struct omap_video_timings timings;
>> +};
>> +
>> +#define to_panel_data(x) container_of(x, struct panel_drv_data, dssdev)
>> +
>> +static int opa362_connect(struct omap_dss_device *dssdev,
>> + struct omap_dss_device *dst)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> + int r;
>> +
>> + dev_dbg(dssdev->dev, "connect\n");
>> +
>> + if (omapdss_device_is_connected(dssdev))
>> + return -EBUSY;
>> +
>> + r = in->ops.atv->connect(in, dssdev);
>> + if (r)
>> + return r;
>> +
>> + dst->src = dssdev;
>> + dssdev->dst = dst;
>> +
>> + return 0;
>> +}
>> +
>> +static void opa362_disconnect(struct omap_dss_device *dssdev,
>> + struct omap_dss_device *dst)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> +
>> + dev_dbg(dssdev->dev, "disconnect\n");
>> +
>> + WARN_ON(!omapdss_device_is_connected(dssdev));
>> + if (!omapdss_device_is_connected(dssdev))
>> + return;
>> +
>> + WARN_ON(dst != dssdev->dst);
>> + if (dst != dssdev->dst)
>> + return;
>> +
>> + dst->src = NULL;
>> + dssdev->dst = NULL;
>> +
>> + in->ops.atv->disconnect(in, &ddata->dssdev);
>> +}
>> +
>> +static int opa362_enable(struct omap_dss_device *dssdev)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> + int r;
>> +
>> + dev_dbg(dssdev->dev, "enable\n");
>> +
>> + if (!omapdss_device_is_connected(dssdev))
>> + return -ENODEV;
>> +
>> + if (omapdss_device_is_enabled(dssdev))
>> + return 0;
>> +
>> + in->ops.atv->set_timings(in, &ddata->timings);
>> + /* fixme: should we receive the invert from our consumer, i.e. the connector? */
>> + in->ops.atv->invert_vid_out_polarity(in, true);
>
> Well this does seem to be broken. I don't know what the answer to the
> question above is, but the code doesn't work properly.
>
> In the opa362_invert_vid_out_polarity function below, you get the invert
> boolean from the connector. This you pass to the OMAP venc. However,
> above you always override that value in venc with true.
>
> So, either the invert_vid_out_polarity value has to be always true or
> false, because _OPA362_ requires it to be true or false, OR you need use
> the value from the connector.
>
> Seeing the comment in opa362_invert_vid_out_polarity, my guess is the
> latter, and the call above should be removed.
Yes, you are right - this is not systematic.
But the problem is that we can’t ask the connector here what it wants
to see. It might (or might not) call our opa362_invert_vid_out_polarity() later
which we can then forward to overwrite the inital state of this opa362_enable().
What it should be is something like
in->ops.atv->invert_vid_out_polarity(in, ! out->ops.atv->get_expected_vid_output_polarity())
but that does not exist and I am not sure if you want to introduce it.
So I only see the option that we always use
in->ops.atv->invert_vid_out_polarity(in, true);
and do a sanity check in opa362_invert_vid_out_polarity() to block a connector
that is requesting the wrong polarity.
>
>> +
>> + r = in->ops.atv->enable(in);
>> + if (r)
>> + return r;
>> +
>> + if (gpio_is_valid(ddata->enable_gpio))
>> + gpio_set_value_cansleep(ddata->enable_gpio, 1);
>> +
>> + dssdev->state = OMAP_DSS_DISPLAY_ACTIVE;
>> +
>> + return 0;
>> +}
>> +
>> +static void opa362_disable(struct omap_dss_device *dssdev)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> +
>> + dev_dbg(dssdev->dev, "disable\n");
>> +
>> + if (!omapdss_device_is_enabled(dssdev))
>> + return;
>> +
>> + if (gpio_is_valid(ddata->enable_gpio))
>> + gpio_set_value_cansleep(ddata->enable_gpio, 0);
>> +
>> + in->ops.atv->disable(in);
>> +
>> + dssdev->state = OMAP_DSS_DISPLAY_DISABLED;
>> +}
>> +
>> +static void opa362_set_timings(struct omap_dss_device *dssdev,
>> + struct omap_video_timings *timings)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> +
>> + dev_dbg(dssdev->dev, "set_timings\n");
>> +
>> + ddata->timings = *timings;
>> + dssdev->panel.timings = *timings;
>> +
>> + in->ops.atv->set_timings(in, timings);
>> +}
>> +
>> +static void opa362_get_timings(struct omap_dss_device *dssdev,
>> + struct omap_video_timings *timings)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> +
>> + dev_dbg(dssdev->dev, "get_timings\n");
>> +
>> + *timings = ddata->timings;
>> +}
>> +
>> +static int opa362_check_timings(struct omap_dss_device *dssdev,
>> + struct omap_video_timings *timings)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> +
>> + dev_dbg(dssdev->dev, "check_timings\n");
>> +
>> + return in->ops.atv->check_timings(in, timings);
>> +}
>> +
>> +static void opa362_set_type(struct omap_dss_device *dssdev,
>> + enum omap_dss_venc_type type)
>> +{
>> + /* we can only drive a COMPOSITE output */
>> + WARN_ON(type != OMAP_DSS_VENC_TYPE_COMPOSITE);
>> +
>> +}
>> +
>> +static void opa362_invert_vid_out_polarity(struct omap_dss_device *dssdev,
>> + bool invert_polarity)
>> +{
>> + struct panel_drv_data *ddata = to_panel_data(dssdev);
>> + struct omap_dss_device *in = ddata->in;
>> +
>> + /* OPA362 inverts the polarity */
>> + in->ops.atv->invert_vid_out_polarity(in, !invert_polarity);
>> +}
>> +
>> +static const struct omapdss_atv_ops opa362_atv_ops = {
>> + .connect = opa362_connect,
>> + .disconnect = opa362_disconnect,
>> +
>> + .enable = opa362_enable,
>> + .disable = opa362_disable,
>> +
>> + .check_timings = opa362_check_timings,
>> + .set_timings = opa362_set_timings,
>> + .get_timings = opa362_get_timings,
>> +
>> + .set_type = opa362_set_type,
>> + .invert_vid_out_polarity = opa362_invert_vid_out_polarity,
>> +};
>> +
>> +static int opa362_probe_pdata(struct platform_device *pdev)
>> +{
>> + struct panel_drv_data *ddata = platform_get_drvdata(pdev);
>> + struct amplifier_opa362_platform_data *pdata;
>> + struct omap_dss_device *dssdev, *in;
>> +
>> + pdata = dev_get_platdata(&pdev->dev);
>> +
>> + ddata->enable_gpio = pdata->enable_gpio;
>> +
>> + in = omap_dss_find_output(pdata->source);
>> + if (in == NULL) {
>> + dev_err(&pdev->dev, "Failed to find video source\n");
>> + return -ENODEV;
>> + }
>> +
>> + ddata->in = in;
>> +
>> + dssdev = &ddata->dssdev;
>> + dssdev->name = pdata->name;
>> +
>> + return 0;
>> +}
>
> We are going to support only DT boot at some point. Thus I think the
> whole platform data code should be left out.
Is there already a decision? I think it should not be done before. And it
does not harm to still have it.
>
>> +
>> +static int opa362_probe_of(struct platform_device *pdev)
>> +{
>> + struct panel_drv_data *ddata = platform_get_drvdata(pdev);
>> + struct device_node *node = pdev->dev.of_node;
>> + struct omap_dss_device *in;
>> + int gpio;
>> +
>> + gpio = of_get_gpio(node, 0);
>> +
>> + if (gpio_is_valid(gpio) || gpio == -ENOENT) {
>> + ddata->enable_gpio = gpio;
>> + } else {
>> + dev_err(&pdev->dev, "failed to parse enable gpio\n");
>> + return gpio;
>> + }
>
> You should use the new GPIO API to get full support from the DT data.
> For an example, see panel-dpi.c's enable_gpio.
Ok.
BR,
Nikolaus
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 1/5] video: omapdss: Add opa362 driver
@ 2014-11-13 16:41 ` Tomi Valkeinen
0 siblings, 0 replies; 40+ messages in thread
From: Tomi Valkeinen @ 2014-11-13 16:41 UTC (permalink / raw)
To: linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 3227 bytes --]
On 13/11/14 18:25, Dr. H. Nikolaus Schaller wrote:
> Hi,
>
> Am 13.11.2014 um 12:51 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>:
>
>> On 13/11/14 00:10, Marek Belisko wrote:
>>> opa362 is amplifier for video and can be connected to the tvout pads
>>> of the OMAP3. It has one gpio control for enable/disable of the output
>>> (high impedance).
>>>
>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>> ---
>>> drivers/video/fbdev/omap2/displays-new/Kconfig | 6 +
>>> drivers/video/fbdev/omap2/displays-new/Makefile | 1 +
>>> .../fbdev/omap2/displays-new/amplifier-opa362.c | 343 +++++++++++++++++++++
>>
>> I think it would be better to rename this to encoder-opa362.c. It's not
>
> When developing this driver we did simply rename the encoder-tfp410 file,
> but thent hough that it does not fit into the „encoder“ category, because we
> would expect something digital or digital to analog „encoding“ which it does not.
That is true, but we already have other "encoders" like
encoder-tpd12s015.c, which also do not encode. "encoder" in this context
means something that takes a video input, and has a video output. In
contrast to a panel or a connector.
>>> +
>>> + in->ops.atv->set_timings(in, &ddata->timings);
>>> + /* fixme: should we receive the invert from our consumer, i.e. the connector? */
>>> + in->ops.atv->invert_vid_out_polarity(in, true);
>>
>> Well this does seem to be broken. I don't know what the answer to the
>> question above is, but the code doesn't work properly.
>>
>> In the opa362_invert_vid_out_polarity function below, you get the invert
>> boolean from the connector. This you pass to the OMAP venc. However,
>> above you always override that value in venc with true.
>>
>> So, either the invert_vid_out_polarity value has to be always true or
>> false, because _OPA362_ requires it to be true or false, OR you need use
>> the value from the connector.
>>
>> Seeing the comment in opa362_invert_vid_out_polarity, my guess is the
>> latter, and the call above should be removed.
>
> Yes, you are right - this is not systematic.
>
> But the problem is that we can’t ask the connector here what it wants
> to see. It might (or might not) call our opa362_invert_vid_out_polarity() later
> which we can then forward to overwrite the inital state of this opa362_enable().
You don't need to ask. The connector calls invert_vid_out_polarity
before enabling the output. You can just pass it forward inverted, as
you already do in this driver. If it doesn't, it's either a bug or you
can just rely on the value that is already programmed to venc.
>> We are going to support only DT boot at some point. Thus I think the
>> whole platform data code should be left out.
>
> Is there already a decision? I think it should not be done before. And it
> does not harm to still have it.
It's just a matter of time. I don't accept any new boards using platform
data for display, or new display drivers using platform data, because I
don't want to spend my time converting them later. And as this is a new
driver, no existing board can be using the opa362 platform_data. So we
can support DT only.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/5] video: omapdss: Add opa362 driver
@ 2014-11-13 16:41 ` Tomi Valkeinen
0 siblings, 0 replies; 40+ messages in thread
From: Tomi Valkeinen @ 2014-11-13 16:41 UTC (permalink / raw)
To: Dr. H. Nikolaus Schaller
Cc: Marek Belisko, robh+dt, pawel.moll, mark.rutland, ijc+devicetree,
galak, bcousson, tony, linux, plagnioj, grant.likely, devicetree,
linux-kernel, linux-omap, linux-arm-kernel, linux-fbdev,
gta04-owner
[-- Attachment #1: Type: text/plain, Size: 3227 bytes --]
On 13/11/14 18:25, Dr. H. Nikolaus Schaller wrote:
> Hi,
>
> Am 13.11.2014 um 12:51 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>:
>
>> On 13/11/14 00:10, Marek Belisko wrote:
>>> opa362 is amplifier for video and can be connected to the tvout pads
>>> of the OMAP3. It has one gpio control for enable/disable of the output
>>> (high impedance).
>>>
>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>> ---
>>> drivers/video/fbdev/omap2/displays-new/Kconfig | 6 +
>>> drivers/video/fbdev/omap2/displays-new/Makefile | 1 +
>>> .../fbdev/omap2/displays-new/amplifier-opa362.c | 343 +++++++++++++++++++++
>>
>> I think it would be better to rename this to encoder-opa362.c. It's not
>
> When developing this driver we did simply rename the encoder-tfp410 file,
> but thent hough that it does not fit into the „encoder“ category, because we
> would expect something digital or digital to analog „encoding“ which it does not.
That is true, but we already have other "encoders" like
encoder-tpd12s015.c, which also do not encode. "encoder" in this context
means something that takes a video input, and has a video output. In
contrast to a panel or a connector.
>>> +
>>> + in->ops.atv->set_timings(in, &ddata->timings);
>>> + /* fixme: should we receive the invert from our consumer, i.e. the connector? */
>>> + in->ops.atv->invert_vid_out_polarity(in, true);
>>
>> Well this does seem to be broken. I don't know what the answer to the
>> question above is, but the code doesn't work properly.
>>
>> In the opa362_invert_vid_out_polarity function below, you get the invert
>> boolean from the connector. This you pass to the OMAP venc. However,
>> above you always override that value in venc with true.
>>
>> So, either the invert_vid_out_polarity value has to be always true or
>> false, because _OPA362_ requires it to be true or false, OR you need use
>> the value from the connector.
>>
>> Seeing the comment in opa362_invert_vid_out_polarity, my guess is the
>> latter, and the call above should be removed.
>
> Yes, you are right - this is not systematic.
>
> But the problem is that we can’t ask the connector here what it wants
> to see. It might (or might not) call our opa362_invert_vid_out_polarity() later
> which we can then forward to overwrite the inital state of this opa362_enable().
You don't need to ask. The connector calls invert_vid_out_polarity
before enabling the output. You can just pass it forward inverted, as
you already do in this driver. If it doesn't, it's either a bug or you
can just rely on the value that is already programmed to venc.
>> We are going to support only DT boot at some point. Thus I think the
>> whole platform data code should be left out.
>
> Is there already a decision? I think it should not be done before. And it
> does not harm to still have it.
It's just a matter of time. I don't accept any new boards using platform
data for display, or new display drivers using platform data, because I
don't want to spend my time converting them later. And as this is a new
driver, no existing board can be using the opa362 platform_data. So we
can support DT only.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 1/5] video: omapdss: Add opa362 driver
@ 2014-11-13 16:41 ` Tomi Valkeinen
0 siblings, 0 replies; 40+ messages in thread
From: Tomi Valkeinen @ 2014-11-13 16:41 UTC (permalink / raw)
To: linux-arm-kernel
On 13/11/14 18:25, Dr. H. Nikolaus Schaller wrote:
> Hi,
>
> Am 13.11.2014 um 12:51 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>:
>
>> On 13/11/14 00:10, Marek Belisko wrote:
>>> opa362 is amplifier for video and can be connected to the tvout pads
>>> of the OMAP3. It has one gpio control for enable/disable of the output
>>> (high impedance).
>>>
>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>> ---
>>> drivers/video/fbdev/omap2/displays-new/Kconfig | 6 +
>>> drivers/video/fbdev/omap2/displays-new/Makefile | 1 +
>>> .../fbdev/omap2/displays-new/amplifier-opa362.c | 343 +++++++++++++++++++++
>>
>> I think it would be better to rename this to encoder-opa362.c. It's not
>
> When developing this driver we did simply rename the encoder-tfp410 file,
> but thent hough that it does not fit into the ?encoder? category, because we
> would expect something digital or digital to analog ?encoding? which it does not.
That is true, but we already have other "encoders" like
encoder-tpd12s015.c, which also do not encode. "encoder" in this context
means something that takes a video input, and has a video output. In
contrast to a panel or a connector.
>>> +
>>> + in->ops.atv->set_timings(in, &ddata->timings);
>>> + /* fixme: should we receive the invert from our consumer, i.e. the connector? */
>>> + in->ops.atv->invert_vid_out_polarity(in, true);
>>
>> Well this does seem to be broken. I don't know what the answer to the
>> question above is, but the code doesn't work properly.
>>
>> In the opa362_invert_vid_out_polarity function below, you get the invert
>> boolean from the connector. This you pass to the OMAP venc. However,
>> above you always override that value in venc with true.
>>
>> So, either the invert_vid_out_polarity value has to be always true or
>> false, because _OPA362_ requires it to be true or false, OR you need use
>> the value from the connector.
>>
>> Seeing the comment in opa362_invert_vid_out_polarity, my guess is the
>> latter, and the call above should be removed.
>
> Yes, you are right - this is not systematic.
>
> But the problem is that we can?t ask the connector here what it wants
> to see. It might (or might not) call our opa362_invert_vid_out_polarity() later
> which we can then forward to overwrite the inital state of this opa362_enable().
You don't need to ask. The connector calls invert_vid_out_polarity
before enabling the output. You can just pass it forward inverted, as
you already do in this driver. If it doesn't, it's either a bug or you
can just rely on the value that is already programmed to venc.
>> We are going to support only DT boot at some point. Thus I think the
>> whole platform data code should be left out.
>
> Is there already a decision? I think it should not be done before. And it
> does not harm to still have it.
It's just a matter of time. I don't accept any new boards using platform
data for display, or new display drivers using platform data, because I
don't want to spend my time converting them later. And as this is a new
driver, no existing board can be using the opa362 platform_data. So we
can support DT only.
Tomi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141113/26f87c09/attachment.sig>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/5] video: omapdss: Add opa362 driver
@ 2014-11-13 16:41 ` Tomi Valkeinen
0 siblings, 0 replies; 40+ messages in thread
From: Tomi Valkeinen @ 2014-11-13 16:41 UTC (permalink / raw)
To: Dr. H. Nikolaus Schaller
Cc: Marek Belisko, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
pawel.moll-5wv7dgnIgG8, mark.rutland-5wv7dgnIgG8,
ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
galak-sgV2jX0FEOL9JmXXK+q4OQ, bcousson-rdvid1DuHRBWk0Htik3J/w,
tony-4v6yS6AI5VpBDgjK7y7TUQ, linux-lFZ/pmaqli7XmaaqVzeoHQ,
plagnioj-sclMFOaUSTBWk0Htik3J/w,
grant.likely-QSEj5FYQhm4dnm+yROfE0A,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-omap-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
gta04-owner-xXXSsgcRVICgSpxsJD1C4w
[-- Attachment #1: Type: text/plain, Size: 3275 bytes --]
On 13/11/14 18:25, Dr. H. Nikolaus Schaller wrote:
> Hi,
>
> Am 13.11.2014 um 12:51 schrieb Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org>:
>
>> On 13/11/14 00:10, Marek Belisko wrote:
>>> opa362 is amplifier for video and can be connected to the tvout pads
>>> of the OMAP3. It has one gpio control for enable/disable of the output
>>> (high impedance).
>>>
>>> Signed-off-by: H. Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
>>> ---
>>> drivers/video/fbdev/omap2/displays-new/Kconfig | 6 +
>>> drivers/video/fbdev/omap2/displays-new/Makefile | 1 +
>>> .../fbdev/omap2/displays-new/amplifier-opa362.c | 343 +++++++++++++++++++++
>>
>> I think it would be better to rename this to encoder-opa362.c. It's not
>
> When developing this driver we did simply rename the encoder-tfp410 file,
> but thent hough that it does not fit into the „encoder“ category, because we
> would expect something digital or digital to analog „encoding“ which it does not.
That is true, but we already have other "encoders" like
encoder-tpd12s015.c, which also do not encode. "encoder" in this context
means something that takes a video input, and has a video output. In
contrast to a panel or a connector.
>>> +
>>> + in->ops.atv->set_timings(in, &ddata->timings);
>>> + /* fixme: should we receive the invert from our consumer, i.e. the connector? */
>>> + in->ops.atv->invert_vid_out_polarity(in, true);
>>
>> Well this does seem to be broken. I don't know what the answer to the
>> question above is, but the code doesn't work properly.
>>
>> In the opa362_invert_vid_out_polarity function below, you get the invert
>> boolean from the connector. This you pass to the OMAP venc. However,
>> above you always override that value in venc with true.
>>
>> So, either the invert_vid_out_polarity value has to be always true or
>> false, because _OPA362_ requires it to be true or false, OR you need use
>> the value from the connector.
>>
>> Seeing the comment in opa362_invert_vid_out_polarity, my guess is the
>> latter, and the call above should be removed.
>
> Yes, you are right - this is not systematic.
>
> But the problem is that we can’t ask the connector here what it wants
> to see. It might (or might not) call our opa362_invert_vid_out_polarity() later
> which we can then forward to overwrite the inital state of this opa362_enable().
You don't need to ask. The connector calls invert_vid_out_polarity
before enabling the output. You can just pass it forward inverted, as
you already do in this driver. If it doesn't, it's either a bug or you
can just rely on the value that is already programmed to venc.
>> We are going to support only DT boot at some point. Thus I think the
>> whole platform data code should be left out.
>
> Is there already a decision? I think it should not be done before. And it
> does not harm to still have it.
It's just a matter of time. I don't accept any new boards using platform
data for display, or new display drivers using platform data, because I
don't want to spend my time converting them later. And as this is a new
driver, no existing board can be using the opa362 platform_data. So we
can support DT only.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/5] video: omapdss: Add opa362 driver
2014-11-13 16:41 ` Tomi Valkeinen
(?)
@ 2014-11-19 15:10 ` Dr. H. Nikolaus Schaller
-1 siblings, 0 replies; 40+ messages in thread
From: Dr. H. Nikolaus Schaller @ 2014-11-19 15:10 UTC (permalink / raw)
To: linux-arm-kernel
Am 13.11.2014 um 17:41 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>:
> On 13/11/14 18:25, Dr. H. Nikolaus Schaller wrote:
>> Hi,
>>
>> Am 13.11.2014 um 12:51 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>:
>>
>>> On 13/11/14 00:10, Marek Belisko wrote:
>>>> opa362 is amplifier for video and can be connected to the tvout pads
>>>> of the OMAP3. It has one gpio control for enable/disable of the output
>>>> (high impedance).
>>>>
>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>> ---
>>>> drivers/video/fbdev/omap2/displays-new/Kconfig | 6 +
>>>> drivers/video/fbdev/omap2/displays-new/Makefile | 1 +
>>>> .../fbdev/omap2/displays-new/amplifier-opa362.c | 343 +++++++++++++++++++++
>>>
>>> I think it would be better to rename this to encoder-opa362.c. It's not
>>
>> When developing this driver we did simply rename the encoder-tfp410 file,
>> but thent hough that it does not fit into the „encoder“ category, because we
>> would expect something digital or digital to analog „encoding“ which it does not.
>
> That is true, but we already have other "encoders" like
> encoder-tpd12s015.c, which also do not encode. "encoder" in this context
> means something that takes a video input, and has a video output. In
> contrast to a panel or a connector.
>
>>>> +
>>>> + in->ops.atv->set_timings(in, &ddata->timings);
>>>> + /* fixme: should we receive the invert from our consumer, i.e. the connector? */
>>>> + in->ops.atv->invert_vid_out_polarity(in, true);
>>>
>>> Well this does seem to be broken. I don't know what the answer to the
>>> question above is, but the code doesn't work properly.
>>>
>>> In the opa362_invert_vid_out_polarity function below, you get the invert
>>> boolean from the connector. This you pass to the OMAP venc. However,
>>> above you always override that value in venc with true.
>>>
>>> So, either the invert_vid_out_polarity value has to be always true or
>>> false, because _OPA362_ requires it to be true or false, OR you need use
>>> the value from the connector.
>>>
>>> Seeing the comment in opa362_invert_vid_out_polarity, my guess is the
>>> latter, and the call above should be removed.
>>
>> Yes, you are right - this is not systematic.
>>
>> But the problem is that we can’t ask the connector here what it wants
>> to see. It might (or might not) call our opa362_invert_vid_out_polarity() later
>> which we can then forward to overwrite the inital state of this opa362_enable().
>
> You don't need to ask. The connector calls invert_vid_out_polarity
> before enabling the output.
Unfortunately it doesn’t. At least not always.
It does only for pdata systems but not for DT based systems:
if (!ddata->dev->of_node) {
in->ops.atv->set_type(in, ddata->connector_type);
in->ops.atv->invert_vid_out_polarity(in,
ddata->invert_polarity);
}
Not calling is in our case different from calling with ddata->invert_polarity = 0.
> You can just pass it forward inverted, as
> you already do in this driver. If it doesn't, it's either a bug or you
> can just rely on the value that is already programmed to venc.
Therefore it is not called with “false” which would make our invert_vid_out_polarity
invert it and send “true” towards the VENC. So VENC remains non-inverted.
We will also add a patch for the connector-analog.c
>>> We are going to support only DT boot at some point. Thus I think the
>>> whole platform data code should be left out.
>>
>> Is there already a decision? I think it should not be done before. And it
>> does not harm to still have it.
>
> It's just a matter of time. I don't accept any new boards using platform
> data for display, or new display drivers using platform data, because I
> don't want to spend my time converting them later. And as this is a new
> driver, no existing board can be using the opa362 platform_data. So we
> can support DT only.
Ok, that looks reasonable - as long as we can rely on that all mainline DSS
drivers are already fully converted to DT :)
BR,
Nikolaus
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v2 1/5] video: omapdss: Add opa362 driver
@ 2014-11-19 15:10 ` Dr. H. Nikolaus Schaller
0 siblings, 0 replies; 40+ messages in thread
From: Dr. H. Nikolaus Schaller @ 2014-11-19 15:10 UTC (permalink / raw)
To: linux-arm-kernel
Am 13.11.2014 um 17:41 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>:
> On 13/11/14 18:25, Dr. H. Nikolaus Schaller wrote:
>> Hi,
>>
>> Am 13.11.2014 um 12:51 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>:
>>
>>> On 13/11/14 00:10, Marek Belisko wrote:
>>>> opa362 is amplifier for video and can be connected to the tvout pads
>>>> of the OMAP3. It has one gpio control for enable/disable of the output
>>>> (high impedance).
>>>>
>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>> ---
>>>> drivers/video/fbdev/omap2/displays-new/Kconfig | 6 +
>>>> drivers/video/fbdev/omap2/displays-new/Makefile | 1 +
>>>> .../fbdev/omap2/displays-new/amplifier-opa362.c | 343 +++++++++++++++++++++
>>>
>>> I think it would be better to rename this to encoder-opa362.c. It's not
>>
>> When developing this driver we did simply rename the encoder-tfp410 file,
>> but thent hough that it does not fit into the ?encoder? category, because we
>> would expect something digital or digital to analog ?encoding? which it does not.
>
> That is true, but we already have other "encoders" like
> encoder-tpd12s015.c, which also do not encode. "encoder" in this context
> means something that takes a video input, and has a video output. In
> contrast to a panel or a connector.
>
>>>> +
>>>> + in->ops.atv->set_timings(in, &ddata->timings);
>>>> + /* fixme: should we receive the invert from our consumer, i.e. the connector? */
>>>> + in->ops.atv->invert_vid_out_polarity(in, true);
>>>
>>> Well this does seem to be broken. I don't know what the answer to the
>>> question above is, but the code doesn't work properly.
>>>
>>> In the opa362_invert_vid_out_polarity function below, you get the invert
>>> boolean from the connector. This you pass to the OMAP venc. However,
>>> above you always override that value in venc with true.
>>>
>>> So, either the invert_vid_out_polarity value has to be always true or
>>> false, because _OPA362_ requires it to be true or false, OR you need use
>>> the value from the connector.
>>>
>>> Seeing the comment in opa362_invert_vid_out_polarity, my guess is the
>>> latter, and the call above should be removed.
>>
>> Yes, you are right - this is not systematic.
>>
>> But the problem is that we can?t ask the connector here what it wants
>> to see. It might (or might not) call our opa362_invert_vid_out_polarity() later
>> which we can then forward to overwrite the inital state of this opa362_enable().
>
> You don't need to ask. The connector calls invert_vid_out_polarity
> before enabling the output.
Unfortunately it doesn?t. At least not always.
It does only for pdata systems but not for DT based systems:
if (!ddata->dev->of_node) {
in->ops.atv->set_type(in, ddata->connector_type);
in->ops.atv->invert_vid_out_polarity(in,
ddata->invert_polarity);
}
Not calling is in our case different from calling with ddata->invert_polarity == 0.
> You can just pass it forward inverted, as
> you already do in this driver. If it doesn't, it's either a bug or you
> can just rely on the value that is already programmed to venc.
Therefore it is not called with ?false? which would make our invert_vid_out_polarity
invert it and send ?true? towards the VENC. So VENC remains non-inverted.
We will also add a patch for the connector-analog.c
>>> We are going to support only DT boot at some point. Thus I think the
>>> whole platform data code should be left out.
>>
>> Is there already a decision? I think it should not be done before. And it
>> does not harm to still have it.
>
> It's just a matter of time. I don't accept any new boards using platform
> data for display, or new display drivers using platform data, because I
> don't want to spend my time converting them later. And as this is a new
> driver, no existing board can be using the opa362 platform_data. So we
> can support DT only.
Ok, that looks reasonable - as long as we can rely on that all mainline DSS
drivers are already fully converted to DT :)
BR,
Nikolaus
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 1/5] video: omapdss: Add opa362 driver
@ 2014-11-19 15:10 ` Dr. H. Nikolaus Schaller
0 siblings, 0 replies; 40+ messages in thread
From: Dr. H. Nikolaus Schaller @ 2014-11-19 15:10 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Marek Belisko, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Benoit Cousson, Tony Lindgren,
Russell King - ARM Linux, Jean-Christophe PLAGNIOL-VILLARD,
grant.likely, devicetree, LKML, linux-omap, linux-arm-kernel,
linux-fbdev, gta04-owner
Am 13.11.2014 um 17:41 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>:
> On 13/11/14 18:25, Dr. H. Nikolaus Schaller wrote:
>> Hi,
>>
>> Am 13.11.2014 um 12:51 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>:
>>
>>> On 13/11/14 00:10, Marek Belisko wrote:
>>>> opa362 is amplifier for video and can be connected to the tvout pads
>>>> of the OMAP3. It has one gpio control for enable/disable of the output
>>>> (high impedance).
>>>>
>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>> ---
>>>> drivers/video/fbdev/omap2/displays-new/Kconfig | 6 +
>>>> drivers/video/fbdev/omap2/displays-new/Makefile | 1 +
>>>> .../fbdev/omap2/displays-new/amplifier-opa362.c | 343 +++++++++++++++++++++
>>>
>>> I think it would be better to rename this to encoder-opa362.c. It's not
>>
>> When developing this driver we did simply rename the encoder-tfp410 file,
>> but thent hough that it does not fit into the „encoder“ category, because we
>> would expect something digital or digital to analog „encoding“ which it does not.
>
> That is true, but we already have other "encoders" like
> encoder-tpd12s015.c, which also do not encode. "encoder" in this context
> means something that takes a video input, and has a video output. In
> contrast to a panel or a connector.
>
>>>> +
>>>> + in->ops.atv->set_timings(in, &ddata->timings);
>>>> + /* fixme: should we receive the invert from our consumer, i.e. the connector? */
>>>> + in->ops.atv->invert_vid_out_polarity(in, true);
>>>
>>> Well this does seem to be broken. I don't know what the answer to the
>>> question above is, but the code doesn't work properly.
>>>
>>> In the opa362_invert_vid_out_polarity function below, you get the invert
>>> boolean from the connector. This you pass to the OMAP venc. However,
>>> above you always override that value in venc with true.
>>>
>>> So, either the invert_vid_out_polarity value has to be always true or
>>> false, because _OPA362_ requires it to be true or false, OR you need use
>>> the value from the connector.
>>>
>>> Seeing the comment in opa362_invert_vid_out_polarity, my guess is the
>>> latter, and the call above should be removed.
>>
>> Yes, you are right - this is not systematic.
>>
>> But the problem is that we can’t ask the connector here what it wants
>> to see. It might (or might not) call our opa362_invert_vid_out_polarity() later
>> which we can then forward to overwrite the inital state of this opa362_enable().
>
> You don't need to ask. The connector calls invert_vid_out_polarity
> before enabling the output.
Unfortunately it doesn’t. At least not always.
It does only for pdata systems but not for DT based systems:
if (!ddata->dev->of_node) {
in->ops.atv->set_type(in, ddata->connector_type);
in->ops.atv->invert_vid_out_polarity(in,
ddata->invert_polarity);
}
Not calling is in our case different from calling with ddata->invert_polarity == 0.
> You can just pass it forward inverted, as
> you already do in this driver. If it doesn't, it's either a bug or you
> can just rely on the value that is already programmed to venc.
Therefore it is not called with “false” which would make our invert_vid_out_polarity
invert it and send “true” towards the VENC. So VENC remains non-inverted.
We will also add a patch for the connector-analog.c
>>> We are going to support only DT boot at some point. Thus I think the
>>> whole platform data code should be left out.
>>
>> Is there already a decision? I think it should not be done before. And it
>> does not harm to still have it.
>
> It's just a matter of time. I don't accept any new boards using platform
> data for display, or new display drivers using platform data, because I
> don't want to spend my time converting them later. And as this is a new
> driver, no existing board can be using the opa362 platform_data. So we
> can support DT only.
Ok, that looks reasonable - as long as we can rely on that all mainline DSS
drivers are already fully converted to DT :)
BR,
Nikolaus
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 1/5] video: omapdss: Add opa362 driver
2014-11-19 15:10 ` Dr. H. Nikolaus Schaller
(?)
(?)
@ 2014-11-24 11:47 ` Tomi Valkeinen
-1 siblings, 0 replies; 40+ messages in thread
From: Tomi Valkeinen @ 2014-11-24 11:47 UTC (permalink / raw)
To: linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 838 bytes --]
On 19/11/14 17:10, Dr. H. Nikolaus Schaller wrote:
>> You don't need to ask. The connector calls invert_vid_out_polarity
>> before enabling the output.
>
> Unfortunately it doesn’t. At least not always.
>
> It does only for pdata systems but not for DT based systems:
>
> if (!ddata->dev->of_node) {
> in->ops.atv->set_type(in, ddata->connector_type);
> in->ops.atv->invert_vid_out_polarity(in,
> ddata->invert_polarity);
> }
>
> Not calling is in our case different from calling with ddata->invert_polarity == 0.
Ah, sorry, my mistake. I should've read the code more carefully =).
So, with DT only approach, those calls above are not supported. If you
make the OPA driver DT only, you can remove those functions.
You only need to set the invert-polarity property in the venc DT node.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 1/5] video: omapdss: Add opa362 driver
@ 2014-11-24 11:47 ` Tomi Valkeinen
0 siblings, 0 replies; 40+ messages in thread
From: Tomi Valkeinen @ 2014-11-24 11:47 UTC (permalink / raw)
To: Dr. H. Nikolaus Schaller
Cc: Marek Belisko, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Benoit Cousson, Tony Lindgren,
Russell King - ARM Linux, Jean-Christophe PLAGNIOL-VILLARD,
grant.likely, devicetree, LKML, linux-omap, linux-arm-kernel,
linux-fbdev, gta04-owner
[-- Attachment #1: Type: text/plain, Size: 838 bytes --]
On 19/11/14 17:10, Dr. H. Nikolaus Schaller wrote:
>> You don't need to ask. The connector calls invert_vid_out_polarity
>> before enabling the output.
>
> Unfortunately it doesn’t. At least not always.
>
> It does only for pdata systems but not for DT based systems:
>
> if (!ddata->dev->of_node) {
> in->ops.atv->set_type(in, ddata->connector_type);
> in->ops.atv->invert_vid_out_polarity(in,
> ddata->invert_polarity);
> }
>
> Not calling is in our case different from calling with ddata->invert_polarity == 0.
Ah, sorry, my mistake. I should've read the code more carefully =).
So, with DT only approach, those calls above are not supported. If you
make the OPA driver DT only, you can remove those functions.
You only need to set the invert-polarity property in the venc DT node.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v2 1/5] video: omapdss: Add opa362 driver
@ 2014-11-24 11:47 ` Tomi Valkeinen
0 siblings, 0 replies; 40+ messages in thread
From: Tomi Valkeinen @ 2014-11-24 11:47 UTC (permalink / raw)
To: linux-arm-kernel
On 19/11/14 17:10, Dr. H. Nikolaus Schaller wrote:
>> You don't need to ask. The connector calls invert_vid_out_polarity
>> before enabling the output.
>
> Unfortunately it doesn?t. At least not always.
>
> It does only for pdata systems but not for DT based systems:
>
> if (!ddata->dev->of_node) {
> in->ops.atv->set_type(in, ddata->connector_type);
> in->ops.atv->invert_vid_out_polarity(in,
> ddata->invert_polarity);
> }
>
> Not calling is in our case different from calling with ddata->invert_polarity == 0.
Ah, sorry, my mistake. I should've read the code more carefully =).
So, with DT only approach, those calls above are not supported. If you
make the OPA driver DT only, you can remove those functions.
You only need to set the invert-polarity property in the venc DT node.
Tomi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141124/6701141c/attachment.sig>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 1/5] video: omapdss: Add opa362 driver
@ 2014-11-24 11:47 ` Tomi Valkeinen
0 siblings, 0 replies; 40+ messages in thread
From: Tomi Valkeinen @ 2014-11-24 11:47 UTC (permalink / raw)
To: Dr. H. Nikolaus Schaller
Cc: Marek Belisko, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Benoit Cousson, Tony Lindgren,
Russell King - ARM Linux, Jean-Christophe PLAGNIOL-VILLARD,
grant.likely, devicetree, LKML, linux-omap, linux-arm-kernel,
linux-fbdev, gta04-owner
[-- Attachment #1: Type: text/plain, Size: 838 bytes --]
On 19/11/14 17:10, Dr. H. Nikolaus Schaller wrote:
>> You don't need to ask. The connector calls invert_vid_out_polarity
>> before enabling the output.
>
> Unfortunately it doesn’t. At least not always.
>
> It does only for pdata systems but not for DT based systems:
>
> if (!ddata->dev->of_node) {
> in->ops.atv->set_type(in, ddata->connector_type);
> in->ops.atv->invert_vid_out_polarity(in,
> ddata->invert_polarity);
> }
>
> Not calling is in our case different from calling with ddata->invert_polarity == 0.
Ah, sorry, my mistake. I should've read the code more carefully =).
So, with DT only approach, those calls above are not supported. If you
make the OPA driver DT only, you can remove those functions.
You only need to set the invert-polarity property in the venc DT node.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread