Re: [RFC PATCH 1/1] dt-bindings: clk: Mstar msc313 clkgen mux

[Rob Herring <robh@kernel.org>]

On Sat, Feb 13, 2021 at 10:18:14AM +0900, Daniel Palmer wrote:
> Hi Stephen,
> 
> On Sat, 13 Feb 2021 at 09:11, Stephen Boyd <sboyd@kernel.org> wrote:
> > > +examples:
> > > +  - |
> > > +    clkgen_mux_mspi0: clkgen_mux_mspi0 {
> > > +      compatible = "mstar,msc313-clkgen-mux";
> > > +      regmap = <&clkgen>;
> > > +      offset = <0xcc>;
> > > +      #clock-cells = <1>;
> > > +      mstar,gate = <0>;
> > > +      mstar,mux-shift = <2>;
> > > +      mstar,mux-width = <2>;
> >
> > It looks like a node-per clk sort of binding which has been rejected
> > multiple times in the past. If the clks are spread across various
> > devices then it sounds like the mediatek design where they have many
> > syscon nodes that also register clks inside those register spaces. In
> > this case, I would expect the clkgen node to be registering clks. Given
> > that there isn't a reg property and there's these mstar specific
> > properties like shift/width it looks really wrong. Please don't do this.
> 
> Ok. I will rethink this. One of the problems I face here is that there
> isn't any documentation for what the clkgen looks like.

All the more reason to not do a node per clock.

> I have a list of offsets and bit positions for these muxes but very little else.
> Looking at the mediatek clock drivers it seems like they have a driver
> that consumes some register areas and then creates all of the muxes
> etc within those areas within the driver instead. If that's an
> acceptable solution I will go for that.
> There would probably be 2 compatible strings right now (one for the pm
> area and one for the normal area) and that would take a phandle to the
> syscon that holds the registers. Then there would be a big table of
> the offsets, masks etc in the driver.

Ideally, the 'syscon' is just the clock provider or a child node is.

Rob