Re: [PATCH 03/11] drm: add driver for simple vga encoders

Hi Laurent,

thanks for the comments

Am Donnerstag, 26. Februar 2015, 20:33:33 schrieb Laurent Pinchart:

On Saturday 31 January 2015 17:32:56 Heiko Stuebner wrote:

...

There exist simple vga encoders without any type of management interface and just maybe a simple gpio for turning it on or off. Examples for these are the Analog Devices ADV7123, Chipsea CS7123 or Micronas SDA7123.

Add a generic encoder driver for those that can be used by drm drivers using the component framework.

The rcar-du driver already handles the adi,adv7123 compatible string used by this driver. A generic driver is in my opinion a very good idea, but we can't break the existing support. Porting the rcar-du driver to the component model is needed.

is this in someones plans already? Because I'm still having trouble understanding parts of the rockchip driver sometimes, so feel in no way qualified to try to get this tackled in some reasonable timeframe :-)

...

Signed-off-by: Heiko Stuebner heiko@sntech.de

drivers/gpu/drm/i2c/Kconfig | 6 + drivers/gpu/drm/i2c/Makefile | 2 + drivers/gpu/drm/i2c/vga-simple.c | 325 ++++++++++++++++++++++++++++++++++++

drivers/gpu/drm/i2c/ feels wrong for a platform device.

yep, i2c probably being the wrong place was one of the caveats in the cover letter and I was hoping some more seasoned drm people could suggest where this should live after all.

As your comments further down suggest that we might introduce some different components, simply congregate them in a "components" subdir or something splitting them more?

...

3 files changed, 333 insertions(+) create mode 100644 drivers/gpu/drm/i2c/vga-simple.c

diff --git a/drivers/gpu/drm/i2c/Kconfig b/drivers/gpu/drm/i2c/Kconfig index 22c7ed6..319f2cb 100644 --- a/drivers/gpu/drm/i2c/Kconfig +++ b/drivers/gpu/drm/i2c/Kconfig @@ -31,4 +31,10 @@ config DRM_I2C_NXP_TDA998X

help

 Support for NXP Semiconductors TDA998X HDMI encoders.

+config DRM_VGA_SIMPLE

• tristate "Generic simple vga encoder"
• help

•  Support for vga encoder chips without any special settings
  

•  and at most a power-control gpio.
  

endmenu

diff --git a/drivers/gpu/drm/i2c/Makefile b/drivers/gpu/drm/i2c/Makefile index 2c72eb5..858961f 100644 --- a/drivers/gpu/drm/i2c/Makefile +++ b/drivers/gpu/drm/i2c/Makefile @@ -10,3 +10,5 @@ obj-$(CONFIG_DRM_I2C_SIL164) += sil164.o

tda998x-y := tda998x_drv.o obj-$(CONFIG_DRM_I2C_NXP_TDA998X) += tda998x.o

+obj-$(CONFIG_DRM_VGA_SIMPLE) += vga-simple.o diff --git a/drivers/gpu/drm/i2c/vga-simple.c b/drivers/gpu/drm/i2c/vga-simple.c new file mode 100644 index 0000000..bb9d19c --- /dev/null +++ b/drivers/gpu/drm/i2c/vga-simple.c @@ -0,0 +1,325 @@ +/*

• • Simple vga encoder driver
• • Copyright (C) 2014 Heiko Stuebner heiko@sntech.de
• • This software is licensed under the terms of the GNU General Public
• • License version 2, as published by the Free Software Foundation, and
• • may be copied, distributed, and modified under those terms.
• • This program is distributed in the hope that it will be useful,
• • but WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
• • GNU General Public License for more details.
• */

+#include <linux/module.h> +#include <linux/component.h> +#include <linux/gpio.h> +#include <linux/gpio/consumer.h> +#include <linux/i2c.h> +#include <linux/regulator/consumer.h> +#include <drm/drm_of.h> +#include <drm/drmP.h> +#include <drm/drm_crtc.h> +#include <drm/drm_crtc_helper.h> +#include <drm/drm_edid.h>

+#define connector_to_vga_simple(x) container_of(x, struct vga_simple, connector) +#define encoder_to_vga_simple(x) container_of(x, struct vga_simple, encoder) + +struct vga_simple {

