Re: [PATCH RFC 1/5] phy: qcom-qmp: add support for pipe clock muxing

[Johan Hovold <johan@kernel.org>]

[ Again, please trim unnecessary context from your replies. ]

On Fri, Apr 22, 2022 at 01:35:01PM +0300, Dmitry Baryshkov wrote:
> On Fri, 22 Apr 2022 at 13:20, Johan Hovold <johan@kernel.org> wrote:
> >
> > On Thu, Apr 21, 2022 at 02:08:27PM +0300, Dmitry Baryshkov wrote:
> > > On 21/04/2022 13:20, Johan Hovold wrote:

> > > > +   /* Get optional pipe clock mux and default reference source clock. */
> > > > +   qphy->pipemux_clk = of_clk_get_by_name(np, "mux");
> > > > +   if (IS_ERR(qphy->pipemux_clk)) {
> > > > +           ret = PTR_ERR(qphy->pipemux_clk);
> > > > +           if (ret == -EPROBE_DEFER)
> > > > +                   return ret;
> > > > +
> > > > +           qphy->pipemux_clk = NULL;

> > > > +   } else {
> > > > +           qphy->piperef_clk = of_clk_get_by_name(np, "ref");
> > > > +           if (IS_ERR(qphy->piperef_clk)) {
> > > > +                   ret = PTR_ERR(qphy->piperef_clk);
> > > > +                   return dev_err_probe(dev, ret,
> > > > +                                        "failed to get lane%d piperef_clk\n",
> > > > +                                        id);
> > > > +           }
> > > > +   }
> > > > +
> > >
> > > As a second thought.
> > > This needs to be more explicit. If the chipset requires the pipe clock
> > > remuxing, we must fail if the clocks were not provided. So depending on
> > > the qmp instance/property the driver should either use devm_clk_get()
> > > (instead of _optional) or skip this block completely.
> >
> > No, the kernel is not a DT validator (and we have the YAML bindings for
> > that now).
> 
> It is not about DT validation. It is about passing a correct DT.

Heh. That's the same thing.

> The file can come up from the kernel. It can come from the older
> kernel.  OR it can come from the vendor. Or it even might be being a
> part of firmware flashed into the device.  So we can not assume that
> the DT is correct just because the in-kernel DT passes YAML
> validation.

Again, no. The kernel does not need to implement DT validation and can
assume that the DT describes the hardware accurately. If the DT says
there's a mux, the driver can use it. If there's no mux in DT, the
driver can assume it isn't there.

The only thing that complicates things here is the sc7280 dts which has
been released in 5.16. We don't care about Qualcomm's kernels and dts.

> So, as I wrote, the whole patchset needs much more care about compatibility.
> 
> > > But this will not work with earlier DTS files.
> >
> > So this is not a problem (but if we really wanted to have the driver
> > validate the DT it can be done by updating the compatible strings).
> 
> We should not update compatible strings just because the driver
> changes. Compat strings describe the hardware, not the Linux point of
> view on it.

We can, it's a documented practise in case a binding needs to be
updated in an incompatible way:

	https://www.kernel.org/doc/html/latest/devicetree/bindings/ABI.html

But I don't think it'll be needed here.

Johan