Re: [PATCH v4] pwm: add pwm driver for HiSilicon BVT SOCs

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2960 bytes --]

On Fri, Oct 21, 2016 at 09:22:36AM +0200, Thierry Reding wrote:
> On Mon, Oct 10, 2016 at 04:53:39PM -0500, Rob Herring wrote:
> > On Mon, Oct 10, 2016 at 07:05:16PM +0800, Jian Yuan wrote:
> > > From: yuanjian <yuanjian12@hisilicon.com>
> > > 
> > > Add PWM driver for the PWM controller found on HiSilicon BVT SOCs, like Hi3519V100, Hi3516CV300, etc.
> > > The PWM controller is primarily in charge of controlling P-Iris lens.
> > > 
> > > Reviewed-by: Jiancheng Xue <xuejiancheng@hisilicon.com>
> > > Signed-off-by: Jian Yuan <yuanjian12@hisilicon.com>
> > > ---
> > > Change Log:
> > > v4:
> > > Add #pwm-cells in the bindings document.
> > > v3:
> > > fixed issues pointed by thierry.
> > > Add PWM compatible string for Hi3519V100.
> > > Implement .apply() function which support atomic, instead of .enable()/.disable()/.config().
> > > v2:
> > > The number of PWMs is change to be probeable based on the compatible string.
> > > 
> > >  .../devicetree/bindings/pwm/pwm-hibvt.txt          |  23 ++
> > >  drivers/pwm/Kconfig                                |   9 +
> > >  drivers/pwm/Makefile                               |   1 +
> > >  drivers/pwm/pwm-hibvt.c                            | 270 +++++++++++++++++++++
> > >  4 files changed, 303 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-hibvt.txt
> > >  create mode 100644 drivers/pwm/pwm-hibvt.c
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-hibvt.txt b/Documentation/devicetree/bindings/pwm/pwm-hibvt.txt
> > > new file mode 100644
> > > index 0000000..609284f
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pwm/pwm-hibvt.txt
> > > @@ -0,0 +1,23 @@
> > > +Hisilicon PWM controller
> > > +
> > > +Required properties:
> > > +-compatible: should contain one SoC specific compatible string and one generic compatible
> > > +string "hisilicon, hibvt-pwm". The SoC specific strings supported including:
> > 
> > Extra space          ^
> > 
> > With that, for the binding:
> > 
> > Acked-by: Rob Herring <robh@kernel.org>
> 
> I'm wondering what the use is of the generic compatible string? Not only
> is the driver not listing it, nor is it in any way useful since there is
> nothing the driver could do based on the generic compatible string (each
> version of the IP currently supported has a different number of PWM
> channels).

Actually it's worse. The driver does have an entry for the generic
compatible string but it doesn't assign .data in that case (which is
really what I was saying above). Now according to the bindings it is
disallowed to have only the generic compatible string, which is good
because doing so would crash the driver on probe.

So, if it's not allowed to have only the generic compatible string,
why even have one at all? We always have an SoC-specific compatible
string as well, so the generic one is completely redundant.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]