Re: [PATCH v8 3/7] media: platform: amd: Add isp4 fw and hw interface
From: Du, Bin
Date: Sat Feb 28 2026 - 03:52:17 EST
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