• struct drm_connector connector;
• struct drm_encoder encoder;
• struct device *dev;
• struct i2c_adapter *ddc;
• struct regulator *vaa_reg;
• struct gpio_desc *enable_gpio;
• struct mutex enable_lock;
• bool enabled;

+};

+/*

• • Connector functions
• */

+enum drm_connector_status vga_simple_detect(struct drm_connector *connector, + bool force) +{

• struct vga_simple *vga = connector_to_vga_simple(connector);
• if (!vga->ddc)

• return connector_status_unknown;
  

• if (drm_probe_ddc(vga->ddc))

• return connector_status_connected;
  

• return connector_status_disconnected;

+}

+void vga_simple_connector_destroy(struct drm_connector *connector) +{

• drm_connector_unregister(connector);
• drm_connector_cleanup(connector);

+}

+struct drm_connector_funcs vga_simple_connector_funcs = {

• .dpms = drm_helper_connector_dpms,
• .fill_modes = drm_helper_probe_single_connector_modes,
• .detect = vga_simple_detect,
• .destroy = vga_simple_connector_destroy,

+};

+/*

• • Connector helper functions
• */

+static int vga_simple_connector_get_modes(struct drm_connector *connector) +{

• struct vga_simple *vga = connector_to_vga_simple(connector);
• struct edid *edid;
• int ret = 0;
• if (!vga->ddc)

• return 0;
  

• edid = drm_get_edid(connector, vga->ddc);
• if (edid) {

• drm_mode_connector_update_edid_property(connector, edid);
  

• ret = drm_add_edid_modes(connector, edid);
  

• kfree(edid);
  

• }
• return ret;

+}

+static int vga_simple_connector_mode_valid(struct drm_connector *connector, + struct drm_display_mode *mode) +{

• return MODE_OK;

+}

+static struct drm_encoder +*vga_simple_connector_best_encoder(struct drm_connector *connector) +{

• struct vga_simple *vga = connector_to_vga_simple(connector);
• return &vga->encoder;

+}

+static struct drm_connector_helper_funcs vga_simple_connector_helper_funcs = { + .get_modes = vga_simple_connector_get_modes,

• .best_encoder = vga_simple_connector_best_encoder,
• .mode_valid = vga_simple_connector_mode_valid,

+};

+/*

• • Encoder functions
• */

+static void vga_simple_encoder_destroy(struct drm_encoder *encoder) +{

• drm_encoder_cleanup(encoder);

+}

+static const struct drm_encoder_funcs vga_simple_encoder_funcs = {

• .destroy = vga_simple_encoder_destroy,

+};

+/*

• • Encoder helper functions
• */

+static void vga_simple_encoder_dpms(struct drm_encoder *encoder, int mode) +{

• struct vga_simple *vga = encoder_to_vga_simple(encoder);
• mutex_lock(&vga->enable_lock);
• switch (mode) {
• case DRM_MODE_DPMS_ON:

• if (vga->enabled)
  

• 	goto out;
  

• if (!IS_ERR(vga->vaa_reg))
  

• 	regulator_enable(vga->vaa_reg);
  

• if (vga->enable_gpio)
  

• 	gpiod_set_value(vga->enable_gpio, 1);
  

• vga->enabled = true;
  

• break;
  

• case DRM_MODE_DPMS_STANDBY:
• case DRM_MODE_DPMS_SUSPEND:
• case DRM_MODE_DPMS_OFF:

• if (!vga->enabled)
  

• 	goto out;
  

• if (vga->enable_gpio)
  

• 	gpiod_set_value(vga->enable_gpio, 0);
  

• if (!IS_ERR(vga->vaa_reg))
  

• 	regulator_enable(vga->vaa_reg);
  

• vga->enabled = false;
  

• break;
  

• default:

• break;
  

• }

+out:

• mutex_unlock(&vga->enable_lock);

+}

+static bool vga_simple_mode_fixup(struct drm_encoder *encoder,

• 		  const struct drm_display_mode *mode,
  

• 		  struct drm_display_mode *adjusted_mode)
  

+{

• return true;

+}

