[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5317cb9b01cf5668837ad7ccdcb9eb72b95d98d6.camel@mediatek.com>
Date: Thu, 11 Apr 2024 02:04:17 +0000
From: Zhi Mao (毛智) <zhi.mao@...iatek.com>
To: "conor@...nel.org" <conor@...nel.org>
CC: "heiko@...ech.de" <heiko@...ech.de>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "laurent.pinchart+renesas@...asonboard.com"
<laurent.pinchart+renesas@...asonboard.com>, "yunkec@...omium.org"
<yunkec@...omium.org>, "linux-mediatek@...ts.infradead.org"
<linux-mediatek@...ts.infradead.org>, "linux-media@...r.kernel.org"
<linux-media@...r.kernel.org>, "hdegoede@...hat.com" <hdegoede@...hat.com>,
"bingbu.cao@...el.com" <bingbu.cao@...el.com>, "paul.elder@...asonboard.com"
<paul.elder@...asonboard.com>, "devicetree@...r.kernel.org"
<devicetree@...r.kernel.org>, "andy.shevchenko@...il.com"
<andy.shevchenko@...il.com>, "mchehab@...nel.org" <mchehab@...nel.org>,
Shengnan Wang (王圣男)
<shengnan.wang@...iatek.com>, Yaya Chang (張雅清)
<Yaya.Chang@...iatek.com>, "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
"alain.volmat@...s.st.com" <alain.volmat@...s.st.com>, "conor+dt@...nel.org"
<conor+dt@...nel.org>, "sakari.ailus@...ux.intel.com"
<sakari.ailus@...ux.intel.com>, "robh@...nel.org" <robh@...nel.org>,
"hverkuil-cisco@...all.nl" <hverkuil-cisco@...all.nl>,
"tomi.valkeinen@...asonboard.com" <tomi.valkeinen@...asonboard.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, "matthias.bgg@...il.com"
<matthias.bgg@...il.com>, "mehdi.djait@...tlin.com"
<mehdi.djait@...tlin.com>, "krzk+dt@...nel.org" <krzk+dt@...nel.org>,
"angelogioacchino.delregno@...labora.com"
<angelogioacchino.delregno@...labora.com>, "10572168@...com"
<10572168@...com>
Subject: Re: [PATCH 1/2] media: dt-bindings: i2c: add Giantec GT97xx VCM
driver
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.
>
> >
> >
Powered by blists - more mailing lists