RE: [PATCH v5 03/10] drm/bridge: it6505: add AUX operation for HDCP KSV list read

[Hermes.Wu]

>-----Original Message-----
>From: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> 
>Sent: Monday, October 14, 2024 7:53 PM
>To: Hermes Wu (吳佳宏) <Hermes.Wu@xxxxxxxxxx>
>Cc: Pin-yen Lin <treapking@xxxxxxxxxxxx>; Kenneth Hung (洪家倫) <Kenneth.Hung@xxxxxxxxxx>; Pet Weng (翁玉芬) <Pet.Weng@xxxxxxxxxx>; Andrzej Hajda <andrzej.hajda@xxxxxxxxx>; Neil Armstrong <neil.armstrong@xxxxxxxxxx>; Robert Foss <rfoss@xxxxxxxxxx>; Laurent Pinchart <Laurent.pinchart@xxxxxxxxxxxxxxxx>; Jonas Karlman <jonas@xxxxxxxxx>; Jernej Skrabec <jernej.skrabec@xxxxxxxxx>; Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>; Maxime Ripard <mripard@xxxxxxxxxx>; Thomas Zimmermann <tzimmermann@xxxxxxx>; David Airlie <airlied@xxxxxxxxx>; Simona Vetter <simona@xxxxxxxx>; open list:DRM DRIVERS <dri-devel@xxxxxxxxxxxxxxxxxxxxx>; open list <linux-kernel@xxxxxxxxxxxxxxx>
>Subject: Re: [PATCH v5 03/10] drm/bridge: it6505: add AUX operation for HDCP KSV list read
>
>On Tue, Oct 01, 2024 at 02:43:02PM +0800, Hermes Wu wrote:
>> From: Hermes Wu <Hermes.wu@xxxxxxxxxx>
>> 
>> HDCP KSV list readback can choose to use AUX FIFO or general data register.
>> For some DisplayPort devices, the KSV list must be read in 5 byte boundaries.
>> The original AUX read command does not support these devices.
>> 
>> The AUX command operation control register "REG_AUX_CMD_REQ" uses b[3:0] as AUX operacion control, and b[7:4] are status bits and read only.
>> To change KSV read operation uses "CMD_AUX_NATIVE_READ" from using the data registers to using AUX FIFO.
>> The extended command "CMD_AUX_GET_KSV_LIST" is added as "CMD_AUX_NATIVE_READ" with the 0x10 flag which selects AUX FIFO mode.
>
>Please keep the commit message wrapped at 72-75 chars.
>
>Other than that:

Should I send new v6 patches with commit message wrapped at 72-75 chars?

Thanks.
>
>Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
>
>
>> 
>> Signed-off-by: Hermes Wu <Hermes.wu@xxxxxxxxxx>
>> ---
>>  drivers/gpu/drm/bridge/ite-it6505.c | 16 +++++++++++-----
>>  1 file changed, 11 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c 
>> b/drivers/gpu/drm/bridge/ite-it6505.c
>> index 864fab7e388f..9a022c095af4 100644
>> --- a/drivers/gpu/drm/bridge/ite-it6505.c
>> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
>> @@ -126,6 +126,7 @@
>>  #define REG_AUX_OUT_DATA0 0x27
>>  
>>  #define REG_AUX_CMD_REQ 0x2B
>> +#define M_AUX_REQ_CMD 0x0F
>>  #define AUX_BUSY BIT(5)
>>  
>>  #define REG_AUX_DATA_0_7 0x2C
>> @@ -324,6 +325,9 @@ enum aux_cmd_type {
>>  	CMD_AUX_NATIVE_READ = 0x0,
>>  	CMD_AUX_NATIVE_WRITE = 0x5,
>>  	CMD_AUX_I2C_EDID_READ = 0xB,
>> +
>> +	/* KSV read with AUX FIFO extend from CMD_AUX_NATIVE_READ*/
>> +	CMD_AUX_GET_KSV_LIST = 0x10,
>>  };
>>  
>>  enum aux_cmd_reply {
>> @@ -965,7 +969,8 @@ static ssize_t it6505_aux_operation(struct it6505 *it6505,
>>  	it6505_set_bits(it6505, REG_AUX_CTRL, AUX_USER_MODE, AUX_USER_MODE);
>>  
>>  aux_op_start:
>> -	if (cmd == CMD_AUX_I2C_EDID_READ) {
>> +	/* HW AUX FIFO supports only EDID and DCPD KSV FIFO area */
>> +	if (cmd == CMD_AUX_I2C_EDID_READ || cmd == CMD_AUX_GET_KSV_LIST) {
>>  		/* AUX EDID FIFO has max length of AUX_FIFO_MAX_SIZE bytes. */
>>  		size = min_t(size_t, size, AUX_FIFO_MAX_SIZE);
>>  		/* Enable AUX FIFO read back and clear FIFO */ @@ -996,7 +1001,7 @@ 
>> static ssize_t it6505_aux_operation(struct it6505 *it6505,
>>  				  size);
>>  
>>  	/* Aux Fire */
>> -	it6505_write(it6505, REG_AUX_CMD_REQ, cmd);
>> +	it6505_write(it6505, REG_AUX_CMD_REQ, FIELD_GET(M_AUX_REQ_CMD, 
>> +cmd));
>>  
>>  	ret = it6505_aux_wait(it6505);
>>  	if (ret < 0)
>> @@ -1030,7 +1035,7 @@ static ssize_t it6505_aux_operation(struct it6505 *it6505,
>>  		goto aux_op_start;
>>  	}
>>  
>> -	if (cmd == CMD_AUX_I2C_EDID_READ) {
>> +	if (cmd == CMD_AUX_I2C_EDID_READ || cmd == CMD_AUX_GET_KSV_LIST) {
>>  		for (i = 0; i < size; i++) {
>>  			ret = it6505_read(it6505, REG_AUX_DATA_FIFO);
>>  			if (ret < 0)
>> @@ -1055,7 +1060,7 @@ static ssize_t it6505_aux_operation(struct it6505 *it6505,
>>  	ret = i;
>>  
>>  aux_op_err:
>> -	if (cmd == CMD_AUX_I2C_EDID_READ) {
>> +	if (cmd == CMD_AUX_I2C_EDID_READ || cmd == CMD_AUX_GET_KSV_LIST) {
>>  		/* clear AUX FIFO */
>>  		it6505_set_bits(it6505, REG_AUX_CTRL,
>>  				AUX_EN_FIFO_READ | CLR_EDID_FIFO, @@ -1076,7 +1081,8 @@ static 
>> ssize_t it6505_aux_do_transfer(struct it6505 *it6505,
>>  				      size_t size, enum aux_cmd_reply *reply)  {
>>  	int i, ret_size, ret = 0, request_size;
>> -	int fifo_max_size = (cmd == CMD_AUX_I2C_EDID_READ) ? AUX_FIFO_MAX_SIZE : 4;
>> +	int fifo_max_size = (cmd == CMD_AUX_I2C_EDID_READ || cmd == CMD_AUX_GET_KSV_LIST) ?
>> +						 AUX_FIFO_MAX_SIZE : 4;
>>  
>>  	mutex_lock(&it6505->aux_lock);
>>  	i = 0;
>> --
>> 2.34.1
>> 
>
>-- 
>With best wishes
>Dmitry
>
>
BR,
Hermes