Re: [PATCH 1/4] dt-bindings: display/msm: Redocument the dp-controller for QCS8300

[Yongxing Mou <quic_yongmou@quicinc.com>]

On 2025/2/12 21:44, Konrad Dybcio wrote:
> On 12.02.2025 12:28 PM, Krzysztof Kozlowski wrote:
>> On 12/02/2025 12:13, Yongxing Mou wrote:
>>>
>>>
>>> On 2025/2/12 16:35, Krzysztof Kozlowski wrote:
>>>> On 12/02/2025 08:12, Yongxing Mou wrote:
>>>>> We need to enable mst for qcs8300, dp0 controller will support 2 streams
>>>>> output. So not reuse sm8650 dp controller driver and will add a new driver
>>>>> patch for qcs8300 mst feature. Modify the corresponding dt-bingding file
>>>>> to compatible with the qcs8300-dp.
>>>>>
>>>>> Signed-off-by: Yongxing Mou <quic_yongmou@quicinc.com>
>>>> NAK. You just said qcs8300 is compatible with sm8650. I did not ask
>>>> about drivers, I asked about hardware.
>>>>
>>>> This is messy approach. Describe properly the hardware first, instead of
>>>> sending two conflicting patchsets.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>
>>> Hi, Krzysztof, thanks for reviewing, i want to explain why i submitted
>>> this patch. Patch
>>> https://lore.kernel.org/all/20250114-dts_qcs8300-v3-1-d114cc5e4af9@quicinc.com/
>>> and
>>> https://lore.kernel.org/all/20250120-mdssdt_qcs8300-v4-2-1687e7842125@quicinc.com/
>>> is the qcs8300 display enablement changes. It base on current linux base
>>> code and it only support SST mode, so in the SST mode, qcs8300 dp
>>> controller driver is quite same with sm8650, struct msm_dp_desc only
>>> have 3 members(io_start, id and wide_bus_supported) and they are same
>>> both in qcs8300 and sm8650, so we reuse it. BTW, for dp phy hardware
>>> version, qcs8300 and sm8650 is different.
>>
>> No. In one patchset you claim hardware is like that, in other patchset
>> you say hardware is different.
>>
>> Sorry, hardware does not change based on your patchsets.
>>
>> Sort out this before posting new versions.
> 
> In other words, fallback compatibles must be chosen with features that
> are present in hardware, but not yet supported upstream in mind.
> 
> It's totally fine (and even preferred/expected) to describe hardware resources
> (such as MST clocks here) when initially creating bindings for a piece of hw,
> even though the drivers don't use them yet at that moment.
> 
> dt-bindings are supposed to give the OS a complete representation of the
> hardware and ideally be immutable (which is a struggle, but we're getting
> better). Driver specifics should not influence your decisions (or at least
> do so very minimally) when adding these.
> 
> Now you're in a """good""" position as the display bindings haven't been merged
> yet, so you can still upload a new patchset where the description is more
> accurate. If it was merged, we'd have to break the ABI or add some crazy
> workarounds..
> 
> Please coalesce this patchset with the "add 8300 display support" one.
> 
> Please also describe all 4 MST clocks and whatever other clocks/resets that
> may be necessary down the line.
>   
> Konrad
Thanks, will update it in this patch "add 8300 display support". Because 
this will depend on this change: 
https://patchwork.freedesktop.org/series/142016/. we will first fix our 
dependecy comments and then repost it.