[1/2] media: dt-bindings: i2c: add Giantec GT97xx VCM driver

Add YAML device tree binding for GT97xx VCM driver,
and the relevant MAINTAINERS entries.

Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
---
 .../bindings/media/i2c/giantec,gt97xx.yaml    | 91 +++++++++++++++++++
 1 file changed, 91 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml

Hey,

On Wed, Apr 10, 2024 at 06:40:01PM +0800, Zhi Mao wrote:
> Add YAML device tree binding for GT97xx VCM driver,

Please don't mention drivers here, bindings are for hardware.

> and the relevant MAINTAINERS entries.
> 
> Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> ---
>  .../bindings/media/i2c/giantec,gt97xx.yaml    | 91 +++++++++++++++++++
>  1 file changed, 91 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml b/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
> new file mode 100644
> index 000000000000..8c9f1eb4dac8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
> @@ -0,0 +1,91 @@
> +# 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/giantec,gt97xx.yaml#

Filename patching compatible please.

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Giantec Semiconductor, Crop. GT97xx Voice Coil Motor (VCM)
> +
> +maintainers:
> +  - Zhi Mao <zhi.mao@mediatek.com>
> +
> +description: |-
> +  The Giantec GT97xx is a 10-bit DAC with current sink capability.
> +  The DAC is controlled via I2C bus that operates at clock rates up to 1MHz.
> +  This chip integrates Advanced Actuator Control (AAC) technology
> +  and is intended for driving voice coil lens in camera modules.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - giantec,gt9768 # for GT9768 VCM
> +      - giantec,gt9769 # for GT9769 VCM

I don't think these comments are needed, they should be clear from the
compatibles, no?

> +
> +  reg:
> +    maxItems: 1
> +
> +  vin-supply: true
> +
> +  vdd-supply: true
> +
> +  giantec,aac-mode:
> +    description:
> +      Indication of AAC mode select.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum:
> +      - 1    #  AAC2 mode(operation time# 0.48 x Tvib)
> +      - 2    #  AAC3 mode(operation time# 0.70 x Tvib)
> +      - 3    #  AAC4 mode(operation time# 0.75 x Tvib)
> +      - 5    #  AAC8 mode(operation time# 1.13 x Tvib)

I dislike these enum based properties and I would rather this either be
the values themselves (0.48, 0.70 etc).

> +    default: 2
> +
> +  giantec,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 100 us.

Then the property should be in a standard unit of time, not "random" hex
numbers that correspond to register values.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 0x20
> +    minimum: 0x00
> +    maximum: 0x3f
> +
> +  giantec,clock-presc:
> +    description:
> +      Indication of VCM internal clock dividing rate select, as one multiple
> +      factor to calculate VCM ring periodic time Tvib.
> +    $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

Same here, you should not need these comments explaining the values, use
an enum with meaningful values please. 

> +    default: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - vin-supply
> +  - vdd-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        vcm@c {

"vcm" is not a generic node-name, can you use one please?
Look around similar bindings or at the dt spec for generic node-names.

Thanks,
Conor.

> +            compatible = "giantec,gt9768";
> +            reg = <0x0c>;
> +
> +            vin-supply = <&gt97xx_vin>;
> +            vdd-supply = <&gt97xx_vdd>;
> +            giantec,aac-timing = <0x20>;
> +        };
> +    };
> +
> +...
> -- 
> 2.25.1
>

Hi Conor, Zhi,

