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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 10 Apr 2024 12:27:07 +0100
From: Conor Dooley <conor@...nel.org>
To: Zhi Mao <zhi.mao@...iatek.com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
	Heiko Stuebner <heiko@...ech.de>,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	Hans Verkuil <hverkuil-cisco@...all.nl>,
	Hans de Goede <hdegoede@...hat.com>,
	Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
	Alain Volmat <alain.volmat@...s.st.com>,
	Paul Elder <paul.elder@...asonboard.com>,
	Mehdi Djait <mehdi.djait@...tlin.com>,
	Andy Shevchenko <andy.shevchenko@...il.com>,
	Bingbu Cao <bingbu.cao@...el.com>, linux-media@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org, shengnan.wang@...iatek.com,
	yaya.chang@...iatek.com, yunkec@...omium.org, 10572168@...com
Subject: Re: [PATCH 1/2] media: dt-bindings: i2c: add Giantec GT97xx VCM
 driver

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

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)