On Thu, Jan 24, 2013 at 6:43 AM, Daniel Vetter <daniel@xxxxxxxx> wrote:
> On Tue, Jan 22, 2013 at 04:36:24PM -0600, Rob Clark wrote:
>> Add output panel driver for i2c encoder slaves.
>>
>> Signed-off-by: Rob Clark <robdclark@xxxxxxxxx>
>
> A few questions below, otherwise
>
> Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
>> ---
[snip]
>> diff --git a/drivers/gpu/drm/tilcdc/Kconfig b/drivers/gpu/drm/tilcdc/Kconfig
>> index ee9b592..99beca2 100644
>> --- a/drivers/gpu/drm/tilcdc/Kconfig
>> +++ b/drivers/gpu/drm/tilcdc/Kconfig
>> @@ -8,3 +8,15 @@ config DRM_TILCDC
>> Choose this option if you have an TI SoC with LCDC display
>> controller, for example AM33xx in beagle-bone, DA8xx, or
>> OMAP-L1xx. This driver replaces the FB_DA8XX fbdev driver.
>> +
>> +menu "I2C encoder or helper chips"
>> + depends on DRM && DRM_KMS_HELPER && I2C
>> +
>> +config DRM_I2C_NXP_TDA998X
>> + tristate "NXP Semiconductors TDA998X HDMI encoder"
>> + default m if DRM_TILCDC
>> + help
>> + Support for NXP Semiconductors TDA998X HDMI encoders.
>> +
>> +endmenu
>> +
>
> Shouldn't that hunk be in patch 2?
yeah, probably.. I just copied how it was done in nouveau, but I think
probably the Kconfig for the i2c encoder slaves should be moved into
drivers/gpu/drm/i2c
[snip]
>> +/*
>> + * Encoder:
>> + */
>> +
>> +struct slave_encoder {
>> + struct drm_encoder_slave base;
>> + struct slave_module *mod;
>> +};
>> +#define to_slave_encoder(x) container_of(to_encoder_slave(x), struct slave_encoder, base)
>
> Since you have a 1:1:1 relationship between module/drm_encoder, the
> drm_encoder_slave and the connector I'd just smash this all into one
> struct. Pure bikeshed though ;-)
yeah, but drm_encoder_slave is coming from drm core
[snip]
>> +static struct drm_encoder *slave_encoder_create(struct drm_device *dev,
>> + struct slave_module *mod)
>> +{
>> + struct slave_encoder *slave_encoder;
>> + struct drm_encoder *encoder;
>> + int ret;
>> +
>> + slave_encoder = kzalloc(sizeof(*slave_encoder), GFP_KERNEL);
>> + if (!slave_encoder) {
>> + dev_err(dev->dev, "allocation failed\n");
>> + return NULL;
>> + }
>> +
>> + slave_encoder->mod = mod;
>> +
>> + encoder = &slave_encoder->base.base;
>> + encoder->possible_crtcs = 1;
>> +
>> + ret = drm_encoder_init(dev, encoder, &slave_encoder_funcs,
>> + DRM_MODE_ENCODER_LVDS);
>
> DRM_MODE_ENCODER_TMDS? Although I guess adding a new kind of
> multi-function encoder type would make more sense and also useful in other
> places. E.g. i915-sdvo/dvo just set meaningless types for multi-function
> encoders (i.e. more than one connector on the same output), namely the
> type which matches the connector last initalized.
I suppose TDMS makes more sense.. perhaps getting both this and
connector type from the encoder-slave would make the most sense, but I
can change it to TDMS for now
[snip]
>> +static struct drm_connector *slave_connector_create(struct drm_device *dev,
>> + struct slave_module *mod, struct drm_encoder *encoder)
>> +{
>> + struct slave_connector *slave_connector;
>> + struct drm_connector *connector;
>> + int ret;
>> +
>> + slave_connector = kzalloc(sizeof(*slave_connector), GFP_KERNEL);
>> + if (!slave_connector) {
>> + dev_err(dev->dev, "allocation failed\n");
>> + return NULL;
>> + }
>> +
>> + slave_connector->encoder = encoder;
>> + slave_connector->mod = mod;
>> +
>> + connector = &slave_connector->base;
>> +
>> + drm_connector_init(dev, connector, &slave_connector_funcs,
>> + DRM_MODE_CONNECTOR_HDMIA);
>
> Shouldn't we get the connector type from the drm_encoder_slave somehow?
> Works here for now, so just food for thought for future encoder slave
> refactorings and infrastructure work.
yeah, getting it from the encoder slave makes the most sense
[snip]
>> +static struct of_device_id slave_of_match[] = {
>> + { .compatible = "tilcdc,slave", },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(of, slave_of_match);
>> +
>> +struct platform_driver slave_driver = {
>> + .probe = slave_probe,
>> + .remove = slave_remove,
>> + .driver = {
>> + .owner = THIS_MODULE,
>> + .name = "slave",
>> + .of_match_table = slave_of_match,
>> + },
>> +};
>
> No idea how this devicetree matching stuff is supposed to work, but I
> guess this needs to be updated in the devictree docs like the base match.
yeah, I didn't realize previously about the DT bindings docs, so I
need to look into that
BR,
-R
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
