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á
