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

[Thierry Reding]

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@xxxxxxxxxxxxx>
> > 
> > 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@xxxxxxxxxxxxx>
> > Signed-off-by: Jian Yuan <yuanjian12@xxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>

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

Should I just remove that from the above text and the example?

Thierry

> > +	"hisilicon,hi3516cv300-pwm"
> > +	"hisilicon,hi3519v100-pwm"
> > +- reg: physical base address and length of the controller's registers.
> > +- clocks: phandle and clock specifier of the PWM reference clock.
> > +- resets: phandle and reset specifier for the PWM controller reset.
> > +- #pwm-cells: Should be 2. See pwm.txt in this directory for a description of
> > +  the cells format.
> > +
> > +Example:
> > +	pwm: pwm@12130000 {
> > +
> > +		compatible = "hisilicon,hi3516cv300-pwm", "hisilicon,hibvt-pwm";
> > +		compatible = "hisilicon,hi3519v100-pwm", "hisilicon,hibvt-pwm";
> > +		reg = <0x12130000 0x10000>;
> > +		clocks = <&crg_ctrl HI3516CV300_PWM_CLK>;
> > +		resets = <&crg_ctrl 0x38 0>;
> > +		#pwm-cells = <2>;
> > +	};

Attachment: signature.asc
Description: PGP signature