On Mon, Jun 10, 2013 at 01:10:18PM +0200, Sebastian Hesselbarth wrote:
> On 06/09/13 21:29, Russell King wrote:
>> + /*
>> + * The spec is unclear about the polarities of the syncs.
>> + * We assume their non-inverted state is active high.
>> + */
>
> nit: "We confirmed their non-inverted state is active high."
Thanks, it's been a while since I looked at this so I had forgotten
to update the comment. Now done.
>> + if (resource_size(r) > SZ_4K)
>> + mem = r;
>
> nit again: The register address window of Dove LCD is 64k although you
> are right an no more than 512B are used. Also a comment would be nice,
> that everything above 4k (64k) is interpreted as gfx mem.
Fixed and comment added.
>> + /* Create all LCD controllers */
>> + for (n = 0; n < ARRAY_SIZE(priv->dcrtc); n++) {
>> + struct clk *clk;
>> +
>> + if (!res[n])
>> + break;
>> +
>> + clk = clk_get_sys("dovefb.0", "extclk");
>
> To be precise: the above should have the index at extclk as there
> is two extclk inputs that can be used for both lcdcs. So currently,
> as armada_crtc is hard-coding extclk0 input it should be
> "armadafb.%d", "extclk0".
>
> But I know, clocking in general will work-out with parent select for
> clk-mux and DT support.
I've sorted that out, but I'd forgotten there were three additional
patches on top of what I've posted sorting that stuff out - the
second one in particular:
4a5e9b7 DRM: armada: store kernel address for gem objects
f760c94 DRM: Dove: alternative variant based clocking
2e051fd DRM: Armada: convert hardware cursor support to 64x32 or 32x64 ARGB
Because they're scattered in other branches (the h/w cursor stuff
is separate) it's not trivial to generate a single series from git
for these.
>> +static const struct armada_output_type armada_drm_conn_slave = {
>> + .connector_type = DRM_MODE_CONNECTOR_HDMIA,
>
> For a rework of DRM slave encoder API, there should also be some way to
> get .connector_type and .encoder_type above from that slave encoder.
> IMHO it should be up to the slave encoder to determine connector and
> encoder type.
Encoder type - yes, but connector type doesn't seem sensible. It's
possible for the TDA998x to be connected to a DVI connector - how
would the slave encoder know that?
>> diff --git a/drivers/gpu/drm/armada/armada_slave.h b/drivers/gpu/drm/armada/armada_slave.h
>> new file mode 100644
>> index 0000000..1b86696
>> --- /dev/null
>> +++ b/drivers/gpu/drm/armada/armada_slave.h
>> @@ -0,0 +1,26 @@
>> +/*
>> + * Copyright (C) 2012 Russell King
>> + *
>> + * 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.
>> + */
>> +#ifndef ARMADA_TDA19988_H
>> +#define ARMADA_TDA19988_H
>
> nit: ARMADA_SLAVE_H
Nobbled. :)
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
