Re: [V6, 1/2] media: dt-bindings: media: i2c: Document DW9768 bindings

[Dongchun Zhu <dongchun.zhu@mediatek.com>]

Hi Tomasz, Rob,

Thanks for the timely review and good suggestions.

On Mon, 2020-05-18 at 08:31 -0600, Rob Herring wrote:
> On Mon, May 18, 2020 at 04:12:28PM +0200, Tomasz Figa wrote:
> > Hi Dongchun,
> > 
> > On Mon, May 18, 2020 at 3:29 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > >
> > > Add DeviceTree binding documentation for Dongwoon Anatech DW9768 voice
> > > coil actuator.
> > 
> > Thanks for the patch. Please see my comments below.
> > 
> > >
> > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > Reviewed-by: Rob Herring <robh@kernel.org>
> > 
> > This version includes significant changes, so the reviewed-by tag
> > shouldn't have been carried out.
> > 

Sorry, this is my fault.
In fact, I've struggled with it at the beginning.
Yes, you are right.
Now the new version of patch-set includes huge changes relative to the
last edition.
It would be removed in next release.

> > > ---
> > >  .../bindings/media/i2c/dongwoon,dw9768.yaml        | 105 +++++++++++++++++++++
> > >  MAINTAINERS                                        |   7 ++
> > >  2 files changed, 112 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml b/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
> > > new file mode 100644
> > > index 0000000..b909e83
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
> > > @@ -0,0 +1,105 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +# Copyright (c) 2020 MediaTek Inc.
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/media/i2c/dongwoon,dw9768.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Dongwoon Anatech DW9768 Voice Coil Motor (VCM) Lens Device Tree Bindings
> > > +
> > > +maintainers:
> > > +  - Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > +
> > > +description: |-
> > > +  The Dongwoon DW9768 is a single 10-bit digital-to-analog (DAC) converter
> > > +  with 100 mA output current sink capability. VCM current is controlled with
> > > +  a linear mode driver. The DAC is controlled via a 2-wire (I2C-compatible)
> > > +  serial interface that operates at clock rates up to 1MHz. This chip
> > > +  integrates Advanced Actuator Control (AAC) technology and is intended for
> > > +  driving voice coil lenses in camera modules.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      # for DW9768 VCM
> > > +      - dongwoon,dw9768
> > > +      # for GT9769 VCM
> > > +      - giantec,gt9769
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  vin-supply:
> > > +    description:
> > > +      Definition of the regulator used as I2C I/O interface power supply.
> > > +
> > > +  vdd-supply:
> > > +    description:
> > > +      Definition of the regulator used as VCM chip power supply.
> > > +
> > > +  dongwoon,aac-mode:
> > > +    description:
> > > +      Indication of AAC mode select.
> > > +    allOf:
> > > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > > +      - enum:
> > > +          - 0    #  Direct (default)
> 
> Default can be expressed as 'default: 0'.
> 

Thanks for the reminder.
Next release I'd try to write DT property "dongwoon,aac-mode" like this:
dongwoon,aac-mode:
  description:
    Indication of AAC mode select.
  allOf:
    - $ref: "/schemas/types.yaml#/definitions/uint32"
    - enum:
        - 0  # Direct
        - 1  # AAC2 (operation time# 0.48 x Tvib)
        - 2  # AAC3 (operation time# 0.70 x Tvib)
        - 3  # AAC4 (operation time# 0.75 x Tvib)
        - 5  # AAC8 (operation time# 1.13 x Tvib)
      default: 0

> > > +          - 1    #  AAC2 (operation time# 0.48 x Tvib)
> > > +          - 2    #  AAC3 (operation time# 0.70 x Tvib)
> > > +          - 3    #  AAC4 (operation time# 0.75 x Tvib)
> > > +          - 4    #  Reserved
> > > +          - 5    #  AAC8 (operation time# 1.13 x Tvib)
> > > +          - 6    #  Reserved
> > > +          - 7    #  Reserved
> > 
> > I'll ultimately leave it to DT maintainers, but is there any reason to
> > define the reserved values?
> 
> No.
> 
> > 
> > > +
> > > +  dongwoon,aac-timing:
> > > +    description:
> > > +      Indication of AAC Timing count, unit of 0.1 milliseconds.
> 
> Why not just use standard units (-us)?
> 

That sounds nice.
I'd re-write the description like this in next release:
dongwoon,aac-timing:
  description:
    Number of AAC Timing count that controlled by one 6-bit period
    of vibration register AACT[5:0], the unit of which is 100us.
  allOf:
    - $ref: "/schemas/types.yaml#/definitions/uint32"
    - minimum: 0x00
    - maximum: 0x3F
    - default: 0x20

> > > +      Valid values vary from 0 to 63 (default 32).
> 
> Looks like constraints.
> 

Yes. This property is controlled by one 6-bit reg.
So here we need to constrain the data set to a narrow range.

> > > +    allOf:
> > > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > > +
> > > +  dongwoon,clock-dividing-rate:
> > > +    description:
> > > +      Indication of VCM internal clock dividing rate select, as one multiple
> > > +      factor to calculate VCM ring periodic time Tvib.
> > > +    allOf:
> > > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > > +      - enum:
> > > +          - 0    #  Dividing Rate -  2
> > > +          - 1    #  Dividing Rate -  1 (default)
> > > +          - 2    #  Dividing Rate -  1/2
> > > +          - 3    #  Dividing Rate -  1/4
> > > +          - 4    #  Dividing Rate -  8
> > > +          - 5    #  Dividing Rate -  4
> > > +          - 6    #  Dividing Rate -  Reserved
> > > +          - 7    #  Dividing Rate -  Reserved
> > 
> > Ditto.
> > 

Thanks for kindly reminder.
Reserved values would be removed in next release.
Like this:
dongwoon,clock-dividing-rate:
  description:
    Indication of VCM internal clock dividing rate select, as one    
    multiplier to calculate VCM ring periodic time Tvib.
  allOf:
    - $ref: "/schemas/types.yaml#/definitions/uint32"
    - enum:
        - 0   #  Dividing Rate -  2
        - 1   #  Dividing Rate -  1
        - 2    #  Dividing Rate -  1/2
        - 3    #  Dividing Rate -  1/4
        - 4    #  Dividing Rate -  8
        - 5    #  Dividing Rate -  4
      default: 1

> > Best regards,
> > Tomasz

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek