Re: [PATCH v2] arm64: dts: qcom: msm8916-samsung-serranove: Add RT5033 PMIC with charger

[Jakob Hauser]

Hi Krzysztof,

On 12.07.23 22:28, Krzysztof Kozlowski wrote:
> On 12/07/2023 21:50, Jakob Hauser wrote:
...
> > On 11.07.23 08:13, Krzysztof Kozlowski wrote:
> > ...
> > > This appeared in today's next next-20230711 and causes new warnings
> > >
> > > msm8916-samsung-serranove.dtb: extcon@14: 'connector' does not match any
> > > of the regexes: 'pinctrl-[0-9]+'
> > > https://krzk.eu/#/builders/90/builds/40/steps/17/logs/stdio
> > >
> > > The commit mentions rt5033, but that is not the schema being here
> > > tested, so clearly this is wrong or bindings were not updated.
> > >
> > > Please fix (and test your future patches).
> >
> > The implementation you see in this patch follows the guidance of yours
> > and Rob’s. I already expressed my discontent about it before.
> >
> > To solve the message, the dt-bindings of extcon device sm5502-muic [1]
> > would need to be changed to allow a "connector" sub-node. That’s not the
> > right approach.
> >
> > I still have the impression that the current implementation is based on
> > misunderstandings. I do think Rob’s comment that excon phandle being
> > deprecated [2] is valid for the USB subsystem. Your suggestion to check
> > "ports graph", "orientation" and "usb-role-switch" applies to USB
> > subsystem as well [3]. Rob took the time to add more explanation [4] but
> > it’s still about handling connectors in the more strict sense, which is
> > circling around UBS subsystem.
> >
> > These discussions led to a strangely mixed-up result. I was pushed to
> > implement the USB subsystem connector approach upon an excton subsystem
> > device. As the standard USB connector approach didn’t fit, we switched
> > to a vendor-specific connector phandle [5]. In fact it’s kind of a
> > workaround for the extcon phandle.
> >
> > The extcon device sm5504 is a real piece of hardware. It’s not handled
> > by USB subsystem but by extcon subsystem. The excton subsystem has a
> > method implemented to get the device by phandle [6].
>
> I am not sure if we discuss the same problem. My email was about the DTS
> and bindings, not whether this works in Linux drivers. From your reply I
> feel that this patch might actually not work? This would be quite
> confusing...
>
> You added new child node "connector" to the siliconmitus,sm5504-muic, so
> all I would expect that we miss here only updating that binding.
> Assuming that your code was working...

The patch works.

> > I therefore propose to use the phandle of the extcon subsystem.
>
> extcon in the bindings? Then we would be back to square one.

If square one is a reasonable proposal, it should be considered. 
Discussions can go astray. It's a process.

The extcon subsystem offers methods to access an excton device. If there 
is extcon hardware installed, using one of those methods is a pretty 
straight-forward and an obvious approach.

What speaks against the use of this method? Rob argued that the 
complexity of connector implementation grew over time and therefore 
standard connector bindings should be used. I understand this and the 
example you linked in the previous discussion shows such a complexity. 
But this is about USB subsystem.

USB subsystem is not involved here. Why involving it by force?

I sure don't have the full picture. However, so far the whole discussion 
seems to be based on the confusion of different extcon phandles: 
"virtual" ones in USB subsystem and "real" ones in extcon subsystem. If 
that's the case, we've been drifting into the wrong direction all the time.

> > I mean
> > extcon subsystem, not USB subsystem. In case you disagree, I kindly ask
> > you to take more time to answer in more detail and especially
> > case-related.
>
> Assuming your patch works, I think above is quite specific answer - new
> property is missing in sm5504 binding.

I don't think this is the right way to get rid of the issue. Sure, 
technically the message disappears. Contentwise, however, we're sneaking 
the confusion of our discussion into the dt-bindings. Imagine what the 
description of that "connector" property in 
siliconmitus,sm5502-muic.yaml would look like: "Standard USB connector 
node according to usb-connector.yaml for accessing the extcon device via 
devicetree." A device in the extcon subsystem doesn't need this, the 
extcon subsystem already provides the method to access the extcon device 
via devicetree.

Well, I guess we would silently skip a description like that by changing 
"additionalProperties" from false to true.

Why do I make up such a big thing if the message could be made disappear 
that easily (and burning time of yours, sorry)? This mix-up we're 
implementing here is confusing. It's not helpful for further development 
and implementation of rt5033 and similar hardware arrangements. The 
issue that came up within the samsung-serranove dts patch here is a good 
indication of that.

I can prepare a patchset to dissolve this USB/extcon mix-up (basically 
square one, as you called it).

Alternatively, if all my tries to clarify a possible misunderstanding 
are in vain and no one else intervenes, I guess I have no other option 
than preparing a patch to change the dt-bindungs of 
siliconmitus,sm5502-muic.yaml.

> > And specifically to Krzysztof I ask for more politeness in
> > your way of communicating. I understand you’re answering hundreds of
> > requests a day but the communication we had in the past weeks is really
> > frustrating.
>
> Sorry to hear that, please accept my apologies. I went through all my
> replies to you in past few weeks and could not find any particular
> impolite behavior from my side.

I'm not used to the fast-paced interaction on the kernel lists. Maybe I 
have mistaken some of your comments. In that case sorry for my accusation.

...

Kind regards,
Jakob