On Mon, Dec 02, 2024 at 04:08:32PM +0530, Mukesh Kumar Savaliya wrote: > 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. The cover-letter is generally not included in the git history, so that explanation would be lost on future readers. > > > 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. I think the understanding is necessary, but that the wording should be different. Imaging you're implementing MukeshOS and reading this binding document to understand what you're expected to do in your I2C driver when you see this property. Similarly, the binding document should be sufficiently clear such that a newly hired colleague of ours would understand if they should put this property or not in the dts file they are writing. > > > > > 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 ? > What I'm saying is that this binding is for the serial engine, if you need to describe what SE or a serial engine is you should do that in the top-level description, not within one of the properties (or in a possible future repeat that explanation in multiple different properties). > I will need to get agreement on this patch first and then upload V6. > Sounds good. Regards, Bjorn > > Regards, > > Bjorn > > > > > + type: boolean > > > + > > > reg: > > > maxItems: 1 > > > -- > > > 2.25.1 > > >
