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

[Rob Herring <robh@kernel.org>]

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