Re: [PATCH v8 3/7] media: platform: amd: Add isp4 fw and hw interface

["Du, Bin" <bin.du@amd.com>]

Thanks, Hans, for your comments.

On 2/24/2026 8:21 PM, Hans Anda wrote:
> Hi Bin,
> 
> I have just read your patch and have a minor issues and a few style suggestions.
> 
>> ISP firmware controls ISP HW pipeline using dedicated embedded processor
>> called ccpu. The communication between ISP FW and driver is using commands
>> and response messages sent through the ring buffer. Command buffers support
>> either global setting that is not specific to the steam and support stream
> 
> One last typo of steam -> stream
> 
> 

Thanks for catching that. Will correct it.

>> +/* [...]
>> + * @brief Host and Firmware command & response channel.
>> + *        Two types of command/response channel.
>> + *          Type Global Command has one command/response channel.
>> + *          Type Stream Command has one command/response channel.
>> + *-----------                                        ------------
>> + *|         |       ---------------------------      |          |
>> + *|         |  ---->|  Global Command         |----> |          |
>> + *|         |       ---------------------------      |          |
>> + *|         |                                        |          |
>> + *|         |                                        |          |
>> + *|         |       ---------------------------      |          |
>> + *|         |  ---->|   Stream Command        |----> |          |
>> + *|         |       ---------------------------      |          |
>> + *|         |                                        |          |
>> + *|         |                                        |          |
>> + *|         |                                        |          |
>> + *|  HOST   |                                        | Firmware |
>> + *|         |                                        |          |
>> + *|         |                                        |          |
>> + *|         |       --------------------------       |          |
>> + *|         |  <----|  Global Response       |<----  |          |
>> + *|         |       --------------------------       |          |
>> + *|         |                                        |          |
>> + *|         |                                        |          |
>> + *|         |       --------------------------       |          |
>> + *|         |  <----|  Stream Response       |<----  |          |
>> + *|         |       --------------------------       |          |
>> + *|         |                                        |          |
>> + *|         |                                        |          |
>> + *-----------                                        ------------
>> + */
> 
> + *        Two types of command/response channel.
> + *          Type Global Command has one command/response channel.
> + *          Type Stream Command has one command/response channel.
> + *-----------                                        ------------
> + *|         |                                        |          |
> + *|         |       ---------------------------      |          |
> + *|         |  ---->|     Global Command      |----> |          |
> + *|         |       ---------------------------      |          |
> + *|         |       ---------------------------      |          |
> + *|         |  <----|     Global Response     |<---- |          |
> + *|         |       ---------------------------      |          |
> + *|         |                                        |          |
> + *|         |                                        |          |
> + *|         |                                        |          |
> + *|  HOST   |                                        | Firmware |
> + *|         |                                        |          |
> + *|         |                                        |          |
> + *|         |       ---------------------------      |          |
> + *|         |  ---->|     Stream Command      |----> |          |
> + *|         |       ---------------------------      |          |
> + *|         |       ---------------------------      |          |
> + *|         |  <----|     Stream Response     |<---- |          |
> + *|         |       ---------------------------      |          |
> + *|         |                                        |          |
> + *|         |                                        |          |
> + *-----------                                        ------------
> + */
> 
> This way the order of the text comment is structured the same as the visual
> comment. i made some adjustments for symmetry.  It's no error, it's a style
> suggestion.	
> 
> 

I think both are acceptable, my original order has been structured in 
accordance with the register definition from an engineering standpoint, 
all command registers first, then followed by all response registers.

>> +/*
>> + * @brief command ID format
>> + *        cmd_id is in the format of following type:
>> + *        type: indicate command type, global/stream commands.
>> + *        group: indicate the command group.
>> + *        id: A unique command identification in one type and group.
>> + *        |<-Bit31 ~ Bit24->|<-Bit23 ~ Bit16->|<-Bit15 ~ Bit0->|
>> + *        |      type       |      group      |       id       |
> 
> + *        id: A unique command identification in one type and group.
> + *        |<-Bit31~24->|<-Bit23~16->|<-  Bit15   ~   Bit0  ->|
> + *        |    type    |   group    |           id           |
> 
> So the width of the parts would co relate to the number of bits (8,8,16).
> Again, a suggestion
> 

Since this is about the bits layout, matching width with bit count isn't 
mandatory.

> + *        id: A unique command identification in one type and group.
> + *        |<-  Bit0   ~   Bit15  ->|<-Bit16~23->|<-Bit24~31->|
> + *        |           id           |   group    |    type    |
> + */
> 
> If there is no technical reason for numbering Right to Left you could flip it.
> Left to Right is simpler to read.
> I guess there is a technical reason for your way.
> 

I believe my original style is more common to describe bits layout, 
e.g., drivers/gpu/drm/imagination/pvr_device.h at line 466
It matches how:
- Humans read: left to right = most significant to least significant
- Hex values are written: 0x02010001, type=0x02, group=0x01, id=0x0001
- Register documentation from hardware vendors is formatted

>> +	/*
>> +	 * A check num for debug usage, host can set the buf_tags
>> +	 * to different number
> 
> +	 * to different numbers
> or
> +	 * to a different number
> 
> Both versions work grammatically.
> 
> 

Yes, will fix it.

>> + [...]
>> +};
>> +
>> +/* FW cmd ring buffer configuration */
> 
>> + [...]
>> +};
>> +
>> +/* FW resp ring buffer configuration */
>> +static struct isp4if_rb_config isp4if_resp_rb_config[ISP4IF_STREAM_ID_MAX] = {
>>   [...]
>> +/* FW log ring buffer configuration */
>> +static struct isp4if_rb_config isp4if_log_rb_config = {
>> +	.name = "LOG_RB",
>>
>> +
>> +	rd_ptr = isp4hw_rreg(ispif->mmio, rreg);
>> +	wr_ptr = isp4hw_rreg(ispif->mmio, wreg);
>> +
>> +	/* Read and write pointers are equal, indicating the ringbuf is empty
>> */
> 
>>   [...]
>> +	/*
>> +	 * Ignore one byte from the bytes free to prevent rd_ptr from equaling
>> +	 * wr_ptr when the ringbuf is full, because rd_ptr == wr_ptr is
>> +	 * supposed to indicate that the ringbuf is empty.
> 
> ringbuf -> ringbuffer or ring buffer, it's best to stick to one.
> just in case someone greps for. That is a suggestion as well.
> 

Yes, will change it to ring buffer.

> Good Work.
> I'm just getting to know c - That's why I only found grammar/ style issues.
> But maybe this way i will get into programming
> 

Welcome to this field.

> Greeting Hans

-- 
Regards,
Bin