diff mbox series

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

Message ID 20240410104002.1197-2-zhi.mao@mediatek.com
State New
Headers show
Series media: i2c: Add support for GT97xx VCM | expand

Commit Message

Zhi Mao (毛智) April 10, 2024, 10:40 a.m. UTC 
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

Comments

Zhi Mao (毛智) April 11, 2024, 2:04 a.m. UTC | #1 
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.
> 
> > 
> >
Krzysztof Kozlowski April 11, 2024, 6:05 a.m. UTC | #2 
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
diff mbox series

Patch

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#
+$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
+
+  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)
+    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.
+    $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
+    default: 1
+
+required:
+  - compatible
+  - reg
+  - vin-supply
+  - vdd-supply
+
+additionalProperties: false
+
+examples:
+  - |
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        vcm@c {
+            compatible = "giantec,gt9768";
+            reg = <0x0c>;
+
+            vin-supply = <&gt97xx_vin>;
+            vdd-supply = <&gt97xx_vdd>;
+            giantec,aac-timing = <0x20>;
+        };
+    };
+
+...