diff mbox

[v2,RESEND] pwm: Add CLPS711X PWM support

Message ID 1393342067-9086-1-git-send-email-shc_work@mail.ru
State Superseded
Headers show

Commit Message

Alexander Shiyan Feb. 25, 2014, 3:27 p.m. UTC
Add a new driver for the ARM CLPS711X Pulse Width Modulator (PWM) interface.
This CPU contain two 4-bit PWM outputs with constant period, based on CPU
PLL frequency. PWM polarity is determined by hardware by power on reset.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 .../bindings/pwm/cirrus-clps711x-pwm.txt           |  15 ++
 drivers/pwm/Kconfig                                |   9 ++
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-clps711x.c                         | 176 +++++++++++++++++++++
 4 files changed, 201 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/cirrus-clps711x-pwm.txt
 create mode 100644 drivers/pwm/pwm-clps711x.c

Comments

Arnd Bergmann Feb. 25, 2014, 3:33 p.m. UTC | #1
On Tuesday 25 February 2014 19:27:47 Alexander Shiyan wrote:
> Add a new driver for the ARM CLPS711X Pulse Width Modulator (PWM) interface.
> This CPU contain two 4-bit PWM outputs with constant period, based on CPU
> PLL frequency. PWM polarity is determined by hardware by power on reset.
> 
> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>

Looks good, just one comment

> +++ b/Documentation/devicetree/bindings/pwm/cirrus-clps711x-pwm.txt
> @@ -0,0 +1,15 @@
> +* Cirris Logic CLPS711X PWM controller
> +
> +Required properties:
> +- compatible: Should be "cirrus,clps711x-pwm".
> +- reg: Physical base address and length of the controller's registers.
> +- clocks: phandle to the PWM reference clock.
> +- #pwm-cells: Should be 1. The cell specifies the index of the channel.
> +
> +Example:
> +	pwm: pwm@80000400 {
> +		compatible = "cirrus,clps711x-pwm";
> +		reg = <0x80000400 0x4>;
> +		clocks = <&clks 8>;
> +		#pwm-cells = <1>;
> +	};

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.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Shiyan Feb. 25, 2014, 3:47 p.m. UTC | #2
Вторник, 25 февраля 2014, 16:33 +01:00 от Arnd Bergmann <arnd@arndb.de>:
> On Tuesday 25 February 2014 19:27:47 Alexander Shiyan wrote:
> > Add a new driver for the ARM CLPS711X Pulse Width Modulator (PWM) interface.
> > This CPU contain two 4-bit PWM outputs with constant period, based on CPU
> > PLL frequency. PWM polarity is determined by hardware by power on reset.
> > 
> > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> 
> Looks good, just one comment
> 
> > +++ b/Documentation/devicetree/bindings/pwm/cirrus-clps711x-pwm.txt
> > @@ -0,0 +1,15 @@
> > +* Cirris Logic CLPS711X PWM controller
> > +
> > +Required properties:
> > +- compatible: Should be "cirrus,clps711x-pwm".
> > +- reg: Physical base address and length of the controller's registers.
> > +- clocks: phandle to the PWM reference clock.
> > +- #pwm-cells: Should be 1. The cell specifies the index of the channel.
> > +
> > +Example:
> > +	pwm: pwm@80000400 {
> > +		compatible = "cirrus,clps711x-pwm";
> > +		reg = <0x80000400 0x4>;
> > +		clocks = <&clks 8>;
> > +		#pwm-cells = <1>;
> > +	};
> 
> 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...

---
Arnd Bergmann Feb. 25, 2014, 3:50 p.m. UTC | #3
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.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Rutland Feb. 26, 2014, 4 p.m. UTC | #4
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.

Cheers,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Shiyan Feb. 26, 2014, 4:50 p.m. UTC | #5
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?

Thanks.
---
Mark Rutland Feb. 27, 2014, 10:27 a.m. UTC | #6
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Shiyan March 1, 2014, 6:06 a.m. UTC | #7
On Wed, 26 Feb 2014 16:00:46 +0000
Mark Rutland <mark.rutland@arm.com> wrote:

> 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.

Hello.

I want to elaborate on the current CLPS711X lineup:
PS7110  - EOL October 30, 2001
PS7111  - EOL October 30, 2001
EP7209  - EOL April 25, 2003
EP7211  - EOL April 25, 2003
EP7212  - EOL April 25, 2003
CS89712 - EOL August 15, 2005
EP7309  - Production
EP7311  - Production
EP7312  - Production

So at the moment, and long enough, produced only three models of CPUs.
I am sure that the hardware with old CPUs will not be able to use the
new kernel due to memory limitations, low CPU frequency, etc.

My suggestion is to always use a compatibility string for platform entirely,
ie "cirrus,clps711x-$UNIT", otherwise it may result in misleading.

In any case, I would like to hear the final decision on this issue,
since there are several drivers already using this scheme and some drivers
awaiting applying into the kernel.
I do not insist on its position, but just want to clarify to make changes
to existing drivers and use the approved compatibility string in the future.
At the current stage, these changes will not lead to anything terrible.

Thanks.
Arnd Bergmann March 1, 2014, 11:40 a.m. UTC | #8
On Saturday 01 March 2014, Alexander Shiyan wrote:
> I want to elaborate on the current CLPS711X lineup:
> PS7110  - EOL October 30, 2001
> PS7111  - EOL October 30, 2001
> EP7209  - EOL April 25, 2003
> EP7211  - EOL April 25, 2003
> EP7212  - EOL April 25, 2003
> CS89712 - EOL August 15, 2005
> EP7309  - Production
> EP7311  - Production
> EP7312  - Production

Thanks for the list. I wonder if there is a good place to keep this in the
kernel source for reference.

> So at the moment, and long enough, produced only three models of CPUs.
> I am sure that the hardware with old CPUs will not be able to use the
> new kernel due to memory limitations, low CPU frequency, etc.
> 
> My suggestion is to always use a compatibility string for platform entirely,
> ie "cirrus,clps711x-$UNIT", otherwise it may result in misleading.

From your list, it's pretty clear that there won't be any new chips named
PS711x for x > 1, so using that string is not ambiguous. That is good
to know. It's also the oldest chip in the family, so naming things after that
is consistent.

> In any case, I would like to hear the final decision on this issue,
> since there are several drivers already using this scheme and some drivers
> awaiting applying into the kernel.
> I do not insist on its position, but just want to clarify to make changes
> to existing drivers and use the approved compatibility string in the future.
> At the current stage, these changes will not lead to anything terrible.

How about we keep using the cirrus,clps711x as the string that drivers
match against, but change the DT representation to always list the specific
model in addition? That way we are prepared to handle quirks if we need to,
but you don't have to change the binding in incompatible ways.

This means you should list e.g.

	compatible = "cirrus,ep7309-pwm", "cirrus,clps711x-pwm";

or 

	compatible = "cirrus,clps7110-pwm", "cirrus,clps711x-pwm";

depending on the model. For units that were introduced in a later
model, I would recommend to use the first model that had it rather
than the original clps711x that didn't, like

	compatible = "cirrus,ep7311-kitchensink", "cirrus-ep7211-kitchensink";

For bindings that are already reviewed and merged, I wouldn't change
them any more even if they inaccurately list clps711x for units that
were not present in clps7111, but for new ones I'd prefer to follow
the common practice to your best knowledge.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Shiyan March 1, 2014, 1:56 p.m. UTC | #9
Суббота,  1 марта 2014, 12:40 +01:00 от Arnd Bergmann <arnd@arndb.de>:
> On Saturday 01 March 2014, Alexander Shiyan wrote:
> > I want to elaborate on the current CLPS711X lineup:
> > PS7110  - EOL October 30, 2001
> > PS7111  - EOL October 30, 2001
> > EP7209  - EOL April 25, 2003
> > EP7211  - EOL April 25, 2003
> > EP7212  - EOL April 25, 2003
> > CS89712 - EOL August 15, 2005
> > EP7309  - Production
> > EP7311  - Production
> > EP7312  - Production
> 
> Thanks for the list. I wonder if there is a good place to keep this in the
> kernel source for reference.
> 
> > So at the moment, and long enough, produced only three models of CPUs.
> > I am sure that the hardware with old CPUs will not be able to use the
> > new kernel due to memory limitations, low CPU frequency, etc.
> > 
> > My suggestion is to always use a compatibility string for platform entirely,
> > ie "cirrus,clps711x-$UNIT", otherwise it may result in misleading.
> 
> From your list, it's pretty clear that there won't be any new chips named
> PS711x for x > 1, so using that string is not ambiguous. That is good
> to know. It's also the oldest chip in the family, so naming things after that
> is consistent.
> 
> > In any case, I would like to hear the final decision on this issue,
> > since there are several drivers already using this scheme and some drivers
> > awaiting applying into the kernel.
> > I do not insist on its position, but just want to clarify to make changes
> > to existing drivers and use the approved compatibility string in the future.
> > At the current stage, these changes will not lead to anything terrible.
> 
> How about we keep using the cirrus,clps711x as the string that drivers
> match against, but change the DT representation to always list the specific
> model in addition? That way we are prepared to handle quirks if we need to,
> but you don't have to change the binding in incompatible ways.
> 
> This means you should list e.g.
> 
> 	compatible = "cirrus,ep7309-pwm", "cirrus,clps711x-pwm";
> 
> or 
> 
> 	compatible = "cirrus,clps7110-pwm", "cirrus,clps711x-pwm";
> 
> depending on the model. For units that were introduced in a later
> model, I would recommend to use the first model that had it rather
> than the original clps711x that didn't, like
> 
> 	compatible = "cirrus,ep7311-kitchensink", "cirrus-ep7211-kitchensink";
> 
> For bindings that are already reviewed and merged, I wouldn't change
> them any more even if they inaccurately list clps711x for units that
> were not present in clps7111, but for new ones I'd prefer to follow
> the common practice to your best knowledge.

Excellent, let's do so. I will prepare a patch that will change compatibility
strings for examples in the current bindings documents.
For this driver (PWM), I just send a third version.

---
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/pwm/cirrus-clps711x-pwm.txt b/Documentation/devicetree/bindings/pwm/cirrus-clps711x-pwm.txt
new file mode 100644
index 0000000..e252f6d
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/cirrus-clps711x-pwm.txt
@@ -0,0 +1,15 @@ 
+* Cirris Logic CLPS711X PWM controller
+
+Required properties:
+- compatible: Should be "cirrus,clps711x-pwm".
+- reg: Physical base address and length of the controller's registers.
+- clocks: phandle to the PWM reference clock.
+- #pwm-cells: Should be 1. The cell specifies the index of the channel.
+
+Example:
+	pwm: pwm@80000400 {
+		compatible = "cirrus,clps711x-pwm";
+		reg = <0x80000400 0x4>;
+		clocks = <&clks 8>;
+		#pwm-cells = <1>;
+	};
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 22f2f28..d3a2c26 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -71,6 +71,15 @@  config PWM_BFIN
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-bfin.
 
+config PWM_CLPS711X
+	tristate "CLPS711X PWM support"
+	depends on ARCH_CLPS711X || COMPILE_TEST
+	help
+	  Generic PWM framework driver for Cirrus Logic CLPS711X.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-clps711x.
+
 config PWM_EP93XX
 	tristate "Cirrus Logic EP93xx PWM support"
 	depends on ARCH_EP93XX
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index d8906ec..d676681 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -4,6 +4,7 @@  obj-$(CONFIG_PWM_AB8500)	+= pwm-ab8500.o
 obj-$(CONFIG_PWM_ATMEL)		+= pwm-atmel.o
 obj-$(CONFIG_PWM_ATMEL_TCB)	+= pwm-atmel-tcb.o
 obj-$(CONFIG_PWM_BFIN)		+= pwm-bfin.o
+obj-$(CONFIG_PWM_CLPS711X)	+= pwm-clps711x.o
 obj-$(CONFIG_PWM_EP93XX)	+= pwm-ep93xx.o
 obj-$(CONFIG_PWM_IMX)		+= pwm-imx.o
 obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
diff --git a/drivers/pwm/pwm-clps711x.c b/drivers/pwm/pwm-clps711x.c
new file mode 100644
index 0000000..fafb6a0
--- /dev/null
+++ b/drivers/pwm/pwm-clps711x.c
@@ -0,0 +1,176 @@ 
+/*
+ * Cirrus Logic CLPS711X PWM driver
+ *
+ * Copyright (C) 2014 Alexander Shiyan <shc_work@mail.ru>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+
+struct clps711x_chip {
+	struct pwm_chip chip;
+	void __iomem *pmpcon;
+	struct clk *clk;
+	spinlock_t lock;
+};
+
+static inline struct clps711x_chip *to_clps711x_chip(struct pwm_chip *chip)
+{
+	return container_of(chip, struct clps711x_chip, chip);
+}
+
+static void clps711x_pwm_update_val(struct clps711x_chip *priv, u32 n, u32 v)
+{
+	/* PWM0 - bits 4..7, PWM1 - bits 8..11 */
+	u32 shift = (n + 1) * 4;
+	unsigned long flags;
+	u32 tmp;
+
+	spin_lock_irqsave(&priv->lock, flags);
+
+	tmp = readl(priv->pmpcon);
+	tmp &= ~(0xf << shift);
+	tmp |= v << shift;
+	writel(tmp, priv->pmpcon);
+
+	spin_unlock_irqrestore(&priv->lock, flags);
+}
+
+static unsigned int clps711x_get_duty(struct pwm_device *pwm, unsigned int v)
+{
+	/* Duty cycle 0..15 max */
+	return DIV_ROUND_CLOSEST(v * 0xf, pwm_get_period(pwm));
+}
+
+static int clps711x_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct clps711x_chip *priv = to_clps711x_chip(chip);
+	unsigned int freq = clk_get_rate(priv->clk);
+
+	if (!freq)
+		return -EINVAL;
+
+	/* Store constant period value */
+	pwm_set_period(pwm, DIV_ROUND_CLOSEST(NSEC_PER_SEC, freq));
+
+	return 0;
+}
+
+static int clps711x_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			       int duty_ns, int period_ns)
+{
+	struct clps711x_chip *priv = to_clps711x_chip(chip);
+	unsigned int duty;
+
+	if (period_ns != pwm_get_period(pwm))
+		return -EINVAL;
+
+	duty = clps711x_get_duty(pwm, duty_ns);
+	clps711x_pwm_update_val(priv, pwm->hwpwm, duty);
+
+	return 0;
+}
+
+static int clps711x_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct clps711x_chip *priv = to_clps711x_chip(chip);
+	unsigned int duty;
+
+	duty = clps711x_get_duty(pwm, pwm_get_duty_cycle(pwm));
+	clps711x_pwm_update_val(priv, pwm->hwpwm, duty);
+
+	return 0;
+}
+
+static void clps711x_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct clps711x_chip *priv = to_clps711x_chip(chip);
+
+	clps711x_pwm_update_val(priv, pwm->hwpwm, 0);
+}
+
+static const struct pwm_ops clps711x_pwm_ops = {
+	.request = clps711x_pwm_request,
+	.config = clps711x_pwm_config,
+	.enable = clps711x_pwm_enable,
+	.disable = clps711x_pwm_disable,
+	.owner = THIS_MODULE,
+};
+
+static struct pwm_device *clps711x_pwm_xlate(struct pwm_chip *chip,
+					     const struct of_phandle_args *args)
+{
+	if (args->args[0] >= chip->npwm)
+		return ERR_PTR(-EINVAL);
+
+	return pwm_request_from_chip(chip, args->args[0], NULL);
+}
+
+static int clps711x_pwm_probe(struct platform_device *pdev)
+{
+	struct clps711x_chip *priv;
+	struct resource *res;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->pmpcon = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->pmpcon))
+		return PTR_ERR(priv->pmpcon);
+
+	priv->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(priv->clk))
+		return PTR_ERR(priv->clk);
+
+	priv->chip.ops = &clps711x_pwm_ops;
+	priv->chip.dev = &pdev->dev;
+	priv->chip.base = -1;
+	priv->chip.npwm = 2;
+	priv->chip.of_xlate = clps711x_pwm_xlate;
+	priv->chip.of_pwm_n_cells = 1;
+
+	spin_lock_init(&priv->lock);
+
+	platform_set_drvdata(pdev, priv);
+
+	return pwmchip_add(&priv->chip);
+}
+
+static int clps711x_pwm_remove(struct platform_device *pdev)
+{
+	struct clps711x_chip *priv = platform_get_drvdata(pdev);
+
+	return pwmchip_remove(&priv->chip);
+}
+
+static const struct of_device_id __maybe_unused clps711x_pwm_dt_ids[] = {
+	{ .compatible = "cirrus,clps711x-pwm", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, clps711x_pwm_dt_ids);
+
+static struct platform_driver clps711x_pwm_driver = {
+	.driver = {
+		.name = "clps711x-pwm",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(clps711x_pwm_dt_ids),
+	},
+	.probe = clps711x_pwm_probe,
+	.remove = clps711x_pwm_remove,
+};
+module_platform_driver(clps711x_pwm_driver);
+
+MODULE_AUTHOR("Alexander Shiyan <shc_work@mail.ru>");
+MODULE_DESCRIPTION("Cirrus Logic CLPS711X PWM driver");
+MODULE_LICENSE("GPL");