From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6EDE9C4741F for ; Wed, 4 Nov 2020 04:53:45 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 3A7C4223BD for ; Wed, 4 Nov 2020 04:53:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728351AbgKDExo (ORCPT ); Tue, 3 Nov 2020 23:53:44 -0500 Received: from anchovy2.45ru.net.au ([203.30.46.146]:36936 "EHLO anchovy2.45ru.net.au" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728415AbgKDExo (ORCPT ); Tue, 3 Nov 2020 23:53:44 -0500 Received: (qmail 30005 invoked by uid 5089); 4 Nov 2020 04:53:41 -0000 Received: by simscan 1.2.0 ppid: 29833, pid: 29836, t: 0.2493s scanners: regex: 1.2.0 attach: 1.2.0 clamav: 0.88.3/m:40/d:1950 Received: from unknown (HELO ?192.168.0.22?) (preid@electromag.com.au@203.59.235.95) by anchovy3.45ru.net.au with ESMTPA; 4 Nov 2020 04:53:40 -0000 Subject: Re: [PATCH 02/46] dt-bindings:iio:potentiometer:adi,ad5272 yaml conversion To: Rob Herring Cc: Jonathan Cameron , Jonathan Cameron , "open list:IIO SUBSYSTEM AND DRIVERS" , devicetree@vger.kernel.org References: <20201031184854.745828-1-jic23@kernel.org> <20201031184854.745828-3-jic23@kernel.org> <20201103161039.GA1754553@bogus> <20201103172834.00007040@Huawei.com> From: Phil Reid Message-ID: Date: Wed, 4 Nov 2020 12:53:38 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-AU Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org On 4/11/2020 11:12, Rob Herring wrote: > On Tue, Nov 3, 2020 at 6:39 PM Phil Reid wrote: >> >> On 4/11/2020 01:28, Jonathan Cameron wrote: >>> On Tue, 3 Nov 2020 10:10:39 -0600 >>> Rob Herring wrote: >>> >>>> On Sat, Oct 31, 2020 at 06:48:10PM +0000, Jonathan Cameron wrote: >>>>> From: Jonathan Cameron >>>>> >>>>> Simple direct conversion from txt to yaml as part of a general aim of >>>>> converting all IIO bindings to this machine readable format. >>>>> >>>>> Signed-off-by: Jonathan Cameron >>>>> Cc: Phil Reid >>>>> --- >>>>> .../bindings/iio/potentiometer/ad5272.txt | 27 ---------- >>>>> .../iio/potentiometer/adi,ad5272.yaml | 50 +++++++++++++++++++ >>>>> 2 files changed, 50 insertions(+), 27 deletions(-) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt b/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt >>>>> deleted file mode 100644 >>>>> index f9b2eef946aa..000000000000 >>>>> --- a/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt >>>>> +++ /dev/null >>>>> @@ -1,27 +0,0 @@ >>>>> -* Analog Devices AD5272 digital potentiometer >>>>> - >>>>> -The node for this device must be a child node of a I2C controller, hence >>>>> -all mandatory properties for your controller must be specified. See directory: >>>>> - >>>>> - Documentation/devicetree/bindings/i2c >>>>> - >>>>> -for more details. >>>>> - >>>>> -Required properties: >>>>> - - compatible: Must be one of the following, depending on the model: >>>>> - adi,ad5272-020 >>>>> - adi,ad5272-050 >>>>> - adi,ad5272-100 >>>>> - adi,ad5274-020 >>>>> - adi,ad5274-100 >>>>> - >>>>> -Optional properties: >>>>> - - reset-gpios: GPIO specification for the RESET input. This is an >>>>> - active low signal to the AD5272. >>>>> - >>>>> -Example: >>>>> -ad5272: potentiometer@2f { >>>>> - reg = <0x2F>; >>>>> - compatible = "adi,ad5272-020"; >>>>> - reset-gpios = <&gpio3 6 GPIO_ACTIVE_HIGH>; >>>>> -}; >>>>> diff --git a/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml b/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml >>>>> new file mode 100644 >>>>> index 000000000000..b9b7d383bff1 >>>>> --- /dev/null >>>>> +++ b/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml >>>>> @@ -0,0 +1,50 @@ >>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>>>> +%YAML 1.2 >>>>> +--- >>>>> +$id: http://devicetree.org/schemas/iio/potentiometer/adi,ad5272.yaml# >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>>> + >>>>> +title: Analog Devices AD5272 digital potentiometer >>>>> + >>>>> +maintainers: >>>>> + - Phil Reid >>>>> + >>>>> +description: | >>>>> + Datasheet: https://www.analog.com/en/products/ad5272.html >>>>> + >>>>> +properties: >>>>> + compatible: >>>>> + enum: >>>>> + - adi,ad5272-020 >>>>> + - adi,ad5272-050 >>>>> + - adi,ad5272-100 >>>>> + - adi,ad5274-020 >>>>> + - adi,ad5274-100 >>>>> + >>>>> + reg: >>>>> + maxItems: 1 >>>>> + >>>>> + reset-gpios: >>>>> + description: >>>>> + Active low signal to the AD5272 RESET input. >>>> >>>> Not a new problem, but active low or... >>>> >>>>> + >>>>> +additionalProperties: false >>>>> + >>>>> +required: >>>>> + - compatible >>>>> + - reg >>>>> + >>>>> +examples: >>>>> + - | >>>>> + #include >>>>> + i2c { >>>>> + #address-cells = <1>; >>>>> + #size-cells = <0>; >>>>> + >>>>> + potentiometer@2f { >>>>> + compatible = "adi,ad5272-020"; >>>>> + reg = <0x2F>; >>>>> + reset-gpios = <&gpio3 6 GPIO_ACTIVE_HIGH>; >>>> >>>> active high? >>> >>> Good spot! @Phil. Looks like the driver is setting the reset line to >>> 0 and then to 1 to come out of reset. So effectively inverting the logic. >>> I'm tempted to be cynical and suggest we just drop the comment above and leave >>> it vague but is there a better way we can clarify this? >> >> Had a look at a few other iio drivers in regards how they handle the same thing. >> A few do the same thing, ie: the drivers are written to set gpio low to assert reset. >> So they need the device tree gpio config to be active high to work correctly. >> Not sure if this prevents users setting things up as open collector. > > The driver is wrong. 'gpiod_set_value(reset_gpio, 1);' should assert > reset as '1' here is set to (reset) active state as defined in the DT. > > Given no upstream users, maybe it can be fixed... > > We need to make 'reset-gpios' implemented by a reset controller and > stop letting drivers get it wrong. > Yes I agree, the driver is wrong, think I just copied one of the other drivers for the pattern. I'd be happy to change it, there's probably few (if any) users. Having a software interface to assert the reset would be nice. -- Regards Phil Reid