On Wed, Apr 10, 2024 at 12:27:07PM +0100, Conor Dooley wrote:
> Hey,
> 
> On Wed, Apr 10, 2024 at 06:40:01PM +0800, Zhi Mao wrote:
> > Add YAML device tree binding for GT97xx VCM driver,
> 
> Please don't mention drivers here, bindings are for hardware.
> 
> > and the relevant MAINTAINERS entries.
> > 
> > Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> > ---
> >  .../bindings/media/i2c/giantec,gt97xx.yaml    | 91 +++++++++++++++++++
> >  1 file changed, 91 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml b/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
> > new file mode 100644
> > index 000000000000..8c9f1eb4dac8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
> > @@ -0,0 +1,91 @@
> > +# 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/giantec,gt97xx.yaml#
> 
> Filename patching compatible please.
> 
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Giantec Semiconductor, Crop. GT97xx Voice Coil Motor (VCM)
> > +
> > +maintainers:
> > +  - Zhi Mao <zhi.mao@mediatek.com>
> > +
> > +description: |-
> > +  The Giantec GT97xx is a 10-bit DAC with current sink capability.
> > +  The DAC is controlled via I2C bus that operates at clock rates up to 1MHz.
> > +  This chip integrates Advanced Actuator Control (AAC) technology
> > +  and is intended for driving voice coil lens in camera modules.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - giantec,gt9768 # for GT9768 VCM
> > +      - giantec,gt9769 # for GT9769 VCM
> 
> I don't think these comments are needed, they should be clear from the
> compatibles, no?
> 
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  vin-supply: true
> > +
> > +  vdd-supply: true
> > +
> > +  giantec,aac-mode:
> > +    description:
> > +      Indication of AAC mode select.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum:
> > +      - 1    #  AAC2 mode(operation time# 0.48 x Tvib)
> > +      - 2    #  AAC3 mode(operation time# 0.70 x Tvib)
> > +      - 3    #  AAC4 mode(operation time# 0.75 x Tvib)
> > +      - 5    #  AAC8 mode(operation time# 1.13 x Tvib)
> 
> I dislike these enum based properties and I would rather this either be
> the values themselves (0.48, 0.70 etc).
> 
> > +    default: 2
> > +
> > +  giantec,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 100 us.
> 
> Then the property should be in a standard unit of time, not "random" hex
> numbers that correspond to register values.
> 
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    default: 0x20
> > +    minimum: 0x00
> > +    maximum: 0x3f
> > +
> > +  giantec,clock-presc:
> > +    description:
> > +      Indication of VCM internal clock dividing rate select, as one multiple
> > +      factor to calculate VCM ring periodic time Tvib.
> > +    $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
> 
> Same here, you should not need these comments explaining the values, use
> an enum with meaningful values please. 
> 
> > +    default: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - vin-supply
> > +  - vdd-supply
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        vcm@c {
> 
> "vcm" is not a generic node-name, can you use one please?
> Look around similar bindings or at the dt spec for generic node-names.

"camera-lens" would seem to be widely used. I can post patches to change
some of the rest out there that aren't aligned.

> 
> Thanks,
> Conor.
> 
> > +            compatible = "giantec,gt9768";
> > +            reg = <0x0c>;
> > +
> > +            vin-supply = <&gt97xx_vin>;
> > +            vdd-supply = <&gt97xx_vdd>;
> > +            giantec,aac-timing = <0x20>;
> > +        };
> > +    };
> > +
> > +...

On Wed, Apr 10, 2024 at 12:27:07PM +0100, Conor Dooley wrote:
> Hey,
> 
> On Wed, Apr 10, 2024 at 06:40:01PM +0800, Zhi Mao wrote:
> > Add YAML device tree binding for GT97xx VCM driver,
> 
> Please don't mention drivers here, bindings are for hardware.
> 
> > and the relevant MAINTAINERS entries.
> > 
> > Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> > ---
> >  .../bindings/media/i2c/giantec,gt97xx.yaml    | 91 +++++++++++++++++++
> >  1 file changed, 91 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml b/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
> > new file mode 100644
> > index 000000000000..8c9f1eb4dac8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
> > @@ -0,0 +1,91 @@
> > +# 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/giantec,gt97xx.yaml#
> 
> Filename patching compatible please.
> 
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Giantec Semiconductor, Crop. GT97xx Voice Coil Motor (VCM)
> > +
> > +maintainers:
> > +  - Zhi Mao <zhi.mao@mediatek.com>
> > +
> > +description: |-
> > +  The Giantec GT97xx is a 10-bit DAC with current sink capability.
> > +  The DAC is controlled via I2C bus that operates at clock rates up to 1MHz.
> > +  This chip integrates Advanced Actuator Control (AAC) technology
> > +  and is intended for driving voice coil lens in camera modules.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - giantec,gt9768 # for GT9768 VCM
> > +      - giantec,gt9769 # for GT9769 VCM
> 
> I don't think these comments are needed, they should be clear from the
> compatibles, no?
> 
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  vin-supply: true
> > +
> > +  vdd-supply: true
> > +
> > +  giantec,aac-mode:
> > +    description:
> > +      Indication of AAC mode select.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum:
> > +      - 1    #  AAC2 mode(operation time# 0.48 x Tvib)
> > +      - 2    #  AAC3 mode(operation time# 0.70 x Tvib)
> > +      - 3    #  AAC4 mode(operation time# 0.75 x Tvib)
> > +      - 5    #  AAC8 mode(operation time# 1.13 x Tvib)
> 
> I dislike these enum based properties and I would rather this either be
> the values themselves (0.48, 0.70 etc).

