Re: [PATCH v2 1/4] dt-bindindgs: i2c: qcom,i2c-geni: Document shared flag

[Krzysztof Kozlowski]

On 10/09/2024 11:09, Mukesh Kumar Savaliya wrote:
> Thanks Krzysztof.
> 
> On 9/7/2024 2:34 PM, Krzysztof Kozlowski wrote:
>> On 06/09/2024 21:14, Mukesh Kumar Savaliya wrote:
>>> Adds qcom,shared-se flag usage. Use this when particular I2C serial
>>> controller needs to be shared between two subsystems.
>>
>> <form letter>
>> Please use scripts/get_maintainers.pl to get a list of necessary people
>> and lists to CC (and consider --no-git-fallback argument). It might
>> happen, that command when run on an older kernel, gives you outdated
>> entries. Therefore please be sure you base your patches on recent Linux
>> kernel.
>>
>> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
>> people, so fix your workflow. Tools might also fail if you work on some
>> ancient tree (don't, instead use mainline) or work on fork of kernel
>> (don't, instead use mainline). Just use b4 and everything should be
>> fine, although remember about `b4 prep --auto-to-cc` if you added new
>> patches to the patchset.
>> </form letter>
>>
>> You already got this comment, so how many times it has to be repeated?
>> Your process is just wrong if you do not use the tools for this.
>>
> Sorry, I was already using scripts/get_maintainer.pl but i kept everyone 
> into To list (That's my mistake here). I shall keep maintainers in TO 
> list and rest in CC list.

No, To or Cc does not matter. Your list is just incomplete.

> 
> Question: With <Form Letter> , are you asking to add letter in this 
> first patch ? I have cover letter, but it will get removed when patch 
> gets merged. Please help suggest and clarify.

No, it's just template. Form letter... I am just bored to repeat the
same comment.

>>
>>>
>>> SE = Serial Engine, meant for I2C controller here.
>>> TRE = Transfer Ring Element, refers to Queued Descriptor.
>>>
>>> Example :
>>> Two clients from different SS can share an I2C SE for same slave device
>>
>> What is SS?
>>
> SS = Subsystem (EE - Execution Environment, can be Apps 
> processor/TZ/Modem/ADSP etc). Let me add this too in next patch.

Yes, please explain in the binding itself.

>>> 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.
>>> This gets serialized by lock TRE + DMA Transfers + Unlock TRE at HW level.
>>>
>>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@xxxxxxxxxxx>
>>> ---
>>>   Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml | 4 ++++
>>>   1 file changed, 4 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..ae423127f736 100644
>>> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>>> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>>> @@ -60,6 +60,10 @@ properties:
>>>     power-domains:
>>>       maxItems: 1
>>>   
>>> +  qcom,shared-se:
>>> +    description: True if I2C needs to be shared between two or more subsystems.
>>
>> What is a subsystem? With commit msg I still do not understand this.
> SS = Subsystem (EE - Execution Environment, can be Apps 
> processor/TZ/Modem/ADSP etc). Let me add EE too with full form.
>> Maybe presence of hwlock defines it anyway, so this is redundant?
> No, this flag is required. As hwlock comes into picture if this flag is 

Flag is required? By what? Sorry, you push your downstream solution to us.

> defined. So flag is acting as a condition to take hwlock TRE 
> descriptor(transfer ring element). Hope i could answer your query.

Hm, not sure, maybe indeed hwlock would not be enough. However I think
existing binding misses hwlock property.

Best regards,
Krzysztof