Linux-Hwmon Archive on lore.kernel.org
 help / color / Atom feed
From: "Sa, Nuno" <Nuno.Sa@analog.com>
To: "linux@roeck-us.net" <linux@roeck-us.net>
Cc: "robh+dt@kernel.org" <robh+dt@kernel.org>,
	"corbet@lwn.net" <corbet@lwn.net>,
	"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-hwmon@vger.kernel.org" <linux-hwmon@vger.kernel.org>,
	"jdelvare@suse.com" <jdelvare@suse.com>,
	"mark.rutland@arm.com" <mark.rutland@arm.com>
Subject: Re: [PATCH 2/3] hwmon: Add support for ltc2947
Date: Thu, 10 Oct 2019 07:13:06 +0000
Message-ID: <ea52f348c9fab8c9a3c3417909053f4abb12cbcb.camel@analog.com> (raw)
In-Reply-To: <5f280daff806cb73a160d10f5ec97be18d42aebb.camel@analog.com>

Hi Guenter,

I got some feedback about energy max/min...

On Mon, 2019-10-07 at 14:51 +0000, Sa, Nuno wrote:
> On Mon, 2019-10-07 at 05:44 -0700, Guenter Roeck wrote:
> > On 10/7/19 5:25 AM, Sa, Nuno wrote:
> > > On Fri, 2019-10-04 at 08:06 -0700, Guenter Roeck wrote:
> > > > On Fri, Oct 04, 2019 at 07:45:07AM +0000, Sa, Nuno wrote:
> > > > [ ... ]
> > > > > > > +static int ltc2947_val_read(struct ltc2947_data *st,
> > > > > > > const
> > > > > > > u8
> > > > > > > reg,
> > > > > > > +			    const u8 page, const size_t size,
> > > > > > > s64 *val)
> > > > > > > +{
> > > > > > > +	int ret;
> > > > > > > +	u64 __val = 0;
> > > > > > > +
> > > > > > > +	mutex_lock(&st->lock);
> > > > > > > +
> > > > 
> > > > On a side note, suspend code is supposed to be atomic,
> > > > If this lock is held, the process or thread holding it
> > > > will likely stall suspend since it won't run.
> > > > At the very least, this would have to be a trylock,
> > > > with suspend failing if the lock can not be acquired.
> > > 
> > > Got it. Even more, Now I don't see the point of having the mutex
> > > in
> > > the
> > > PM callbacks (though I saw other drivers doing this). As you
> > > said,
> > > at
> > > the very least the trylock should be used since a frozen task
> > > might
> > > have the mutex locked. Either way, I will drop both the flag and
> > > the
> > > call to mutex_* functions is suspend()/resume().
> > > 
> > > > > > > +	if (st->reset) {
> > > > > > > +		mutex_unlock(&st->lock);
> > > > > > > +		return -EPERM;
> > > > > > 
> > > > > > Not sure what the error here should be, but EPERM is not
> > > > > > correct.
> > > > > > 
> > > > > > Under which conditions would this function be called while
> > > > > > in
> > > > > > suspend ?
> > > > > 
> > > > > Honestly, this is more like a sanity check. I'm not sure if
> > > > > we
> > > > > can
> > > > > get
> > > > > here in suspend mode. Don't userland apps can still run in
> > > > > suspend?
> > > > > I
> > > > > guess so but I'm not 100% sure on this. Do you have any
> > > > > recommendation
> > > > > for the error here?
> > > > > 
> > > > Sorry, I won't accept "just in case" code.
> > > > 
> > > > Having said that, please inform yourself how suspend works.
> > > > Userland
> > > > code
> > > > has to be stopped before any hardware can be disabled. Similar,
> > > > hardware
> > > > has to be re-enabled before userland code can resume.
> > > > Otherwise the kernel would crash all over the place. In many
> > > > cases,
> > > > disabling the hardware means that trying to access hardware
> > > > registers
> > > > will cause an acess fault.
> > > 
> > > Yes, you are right. I did checked the suspend code and all
> > > userland
> > > tasks and kthreads are stopped before the suspend() callback is
> > > called
> > > for the HW devices.
> > > 
> > > > [...]
> > > > 
> > > > > > > +
> > > > > > > +static struct attribute *ltc2947_attrs[] = {
> > > > > > > +	&sensor_dev_attr_in0_fault.dev_attr.attr,
> > > > > > > +	&sensor_dev_attr_in1_fault.dev_attr.attr,
> > > > > > > +	&sensor_dev_attr_curr1_fault.dev_attr.attr,
> > > > > > > +	&sensor_dev_attr_temp1_fault.dev_attr.attr,
> > > > > > > +	&sensor_dev_attr_power1_input.dev_attr.attr,
> > > > > > > +	&sensor_dev_attr_power1_max.dev_attr.attr,
> > > > > > > +	&sensor_dev_attr_power1_min.dev_attr.attr,
> > > > > > > +	&sensor_dev_attr_power1_input_highest.dev_attr.attr,
> > > > > > > +	&sensor_dev_attr_power1_input_lowest.dev_attr.attr,
> > > > > > > +	&sensor_dev_attr_power1_fault.dev_attr.attr,
> > > > > > > +	&sensor_dev_attr_energy1_input.dev_attr.attr,
> > > > > > > +	&sensor_dev_attr_energy1_max.dev_attr.attr,
> > > > > > > +	&sensor_dev_attr_energy1_min.dev_attr.attr,
> > > > > > > +	&sensor_dev_attr_energy1_max_alarm.dev_attr.attr,
> > > > > > > +	&sensor_dev_attr_energy1_min_alarm.dev_attr.attr,
> > > > > > > +	&sensor_dev_attr_energy2_input.dev_attr.attr,
> > > > > > > +	&sensor_dev_attr_energy2_max.dev_attr.attr,
> > > > > > > +	&sensor_dev_attr_energy2_min.dev_attr.attr,
> > > > > > > +	&sensor_dev_attr_energy2_max_alarm.dev_attr.attr,
> > > > > > > +	&sensor_dev_attr_energy2_min_alarm.dev_attr.attr,
> > > > > > > +	&sensor_dev_attr_energy1_overflow_alarm.dev_attr.attr,
> > > > > > > +	&sensor_dev_attr_energy2_overflow_alarm.dev_attr.attr,
> > > > > 
> > > > > These overflow attributes are kind of an alarm for the energy
> > > > > ones.
> > > > > It
> > > > > tells that the energy registers are about to overflow. I
> > > > > guess
> > > > > that
> > > > > some application can easily find out the maximum values
> > > > > supported
> > > > > on
> > > > > these registers and implement whatever logic they want in the
> > > > > app
> > > > > itself. So, if you prefer I can just drop this ones?
> > > > > 
> > > > I understand the overflow use case. However, the mere presence
> > > > of min/max attributes for energy attributes suggests that this
> > > > is not the min/max use case for hwmon attributes. There is no
> > > > "minimum"
> > > > or "maximum" energy for an accumulating value. Such attributes
> > > > only make sense in an application abler to measure available
> > > > energy (eg a battery controller). I'll have to read the chip
> > > > specification to understand the intended use case.
> > > 
> > > Should I just drop the overflow attributes? I think the part can
> > > be
> > > used to check battery charging efficiency for example (though I
> > > guess
> > > we would need to also support the Charge register's).
> > > 
> > 
> > If chip is (or can be) used as charger, it should register as such.
> > Note my question was the energy limit attributes, not the overflow
> > attributes.
> 
> I don't think it can be used as a charger but it can also measure
> charge (integrating the measured current over time). As they are not
> standard attributes I did not included this on the driver (I sent a
> query on this before starting the driver - 
> https://marc.info/?l=linux-hwmon&m=156507711612877&w=2).
> 
> I see your point about energy and having maximum and minimum for an
> accumulated value. Honestly, looking at the chip specification I
> cannot
> see the intended use case for this. Maybe for
> monitoring/characterizing
> batteries since there are some controls on these accumulated values
> (we
> can set the part to accumulate only when current is positive for
> example).
> I will do some internal querying to see if I can find out more on
> this.
> 

Quoting the reply I had:

"As the LTC2947 is bi-directional, the most likely use for the Min/Max
Energy thresholds are for monitoring a battery being charged or
discharged. 
A limit could be set based around the battery's storage capacity so
that the battery is protected from being overcharging or fully
drained."

So, I'm not sure if this is a valid use case for hwmon subsystem. I'm
aware there's also the power subsystem. So let me know what do you
prefer here. Should I just report energyX_input attributes? Or can we
keep the min/max?


Nuno Sá


  reply index

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-24 12:49 [PATCH 0/3] LTC2947 support Nuno Sá
2019-09-24 12:49 ` [PATCH 1/3] hwmon: Fix HWMON_P_MIN_ALARM mask Nuno Sá
2019-10-02 13:37   ` Guenter Roeck
2019-09-24 12:49 ` [PATCH 2/3] hwmon: Add support for ltc2947 Nuno Sá
2019-10-03  4:14   ` Guenter Roeck
2019-10-04  7:45     ` Sa, Nuno
2019-10-04 15:06       ` Guenter Roeck
2019-10-07 12:25         ` Sa, Nuno
2019-10-07 12:44           ` Guenter Roeck
2019-10-07 14:51             ` Sa, Nuno
2019-10-10  7:13               ` Sa, Nuno [this message]
2019-10-10 15:21                 ` Guenter Roeck
2019-10-11  6:59                   ` Sa, Nuno
2019-09-24 12:49 ` [PATCH 3/3] dt-bindings: iio: Add ltc2947 documentation Nuno Sá
2019-10-02 14:19   ` Rob Herring
2019-10-02 15:09     ` Sa, Nuno
2019-10-02 19:06       ` Rob Herring
2019-10-04 14:58         ` Sa, Nuno
2019-10-04 15:23           ` Rob Herring
2019-10-08 14:20             ` Sa, Nuno
2019-09-26 10:17 ` [PATCH 0/3] LTC2947 support Sa, Nuno

Reply instructions:

You may reply publically 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=ea52f348c9fab8c9a3c3417909053f4abb12cbcb.camel@analog.com \
    --to=nuno.sa@analog.com \
    --cc=corbet@lwn.net \
    --cc=devicetree@vger.kernel.org \
    --cc=jdelvare@suse.com \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-hwmon@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=mark.rutland@arm.com \
    --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

Linux-Hwmon Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hwmon/0 linux-hwmon/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hwmon linux-hwmon/ https://lore.kernel.org/linux-hwmon \
		linux-hwmon@vger.kernel.org linux-hwmon@archiver.kernel.org
	public-inbox-index linux-hwmon

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hwmon


AGPL code for this site: git clone https://public-inbox.org/ public-inbox