Hi Bjorn, Thanks for the review !
On 11/30/2024 10:15 AM, Bjorn Andersson wrote:
On Fri, Nov 29, 2024 at 08:13:54PM +0530, Mukesh Kumar Savaliya wrote:
Adds qcom,shared-se flag usage. Use this flag when I2C serial controller
needs to be shared in multiprocessor system(APPS,Modem,ADSP) environment.
Per https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
your commit message should start with a description of your problem.
"Add" isn't the right word to start a problem description with.
Problem statement i have explained in cover letter, let me add here too
in V6. I thought same story gets repeated here. Will not start with Add,
but problem statement and need of this flag.
SE(Serial Engine HW controller acting as protocol master controller) is an
I2C controller. Basically a programmable SERDES(serializer/deserializer)
"Basically"?
will remove this.
coupled with data DMA entity, capable in handling a bus protocol, and data
moves to/from system memory.
Two clients from different processors can share an I2C controller for same
slave device OR their owned slave devices. Assume I2C Slave EEPROM device
connected with I2C controller. Each client from ADSP SS and APPS Linux SS
can perform i2c transactions.
The DeviceTree binding describes properties used to describe the
hardware; your commit message describes what a SE is and that it can
exist can exist in a configuration with multiple client etc etc.
I have explained the use of flag, and background surrounding to the
feature. See the V4 and V5 and earlier, where i was required to explain
and open up about "what is SE" ?
Because of the SE word in flag name, i had to expand with explanation.
Transfer gets serialized by Lock TRE + DMA xfer + Unlock TRE at HW level.
This isn't what this patch implements. It defines a property which when
specified means to the OS that any DMA transfers should be performed
using TRE lock/unlock operations.
I agree, it adds onto understanding about the flag feature. I can remove
this statement in V6. Let me get complete agreement.
Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@xxxxxxxxxxx>
---
.../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
index 9f66a3bb1f80..88682a333399 100644
--- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
+++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
@@ -60,6 +60,14 @@ properties:
power-domains:
maxItems: 1
+ qcom,shared-se:
+ description: True if I2C controller is shared between two or more system processors.
This attempts to describe the property.
I agree, thats why i limited but there was an ask to understand what is
SE ? hence i added below.
+ SE(Serial Engine HW controller working as protocol master controller) is an
+ I2C controller. Basically, a programmable SERDES(serializer/deserializer)
+ coupled with data DMA entity, capable in handling a bus protocol, and data
+ moves to/from system memory.
But this is basically just 4 lines of text expanding the acronym "se",
but while it might give some insight into what this binding (the whole
binding) is about, I'm afraid it doesn't add value to the understanding
of the property...
""se" is also not explained in the binding - please open it and look for
such explanation."
It was required to explain based on comment on V4, V5 hence i did.
Please let me know if one line is enough to explain flag usage OR we
need exact description from the hardware programming guide ?
I will need to get agreement on this patch first and then upload V6.
Regards,
Bjorn
+ type: boolean
+
reg:
maxItems: 1
--
2.25.1
