Re: [PATCH 3/5] drm/i2c: adv7511: Refactor encoder slave functions

[Archit Taneja <architt@codeaurora.org>]

Hi,

On 07/27/2015 02:29 PM, Laurent Pinchart wrote:
> Hi Archit,
>
> (CC'ing Boris Brezillon)
>
> Thank you for the patch.
>
> On Monday 27 July 2015 11:46:57 Archit Taneja wrote:
>> ADV7511 is represented as an i2c drm slave encoder device. ADV7533, on
>> the other hand, is going be a normal i2c client device creating bridge
>> and connector entities.
>
> Please, no. It's really time to stop piling hacks and fix the problem
> properly. There's no reason to have separate APIs for I2C slave encoders and
> DRM bridges. Those two APIs need to be merged, and then you'll find it much
> easier to implement ADV7533 support.

i2c slave encoders and bridges aren't exactly the same. slave encoders
are used when the there is no real 'encoder' in the display chain.
bridges are used when there is already an encoder available, and the
bridge entity represents another encoder in the chain.

ADV7511 takes in RGB/MIPI DPI data, which is generally the output of a 
crtc for drm drivers.

ADV7533 takes in MIPI DSI data, which is generally the output of an
encoder for drm drivers.

Therefore, having i2c slave encoder for the former and bridge for the
latter made sense to me.

I do agree that it would be better if they were somehow merged. It
would prevent the fragmentation we currently have among encoder
chips.

One possible way would be to convert slave encoder to bridge. With
this, an i2c slave encoder would be a 'dummy encoder' and a bridge.
i2c slave encoders even now just tie the slave encoder helper funcs
to encoder helper funcs. So it's not really any different.

Merging these 2 entities would be nice, but we're still shying away
from the larger problem of creating generic encoder chains. The
ideal solution would be for bridges and slave encoders to just be
'encoders', and the facility to connect on encoder output to the
input of another. I don't know how easy it is to do this, and
whether it'll break userspace.

Archit

>
> Boris, I know you were experimenting with that, do you have anything to report
> ?
>
>> Move the code in encoder slave functions to generate helpers that are
>> agnostic to the drm object type. These helpers will later also be used
>> by bridge and connecter helper functions.
>>
>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
>> ---
>>   drivers/gpu/drm/i2c/adv7511.c | 80 +++++++++++++++++++++++++++------------
>>   1 file changed, 57 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i2c/adv7511.c b/drivers/gpu/drm/i2c/adv7511.c
>> index 63a3d20..46fb24d 100644
>> --- a/drivers/gpu/drm/i2c/adv7511.c
>> +++ b/drivers/gpu/drm/i2c/adv7511.c
>> @@ -612,13 +612,11 @@ static int adv7511_get_edid_block(void *data, u8 *buf,
>> unsigned int block, }
>>
>>   /*
>> ---------------------------------------------------------------------------
>> -- - * Encoder operations
>> + * ADV75xx helpers
>>    */
>> -
>> -static int adv7511_get_modes(struct drm_encoder *encoder,
>> -			     struct drm_connector *connector)
>> +static int adv7511_get_modes(struct adv7511 *adv7511,
>> +		struct drm_connector *connector)
>>   {
>> -	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>>   	struct edid *edid;
>>   	unsigned int count;
>>
>> @@ -656,21 +654,10 @@ static int adv7511_get_modes(struct drm_encoder
>> *encoder, return count;
>>   }
>>
>> -static void adv7511_encoder_dpms(struct drm_encoder *encoder, int mode)
>> -{
>> -	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>> -
>> -	if (mode == DRM_MODE_DPMS_ON)
>> -		adv7511_power_on(adv7511);
>> -	else
>> -		adv7511_power_off(adv7511);
>> -}
>> -
>>   static enum drm_connector_status
>> -adv7511_encoder_detect(struct drm_encoder *encoder,
>> +adv7511_detect(struct adv7511 *adv7511,
>>   		       struct drm_connector *connector)
>>   {
>> -	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>>   	enum drm_connector_status status;
>>   	unsigned int val;
>>   	bool hpd;
>> @@ -694,7 +681,7 @@ adv7511_encoder_detect(struct drm_encoder *encoder,
>>   	if (status == connector_status_connected && hpd && adv7511->powered) {
>>   		regcache_mark_dirty(adv7511->regmap);
>>   		adv7511_power_on(adv7511);
>> -		adv7511_get_modes(encoder, connector);
>> +		adv7511_get_modes(adv7511, connector);
>>   		if (adv7511->status == connector_status_connected)
>>   			status = connector_status_disconnected;
>>   	} else {
>> @@ -708,8 +695,8 @@ adv7511_encoder_detect(struct drm_encoder *encoder,
>>   	return status;
>>   }
>>
>> -static int adv7511_encoder_mode_valid(struct drm_encoder *encoder,
>> -				      struct drm_display_mode *mode)
>> +static int adv7511_mode_valid(struct adv7511 *adv7511,
>> +				     const struct drm_display_mode *mode)
>>   {
>>   	if (mode->clock > 165000)
>>   		return MODE_CLOCK_HIGH;
>> @@ -717,11 +704,10 @@ static int adv7511_encoder_mode_valid(struct
>> drm_encoder *encoder, return MODE_OK;
>>   }
>>
>> -static void adv7511_encoder_mode_set(struct drm_encoder *encoder,
>> +static void adv7511_mode_set(struct adv7511 *adv7511,
>>   				     struct drm_display_mode *mode,
>>   				     struct drm_display_mode *adj_mode)
>>   {
>> -	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>>   	unsigned int low_refresh_rate;
>>   	unsigned int hsync_polarity = 0;
>>   	unsigned int vsync_polarity = 0;
>> @@ -812,12 +798,60 @@ static void adv7511_encoder_mode_set(struct
>> drm_encoder *encoder, adv7511->f_tmds = mode->clock;
>>   }
>>
>> +/*
>> ---------------------------------------------------------------------------
>> -- + * Encoder operations
>> + */
>> +
>> +static int adv7511_encoder_get_modes(struct drm_encoder *encoder,
>> +			     struct drm_connector *connector)
>> +{
>> +	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>> +
>> +	return adv7511_get_modes(adv7511, connector);
>> +}
>> +
>> +static void adv7511_encoder_dpms(struct drm_encoder *encoder, int mode)
>> +{
>> +	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>> +
>> +	if (mode == DRM_MODE_DPMS_ON)
>> +		adv7511_power_on(adv7511);
>> +	else
>> +		adv7511_power_off(adv7511);
>> +}
>> +
>> +static enum drm_connector_status
>> +adv7511_encoder_detect(struct drm_encoder *encoder,
>> +		       struct drm_connector *connector)
>> +{
>> +	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>> +
>> +	return adv7511_detect(adv7511, connector);
>> +}
>> +
>> +static int adv7511_encoder_mode_valid(struct drm_encoder *encoder,
>> +				      struct drm_display_mode *mode)
>> +{
>> +	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>> +
>> +	return adv7511_mode_valid(adv7511, mode);
>> +}
>> +
>> +static void adv7511_encoder_mode_set(struct drm_encoder *encoder,
>> +				     struct drm_display_mode *mode,
>> +				     struct drm_display_mode *adj_mode)
>> +{
>> +	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>> +
>> +	adv7511_mode_set(adv7511, mode, adj_mode);
>> +}
>> +
>>   static struct drm_encoder_slave_funcs adv7511_encoder_funcs = {
>>   	.dpms = adv7511_encoder_dpms,
>>   	.mode_valid = adv7511_encoder_mode_valid,
>>   	.mode_set = adv7511_encoder_mode_set,
>>   	.detect = adv7511_encoder_detect,
>> -	.get_modes = adv7511_get_modes,
>> +	.get_modes = adv7511_encoder_get_modes,
>>   };
>>
>>   /*
>> ---------------------------------------------------------------------------
>> --
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel