From: Ban Feng <baneric926@gmail.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc: "linux-hwmon@vger.kernel.org" <linux-hwmon@vger.kernel.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"conor+dt@kernel.org" <conor+dt@kernel.org>,
"jdelvare@suse.com" <jdelvare@suse.com>,
"corbet@lwn.net" <corbet@lwn.net>,
"openbmc@lists.ozlabs.org" <openbmc@lists.ozlabs.org>,
"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"DELPHINE_CHIU@wiwynn.com" <DELPHINE_CHIU@wiwynn.com>,
"kcfeng0@nuvoton.com" <kcfeng0@nuvoton.com>,
"robh+dt@kernel.org" <robh+dt@kernel.org>,
"krzysztof.kozlowski+dt@linaro.org"
<krzysztof.kozlowski+dt@linaro.org>,
"kwliu@nuvoton.com" <kwliu@nuvoton.com>,
"linux@roeck-us.net" <linux@roeck-us.net>
Subject: [PATCH v1 1/2] dt-bindings: hwmon: Add nct736x bindings
Date: Fri, 8 Dec 2023 11:05:20 +0800 [thread overview]
Message-ID: <CALz278bVoO=bKzgTbV7zBQHLwuKBB2PSxHMyxRYn3HWMA6z8xw@mail.gmail.com> (raw)
In-Reply-To: <4770f744-2309-4de0-8aaf-a221f69c08b7@linaro.org>
Hi Krzysztof,
On Tuesday, December 5, 2023, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 05/12/2023 10:31, Ban Feng wrote:
> > Hi Krzysztof,
> >
> > On Mon, Dec 4, 2023 at 4:04 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 04/12/2023 06:56, baneric926@gmail.com wrote:
> >>> From: Ban Feng <kcfeng0@nuvoton.com>
> >>>
> >>> This change documents the device tree bindings for the Nuvoton
> >>> NCT7362Y, NCT7363Y driver.
> >>>
> >>> Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
> >>> ---
> >>> .../bindings/hwmon/nuvoton,nct736x.yaml | 80 +++++++++++++++++++
> >>> MAINTAINERS | 6 ++
> >>> 2 files changed, 86 insertions(+)
> >>> create mode 100644 Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml b/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml
> >>> new file mode 100644
> >>> index 000000000000..f98fd260a20f
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml
> >>> @@ -0,0 +1,80 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +
> >>> +$id: http://devicetree.org/schemas/hwmon/nuvoton,nct736x.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Nuvoton NCT736X Hardware Monitoring IC
> >>> +
> >>> +maintainers:
> >>> + - Ban Feng <kcfeng0@nuvoton.com>
> >>> +
> >>> +description: |
> >>> + The NCT736X is a Fan controller which provides up to 16 independent
> >>> + FAN input monitors, and up to 16 independent PWM output with SMBus interface.
> >>> + Besides, NCT7363Y has a built-in watchdog timer which is used for
> >>> + conditionally generating a system reset output (INT#).
> >>> +
> >>> +additionalProperties: false
> >>
> >> Please place it just like other bindings are placing it. Not in some
> >> random order. See example-schema.
> >
> > ok, I'll move additionalProperties to the correct place.
> >
> >>
> >> You should use common fan properties. If it was not merged yet, you must
> >> rebase on patchset on LKML and mention the dependency in the change log
> >> (---).
> >
> > please kindly see below
> >
> >>
> >>> +
> >>> +properties:
> >>> + compatible:
> >>> + enum:
> >>> + - nuvoton,nct7362
> >>> + - nuvoton,nct7363
> >>> +
> >>> + reg:
> >>> + maxItems: 1
> >>> +
> >>> + nuvoton,pwm-mask:
> >>> + description: |
> >>> + each bit means PWMx enable/disable setting, where x = 0~15.
> >>> + 0: disabled, 1: enabled
> >>> + $ref: /schemas/types.yaml#/definitions/uint32
> >>> + minimum: 0x0
> >>> + maximum: 0xFFFF
> >>> + default: 0x0
> >>
> >> Use pwms, not own property for this.
> >
> > NCT736X has 16 funtion pins, they can be
> > GPIO/PWM/FANIN/Reserved_or_ALERT#, and default is GPIO.
> > We would like to add such a property that can configure the function
> > pin for pin0~7 and pin10~17.
>
> It looks you are writing custom pinctrl instead of using standard
> bindings and frameworks.
Please kindly see below
>
>
> >
> > My original design is only for PWM/FANIN, however,
> > I noticed that we can change the design to "nuvoton,pin[0-7]$" and
> > "nuvoton,pin[10-17]$", like example in adt7475.yaml.
> > I'm not sure if this can be accepted or not, please kindly review this.
>
> It looks like the same problem as with other fan/Nuvoton bindings we
> discussed some time ago on the lists.
>
> Please do not duplicate threads, work and reviews:
> https://lore.kernel.org/all/20230607101827.8544-5-zev@bewilderbeest.net/
>
> I already gave same comments there.
Thanks for your kindly sharing. I noticed that [1] defines some useful
properties, pwms and tach-ch, like you mentioned.
Therefore, I'll modify our design to follow the common fan properties in v2.
[1] https://lists.openwall.net/linux-kernel/2023/11/07/406
>
>
> >>> + nuvoton,wdt-timeout:
> >>> + description: |
> >>> + Watchdog Timer time configuration for NCT7363Y, as below
> >>> + 0: 15 sec (default)
> >>> + 1: 7.5 sec
> >>> + 2: 3.75 sec
> >>> + 3: 30 sec
> >>> + $ref: /schemas/types.yaml#/definitions/uint32
> >>> + enum: [0, 1, 2, 3]
> >>> + default: 0
> >>
> >> Nope, reference watchdog.yaml and use its properties. See other watchdog
> >> bindings for examples.
> >
> > NCT7363 has a built-in watchdog timer which is used for conditionally
> > generating a system reset
> > output (INT#) if the microcontroller or microprocessor stops to
> > periodically send a pulse signal or
> > interface communication access signal like SCL signal from SMBus interface.
> >
> > We only consider "Watchdog Timer Configuration Register" enable bit
> > and timeout setting.
> > Should we still need to add struct watchdog_device to struct nct736x_data?
>
> I do not see correlation between watchdog.yaml and some struct. I did
> not write anything about driver, so I don't understand your comments.
>
> Anyway, I don't like that we are duplicating entire effort and our
> review. Please join existing discussions, threads and build on top of it.
>
Thanks, I'll remove unused function in hwmon subsystem,
and consider a watchdog subsystem design if necessary.
>
> Best regards,
> Krzysztof
>
prev parent reply other threads:[~2023-12-11 23:17 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20231204055650.788388-1-kcfeng0@nuvoton.com>
2023-12-04 7:04 ` [PATCH v1 0/2] hwmon: Driver for Nuvoton NCT736X Guenter Roeck
[not found] ` <20231204055650.788388-3-kcfeng0@nuvoton.com>
2023-12-04 7:06 ` [PATCH v1 2/2] " Guenter Roeck
2023-12-05 11:04 ` kernel test robot
2023-12-05 14:14 ` kernel test robot
2023-12-07 5:41 ` Dan Carpenter
[not found] ` <20231204055650.788388-2-kcfeng0@nuvoton.com>
[not found] ` <94607c47-9824-4e2c-8f22-99ca2e088b27@linaro.org>
[not found] ` <CALz278ZbjcbjUmFKv4B20DPDW33KPW-nZn4if3qTLjYKZZmWWw@mail.gmail.com>
[not found] ` <4770f744-2309-4de0-8aaf-a221f69c08b7@linaro.org>
2023-12-08 3:05 ` Ban Feng [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CALz278bVoO=bKzgTbV7zBQHLwuKBB2PSxHMyxRYn3HWMA6z8xw@mail.gmail.com' \
--to=baneric926@gmail.com \
--cc=DELPHINE_CHIU@wiwynn.com \
--cc=conor+dt@kernel.org \
--cc=corbet@lwn.net \
--cc=devicetree@vger.kernel.org \
--cc=jdelvare@suse.com \
--cc=kcfeng0@nuvoton.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=krzysztof.kozlowski@linaro.org \
--cc=kwliu@nuvoton.com \
--cc=linux-doc@vger.kernel.org \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=openbmc@lists.ozlabs.org \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).