Message ID | 20231115134453.6656-2-marius.cristea@microchip.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b909:0:b0:403:3b70:6f57 with SMTP id t9csp2545527vqg; Wed, 15 Nov 2023 05:46:30 -0800 (PST) X-Google-Smtp-Source: AGHT+IE3+H7GoPfBJWhY1a4FMSL8gJXRFrsxUtf51EQwv9MGALky+gAFHb8lOyY8LRsXQeLnnkvY X-Received: by 2002:a17:90b:11cc:b0:280:4c83:5f31 with SMTP id gv12-20020a17090b11cc00b002804c835f31mr9274585pjb.48.1700055990631; Wed, 15 Nov 2023 05:46:30 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1700055990; cv=none; d=google.com; s=arc-20160816; b=a1kaTjHz1oU5QBfsVjlL2G/eyqGT6Fu4ImZKbMqniho2PfWpuJQhSKY92JO+dYnWHZ HoWigk+LtQGZVSDaYMKNFp2WsoDdIdifS+WKLjBNpaJaqYZ244ZuxS1IC+No5Q+B1Cnt ldiusOJBBMOUub0BezbSo9IiD5zlLeEjv3bJbFluWaibQwH5e4SfHCMDYeVuIzNumo5s S6WF8PTmx8kTcXeoB8xS6YPx9torY93Vav1yCndoiEu/REOAkO6nmaYy0hHnWBVKdWag ymsUsd/NsUE+sT1M7qomcovAmItQFbw2q9Yk9ijU8nZ2XtTFNsWiP2MparqHNvsecd0m tmaQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=h3jKjabjTDK0ThIwn6+DlNrGQiFk5oHOawzy91h+krc=; fh=nv5mPDyL+gn29yYCkp4H0xRtubiLqqseGw1hdBUIMrI=; b=c7XrrFplNZr+fJUxyEXUHIuz+r4H6+ErP9Nj7tcvnqGYXdkK7WWYJrZXT0+DrxxSnw PkJUvNc1VTfHmkPfsXexHCw4cbPnxqqmnjeLaFMgNR8Gh3zwuC0cthdoT+hf4ixDcG5A QndPa0AOudxc9+kiadvVHiQ8SPyRIcYA/VeQ4csQ5VD1nexwTVdgyWqLFKmm5JwNdeZ7 e4cSZhoaVe6xgm1f8dQClo9xJqdPii0NvtkKGhkrKgDldmqBQ67HQ53TUSAHtWE2hZ91 nDpmblpxqvlPvBm47wWIaD7GCuA2psSA0YLW2aahAj/oAqz9Xq1HiXZAn7uIYKO2r7Ea ZsmQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@microchip.com header.s=mchp header.b=K5qR9s7u; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=microchip.com Received: from lipwig.vger.email (lipwig.vger.email. [23.128.96.33]) by mx.google.com with ESMTPS id 12-20020a17090a194c00b002792c288cc3si9903203pjh.169.2023.11.15.05.46.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 15 Nov 2023 05:46:30 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33; Authentication-Results: mx.google.com; dkim=pass header.i=@microchip.com header.s=mchp header.b=K5qR9s7u; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=microchip.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by lipwig.vger.email (Postfix) with ESMTP id 8F20C801F200; Wed, 15 Nov 2023 05:46:13 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at lipwig.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1343981AbjKONpr (ORCPT <rfc822;heyuhang3455@gmail.com> + 28 others); Wed, 15 Nov 2023 08:45:47 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54392 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1343879AbjKONpk (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 15 Nov 2023 08:45:40 -0500 Received: from esa.microchip.iphmx.com (esa.microchip.iphmx.com [68.232.154.123]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 801A39B; Wed, 15 Nov 2023 05:45:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=microchip.com; i=@microchip.com; q=dns/txt; s=mchp; t=1700055936; x=1731591936; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=OeuBwu619sZ9CKoQPi5K5tN9S8LiI/QU+WIb+tJzElw=; b=K5qR9s7unnXRV3gVy9PsYjzG1vlOI8nl2W8EmYvJpcwh++32NUETLJyE fY6lW/X0ybZMYqGCElq5mqVwaGo6qP31u+ogmM5jKb4czzcfaGTNV3pvd lZ509x8vT8Vw+0utX1fbUYh+msZWP1Xy85jK1dF5Ju/ylD7hj6Z+B1t2s o5lMcb/OSI0NROzKeQHI6mbGyMg70zNCLtB938sFoAnYi2o+rvbdHlj4m ph7JAHJ11osBg0YCejBqPDTdf1ukG1qF/5jCAHRQquaAsJc6t80fMJAxd msf3jetbR6zX3xEFAorxRlSxXf+4kSrCLLEyi0iaDGrMGA3cIYUjNvJ09 g==; X-CSE-ConnectionGUID: 2CxX/TgFSY+eV5q6XTnYdw== X-CSE-MsgGUID: vg1bWUbUSr6+Li1euWtieA== X-ThreatScanner-Verdict: Negative X-IronPort-AV: E=Sophos;i="6.03,305,1694761200"; d="scan'208";a="178883589" X-Amp-Result: SKIPPED(no attachment in message) Received: from unknown (HELO email.microchip.com) ([170.129.1.10]) by esa6.microchip.iphmx.com with ESMTP/TLS/ECDHE-RSA-AES128-GCM-SHA256; 15 Nov 2023 06:45:35 -0700 Received: from chn-vm-ex01.mchp-main.com (10.10.85.143) by chn-vm-ex03.mchp-main.com (10.10.85.151) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.21; Wed, 15 Nov 2023 06:45:03 -0700 Received: from marius-VM.mshome.net (10.10.85.11) by chn-vm-ex01.mchp-main.com (10.10.85.143) with Microsoft SMTP Server id 15.1.2507.21 via Frontend Transport; Wed, 15 Nov 2023 06:45:00 -0700 From: <marius.cristea@microchip.com> To: <jic23@kernel.org>, <lars@metafoo.de>, <robh+dt@kernel.org>, <jdelvare@suse.com>, <linux@roeck-us.net>, <linux-hwmon@vger.kernel.org> CC: <krzysztof.kozlowski+dt@linaro.org>, <conor+dt@kernel.org>, <linux-iio@vger.kernel.org>, <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <marius.cristea@microchip.com> Subject: [PATCH v3 1/2] dt-bindings: iio: adc: adding support for PAC193X Date: Wed, 15 Nov 2023 15:44:52 +0200 Message-ID: <20231115134453.6656-2-marius.cristea@microchip.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20231115134453.6656-1-marius.cristea@microchip.com> References: <20231115134453.6656-1-marius.cristea@microchip.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-Spam-Status: No, score=-1.0 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lipwig.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (lipwig.vger.email [0.0.0.0]); Wed, 15 Nov 2023 05:46:13 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1782637910227975096 X-GMAIL-MSGID: 1782637910227975096 |
Series |
adding support for Microchip PAC193X Power Monitor
|
|
Commit Message
marius.cristea@microchip.com
Nov. 15, 2023, 1:44 p.m. UTC
From: Marius Cristea <marius.cristea@microchip.com> This is the device tree schema for iio driver for Microchip PAC193X series of Power Monitors with Accumulator. Signed-off-by: Marius Cristea <marius.cristea@microchip.com> --- .../bindings/iio/adc/microchip,pac1934.yaml | 137 ++++++++++++++++++ 1 file changed, 137 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml
Comments
On 15/11/2023 14:44, marius.cristea@microchip.com wrote: > From: Marius Cristea <marius.cristea@microchip.com> > > This is the device tree schema for iio driver for > Microchip PAC193X series of Power Monitors with Accumulator. > > Signed-off-by: Marius Cristea <marius.cristea@microchip.com> > --- > .../bindings/iio/adc/microchip,pac1934.yaml | 137 ++++++++++++++++++ > 1 file changed, 137 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml > > diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml > new file mode 100644 > index 000000000000..2609cb19c377 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml > @@ -0,0 +1,137 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/adc/microchip,pac1934.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Microchip PAC1934 Power Monitors with Accumulator > + > +maintainers: > + - Marius Cristea <marius.cristea@microchip.com> > + > +description: | > + This device is part of the Microchip family of Power Monitors with Accumulator. > + The datasheet for PAC1931, PAC1932, PAC1933 and PAC1934 can be found here: > + https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/PAC1931-Family-Data-Sheet-DS20005850E.pdf > + > +properties: > + compatible: > + enum: > + - microchip,pac1931 > + - microchip,pac1932 > + - microchip,pac1933 > + - microchip,pac1934 > + > + reg: > + maxItems: 1 > + > + "#address-cells": > + const: 1 > + > + "#size-cells": > + const: 0 > + > + interrupts: > + maxItems: 1 > + > + microchip,slow-io: > + type: boolean > + description: | > + A GPIO used to trigger a change is sampling rate (lowering the chip power consumption). Use Linux coding style wrapping (as described in Linux Coding style). I am not going to tell you numbers because I want you to read the document first. This is boolean, not GPIO. I don't understand. "A GPIO", so any GPIO or some specific? How is this property related to GPIO? > + If configured in SLOW mode, if this pin is forced high, sampling rate is forced to eight This pin? This is boolean, not a GPIO. GPIOs are phandles. > + samples/second. When it is forced low, the sampling rate is 1024 samples/second unless > + a different sample rate has been programmed. > + > +patternProperties: > + "^channel@[1-4]+$": > + type: object > + $ref: adc.yaml > + description: Represents the external channels which are connected to the ADC. > + > + properties: > + reg: > + items: > + minimum: 1 > + maximum: 4 > + > + shunt-resistor-micro-ohms: > + description: | > + Value in micro Ohms of the shunt resistor connected between > + the SENSE+ and SENSE- inputs, across which the current is measured. Value > + is needed to compute the scaling of the measured current. > + > + required: > + - reg > + - shunt-resistor-micro-ohms > + > + unevaluatedProperties: false > + > +required: > + - compatible > + - reg > + - "#address-cells" > + - "#size-cells" > + > +allOf: > + - if: > + properties: > + compatible: > + contains: > + const: interrupts I don't understand what do you want to say here. I am also 100% sure you did not test it on a real case (maybe example passes but nothing more). > + then: > + properties: > + microchip,slow-io: false > + else: > + if: > + properties: > + compatible: > + contains: > + const: microchip,slow-io > + then: > + properties: > + interrupts: false Best regards, Krzysztof
On Thu, Nov 16, 2023 at 04:01:43PM +0100, Krzysztof Kozlowski wrote: > On 15/11/2023 14:44, marius.cristea@microchip.com wrote: > > From: Marius Cristea <marius.cristea@microchip.com> > > > > This is the device tree schema for iio driver for > > Microchip PAC193X series of Power Monitors with Accumulator. > > > > Signed-off-by: Marius Cristea <marius.cristea@microchip.com> > > --- > > .../bindings/iio/adc/microchip,pac1934.yaml | 137 ++++++++++++++++++ > > 1 file changed, 137 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml > > new file mode 100644 > > index 000000000000..2609cb19c377 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml > > @@ -0,0 +1,137 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/iio/adc/microchip,pac1934.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Microchip PAC1934 Power Monitors with Accumulator > > + > > +maintainers: > > + - Marius Cristea <marius.cristea@microchip.com> > > + > > +description: | > > + This device is part of the Microchip family of Power Monitors with Accumulator. > > + The datasheet for PAC1931, PAC1932, PAC1933 and PAC1934 can be found here: > > + https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/PAC1931-Family-Data-Sheet-DS20005850E.pdf > > + > > +properties: > > + compatible: > > + enum: > > + - microchip,pac1931 > > + - microchip,pac1932 > > + - microchip,pac1933 > > + - microchip,pac1934 > > + > > + reg: > > + maxItems: 1 > > + > > + "#address-cells": > > + const: 1 > > + > > + "#size-cells": > > + const: 0 > > + > > + interrupts: > > + maxItems: 1 > > + > > + microchip,slow-io: > > + type: boolean > > + description: | > > + A GPIO used to trigger a change is sampling rate (lowering the chip power consumption). > > Use Linux coding style wrapping (as described in Linux Coding style). I > am not going to tell you numbers because I want you to read the document > first. > > This is boolean, not GPIO. I don't understand. "A GPIO", so any GPIO or > some specific? How is this property related to GPIO? > > > > + If configured in SLOW mode, if this pin is forced high, sampling rate is forced to eight > > This pin? This is boolean, not a GPIO. GPIOs are phandles. I said it on the previous version, but this really seems like it should be something like "slow-io-gpios". I know Jonathan expressed some concerns about having to deal with it on the operating system side (as the pin is either an input & used for this slow-io control, or an output and used as an interrupt) but that is, in my opinion, a problem for the operating system & the binding should describe how the hardware works, even if that is not convenient. With this sort of property, a GPIO hog would be required to be set up (and the driver for that gpio controller bound etc before the pac driver loads) for correction functionality if this property was in the non-default state. > > + samples/second. When it is forced low, the sampling rate is 1024 samples/second unless > > + a different sample rate has been programmed. > > + > > +patternProperties: > > + "^channel@[1-4]+$": > > + type: object > > + $ref: adc.yaml > > + description: Represents the external channels which are connected to the ADC. > > + > > + properties: > > + reg: > > + items: > > + minimum: 1 > > + maximum: 4 > > + > > + shunt-resistor-micro-ohms: > > + description: | > > + Value in micro Ohms of the shunt resistor connected between > > + the SENSE+ and SENSE- inputs, across which the current is measured. Value > > + is needed to compute the scaling of the measured current. > > + > > + required: > > + - reg > > + - shunt-resistor-micro-ohms > > + > > + unevaluatedProperties: false > > + > > +required: > > + - compatible > > + - reg > > + - "#address-cells" > > + - "#size-cells" > > + > > +allOf: > > + - if: > > + properties: > > + compatible: > > + contains: > > + const: interrupts > > > I don't understand what do you want to say here. I am also 100% sure you > did not test it on a real case (maybe example passes but nothing more). As far as I understand, the same pin on the device is used for both an output or an input depending on the configuration. As an input, it is the "slow-io" control, and as an output it is an interrupt. I think Marius is trying to convey that either this pin can be in exclusively one state or another. _However_ I am not sure that that is really the right thing to do - they might well be mutually exclusive modes, but I think the decision can be made at runtime, rather than at devicetree creation time. Say for example the GPIO controller this is connected to is capable of acting as an interrupt controller. Unless I am misunderstanding the runtime configurability of this hardware, I think it is possible to actually provide a "slow-io-gpios" and an interrupt property & let the operating system decide at runtime which mode it wants to work in. I'm off travelling at the moment Marius, but I should be back in work on Monday if you want to have a chat about it & explain a bit more to me? Cheers, Conor. > > > + then: > > + properties: > > + microchip,slow-io: false > > + else: > > + if: > > + properties: > > + compatible: > > + contains: > > + const: microchip,slow-io > > + then: > > + properties: > > + interrupts: false > > Best regards, > Krzysztof >
On 16/11/2023 19:21, Conor Dooley wrote: >>> +allOf: >>> + - if: >>> + properties: >>> + compatible: >>> + contains: >>> + const: interrupts >> >> >> I don't understand what do you want to say here. I am also 100% sure you >> did not test it on a real case (maybe example passes but nothing more). > > As far as I understand, the same pin on the device is used for both an > output or an input depending on the configuration. As an input, it is > the "slow-io" control, and as an output it is an interrupt. > I think Marius is trying to convey that either this pin can be in > exclusively one state or another. > > _However_ I am not sure that that is really the right thing to do - they > might well be mutually exclusive modes, but I think the decision can be > made at runtime, rather than at devicetree creation time. Say for > example the GPIO controller this is connected to is capable of acting as > an interrupt controller. Unless I am misunderstanding the runtime > configurability of this hardware, I think it is possible to actually > provide a "slow-io-gpios" and an interrupt property & let the operating > system decide at runtime which mode it wants to work in. > > I'm off travelling at the moment Marius, but I should be back in work on > Monday if you want to have a chat about it & explain a bit more to me? Sure, but which compatible contains "interrupts"? Best regards, Krzysztof
On Thu, Nov 16, 2023 at 09:00:50PM +0100, Krzysztof Kozlowski wrote: > On 16/11/2023 19:21, Conor Dooley wrote: > > >>> +allOf: > >>> + - if: > >>> + properties: > >>> + compatible: > >>> + contains: > >>> + const: interrupts > >> > >> > >> I don't understand what do you want to say here. I am also 100% sure you > >> did not test it on a real case (maybe example passes but nothing more). > > > > As far as I understand, the same pin on the device is used for both an > > output or an input depending on the configuration. As an input, it is > > the "slow-io" control, and as an output it is an interrupt. > > I think Marius is trying to convey that either this pin can be in > > exclusively one state or another. > > > > _However_ I am not sure that that is really the right thing to do - they > > might well be mutually exclusive modes, but I think the decision can be > > made at runtime, rather than at devicetree creation time. Say for > > example the GPIO controller this is connected to is capable of acting as > > an interrupt controller. Unless I am misunderstanding the runtime > > configurability of this hardware, I think it is possible to actually > > provide a "slow-io-gpios" and an interrupt property & let the operating > > system decide at runtime which mode it wants to work in. > > > > I'm off travelling at the moment Marius, but I should be back in work on > > Monday if you want to have a chat about it & explain a bit more to me? > > Sure, but which compatible contains "interrupts"? Yeah, I did notice that - I figured you understood that that was meant to not be a check on compatibles, but rather on regular old properties & the rationale for the mutual exclusion was what you were missing.
On Thu, 16 Nov 2023 18:21:33 +0000 Conor Dooley <conor@kernel.org> wrote: > On Thu, Nov 16, 2023 at 04:01:43PM +0100, Krzysztof Kozlowski wrote: > > On 15/11/2023 14:44, marius.cristea@microchip.com wrote: > > > From: Marius Cristea <marius.cristea@microchip.com> > > > > > > This is the device tree schema for iio driver for > > > Microchip PAC193X series of Power Monitors with Accumulator. > > > > > > Signed-off-by: Marius Cristea <marius.cristea@microchip.com> > > > --- > > > .../bindings/iio/adc/microchip,pac1934.yaml | 137 ++++++++++++++++++ > > > 1 file changed, 137 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml > > > new file mode 100644 > > > index 000000000000..2609cb19c377 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml > > > @@ -0,0 +1,137 @@ > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/iio/adc/microchip,pac1934.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Microchip PAC1934 Power Monitors with Accumulator > > > + > > > +maintainers: > > > + - Marius Cristea <marius.cristea@microchip.com> > > > + > > > +description: | > > > + This device is part of the Microchip family of Power Monitors with Accumulator. > > > + The datasheet for PAC1931, PAC1932, PAC1933 and PAC1934 can be found here: > > > + https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/PAC1931-Family-Data-Sheet-DS20005850E.pdf > > > + > > > +properties: > > > + compatible: > > > + enum: > > > + - microchip,pac1931 > > > + - microchip,pac1932 > > > + - microchip,pac1933 > > > + - microchip,pac1934 > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + "#address-cells": > > > + const: 1 > > > + > > > + "#size-cells": > > > + const: 0 > > > + > > > + interrupts: > > > + maxItems: 1 > > > + > > > + microchip,slow-io: > > > + type: boolean > > > + description: | > > > + A GPIO used to trigger a change is sampling rate (lowering the chip power consumption). > > > > Use Linux coding style wrapping (as described in Linux Coding style). I > > am not going to tell you numbers because I want you to read the document > > first. > > > > This is boolean, not GPIO. I don't understand. "A GPIO", so any GPIO or > > some specific? How is this property related to GPIO? > > > > > > > + If configured in SLOW mode, if this pin is forced high, sampling rate is forced to eight > > > > This pin? This is boolean, not a GPIO. GPIOs are phandles. > > I said it on the previous version, but this really seems like it should > be something like "slow-io-gpios". I know Jonathan expressed some > concerns about having to deal with it on the operating system side (as > the pin is either an input & used for this slow-io control, or an output > and used as an interrupt) but that is, in my opinion, a problem for the > operating system & the binding should describe how the hardware works, > even if that is not convenient. With this sort of property, a GPIO hog > would be required to be set up (and the driver for that gpio controller > bound etc before the pac driver loads) for correction functionality if > this property was in the non-default state. I'd forgotten the discussion completely ;) My main question was why bother with slow? You can do it without the GPIO anyway as there is a register bit for it. I can conceive of various possible reasons, the evil one being that it's actually out of the host processors control. Is that what we care about here? If so it's nothing to do with a GPIO in the Linux sense at all and we can assume that it's not connected to an interrupt at the same time. We 'might' need a control for that case that says configure the device for an external entity to use the slow pin. It wouldn't be the first device to have that sort of thing, but normally they are sequencing pins that are wired up to some mechanical device or similar. > > > > + samples/second. When it is forced low, the sampling rate is 1024 samples/second unless > > > + a different sample rate has been programmed. > > > + > > > +patternProperties: > > > + "^channel@[1-4]+$": > > > + type: object > > > + $ref: adc.yaml > > > + description: Represents the external channels which are connected to the ADC. > > > + > > > + properties: > > > + reg: > > > + items: > > > + minimum: 1 > > > + maximum: 4 > > > + > > > + shunt-resistor-micro-ohms: > > > + description: | > > > + Value in micro Ohms of the shunt resistor connected between > > > + the SENSE+ and SENSE- inputs, across which the current is measured. Value > > > + is needed to compute the scaling of the measured current. > > > + > > > + required: > > > + - reg > > > + - shunt-resistor-micro-ohms > > > + > > > + unevaluatedProperties: false > > > + > > > +required: > > > + - compatible > > > + - reg > > > + - "#address-cells" > > > + - "#size-cells" > > > + > > > +allOf: > > > + - if: > > > + properties: > > > + compatible: > > > + contains: > > > + const: interrupts > > > > > > I don't understand what do you want to say here. I am also 100% sure you > > did not test it on a real case (maybe example passes but nothing more). > > As far as I understand, the same pin on the device is used for both an > output or an input depending on the configuration. As an input, it is > the "slow-io" control, and as an output it is an interrupt. > I think Marius is trying to convey that either this pin can be in > exclusively one state or another. > > _However_ I am not sure that that is really the right thing to do - they > might well be mutually exclusive modes, but I think the decision can be > made at runtime, rather than at devicetree creation time. Say for > example the GPIO controller this is connected to is capable of acting as > an interrupt controller. Unless I am misunderstanding the runtime > configurability of this hardware, I think it is possible to actually > provide a "slow-io-gpios" and an interrupt property & let the operating > system decide at runtime which mode it wants to work in. I'll admit I've long forgotten what was going on here, but based just on this bit of text I agree. There is nothing 'stopping' us having a pin uses as either / or / both interrupt and gpio. It'll be a bit messy to support in the driver as IIRC there are some sanity checks that limit combinations on IRQs and output GPIOS. Can't remember how bad the dance to navigate it safely is. First version I'd just say pick one option if both are provided and don't support configuring it at runtime. Jonathan > > I'm off travelling at the moment Marius, but I should be back in work on > Monday if you want to have a chat about it & explain a bit more to me? > > Cheers, > Conor. > > > > > > + then: > > > + properties: > > > + microchip,slow-io: false > > > + else: > > > + if: > > > + properties: > > > + compatible: > > > + contains: > > > + const: microchip,slow-io > > > + then: > > > + properties: > > > + interrupts: false > > > > Best regards, > > Krzysztof > >
On Sat, Nov 25, 2023 at 07:47:54PM +0000, Jonathan Cameron wrote: > On Thu, 16 Nov 2023 18:21:33 +0000 > Conor Dooley <conor@kernel.org> wrote: > > On Thu, Nov 16, 2023 at 04:01:43PM +0100, Krzysztof Kozlowski wrote: > > > On 15/11/2023 14:44, marius.cristea@microchip.com wrote: > > > > From: Marius Cristea <marius.cristea@microchip.com> > > > > +patternProperties: > > > > + "^channel@[1-4]+$": > > > > + type: object > > > > + $ref: adc.yaml > > > > + description: Represents the external channels which are connected to the ADC. > > > > + > > > > + properties: > > > > + reg: > > > > + items: > > > > + minimum: 1 > > > > + maximum: 4 > > > > + > > > > + shunt-resistor-micro-ohms: > > > > + description: | > > > > + Value in micro Ohms of the shunt resistor connected between > > > > + the SENSE+ and SENSE- inputs, across which the current is measured. Value > > > > + is needed to compute the scaling of the measured current. > > > > + > > > > + required: > > > > + - reg > > > > + - shunt-resistor-micro-ohms > > > > + > > > > + unevaluatedProperties: false > > > > + > > > > +required: > > > > + - compatible > > > > + - reg > > > > + - "#address-cells" > > > > + - "#size-cells" > > > > + > > > > +allOf: > > > > + - if: > > > > + properties: > > > > + compatible: > > > > + contains: > > > > + const: interrupts > > > > > > > > > I don't understand what do you want to say here. I am also 100% sure you > > > did not test it on a real case (maybe example passes but nothing more). > > > > As far as I understand, the same pin on the device is used for both an > > output or an input depending on the configuration. As an input, it is > > the "slow-io" control, and as an output it is an interrupt. > > I think Marius is trying to convey that either this pin can be in > > exclusively one state or another. > > > > _However_ I am not sure that that is really the right thing to do - they > > might well be mutually exclusive modes, but I think the decision can be > > made at runtime, rather than at devicetree creation time. Say for > > example the GPIO controller this is connected to is capable of acting as > > an interrupt controller. Unless I am misunderstanding the runtime > > configurability of this hardware, I think it is possible to actually > > provide a "slow-io-gpios" and an interrupt property & let the operating > > system decide at runtime which mode it wants to work in. > > I'll admit I've long forgotten what was going on here, but based just on > this bit of text I agree. There is nothing 'stopping' us having a pin > uses as either / or / both interrupt and gpio. > > It'll be a bit messy to support in the driver as IIRC there are some sanity > checks that limit combinations on IRQs and output GPIOS. Can't remember > how bad the dance to navigate it safely is. > > First version I'd just say pick one option if both are provided and > don't support configuring it at runtime. Just to be clear, you are suggesting having the "microchip,slow-io-gpios" and "interrupts" properties in the binding, but the driver will just (for example) put that pin into alert mode always & leave it there? If that is what you are suggesting, that seems pragmatic to me. Cheers, Conor.
On Sun, 26 Nov 2023 11:24:56 +0000 Conor Dooley <conor@kernel.org> wrote: > On Sat, Nov 25, 2023 at 07:47:54PM +0000, Jonathan Cameron wrote: > > On Thu, 16 Nov 2023 18:21:33 +0000 > > Conor Dooley <conor@kernel.org> wrote: > > > On Thu, Nov 16, 2023 at 04:01:43PM +0100, Krzysztof Kozlowski wrote: > > > > On 15/11/2023 14:44, marius.cristea@microchip.com wrote: > > > > > From: Marius Cristea <marius.cristea@microchip.com> > > > > > > +patternProperties: > > > > > + "^channel@[1-4]+$": > > > > > + type: object > > > > > + $ref: adc.yaml > > > > > + description: Represents the external channels which are connected to the ADC. > > > > > + > > > > > + properties: > > > > > + reg: > > > > > + items: > > > > > + minimum: 1 > > > > > + maximum: 4 > > > > > + > > > > > + shunt-resistor-micro-ohms: > > > > > + description: | > > > > > + Value in micro Ohms of the shunt resistor connected between > > > > > + the SENSE+ and SENSE- inputs, across which the current is measured. Value > > > > > + is needed to compute the scaling of the measured current. > > > > > + > > > > > + required: > > > > > + - reg > > > > > + - shunt-resistor-micro-ohms > > > > > + > > > > > + unevaluatedProperties: false > > > > > + > > > > > +required: > > > > > + - compatible > > > > > + - reg > > > > > + - "#address-cells" > > > > > + - "#size-cells" > > > > > + > > > > > +allOf: > > > > > + - if: > > > > > + properties: > > > > > + compatible: > > > > > + contains: > > > > > + const: interrupts > > > > > > > > > > > > I don't understand what do you want to say here. I am also 100% sure you > > > > did not test it on a real case (maybe example passes but nothing more). > > > > > > As far as I understand, the same pin on the device is used for both an > > > output or an input depending on the configuration. As an input, it is > > > the "slow-io" control, and as an output it is an interrupt. > > > I think Marius is trying to convey that either this pin can be in > > > exclusively one state or another. > > > > > > _However_ I am not sure that that is really the right thing to do - they > > > might well be mutually exclusive modes, but I think the decision can be > > > made at runtime, rather than at devicetree creation time. Say for > > > example the GPIO controller this is connected to is capable of acting as > > > an interrupt controller. Unless I am misunderstanding the runtime > > > configurability of this hardware, I think it is possible to actually > > > provide a "slow-io-gpios" and an interrupt property & let the operating > > > system decide at runtime which mode it wants to work in. > > > > I'll admit I've long forgotten what was going on here, but based just on > > this bit of text I agree. There is nothing 'stopping' us having a pin > > uses as either / or / both interrupt and gpio. > > > > It'll be a bit messy to support in the driver as IIRC there are some sanity > > checks that limit combinations on IRQs and output GPIOS. Can't remember > > how bad the dance to navigate it safely is. > > > > First version I'd just say pick one option if both are provided and > > don't support configuring it at runtime. > > Just to be clear, you are suggesting having the > "microchip,slow-io-gpios" and "interrupts" properties in the binding, > but the driver will just (for example) put that pin into alert mode > always & leave it there? Yes. > If that is what you are suggesting, that seems pragmatic to me. If a use case to do something else comes along later, then we can be smarter, but I'd like to keep it simple initially at least. Jonathan > > Cheers, > Conor. >
On Sun, Nov 26, 2023 at 04:04:38PM +0000, Jonathan Cameron wrote: > On Sun, 26 Nov 2023 11:24:56 +0000 > Conor Dooley <conor@kernel.org> wrote: > > On Sat, Nov 25, 2023 at 07:47:54PM +0000, Jonathan Cameron wrote: > > > On Thu, 16 Nov 2023 18:21:33 +0000 > > > Conor Dooley <conor@kernel.org> wrote: > > > > On Thu, Nov 16, 2023 at 04:01:43PM +0100, Krzysztof Kozlowski wrote: > > > > > On 15/11/2023 14:44, marius.cristea@microchip.com wrote: > > > > > > From: Marius Cristea <marius.cristea@microchip.com> > > > > > > +patternProperties: > > > > > > + "^channel@[1-4]+$": > > > > > > + type: object > > > > > > + $ref: adc.yaml > > > > > > + description: Represents the external channels which are connected to the ADC. > > > > > > + > > > > > > + properties: > > > > > > + reg: > > > > > > + items: > > > > > > + minimum: 1 > > > > > > + maximum: 4 > > > > > > + > > > > > > + shunt-resistor-micro-ohms: > > > > > > + description: | > > > > > > + Value in micro Ohms of the shunt resistor connected between > > > > > > + the SENSE+ and SENSE- inputs, across which the current is measured. Value > > > > > > + is needed to compute the scaling of the measured current. > > > > > > + > > > > > > + required: > > > > > > + - reg > > > > > > + - shunt-resistor-micro-ohms > > > > > > + > > > > > > + unevaluatedProperties: false > > > > > > + > > > > > > +required: > > > > > > + - compatible > > > > > > + - reg > > > > > > + - "#address-cells" > > > > > > + - "#size-cells" > > > > > > + > > > > > > +allOf: > > > > > > + - if: > > > > > > + properties: > > > > > > + compatible: > > > > > > + contains: > > > > > > + const: interrupts > > > > > > > > > > > > > > > I don't understand what do you want to say here. I am also 100% sure you > > > > > did not test it on a real case (maybe example passes but nothing more). > > > > > > > > As far as I understand, the same pin on the device is used for both an > > > > output or an input depending on the configuration. As an input, it is > > > > the "slow-io" control, and as an output it is an interrupt. > > > > I think Marius is trying to convey that either this pin can be in > > > > exclusively one state or another. > > > > > > > > _However_ I am not sure that that is really the right thing to do - they > > > > might well be mutually exclusive modes, but I think the decision can be > > > > made at runtime, rather than at devicetree creation time. Say for > > > > example the GPIO controller this is connected to is capable of acting as > > > > an interrupt controller. Unless I am misunderstanding the runtime > > > > configurability of this hardware, I think it is possible to actually > > > > provide a "slow-io-gpios" and an interrupt property & let the operating > > > > system decide at runtime which mode it wants to work in. > > > > > > I'll admit I've long forgotten what was going on here, but based just on > > > this bit of text I agree. There is nothing 'stopping' us having a pin > > > uses as either / or / both interrupt and gpio. > > > > > > It'll be a bit messy to support in the driver as IIRC there are some sanity > > > checks that limit combinations on IRQs and output GPIOS. Can't remember > > > how bad the dance to navigate it safely is. > > > > > > First version I'd just say pick one option if both are provided and > > > don't support configuring it at runtime. > > > > Just to be clear, you are suggesting having the > > "microchip,slow-io-gpios" and "interrupts" properties in the binding, > > but the driver will just (for example) put that pin into alert mode > > always & leave it there? > > Yes. > > > If that is what you are suggesting, that seems pragmatic to me. > > If a use case to do something else comes along later, then we can > be smarter, but I'd like to keep it simple initially at least. Sounds good to me, thanks Jonathan. Seems like a good compromise of depicting the hardware accurately and not overcomplicating the driver implementation. Marius, I completely forgot to get in touch with you about this - give me a shout on teams if there's anything outstanding that I can help you with.
diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml new file mode 100644 index 000000000000..2609cb19c377 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml @@ -0,0 +1,137 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/adc/microchip,pac1934.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Microchip PAC1934 Power Monitors with Accumulator + +maintainers: + - Marius Cristea <marius.cristea@microchip.com> + +description: | + This device is part of the Microchip family of Power Monitors with Accumulator. + The datasheet for PAC1931, PAC1932, PAC1933 and PAC1934 can be found here: + https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/PAC1931-Family-Data-Sheet-DS20005850E.pdf + +properties: + compatible: + enum: + - microchip,pac1931 + - microchip,pac1932 + - microchip,pac1933 + - microchip,pac1934 + + reg: + maxItems: 1 + + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + + interrupts: + maxItems: 1 + + microchip,slow-io: + type: boolean + description: | + A GPIO used to trigger a change is sampling rate (lowering the chip power consumption). + If configured in SLOW mode, if this pin is forced high, sampling rate is forced to eight + samples/second. When it is forced low, the sampling rate is 1024 samples/second unless + a different sample rate has been programmed. + +patternProperties: + "^channel@[1-4]+$": + type: object + $ref: adc.yaml + description: Represents the external channels which are connected to the ADC. + + properties: + reg: + items: + minimum: 1 + maximum: 4 + + shunt-resistor-micro-ohms: + description: | + Value in micro Ohms of the shunt resistor connected between + the SENSE+ and SENSE- inputs, across which the current is measured. Value + is needed to compute the scaling of the measured current. + + required: + - reg + - shunt-resistor-micro-ohms + + unevaluatedProperties: false + +required: + - compatible + - reg + - "#address-cells" + - "#size-cells" + +allOf: + - if: + properties: + compatible: + contains: + const: interrupts + then: + properties: + microchip,slow-io: false + else: + if: + properties: + compatible: + contains: + const: microchip,slow-io + then: + properties: + interrupts: false + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + pac193x: power-monitor@10 { + compatible = "microchip,pac1934"; + reg = <0x10>; + + #address-cells = <1>; + #size-cells = <0>; + + channel@1 { + reg = <0x1>; + shunt-resistor-micro-ohms = <24900000>; + label = "CPU"; + }; + + channel@2 { + reg = <0x2>; + shunt-resistor-micro-ohms = <49900000>; + label = "GPU"; + }; + + channel@3 { + reg = <0x3>; + shunt-resistor-micro-ohms = <75000000>; + label = "MEM"; + bipolar; + }; + + channel@4 { + reg = <0x4>; + shunt-resistor-micro-ohms = <100000000>; + label = "NET"; + bipolar; + }; + }; + }; + +...