From: Guenter Roeck <linux@roeck-us.net>
To: Nicolin Chen <nicoleotsuka@gmail.com>
Cc: jdelvare@suse.com, corbet@lwn.net, afd@ti.com,
linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-doc@vger.kernel.org
Subject: Re: [PATCH 2/2] hwmon: ina3221: Add enable sysfs nodes
Date: Wed, 26 Sep 2018 13:44:55 -0700 [thread overview]
Message-ID: <20180926204455.GA17035@roeck-us.net> (raw)
In-Reply-To: <20180926202519.GB10595@Asurada-Nvidia.nvidia.com>
On Wed, Sep 26, 2018 at 01:25:20PM -0700, Nicolin Chen wrote:
> Hello,
>
> On Wed, Sep 26, 2018 at 12:58:17PM -0700, Guenter Roeck wrote:
> > On Wed, Sep 26, 2018 at 11:02:44AM -0700, Nicolin Chen wrote:
> > > On Wed, Sep 26, 2018 at 06:06:32AM -0700, Guenter Roeck wrote:
> > > > On 09/25/2018 11:42 PM, Nicolin Chen wrote:
> > > > > The inX_enable interface allows user space to enable or disable
> > > > > the corresponding channel. Meanwhile, according to hwmon ABI, a
> > > > > disabled channel/sensor should return -ENODATA as a read result.
> > > > >
> > > > > However, there're configurable nodes sharing the same __show()
> > > > > functions. So this change also adds to check if the attribute is
> > > > > read-only to make sure it's not reading a configuration but the
> > > > > sensor data.
> > >
> > > > One necessary high level change I don't see below: With this change,
> > > > we should no longer drop a channel entirely if it is disabled from
> > > > devicetree. All channels should be visible but report -ENODATA if
> > > > disabled. In other words, it should be possible for the 'enable' flag
> > > > to override settings in DT.
> > >
> > > Hmm...I don't feel so convinced here. The status in DT binding isn't
> > > exactly a setting but a physical status: if a hardware design leaves
> > > a channel to be disconnected, I don't really see a point in enabling
> > > it in the runtime. Or maybe you can shed some light on it?
> > >
> >
> > You are making an assumption from your use case. It might as well be that
> > some or all channels are disabled in DT by default to conserve power,
> > not because they are disconnected.
>
> I think I probably should update my DT binding somehow to say it
> explicitly that the property should be only used in cases of the
> physical disconnections, although I feel the current binding "no
> input source" already has the same meaning.
>
> In my opinion, disabling channels in DT to save power isn't very
> plausible, because it sounds more likely a user decision, while,
> as we know, DT merely describes the hardware design.
>
I try to avoid making such assumptions. All I know is that I'll have
to deal with the fallout if a single person wants to use the property
differently. This is similar to disabling an entire subsystem in DT
because it isn't used in a specific system - say, a SPI or I2C bus
which has nothing connected on some shipping hardware, even though
the board has a connector for it. Your argument is that one shall
not use the status property do disable loading the driver, and that
one must not remove a set of properties for unused hardware either.
That doesn't sound very realistic to me.
Point is that I don't _know_ how this is going to be used, so I'd
rather keep it flexible.
Guenter
> Otherwise, if we want something like a setting for this purpose,
> we should probably use a different property for DT binding, bool
> type "disable-on-boot" for example.
>
> > > Meanwhile, I believe the enable nodes are necessary in either way as
> > > users could decide to disable the connected channels, based on their
> > > use cases, to save power.
> > >
> > Agreed, though I would not say "necessary". "Useful" seems to be more
> > appropriate.
>
> Yea..
next prev parent reply other threads:[~2018-09-26 20:45 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-26 6:42 [PATCH 0/2] hwmon: ina3221: Add power and enable sysfs nodes Nicolin Chen
2018-09-26 6:42 ` [PATCH 1/2] hwmon: ina3221: Add power " Nicolin Chen
2018-09-26 12:34 ` Guenter Roeck
2018-09-26 18:20 ` Nicolin Chen
2018-09-26 19:45 ` Guenter Roeck
2018-09-26 19:49 ` Nicolin Chen
2018-09-26 6:42 ` [PATCH 2/2] hwmon: ina3221: Add enable " Nicolin Chen
2018-09-26 13:06 ` Guenter Roeck
2018-09-26 18:02 ` Nicolin Chen
2018-09-26 19:58 ` Guenter Roeck
2018-09-26 20:25 ` Nicolin Chen
2018-09-26 20:44 ` Guenter Roeck [this message]
2018-09-26 21:55 ` Nicolin Chen
2018-09-27 16:05 ` Guenter Roeck
2018-09-27 18:39 ` Nicolin Chen
2018-09-27 22:26 ` Nicolin Chen
2018-09-27 22:52 ` Guenter Roeck
2018-09-27 23:14 ` Nicolin Chen
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=20180926204455.GA17035@roeck-us.net \
--to=linux@roeck-us.net \
--cc=afd@ti.com \
--cc=corbet@lwn.net \
--cc=jdelvare@suse.com \
--cc=linux-doc@vger.kernel.org \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nicoleotsuka@gmail.com \
/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 NNTP newsgroup(s).