Re: [PATCH v2 RESEND] pwm: Add CLPS711X PWM support

[Mark Rutland <mark.rutland@arm.com>]

On Wed, Feb 26, 2014 at 04:50:46PM +0000, Alexander Shiyan wrote:
> Hello.
> 
> Среда, 26 февраля 2014, 16:00 UTC от Mark Rutland <mark.rutland@arm.com>:
> > On Tue, Feb 25, 2014 at 03:50:32PM +0000, Arnd Bergmann wrote:
> > > On Tuesday 25 February 2014 19:47:57 Alexander Shiyan wrote:
> > > > Вторник, 25 февраля 2014, 16:33 +01:00 от Arnd Bergmann <arnd@arndb.de>:
> > > > > On Tuesday 25 February 2014 19:27:47 Alexander Shiyan wrote:
> > > => > 
> > > > > We really want to avoid wildcards in compatible strings. Can you call this
> > > > > "cirrus,cs89712-pwm" to match the first SoC that came with this hardware?
> > > > > Obviously if there was some chip before that (I'm not familiar with the
> > > > > model numbers), use that instead.
> > > > > 
> > > > > You can either list all chips you know that have this in the driver,
> > > > > or you use "cirrus,cs89712-pwm" as the fallback, and use the name of
> > > > > the SoC you have as the more specific string.
> > > > 
> > > > It seems that in this case we will need to modify the compatibility string
> > > > for other drivers that are already available in the kernel...
> > > 
> > > Ah, right. I missed the binding for gpio and serial going in.
> > > 
> > > DT maintainers, any suggestion on how we should proceed here?
> > > 
> > > AFAICT, clps711x platform support is still work-in-progress, so we don't
> > > have any upstream users to worry about yet, but changing them is still
> > > going to be slightly messy.
> > 
> > When allocating any new compatible strings, as Arnd says, we should
> > avoid wildcards as they're usually far too encompassing and end up
> > causing more trouble than they're worth.
> > 
> > In this case how problematic are the wildcard strings?
> > 
> > I assume we still have specific strings earlier in any compatible list
> > in any case? If not, and there are possible differences, that should be
> > fixed right away.
> > 
> > We have a few options:
> > 
> > a) Update the docs only.
> > 
> >    Note in the docs that "cirrus,clps711x-$UNIT" means anything
> >    compatible with the $UNIT in the cs89712. This may be
> >    counter-intuitive, and if a new clps711x platform comes out with an
> >    incompatible $UNIT it should omit "cirrus,clps711x-$UNIT" from its
> >    compatible list, but otherwise no harm done.
> > 
> > b) Deprecate the existing string.
> >   
> >    Add "cirrus,cs89712-$UNIT" to the binding docs and driver. Mark
> >    "cirrus,clps711x-$UNIT" as deprecated in the binding document.
> >    Replace "cirrus,clps711x-$UNIT" with "cirrus,cs89712-$UNIT" in all
> >    DTs.
> > 
> >    This will mean new DTBs won't work with an older kernel, but as
> >    support is a work in progress that might not matter. Old DTBs will
> >    continue to function.
> > 
> >    Iff you can guarantee that the old string is not possibly being used
> >    by anyone, it can be dropped from the driver. If not it has to remain
> >    (though can be commented to be deprecated), so that old DTBs
> >    function. It's probably best to leave it there.
> >   
> > c) Deprecate, maintaining (forwards) compatibility.
> > 
> >    As above, but rather than replacing "cirrus,clps711x-$UNIT" with
> >    "cirrus,cs89712-$UNIT" in DTs, place "cirrus,cs89712-$UNIT" before
> >    "cirrus,clps711x-$UNIT" in DTs. This allows new DTBs to work with
> >    older kernels too. Depending on what level of support you have in
> >    mainline currently this may or may not be an important consideration.
> 
> If I understood correctly, in the variant "a", we change nothing.
> Ie compatibility string is a global to entire platform, and in case of any
> CPU differences, we simply add the additional compatibility string fully
> meets the CPU name, for example for Cirrus Logic EP7312 this will be
> like: "cirrus,ep7312-hw", "cirrus,clps711x-hw".
> Correct?

That depends. Option a still requires that we give a definite meaning to
"cirrus,clps711x-$UNIT" to mean compatible with the unit on the cs89712.

If a new unit is compatible with this, then it can have
"cirrus,clps711x-$UNIT" as a fallback in its compatible list.

If a new unit is not compatible and must be handled differently, then it
should not have "cirrus,clps711x-$UNIT" in its compatible list.

The naming you suggest for "cirrus,ep7312-$UNIT" seems sensible in
either case.

Cheers,
Mark.