Re: [PATCH 4/4] drm/panel: Add helper for simple panel connector

[Daniel Vetter]

On Thu, May 05, 2016 at 03:24:34PM +0200, Noralf Trønnes wrote:
> Add function to create a simple connector for a panel.
> 
> Signed-off-by: Noralf Trønnes <noralf@xxxxxxxxxxx>

Like in the previous patch please also add a new section for the panel
helpers to gpu.tmpl. I don't think this needs an overview section, it's so
simple. But adding some cross references from the drm_panel.c kerneldoc to
this and back would be real good.

> ---
>  drivers/gpu/drm/Makefile           |   1 +
>  drivers/gpu/drm/drm_panel_helper.c | 117 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/panel/Kconfig      |   7 +++
>  include/drm/drm_panel_helper.h     |  20 +++++++
>  4 files changed, 145 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_panel_helper.c
>  create mode 100644 include/drm/drm_panel_helper.h
> 
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 7e99923..3f3696c 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -18,6 +18,7 @@ drm-$(CONFIG_COMPAT) += drm_ioc32.o
>  drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
>  drm-$(CONFIG_PCI) += ati_pcigart.o
>  drm-$(CONFIG_DRM_PANEL) += drm_panel.o
> +drm-$(CONFIG_DRM_PANEL_HELPER) += drm_panel_helper.o
>  drm-$(CONFIG_OF) += drm_of.o
>  drm-$(CONFIG_AGP) += drm_agpsupport.o
>  
> diff --git a/drivers/gpu/drm/drm_panel_helper.c b/drivers/gpu/drm/drm_panel_helper.c
> new file mode 100644
> index 0000000..5d8b623
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_panel_helper.c
> @@ -0,0 +1,117 @@
> +/*
> + * Copyright (C) 2016 Noralf Trønnes
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_panel.h>
> +
> +struct drm_panel_connector {
> +	struct drm_connector base;
> +	struct drm_panel *panel;
> +};
> +
> +static inline struct drm_panel_connector *
> +to_drm_panel_connector(struct drm_connector *connector)
> +{
> +	return container_of(connector, struct drm_panel_connector, base);
> +}
> +
> +static int drm_panel_connector_get_modes(struct drm_connector *connector)
> +{
> +	return drm_panel_get_modes(to_drm_panel_connector(connector)->panel);
> +}
> +
> +static struct drm_encoder *
> +drm_panel_connector_best_encoder(struct drm_connector *connector)
> +{
> +	return drm_encoder_find(connector->dev, connector->encoder_ids[0]);
> +}

As mentioned in my previous review, this function would be extremely
useful to rid tons of drivers of boilerplate - most drm_connector only
support exactly 1 encoder, statically determined at driver init time.

Please put this helper into the atomic helper library, and maybe call it
something like

drm_atomic_helper_best_encoder.

To make sure it's never abused by accident please also add a 

	WARN_ON(connector->encoder_ids[1]);

to make sure that there's really only just one encoder for this connector.

If you're bored you can also go through drivers and use that everywhere it
fits, it would result in a _lot_ of deleted code. But also needs quite a
bit of audit work ...

Otherwise lgtm, but please ask Thierry for an ack as the panel maintainer.
-Daniel


> +
> +static const struct drm_connector_helper_funcs drm_panel_connector_hfuncs = {
> +	.get_modes = drm_panel_connector_get_modes,
> +	.best_encoder = drm_panel_connector_best_encoder,
> +};
> +
> +static enum drm_connector_status
> +drm_panel_connector_detect(struct drm_connector *connector, bool force)
> +{
> +	if (drm_device_is_unplugged(connector->dev))
> +		return connector_status_disconnected;
> +
> +	return connector->status;
> +}
> +
> +static void drm_panel_connector_destroy(struct drm_connector *connector)
> +{
> +	struct drm_panel_connector *panel_connector;
> +
> +	panel_connector = to_drm_panel_connector(connector);
> +	drm_panel_detach(panel_connector->panel);
> +	drm_panel_remove(panel_connector->panel);
> +	drm_connector_unregister(connector);
> +	drm_connector_cleanup(connector);
> +	kfree(panel_connector);
> +}
> +
> +static const struct drm_connector_funcs drm_panel_connector_funcs = {
> +	.dpms = drm_atomic_helper_connector_dpms,
> +	.reset = drm_atomic_helper_connector_reset,
> +	.detect = drm_panel_connector_detect,
> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.destroy = drm_panel_connector_destroy,
> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +/**
> + * drm_panel_connector_create - Create simple connector for panel
> + * @dev: DRM device
> + * @panel: DRM panel
> + * @connector_type: user visible type of the connector
> + *
> + * Creates a simple connector for a panel.
> + * The panel needs to provide a get_modes() function.
> + *
> + * Returns:
> + * Pointer to new connector or ERR_PTR on failure.
> + */
> +struct drm_connector *drm_panel_connector_create(struct drm_device *dev,
> +						 struct drm_panel *panel,
> +						 int connector_type)
> +{
> +	struct drm_panel_connector *panel_connector;
> +	struct drm_connector *connector;
> +	int ret;
> +
> +	panel_connector = kzalloc(sizeof(*panel_connector), GFP_KERNEL);
> +	if (!panel_connector)
> +		return ERR_PTR(-ENOMEM);
> +
> +	panel_connector->panel = panel;
> +	connector = &panel_connector->base;
> +	drm_connector_helper_add(connector, &drm_panel_connector_hfuncs);
> +	ret = drm_connector_init(dev, connector, &drm_panel_connector_funcs,
> +				 connector_type);
> +	if (ret) {
> +		kfree(panel_connector);
> +		return ERR_PTR(ret);
> +	}
> +
> +	connector->status = connector_status_connected;
> +	drm_panel_init(panel);
> +	drm_panel_add(panel);
> +	drm_panel_attach(panel, connector);
> +
> +	return connector;
> +}
> +EXPORT_SYMBOL(drm_panel_connector_create);
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 1500ab9..87264a3 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -4,6 +4,13 @@ config DRM_PANEL
>  	help
>  	  Panel registration and lookup framework.
>  
> +config DRM_PANEL_HELPER
> +	bool
> +	depends on DRM
> +	select DRM_PANEL
> +	help
> +	  Panel helper.
> +
>  menu "Display Panels"
>  	depends on DRM && DRM_PANEL
>  
> diff --git a/include/drm/drm_panel_helper.h b/include/drm/drm_panel_helper.h
> new file mode 100644
> index 0000000..c3355e3
> --- /dev/null
> +++ b/include/drm/drm_panel_helper.h
> @@ -0,0 +1,20 @@
> +/*
> + * Copyright (C) 2016 Noralf Trønnes
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#ifndef __DRM_PANEL_HELPER_H__
> +#define __DRM_PANEL_HELPER_H__
> +
> +struct drm_device;
> +struct drm_panel;
> +
> +struct drm_connector *drm_panel_connector_create(struct drm_device *dev,
> +						 struct drm_panel *panel,
> +						 int connector_type);
> +
> +#endif
> -- 
> 2.2.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch