diff mbox series

[v8,15/26] pwm: jz4740: Add support for the JZ4725B

Message ID 20181212220922.18759-16-paul@crapouillou.net
State Superseded
Headers show
Series Ingenic TCU patchset v8 | expand

Commit Message

Paul Cercueil Dec. 12, 2018, 10:09 p.m. UTC
The PWM in the JZ4725B works the same as in the JZ4740, except that it
only has 6 channels available instead of 8.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Acked-by: Thierry Reding <thierry.reding@gmail.com>
---

Notes:
     v5: New patch
    
     v6: - Move of_device_id structure back at the bottom (less noise in
           patch)
         - Use device_get_match_data() instead of of_* variant
    
     v7: No change

     v8: No change

 drivers/pwm/pwm-jz4740.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

Comments

Uwe Kleine-König Dec. 13, 2018, 9:24 a.m. UTC | #1
Hello,

On Wed, Dec 12, 2018 at 11:09:10PM +0100, Paul Cercueil wrote:
> The PWM in the JZ4725B works the same as in the JZ4740, except that it
> only has 6 channels available instead of 8.

this driver is probed only from device tree? If yes, it might be
sensible to specify the number of PWMs there and get it from there.
There doesn't seem to be a generic binding for that, but there are
several drivers that could benefit from it. (This is a bigger project
though and shouldn't stop your patch. Still more as it already got
Thierry's ack.)

Best regards
Uwe
Paul Cercueil Dec. 13, 2018, 2:03 p.m. UTC | #2
Hi,

Le jeu. 13 déc. 2018 à 10:24, Uwe Kleine-König 
<u.kleine-koenig@pengutronix.de> a écrit :
> Hello,
> 
> On Wed, Dec 12, 2018 at 11:09:10PM +0100, Paul Cercueil wrote:
>>  The PWM in the JZ4725B works the same as in the JZ4740, except that 
>> it
>>  only has 6 channels available instead of 8.
> 
> this driver is probed only from device tree? If yes, it might be
> sensible to specify the number of PWMs there and get it from there.
> There doesn't seem to be a generic binding for that, but there are
> several drivers that could benefit from it. (This is a bigger project
> though and shouldn't stop your patch. Still more as it already got
> Thierry's ack.)

I think there needs to be a proper guideline, as there doesn't seem to 
be
a consensus about this. I learned from emails with Rob and Linus 
(Walleij)
that I should not have in devicetree what I can deduce from the 
compatible
string.

> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König        
>     |
> Industrial Linux Solutions                 | 
> http://www.pengutronix.de/  |
Thierry Reding Dec. 13, 2018, 4:18 p.m. UTC | #3
On Thu, Dec 13, 2018 at 03:03:15PM +0100, Paul Cercueil wrote:
> Hi,
> 
> Le jeu. 13 déc. 2018 à 10:24, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> a écrit :
> > Hello,
> > 
> > On Wed, Dec 12, 2018 at 11:09:10PM +0100, Paul Cercueil wrote:
> > >  The PWM in the JZ4725B works the same as in the JZ4740, except that
> > > it
> > >  only has 6 channels available instead of 8.
> > 
> > this driver is probed only from device tree? If yes, it might be
> > sensible to specify the number of PWMs there and get it from there.
> > There doesn't seem to be a generic binding for that, but there are
> > several drivers that could benefit from it. (This is a bigger project
> > though and shouldn't stop your patch. Still more as it already got
> > Thierry's ack.)
> 
> I think there needs to be a proper guideline, as there doesn't seem to be
> a consensus about this. I learned from emails with Rob and Linus (Walleij)
> that I should not have in devicetree what I can deduce from the compatible
> string.

Correct. If the compatible string already defines the number of PWMs
that the hardware exposes, there's no need to explicitly say so again in
DT.

Thierry
Uwe Kleine-König Dec. 13, 2018, 8:42 p.m. UTC | #4
[Adding Linus Walleij to Cc:]

Hello,

On Thu, Dec 13, 2018 at 03:03:15PM +0100, Paul Cercueil wrote:
> Le jeu. 13 déc. 2018 à 10:24, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> a écrit :
> > On Wed, Dec 12, 2018 at 11:09:10PM +0100, Paul Cercueil wrote:
> > >  The PWM in the JZ4725B works the same as in the JZ4740, except that
> > > it
> > >  only has 6 channels available instead of 8.
> > 
> > this driver is probed only from device tree? If yes, it might be
> > sensible to specify the number of PWMs there and get it from there.
> > There doesn't seem to be a generic binding for that, but there are
> > several drivers that could benefit from it. (This is a bigger project
> > though and shouldn't stop your patch. Still more as it already got
> > Thierry's ack.)
> 
> I think there needs to be a proper guideline, as there doesn't seem to be
> a consensus about this. I learned from emails with Rob and Linus (Walleij)
> that I should not have in devicetree what I can deduce from the compatible
> string.

I understood them a bit differently. It is ok to deduce things from the
compatible string. But if you define a generic property (say) "num-pwms"
that is used uniformly in most bindings this is ok, too. (And then the
two different devices could use the same compatible.)

An upside of the generic "num-pwms" property is that the pwm core could
sanity check pwm phandles before passing them to the hardware drivers.

Best regards
Uwe
Linus Walleij Dec. 14, 2018, 1:50 p.m. UTC | #5
On Thu, Dec 13, 2018 at 9:42 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> [Adding Linus Walleij to Cc:]
>
> Hello,
>
> On Thu, Dec 13, 2018 at 03:03:15PM +0100, Paul Cercueil wrote:
> > Le jeu. 13 déc. 2018 à 10:24, Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> a écrit :
> > > On Wed, Dec 12, 2018 at 11:09:10PM +0100, Paul Cercueil wrote:
> > > >  The PWM in the JZ4725B works the same as in the JZ4740, except that
> > > > it
> > > >  only has 6 channels available instead of 8.
> > >
> > > this driver is probed only from device tree? If yes, it might be
> > > sensible to specify the number of PWMs there and get it from there.
> > > There doesn't seem to be a generic binding for that, but there are
> > > several drivers that could benefit from it. (This is a bigger project
> > > though and shouldn't stop your patch. Still more as it already got
> > > Thierry's ack.)
> >
> > I think there needs to be a proper guideline, as there doesn't seem to be
> > a consensus about this. I learned from emails with Rob and Linus (Walleij)
> > that I should not have in devicetree what I can deduce from the compatible
> > string.
>
> I understood them a bit differently. It is ok to deduce things from the
> compatible string. But if you define a generic property (say) "num-pwms"
> that is used uniformly in most bindings this is ok, too. (And then the
> two different devices could use the same compatible.)
>
> An upside of the generic "num-pwms" property is that the pwm core could
> sanity check pwm phandles before passing them to the hardware drivers.

I don't know if this helps, but in GPIO we have "ngpios" which is
used to augment an existing block as to the number of lines actually
used with it.

The typical case is that an ASIC engineer synthesize a block for
32 GPIOs but only 12 of them are routed to external pads. So
we augment the behaviour of that driver to only use 12 of the
32 lines.

I guess using the remaining 20 lines "works" in a sense but they
have no practical use and will just bias electrons in the silicon
for no use.

So if the PWM case is something similar, then by all means add
num-pwms.

Yours,
Linus Walleij
Uwe Kleine-König Dec. 14, 2018, 2:26 p.m. UTC | #6
Hello,

On Fri, Dec 14, 2018 at 02:50:20PM +0100, Linus Walleij wrote:
> On Thu, Dec 13, 2018 at 9:42 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > [Adding Linus Walleij to Cc:]
> > On Thu, Dec 13, 2018 at 03:03:15PM +0100, Paul Cercueil wrote:
> > > Le jeu. 13 déc. 2018 à 10:24, Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> a écrit :
> > > > On Wed, Dec 12, 2018 at 11:09:10PM +0100, Paul Cercueil wrote:
> > > > >  The PWM in the JZ4725B works the same as in the JZ4740, except that
> > > > > it
> > > > >  only has 6 channels available instead of 8.
> > > >
> > > > this driver is probed only from device tree? If yes, it might be
> > > > sensible to specify the number of PWMs there and get it from there.
> > > > There doesn't seem to be a generic binding for that, but there are
> > > > several drivers that could benefit from it. (This is a bigger project
> > > > though and shouldn't stop your patch. Still more as it already got
> > > > Thierry's ack.)
> > >
> > > I think there needs to be a proper guideline, as there doesn't seem to be
> > > a consensus about this. I learned from emails with Rob and Linus (Walleij)
> > > that I should not have in devicetree what I can deduce from the compatible
> > > string.
> >
> > I understood them a bit differently. It is ok to deduce things from the
> > compatible string. But if you define a generic property (say) "num-pwms"
> > that is used uniformly in most bindings this is ok, too. (And then the
> > two different devices could use the same compatible.)
> >
> > An upside of the generic "num-pwms" property is that the pwm core could
> > sanity check pwm phandles before passing them to the hardware drivers.
> 
> I don't know if this helps, but in GPIO we have "ngpios" which is
> used to augment an existing block as to the number of lines actually
> used with it.
> 
> The typical case is that an ASIC engineer synthesize a block for
> 32 GPIOs but only 12 of them are routed to external pads. So
> we augment the behaviour of that driver to only use 12 of the
> 32 lines.
> 
> I guess using the remaining 20 lines "works" in a sense but they
> have no practical use and will just bias electrons in the silicon
> for no use.

This looks very similar to the case under discussion.
 
> So if the PWM case is something similar, then by all means add
> num-pwms.

.. or "npwms" to use the same nomenclature as the gpio binding?

Best regards
Uwe
Linus Walleij Dec. 14, 2018, 2:56 p.m. UTC | #7
On Fri, Dec 14, 2018 at 3:26 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Fri, Dec 14, 2018 at 02:50:20PM +0100, Linus Walleij wrote:

> > So if the PWM case is something similar, then by all means add
> > num-pwms.
>
> .. or "npwms" to use the same nomenclature as the gpio binding?

Either works for me, I don't have a very high need of consistency
but there are others that have so your suggestion seems wise.

Yours,
Linus Walleij
Paul Cercueil Dec. 16, 2018, 2:18 p.m. UTC | #8
Hi,

Le ven. 14 déc. 2018 à 15:26, Uwe Kleine-König 
<u.kleine-koenig@pengutronix.de> a écrit :
> Hello,
> 
> On Fri, Dec 14, 2018 at 02:50:20PM +0100, Linus Walleij wrote:
>>  On Thu, Dec 13, 2018 at 9:42 PM Uwe Kleine-König
>>  <u.kleine-koenig@pengutronix.de> wrote:
>>  > [Adding Linus Walleij to Cc:]
>>  > On Thu, Dec 13, 2018 at 03:03:15PM +0100, Paul Cercueil wrote:
>>  > > Le jeu. 13 déc. 2018 à 10:24, Uwe Kleine-König
>>  > > <u.kleine-koenig@pengutronix.de> a écrit :
>>  > > > On Wed, Dec 12, 2018 at 11:09:10PM +0100, Paul Cercueil wrote:
>>  > > > >  The PWM in the JZ4725B works the same as in the JZ4740, 
>> except that
>>  > > > > it
>>  > > > >  only has 6 channels available instead of 8.
>>  > > >
>>  > > > this driver is probed only from device tree? If yes, it might 
>> be
>>  > > > sensible to specify the number of PWMs there and get it from 
>> there.
>>  > > > There doesn't seem to be a generic binding for that, but 
>> there are
>>  > > > several drivers that could benefit from it. (This is a bigger 
>> project
>>  > > > though and shouldn't stop your patch. Still more as it 
>> already got
>>  > > > Thierry's ack.)
>>  > >
>>  > > I think there needs to be a proper guideline, as there doesn't 
>> seem to be
>>  > > a consensus about this. I learned from emails with Rob and 
>> Linus (Walleij)
>>  > > that I should not have in devicetree what I can deduce from the 
>> compatible
>>  > > string.
>>  >
>>  > I understood them a bit differently. It is ok to deduce things 
>> from the
>>  > compatible string. But if you define a generic property (say) 
>> "num-pwms"
>>  > that is used uniformly in most bindings this is ok, too. (And 
>> then the
>>  > two different devices could use the same compatible.)
>>  >
>>  > An upside of the generic "num-pwms" property is that the pwm core 
>> could
>>  > sanity check pwm phandles before passing them to the hardware 
>> drivers.
>> 
>>  I don't know if this helps, but in GPIO we have "ngpios" which is
>>  used to augment an existing block as to the number of lines actually
>>  used with it.
>> 
>>  The typical case is that an ASIC engineer synthesize a block for
>>  32 GPIOs but only 12 of them are routed to external pads. So
>>  we augment the behaviour of that driver to only use 12 of the
>>  32 lines.
>> 
>>  I guess using the remaining 20 lines "works" in a sense but they
>>  have no practical use and will just bias electrons in the silicon
>>  for no use.
> 
> This looks very similar to the case under discussion.
> 
>>  So if the PWM case is something similar, then by all means add
>>  num-pwms.
> 
> .. or "npwms" to use the same nomenclature as the gpio binding?

If we're going to do something like this, should it be the drivers or
the core (within pwmchip_add) that checks for this "npwms" property?

> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König        
>     |
> Industrial Linux Solutions                 | 
> http://www.pengutronix.de/  |
Uwe Kleine-König Dec. 17, 2018, 7:53 a.m. UTC | #9
On Sun, Dec 16, 2018 at 03:18:52PM +0100, Paul Cercueil wrote:
> Hi,
> 
> Le ven. 14 déc. 2018 à 15:26, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> a écrit :
> > Hello,
> > 
> > On Fri, Dec 14, 2018 at 02:50:20PM +0100, Linus Walleij wrote:
> > > On Thu, Dec 13, 2018 at 9:42 PM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > [Adding Linus Walleij to Cc:]
> > > > On Thu, Dec 13, 2018 at 03:03:15PM +0100, Paul Cercueil wrote:
> > > > > Le jeu. 13 déc. 2018 à 10:24, Uwe Kleine-König
> > > > > <u.kleine-koenig@pengutronix.de> a écrit :
> > > > > > On Wed, Dec 12, 2018 at 11:09:10PM +0100, Paul Cercueil wrote:
> > > > > > >  The PWM in the JZ4725B works the same as in the JZ4740,
> > > > > > >  except that it only has 6 channels available instead of
> > > > > > >  8.
> > > > > >
> > > > > > this driver is probed only from device tree? If yes, it
> > > > > > might be sensible to specify the number of PWMs there and
> > > > > > get it from there.
> > > > > > There doesn't seem to be a generic binding for that, but there are
> > > > > > several drivers that could benefit from it. (This is a bigger project
> > > > > > though and shouldn't stop your patch. Still more as it already got
> > > > > > Thierry's ack.)
> > > > >
> > > > > I think there needs to be a proper guideline, as there doesn't seem to be
> > > > > a consensus about this. I learned from emails with Rob and  Linus (Walleij)
> > > > > that I should not have in devicetree what I can deduce from the compatible
> > > > > string.
> > > >
> > > > I understood them a bit differently. It is ok to deduce things from the
> > > > compatible string. But if you define a generic property (say) "num-pwms"
> > > > that is used uniformly in most bindings this is ok, too. (And then the
> > > > two different devices could use the same compatible.)
> > > >
> > > > An upside of the generic "num-pwms" property is that the pwm core could
> > > > sanity check pwm phandles before passing them to the hardware drivers.
> > > 
> > >  I don't know if this helps, but in GPIO we have "ngpios" which is
> > >  used to augment an existing block as to the number of lines actually
> > >  used with it.
> > > 
> > >  The typical case is that an ASIC engineer synthesize a block for
> > >  32 GPIOs but only 12 of them are routed to external pads. So
> > >  we augment the behaviour of that driver to only use 12 of the
> > >  32 lines.
> > > 
> > >  I guess using the remaining 20 lines "works" in a sense but they
> > >  have no practical use and will just bias electrons in the silicon
> > >  for no use.
> > 
> > This looks very similar to the case under discussion.
> > 
> > >  So if the PWM case is something similar, then by all means add
> > >  num-pwms.
> > 
> > .. or "npwms" to use the same nomenclature as the gpio binding?
> 
> If we're going to do something like this, should it be the drivers or
> the core (within pwmchip_add) that checks for this "npwms" property?

Of course this should be done in the core. The driver than can rely on
the validity of the index. But as I wrote before, this shouldn't stop
your patch from going in.

But if Thierry agrees that this npmws (or num-pwms) is a good idea, it
would be great to start early to convert drivers.

Best regards
Uwe
Thierry Reding Dec. 20, 2018, 5:39 p.m. UTC | #10
On Mon, Dec 17, 2018 at 08:53:21AM +0100, Uwe Kleine-König wrote:
> On Sun, Dec 16, 2018 at 03:18:52PM +0100, Paul Cercueil wrote:
> > Hi,
> > 
> > Le ven. 14 déc. 2018 à 15:26, Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> a écrit :
> > > Hello,
> > > 
> > > On Fri, Dec 14, 2018 at 02:50:20PM +0100, Linus Walleij wrote:
> > > > On Thu, Dec 13, 2018 at 9:42 PM Uwe Kleine-König
> > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > [Adding Linus Walleij to Cc:]
> > > > > On Thu, Dec 13, 2018 at 03:03:15PM +0100, Paul Cercueil wrote:
> > > > > > Le jeu. 13 déc. 2018 à 10:24, Uwe Kleine-König
> > > > > > <u.kleine-koenig@pengutronix.de> a écrit :
> > > > > > > On Wed, Dec 12, 2018 at 11:09:10PM +0100, Paul Cercueil wrote:
> > > > > > > >  The PWM in the JZ4725B works the same as in the JZ4740,
> > > > > > > >  except that it only has 6 channels available instead of
> > > > > > > >  8.
> > > > > > >
> > > > > > > this driver is probed only from device tree? If yes, it
> > > > > > > might be sensible to specify the number of PWMs there and
> > > > > > > get it from there.
> > > > > > > There doesn't seem to be a generic binding for that, but there are
> > > > > > > several drivers that could benefit from it. (This is a bigger project
> > > > > > > though and shouldn't stop your patch. Still more as it already got
> > > > > > > Thierry's ack.)
> > > > > >
> > > > > > I think there needs to be a proper guideline, as there doesn't seem to be
> > > > > > a consensus about this. I learned from emails with Rob and  Linus (Walleij)
> > > > > > that I should not have in devicetree what I can deduce from the compatible
> > > > > > string.
> > > > >
> > > > > I understood them a bit differently. It is ok to deduce things from the
> > > > > compatible string. But if you define a generic property (say) "num-pwms"
> > > > > that is used uniformly in most bindings this is ok, too. (And then the
> > > > > two different devices could use the same compatible.)
> > > > >
> > > > > An upside of the generic "num-pwms" property is that the pwm core could
> > > > > sanity check pwm phandles before passing them to the hardware drivers.
> > > > 
> > > >  I don't know if this helps, but in GPIO we have "ngpios" which is
> > > >  used to augment an existing block as to the number of lines actually
> > > >  used with it.
> > > > 
> > > >  The typical case is that an ASIC engineer synthesize a block for
> > > >  32 GPIOs but only 12 of them are routed to external pads. So
> > > >  we augment the behaviour of that driver to only use 12 of the
> > > >  32 lines.
> > > > 
> > > >  I guess using the remaining 20 lines "works" in a sense but they
> > > >  have no practical use and will just bias electrons in the silicon
> > > >  for no use.
> > > 
> > > This looks very similar to the case under discussion.
> > > 
> > > >  So if the PWM case is something similar, then by all means add
> > > >  num-pwms.
> > > 
> > > .. or "npwms" to use the same nomenclature as the gpio binding?
> > 
> > If we're going to do something like this, should it be the drivers or
> > the core (within pwmchip_add) that checks for this "npwms" property?
> 
> Of course this should be done in the core. The driver than can rely on
> the validity of the index. But as I wrote before, this shouldn't stop
> your patch from going in.
> 
> But if Thierry agrees that this npmws (or num-pwms) is a good idea, it
> would be great to start early to convert drivers.

Do we actually need this? It seems like Paul's patch here properly
derives the number of available PWMs from the compatible string, so I
don't see what the extra num-pwms (or whatever) property would add.

Thierry
Uwe Kleine-König Dec. 20, 2018, 8:58 p.m. UTC | #11
On Thu, Dec 20, 2018 at 06:39:04PM +0100, Thierry Reding wrote:
> On Mon, Dec 17, 2018 at 08:53:21AM +0100, Uwe Kleine-König wrote:
> > On Sun, Dec 16, 2018 at 03:18:52PM +0100, Paul Cercueil wrote:
> > > Hi,
> > > 
> > > Le ven. 14 déc. 2018 à 15:26, Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> a écrit :
> > > > Hello,
> > > > 
> > > > On Fri, Dec 14, 2018 at 02:50:20PM +0100, Linus Walleij wrote:
> > > > > On Thu, Dec 13, 2018 at 9:42 PM Uwe Kleine-König
> > > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > > [Adding Linus Walleij to Cc:]
> > > > > > On Thu, Dec 13, 2018 at 03:03:15PM +0100, Paul Cercueil wrote:
> > > > > > > Le jeu. 13 déc. 2018 à 10:24, Uwe Kleine-König
> > > > > > > <u.kleine-koenig@pengutronix.de> a écrit :
> > > > > > > > On Wed, Dec 12, 2018 at 11:09:10PM +0100, Paul Cercueil wrote:
> > > > > > > > >  The PWM in the JZ4725B works the same as in the JZ4740,
> > > > > > > > >  except that it only has 6 channels available instead of
> > > > > > > > >  8.
> > > > > > > >
> > > > > > > > this driver is probed only from device tree? If yes, it
> > > > > > > > might be sensible to specify the number of PWMs there and
> > > > > > > > get it from there.
> > > > > > > > There doesn't seem to be a generic binding for that, but there are
> > > > > > > > several drivers that could benefit from it. (This is a bigger project
> > > > > > > > though and shouldn't stop your patch. Still more as it already got
> > > > > > > > Thierry's ack.)
> > > > > > >
> > > > > > > I think there needs to be a proper guideline, as there doesn't seem to be
> > > > > > > a consensus about this. I learned from emails with Rob and  Linus (Walleij)
> > > > > > > that I should not have in devicetree what I can deduce from the compatible
> > > > > > > string.
> > > > > >
> > > > > > I understood them a bit differently. It is ok to deduce things from the
> > > > > > compatible string. But if you define a generic property (say) "num-pwms"
> > > > > > that is used uniformly in most bindings this is ok, too. (And then the
> > > > > > two different devices could use the same compatible.)
> > > > > >
> > > > > > An upside of the generic "num-pwms" property is that the pwm core could
> > > > > > sanity check pwm phandles before passing them to the hardware drivers.
> > > > > 
> > > > >  I don't know if this helps, but in GPIO we have "ngpios" which is
> > > > >  used to augment an existing block as to the number of lines actually
> > > > >  used with it.
> > > > > 
> > > > >  The typical case is that an ASIC engineer synthesize a block for
> > > > >  32 GPIOs but only 12 of them are routed to external pads. So
> > > > >  we augment the behaviour of that driver to only use 12 of the
> > > > >  32 lines.
> > > > > 
> > > > >  I guess using the remaining 20 lines "works" in a sense but they
> > > > >  have no practical use and will just bias electrons in the silicon
> > > > >  for no use.
> > > > 
> > > > This looks very similar to the case under discussion.
> > > > 
> > > > >  So if the PWM case is something similar, then by all means add
> > > > >  num-pwms.
> > > > 
> > > > .. or "npwms" to use the same nomenclature as the gpio binding?
> > > 
> > > If we're going to do something like this, should it be the drivers or
> > > the core (within pwmchip_add) that checks for this "npwms" property?
> > 
> > Of course this should be done in the core. The driver than can rely on
> > the validity of the index. But as I wrote before, this shouldn't stop
> > your patch from going in.

Actually the core already takes care of the validity of the index with
the number of pwms being provided to pwmchip_add().
 
> > But if Thierry agrees that this npmws (or num-pwms) is a good idea, it
> > would be great to start early to convert drivers.
> 
> Do we actually need this? It seems like Paul's patch here properly
> derives the number of available PWMs from the compatible string, so I
> don't see what the extra num-pwms (or whatever) property would add.

Given that the only difference between the two different implementations
is just the number of pwms provided, this could just be expressed in the
dts as:

	pwm@2000000 {
		compatible = "jz4740";
		num-pwms = <8>;
	}

and

	pwm@2000000 {
		compatible = "jz4740";
		num-pwms = <6>;
	}

instead of

	pwm@2000000 {
		compatible = "jz4740";
	}

and

	pwm@2000000 {
		compatible = "jz4725";
	}

. According to my metric the former is more descriptive and so better.

And then the pwm core could provide parsing of that property (e.g. if
chip.npwm == 0) which simplifies the driver (and all others using that
mechanism).

Regarding the question "Do we actually need this?": We don't, but I
think it would make a nice step to get more descriptive device trees and
so I consider it an improvement. It would also allow to check a dtb
because even without the driver you can notice that

	pwms = <&pwm 12 0>;

is invalid if &pwm has "num-pwms = <8>". Drivers that could benefit are
(at least): hibvt, sun4i, tegra, lpss.

Best regards
Uwe
diff mbox series

Patch

diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index cb8d8cec353f..a3a8da8af0de 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -24,6 +24,10 @@ 
 
 #define NUM_PWM 8
 
+struct jz4740_soc_info {
+	unsigned int num_pwms;
+};
+
 struct jz4740_pwm_chip {
 	struct pwm_chip chip;
 	struct clk *clks[NUM_PWM];
@@ -217,9 +221,14 @@  static const struct pwm_ops jz4740_pwm_ops = {
 
 static int jz4740_pwm_probe(struct platform_device *pdev)
 {
+	const struct jz4740_soc_info *soc_info;
 	struct jz4740_pwm_chip *jz4740;
 	struct device *dev = &pdev->dev;
 
+	soc_info = device_get_match_data(dev);
+	if (!soc_info)
+		return -EINVAL;
+
 	jz4740 = devm_kzalloc(dev, sizeof(*jz4740), GFP_KERNEL);
 	if (!jz4740)
 		return -ENOMEM;
@@ -238,7 +247,7 @@  static int jz4740_pwm_probe(struct platform_device *pdev)
 
 	jz4740->chip.dev = dev;
 	jz4740->chip.ops = &jz4740_pwm_ops;
-	jz4740->chip.npwm = NUM_PWM;
+	jz4740->chip.npwm = soc_info->num_pwms;
 	jz4740->chip.base = -1;
 	jz4740->chip.of_xlate = of_pwm_xlate_with_flags;
 	jz4740->chip.of_pwm_n_cells = 3;
@@ -256,8 +265,17 @@  static int jz4740_pwm_remove(struct platform_device *pdev)
 }
 
 #ifdef CONFIG_OF
+static const struct jz4740_soc_info jz4740_soc_info = {
+	.num_pwms = 8,
+};
+
+static const struct jz4740_soc_info jz4725b_soc_info = {
+	.num_pwms = 6,
+};
+
 static const struct of_device_id jz4740_pwm_dt_ids[] = {
-	{ .compatible = "ingenic,jz4740-pwm", },
+	{ .compatible = "ingenic,jz4740-pwm", .data = &jz4740_soc_info },
+	{ .compatible = "ingenic,jz4725b-pwm", .data = &jz4725b_soc_info },
 	{},
 };
 MODULE_DEVICE_TABLE(of, jz4740_pwm_dt_ids);