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]

Message-ID: <9e41a61d-3607-4698-b942-78fca235afd2@kernel.org>
Date: Thu, 11 Apr 2024 08:05:59 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Zhi Mao (毛智) <zhi.mao@...iatek.com>,
 "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

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