Re: [PATCH v8 3/7] media: platform: amd: Add isp4 fw and hw interface
From: Hans Anda
Date: Fri Feb 27 2026 - 12:19:25 EST
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
> +/* [...]
> + * @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.
> +/*
> + * @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
+ * 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.
> + /*
> + * 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.
> + [...]
> +};
> +
> +/* 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.
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
Greeting Hans