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

[Thierry Reding]

On Thu, Aug 25, 2016 at 05:15:36PM +0800, Jian Yuan wrote:
> 
> 
> On 2016/8/24 21:05, Thierry Reding wrote:
> > On Mon, Aug 22, 2016 at 03:50:13PM +0800, Jian Yuan wrote:
> >> From: yuanjian <yuanjian12@xxxxxxxxxxxxx>
> >>
> >> Add pwm driver for HiSilicon BVT SOCs
> > 
> > pwm -> PWM, please. It'd be good to have more information here about
> > what the hardware can do, where to find it, etc.
> > 
> Not sure what you mean? Should I describe what the PWM or the BVT SoCs can do?

Some of the things you could mention here are how many channels the PWM
controller supports, if there any noteworthy limitations or extra
features. Looking at the driver the controller is pretty standard, but
you could mention that it supports signal polarity (not all controllers
do) and that it can be found on two SoCs and name them. Sometimes it's
also useful to give some information about typical use-cases (perhaps
one of the PWMs is used to control backlight of a panel with some widely
available development board?).

> >> Reviewed-by: Jiancheng Xue <xuejiancheng@xxxxxxxxxxxxx>
> >> Signed-off-by: Jian Yuan <yuanjian12@xxxxxxxxxxxxx>
> >> ---
> >> Change Log:
> >> v2:
> >> The number of PWMs is change to be probeable based on the compatible string.
> >>
> >>  .../devicetree/bindings/pwm/pwm-hibvt.txt          |  18 ++
> >>  drivers/pwm/Kconfig                                |  10 +
> >>  drivers/pwm/Makefile                               |   1 +
> >>  drivers/pwm/pwm-hibvt.c                            | 274 +++++++++++++++++++++
> >>  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..1274119
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/pwm/pwm-hibvt.txt
> >> @@ -0,0 +1,18 @@
> >> +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:
> > 
> > Why the generic compatible string? You've already shown in the driver
> > that the two versions you support aren't compatible.
> >
> The generic compatible string should be contained in every devicetree
> bindings that BVT Socs support.
> But I'll add another specific compatible string to distinguish it from
> "hisilicon,hi3516cv300-pwm".

There's no use in specifying a generic compatible string if a driver
can't do anything useful with it. If you've only got hisilicon,hibvt-pwm
in the device tree, a driver wouldn't know the target SoC and hence has
no knowledge about the number of channels. Therefore the generic
compatible string is useless.

Thierry

Attachment: signature.asc
Description: PGP signature