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

[Zhi Mao (毛智)]

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.
> 
> > 
> >