On Fri, Jun 03, 2016 at 03:06:32PM +0200, Hans de Goede wrote:
> Grain-media GM12U320 based devices are mini video projectors using USB for
> both power and video data transport.
> 
> This commit adds a kms driver for these devices, including prime support.
> 
> This driver is based on the existing udl kms driver, and the gm12u320
> fb driver by Viacheslav Nurmekhamitov <slavrn at yandex.ru>.
> 
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---
> Changes in v2:
> -Rebase on 4.7-rc1
> -Sync with udl, bring in any fixes done to udl since v1

This sounds a little nightmarish in the long term. From a cursory look
this duplicates a bunch of code that's present in UDL, where the main
differences are EDID handling and data transfer. I wonder if we can't
find a better way of reusing code than duplicating it.

> diff --git a/drivers/gpu/drm/gm12u320/gm12u320_connector.c 
> b/drivers/gpu/drm/gm12u320/gm12u320_connector.c
[...]
> +/*
> + * Note this assumes this driver is only ever used with the Acer C120, if we
> + * add support for other devices the vendor and model should be 
> parameterized.
> + */
> +static struct edid gm12u320_edid = {
> +     .header         = { 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00 },
> +     .mfg_id         = { 0x04, 0x72 },       /* "ACR" */
> +     .prod_code      = { 0x20, 0xc1 },       /* C120h */
> +     .mfg_week       = 1,
> +     .mfg_year       = 1,
> +     .version        = 1,                    /* EDID 1.3 */
> +     .revision       = 3,                    /* EDID 1.3 */
> +     .input          = 0x80,                 /* Digital input */
> +     .features       = 0x02,                 /* Pref timing in DTD 1 */
> +     .standard_timings = { { 1, 1 }, { 1, 1 }, { 1, 1 }, { 1, 1 },
> +                           { 1, 1 }, { 1, 1 }, { 1, 1 }, { 1, 1 } },
> +     .detailed_timings = { {
> +             .pixel_clock = 3383,
> +             /* hactive = 852, hblank = 256 */
> +             .data.pixel_data.hactive_lo = 0x54,
> +             .data.pixel_data.hblank_lo = 0x00,
> +             .data.pixel_data.hactive_hblank_hi = 0x31,
> +             /* vactive = 480, vblank = 28 */
> +             .data.pixel_data.vactive_lo = 0xe0,
> +             .data.pixel_data.vblank_lo = 0x1c,
> +             .data.pixel_data.vactive_vblank_hi = 0x10,
> +             /* hsync offset 40 pw 128, vsync offset 1 pw 4 */
> +             .data.pixel_data.hsync_offset_lo = 0x28,
> +             .data.pixel_data.hsync_pulse_width_lo = 0x80,
> +             .data.pixel_data.vsync_offset_pulse_width_lo = 0x14,
> +             .data.pixel_data.hsync_vsync_offset_pulse_width_hi = 0x00,
> +             /* Digital separate syncs, hsync+, vsync+ */
> +             .data.pixel_data.misc = 0x1e,
> +     }, {
> +             .pixel_clock = 0,
> +             .data.other_data.type = 0xfd, /* Monitor ranges */
> +             .data.other_data.data.range.min_vfreq = 59,
> +             .data.other_data.data.range.max_vfreq = 61,
> +             .data.other_data.data.range.min_hfreq_khz = 29,
> +             .data.other_data.data.range.max_hfreq_khz = 32,
> +             .data.other_data.data.range.pixel_clock_mhz = 4, /* 40 MHz */
> +             .data.other_data.data.range.flags = 0,
> +             .data.other_data.data.range.formula.cvt = {
> +                     0xa0, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20 },
> +     }, {
> +             .pixel_clock = 0,
> +             .data.other_data.type = 0xfc, /* Model string */
> +             .data.other_data.data.str.str = {
> +                     'C', '1', '2', '0', 'P', 'r', 'o', 'j', 'e', 'c',
> +                     't', 'o', 'r' },
> +     }, {
> +             .pixel_clock = 0,
> +             .data.other_data.type = 0xfe, /* Unspecified text / padding */
> +             .data.other_data.data.str.str = {
> +                     '\n', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ',
> +                     ' ', ' ',  ' ' },
> +     } },
> +     .checksum = 0x40,
> +};

Where did you get this from? Doesn't this device have a chip that you
can query at runtime?

> +static int gm12u320_connector_set_property(struct drm_connector *connector,
> +                                        struct drm_property *property,
> +                                        uint64_t val)
> +{
> +     return 0;
> +}

Just drop it if it doesn't do anything.

> +int gm12u320_connector_init(struct drm_device *dev,
> +                         struct drm_encoder *encoder)
> +{
> +     struct drm_connector *connector;
> +
> +     connector = kzalloc(sizeof(struct drm_connector), GFP_KERNEL);
> +     if (!connector)
> +             return -ENOMEM;
> +
> +     drm_connector_init(dev, connector, &gm12u320_connector_funcs,
> +                        DRM_MODE_CONNECTOR_Unknown);

According to the Grain Media website this chip is a USB 3.0-to-HDMI
bridge, so DRM_MODE_CONNECTOR_HDMIA might be a better choice here.

> diff --git a/drivers/gpu/drm/gm12u320/gm12u320_drv.c 
> b/drivers/gpu/drm/gm12u320/gm12u320_drv.c
[...]
> new file mode 100644
> index 0000000..eff3a44
> --- /dev/null
> +++ b/drivers/gpu/drm/gm12u320/gm12u320_drv.c
> @@ -0,0 +1,160 @@
> +/*
> + * Copyright (C) 2012-2016 Red Hat Inc.
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License v2. See the file COPYING in the main directory of this archive for
> + * more details.
> + */
> +
> +#include <linux/module.h>
> +#include <drm/drmP.h>
> +#include <drm/drm_crtc_helper.h>
> +#include "gm12u320_drv.h"
> +
> +static int gm12u320_driver_set_busid(struct drm_device *d, struct drm_master 
> *m)
> +{
> +     return 0;
> +}

This is optional, you can drop it.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: 
<https://lists.freedesktop.org/archives/dri-devel/attachments/20160603/007b2658/attachment.sig>

Reply via email to