+static void vga_simple_encoder_prepare(struct drm_encoder *encoder) +{ +}

+static void vga_simple_encoder_mode_set(struct drm_encoder *encoder,

• 			struct drm_display_mode *mode,
  

• 			struct drm_display_mode *adjusted_mode)
  

+{ +}

+static void vga_simple_encoder_commit(struct drm_encoder *encoder) +{

• vga_simple_encoder_dpms(encoder, DRM_MODE_DPMS_ON);

+}

+static void vga_simple_encoder_disable(struct drm_encoder *encoder) +{

• vga_simple_encoder_dpms(encoder, DRM_MODE_DPMS_OFF);

+}

+static const struct drm_encoder_helper_funcs vga_simple_encoder_helper_funcs = {

• .dpms = vga_simple_encoder_dpms,
• .mode_fixup = vga_simple_mode_fixup,
• .prepare = vga_simple_encoder_prepare,
• .mode_set = vga_simple_encoder_mode_set,
• .commit = vga_simple_encoder_commit,
• .disable = vga_simple_encoder_disable,

this is interesting. Some users of this encoder (I'm thinking about rcar-du, for which I've just posted the patches) are already ported to atomic updates, while others are not. How can we support both ?

Wild guess, support both in such generic components and let bind decide which to use ... is there some sort of drm_is_atomic() against the master device.

...

+};

+/*

• • Component helper functions
• */

+static int vga_simple_bind(struct device *dev, struct device *master,

• 		 void *data)
  

+{

• struct platform_device *pdev = to_platform_device(dev);
• struct drm_device *drm = data;
• struct device_node *ddc_node, *np = pdev->dev.of_node;
• struct vga_simple *vga;
• int ret;
• if (!np)

• return -ENODEV;
  

• vga = devm_kzalloc(dev, sizeof(*vga), GFP_KERNEL);
• if (!vga)

• return -ENOMEM;
  

• vga->dev = dev;
• dev_set_drvdata(dev, vga);
• mutex_init(&vga->enable_lock);
• vga->enable_gpio = devm_gpiod_get_optional(dev, "enable",

• 				   GPIOD_OUT_LOW);
  

• if (IS_ERR(vga->enable_gpio)) {

• ret = PTR_ERR(vga->enable_gpio);
  

• dev_err(dev, "failed to request GPIO: %d\n", ret);
  

• return ret;
  

• }
• vga->enabled = false;
• ddc_node = of_parse_phandle(np, "ddc-i2c-bus", 0);
• if (ddc_node) {

• vga->ddc = of_find_i2c_adapter_by_node(ddc_node);
  

• of_node_put(ddc_node);
  

• if (!vga->ddc) {
  

• 	dev_dbg(vga->dev, "failed to read ddc node\n");
  

• 	return -EPROBE_DEFER;
  

• }
  

• } else {

• dev_dbg(vga->dev, "no ddc property found\n");
  

• }
• vga->vaa_reg = devm_regulator_get_optional(dev, "vaa");
• vga->encoder.possible_crtcs = drm_of_find_possible_crtcs(drm, np);
• vga->encoder.of_node = np;
• vga->connector.polled = DRM_CONNECTOR_POLL_CONNECT |

• 		DRM_CONNECTOR_POLL_DISCONNECT;
  

• drm_encoder_helper_add(&vga->encoder,

&vga_simple_encoder_helper_funcs);

...

• drm_encoder_init(drm, &vga->encoder, &vga_simple_encoder_funcs,

• 	 DRM_MODE_ENCODER_DAC);
  

• drm_connector_helper_add(&vga->connector,

• 		 &vga_simple_connector_helper_funcs);
  

I really dislike this, this is an encoder driver, not a connector driver. It shouldn't be responsible for creating the connector. For all it knows, there might not even be a VGA connector connected to the encoder, VGA signals could be sent to a chained encoder. That might be a bit far-fetched in the VGA case, but in the generic case encoder drivers shouldn't create connectors.

ok, so you suggest to have this split. Taking into account Rob's comment probably a "simple-encoder" and a "ddc-connector" for connectors using system i2c busses? And then connect everything via ports.

Thanks Heiko