Re: [PATCH] drm/panel: Add driver for Seiko 43WVF1G panel

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3213 bytes --]

On Tue, Apr 25, 2017 at 01:18:35PM -0300, Marco Franchi wrote:
> Add driver for Seiko Instruments Inc. 4.3" WVGA (800 x RGB x 480)
> TFT with Touch-Panel.
> 
> Datasheet available at:
> http://www.glyn.de/data/glyn/media/doc/43wvf1g-0.pdf
> 
> Seiko 43WVF1G panel has two power supplies: AVDD and DVDD and they
> require a specific power on/down sequence.
> For this reason the simple panel driver cannot be used to drive this
> panel, so create a new one heavily based on simple panel.
> 
> Based on initial patch submission from Breno Lima.
> 
> Signed-off-by: Marco Franchi <marco.franchi@nxp.com>
> ---
>  .../bindings/display/panel/seiko,43wvf1g.txt       |  23 ++
>  drivers/gpu/drm/panel/Kconfig                      |   9 +
>  drivers/gpu/drm/panel/Makefile                     |   1 +
>  drivers/gpu/drm/panel/panel-seiko-43wvf1g.c        | 372 +++++++++++++++++++++
>  4 files changed, 405 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/seiko,43wvf1g.txt
>  create mode 100644 drivers/gpu/drm/panel/panel-seiko-43wvf1g.c
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/seiko,43wvf1g.txt b/Documentation/devicetree/bindings/display/panel/seiko,43wvf1g.txt
> new file mode 100644
> index 0000000..cd1eeda
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/seiko,43wvf1g.txt
> @@ -0,0 +1,23 @@
> +Seiko Instruments Inc. 4.3" WVGA (800 x RGB x 480) TFT with Touch-Panel
> +
> +Required properties:
> +- compatible: should be "sii,43wvf1g".
> +- "DVDD-supply": 3v3 digital regulator.
> +- "AVDD-supply": 5v analog regulator.

I don't think we should be using all-uppercase for supply names. So the
above should be "dvdd-supply" and "avdd-supply". No need to resend only
for that change, I can fix it up when applying.

Rob, can I have your Acked-by with the above change?

Just two minor comments below.

> diff --git a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c
[...]
> +struct seiko_panel_desc {
> +	const struct drm_display_mode *modes;
> +	unsigned int num_modes;
> +	const struct display_timing *timings;
> +	unsigned int num_timings;
> +
> +	unsigned int bpc;
> +
> +	/**
> +	 * @width: width (in millimeters) of the panel's active display area
> +	 * @height: height (in millimeters) of the panel's active display area
> +	 */
> +	struct {
> +		unsigned int width;
> +		unsigned int height;
> +	} size;
> +
> +	u32 bus_format;
> +	u32 bus_flags;
> +};

It's somewhat unfortunate how this has to duplicate a lot of the
panel-simple driver just because it uses two regulators. However, with
some of the work going on to make panel-simple code more reusable this
might improve in the future. For now I think this doesn't hurt.

> +struct seiko_panel {
> +	struct drm_panel base;
> +	bool prepared;
> +	bool enabled;
> +	const struct seiko_panel_desc *desc;
> +	struct backlight_device *backlight;
> +	struct regulator *DVDD;
> +	struct regulator *AVDD;
> +};

Same as for the binding: we don't use all-uppercase names for variables,
so I'll fix those up to be lowercase when applying.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel