Re: [PATCH 0/3] Add support to disable sensor groups in P9

[Guenter Roeck]

On 05/16/2018 11:10 PM, Shilpasri G Bhat wrote:
> On 05/15/2018 08:32 PM, Guenter Roeck wrote:
> > On Thu, Mar 22, 2018 at 04:24:32PM +0530, Shilpasri G Bhat wrote:
> > > This patch series adds support to enable/disable OCC based
> > > inband-sensor groups at runtime. The environmental sensor groups are
> > > managed in HWMON and the remaining platform specific sensor groups are
> > > managed in /sys/firmware/opal.
> > >
> > > The firmware changes required for this patch is posted below:
> > > https://lists.ozlabs.org/pipermail/skiboot/2018-March/010812.html
> >
> > Sorry for not getting back earlier. This is a tough one.
>
> Thanks for the reply. I have tried to answer your questions according to my
> understanding below:
>
> > Key problem is that you are changing the ABI with those new attributes.
> > On top of that, the attributes _do_ make some sense (many chips support
> > enabling/disabling of individual sensors), suggesting that those or
> > similar attributes may or even should at some point be added to the ABI.
> >
> > At the same time, returning "0" as measurement values when sensors are
> > disabled does not seem like a good idea, since "0" is a perfectly valid
> > measurement, at least for most sensors.
>
> I agree.
>
> > Given that, we need to have a discussion about adding _enable attributes to
> > the ABI
>
> > what is the scope,
> IIUC the scope should be RW and the attribute is defined for each supported
> sensor group

That is _your_ need. I am not aware of any other chip where a per-sensor group
attribute would make sense. The discussion we need has to extend beyond the need
of a single chip.

Guenter

> > when should the attributes exist and when not,
> We control this currently via device-tree
>
> > do we want/need power_enable or powerX_enable or both, and so on), and
> We need power_enable right now
>
> > what to return if a sensor is disabled (such as -ENODATA).
> -ENODATA sounds good.
>
> Thanks and Regards,
> Shilpa
>
>   Once we have an
> > agreement, we can continue with an implementation.
> >
> > Guenter
> >
> > > Shilpasri G Bhat (3):
> > >    powernv:opal-sensor-groups: Add support to enable sensor groups
> > >    hwmon: ibmpowernv: Add attributes to enable/disable sensor groups
> > >    powernv: opal-sensor-groups: Add attributes to disable/enable sensors
> > >
> > >   .../ABI/testing/sysfs-firmware-opal-sensor-groups  |  34 ++++++
> > >   Documentation/hwmon/ibmpowernv                     |  31 ++++-
> > >   arch/powerpc/include/asm/opal-api.h                |   4 +-
> > >   arch/powerpc/include/asm/opal.h                    |   2 +
> > >   .../powerpc/platforms/powernv/opal-sensor-groups.c | 104 ++++++++++++-----
> > >   arch/powerpc/platforms/powernv/opal-wrappers.S     |   1 +
> > >   drivers/hwmon/ibmpowernv.c                         | 127 +++++++++++++++++++--
> > >   7 files changed, 265 insertions(+), 38 deletions(-)
> > >   create mode 100644 Documentation/ABI/testing/sysfs-firmware-opal-sensor-groups
> > >
> > > --
> > > 1.8.3.1
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> > > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html