Except that those would have to be strings for floats or fractions. For 
properties which have little chance of being something common and aren't 
any form of standard unit, I think it is fine to just use the h/w 
specific values. 

The first question to ask whether these parameters are common to 
all/many voice coil motors?


> > +    default: 2
> > +
> > +  giantec,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 100 us.
> 
> Then the property should be in a standard unit of time, not "random" hex
> numbers that correspond to register values.

Here, I agree.

Rob

Hi Conor,

Thanks for your review.

On Wed, 2024-04-10 at 12:27 +0100, Conor Dooley wrote:
> > 
> > 
> Hey,
> 
> On Wed, Apr 10, 2024 at 06:40:01PM +0800, Zhi Mao wrote:
> > b/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
> > @@ -0,0 +1,91 @@
> > +# 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/giantec,gt97xx.yaml#
> 
> Filename patching compatible please.
> 
> 
Sorry, I don't catch this point. 
Can you explain more details? 
> > 
> > 
> > +
> > +  giantec,aac-mode:
> > +    description:
> > +      Indication of AAC mode select.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum:
> > +      - 1    #  AAC2 mode(operation time# 0.48 x Tvib)
> > +      - 2    #  AAC3 mode(operation time# 0.70 x Tvib)
> > +      - 3    #  AAC4 mode(operation time# 0.75 x Tvib)
> > +      - 5    #  AAC8 mode(operation time# 1.13 x Tvib)
> 
> I dislike these enum based properties and I would rather this either
> be
> the values themselves (0.48, 0.70 etc).
> 
> > +
> > +  giantec,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 100 us.
> 
> Then the property should be in a standard unit of time, not "random"
> hex
> numbers that correspond to register values.
> 
> > 
> > +  giantec,clock-presc:
> > +    description:
> > +      Indication of VCM internal clock dividing rate select, as
> > one multiple
> > +      factor to calculate VCM ring periodic time Tvib.
> > +    $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
> 
> Same here, you should not need these comments explaining the values,
> use
> an enum with meaningful values please. 
> 
About "aac-mode/aac-timing/clock-presc", we test this driver with
default settings accroding to SPEC and VCM works well, so I will not
export these property in YMAL and let driver use default settings.
How do you think about it?
  

> Thanks,
> Conor.
> 
> > 
> >

On 11/04/2024 04:04, Zhi Mao (毛智) wrote:
> Hi Conor,
> 
> Thanks for your review.
> 
> On Wed, 2024-04-10 at 12:27 +0100, Conor Dooley wrote:
>>>
>>>
>> Hey,
>>
>> On Wed, Apr 10, 2024 at 06:40:01PM +0800, Zhi Mao wrote:
>>> b/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
>>> @@ -0,0 +1,91 @@
>>> +# 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/giantec,gt97xx.yaml#
>>
>> Filename patching compatible please.
>>
>>
> Sorry, I don't catch this point. 
> Can you explain more details? 

s/patching/matching/
Use compatible as filename.


>>>
>>>
>>> +
>>> +  giantec,aac-mode:
>>> +    description:
>>> +      Indication of AAC mode select.
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    enum:
>>> +      - 1    #  AAC2 mode(operation time# 0.48 x Tvib)
>>> +      - 2    #  AAC3 mode(operation time# 0.70 x Tvib)
>>> +      - 3    #  AAC4 mode(operation time# 0.75 x Tvib)
>>> +      - 5    #  AAC8 mode(operation time# 1.13 x Tvib)
>>
>> I dislike these enum based properties and I would rather this either
>> be
>> the values themselves (0.48, 0.70 etc).
>>
>>> +
>>> +  giantec,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 100 us.
>>
>> Then the property should be in a standard unit of time, not "random"
>> hex
>> numbers that correspond to register values.
>>
>>>
>>> +  giantec,clock-presc:
>>> +    description:
>>> +      Indication of VCM internal clock dividing rate select, as
>>> one multiple
>>> +      factor to calculate VCM ring periodic time Tvib.
>>> +    $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
>>
>> Same here, you should not need these comments explaining the values,
>> use
>> an enum with meaningful values please. 
>>
> About "aac-mode/aac-timing/clock-presc", we test this driver with
> default settings accroding to SPEC and VCM works well, so I will not
> export these property in YMAL and let driver use default settings.
> How do you think about it?

You must remove them from the driver code in such case.

Best regards,
Krzysztof