Guenter Roeck <linux@roeck-us.net> wrote on 01/23/=
2017 04:15:40 PM:
> On Mon, Jan 23, 2017 at 04:01:25PM -0600, Edward =
James wrote:
> > 
> > 
> > On Sat, Jan 21, 2017 =
at 12:08 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > &=
gt; On 01/16/2017 01:13 PM, eajames.ibm@gmail.com wrote:
> > >&=
gt;
> > >> From: "Edward A. James" <eajames@us.=
ibm.com>
> > >>
> > >> Add a generic mecha=
nism to expose the sensors provided by the OCC in
> > >> sys=
fs.
> > >>
> > >> Signed-off-by: Edward A. Ja=
mes <eajames@us.ibm.com>
> > >> Signed-off-by: Andrew =
Jeffery <andrew@aj.id.au>
> > >> ---
> > >=
>  Documentation/hwmon/occ       |  62 ++++++++=
++
> > >>  drivers/hwmon/occ/Makefile    | &n=
bsp; 2 +-
> > >>  drivers/hwmon/occ/occ=5Fsysfs.c | 271=

> > >> ++++++++++++++++++++++++++++++++++++++++++
> &=
gt; >>  drivers/hwmon/occ/occ=5Fsysfs.h |  44 +++++++
&g=
t; > >>  4 files changed, 378 insertions(+), 1 deletion(-)> > >>  create mode 100644 drivers/hwmon/occ/occ=5Fsysfs.=
c
> > >>  create mode 100644 drivers/hwmon/occ/occ=5Fsy=
sfs.h
> > >>
> > >> diff --git a/Documentatio=
n/hwmon/occ b/Documentation/hwmon/occ
> > >> index 79d1642..=
d0bdf06 100644
> > >> --- a/Documentation/hwmon/occ
> =
> >> +++ b/Documentation/hwmon/occ
> > >> @@ -25,6 =
+25,68 @@ Currently, all versions of the OCC support four types
> >=
; >> of sensor data: power,
> > >>  temperature, =
frequency, and "caps," which indicate limits and
> > thr=
esholds
> > >> used
> > >>  internally o=
n the OCC.
> > >>
> > >> +sysfs Entries
&g=
t; > >> +-------------
> > >> +
> > >&g=
t; +The OCC driver uses the hwmon sysfs framework to provide data to
>=
; > >> userspace.
> > >> +
> > >> +T=
he driver exports a number of sysfs files for each type of sensor. The
&=
gt; > >> +sensor-specific files vary depending on the processor ty=
pe, though many
> > >> of the
> > >> +attribu=
tes are common for both the POWER8 and POWER9.
> > >> +
&=
gt; > >> +The hwmon interface cannot define every type of sensor t=
hat may be
> > used.
> > >> +Therefore, the frequen=
cy sensor on the OCC uses the "input" type sensor
> > &g=
t;> defined
> > >> +by the hwmon interface, rather than d=
efining a new type of custom
> > sensor.
> > >> +> > >> +Below are detailed the names and meaning of each sens=
or file for both
> > >> types of
> > >> +proc=
essors. All sensors are read-only unless otherwise specified. <x>
=
> > >> indicates
> > >> +the hwmon index. sensor=
 id indicates the unique internal OCC identifer.
> > >> Plea=
se
> > >> +see the POWER OCC specification for details on al=
l these sensor values.
> > >> +
> > >> +frequ=
ency:
> > >> +       all processors:
> =
> >> +               in<x>=
;=5Finput - frequency value
> > >> +       &n=
bsp;       in<x>=5Flabel - sensor id
> > >=
> +temperature:
> > >> +       POWER8:
=
> > >> +               temp&=
lt;x>=5Finput - temperature value
> > >> +     =
          temp<x>=5Flabel - sensor id
>=
; > >> +       POWER9 (in addition to above):
&g=
t; > >> +               temp<=
;x>=5Ftype - FRU type
> > >> +
> > >> +pow=
er:
> > >> +       POWER8:
> > >&=
gt; +               power<x>=5Finp=
ut - power value
> > >> +          =
     power<x>=5Flabel - sensor id
> > >> + &=
nbsp;             power<x>=5Faverage - =
accumulator
> > >> +           &nbs=
p;   power<x>=5Faverage=5Finterval - update tag (number of
&g=
t; > samples
> > >> in
> > >> +   &nb=
sp;                   accumula=
tor)
> > >> +       POWER9:
> > >=
> +               power<x>=5Fin=
put - power value
> > >> +          =
;     power<x>=5Flabel - sensor id
> > >> + =
              power<x>=5Faverage=
=5Fmin - accumulator[0]
> > >> +        =
       power<x>=5Faverage=5Fmax - accumulator[1] (64 b=
its total)
> > >> +            =
;   power<x>=5Faverage=5Finterval - update tag
> > >=
> +               power<x>=5Fre=
set=5Fhistory - (function=5Fid | (apss=5Fchannel <<
> > >=
> 8)
> > >> +
> > >> +caps:
> > &=
gt;> +       POWER8:
> > >> +    =
;           power<x>=5Fcap - current powerca=
p
> > >> +               =
power<x>=5Fcap=5Fmax - max powercap
> > >> +   &n=
bsp;           power<x>=5Fcap=5Fmin - min po=
wercap
> > >> +             &n=
bsp; power<x>=5Fmax - normal powercap
> > >> +   =
            power<x>=5Falarm - user pow=
ercap, r/w
> > >> +       POWER9:
> >=
; >> +               power<x>=
;=5Fcap=5Falarm - user powercap source
> > >> +
> >=
 >> +The driver also provides two sysfs entries through hwmon to bett=
er
> > >> +control the driver and monitor the master OCC. Th=
ough there may be
> > >> multiple
> > >> +OCC=
s present on the system, these two files are only present for the
> &=
gt; >> "master"
> > >> +OCC.
> > &g=
t;> +       name - read the name of the driver
> &g=
t; >> +       update=5Finterval - read or write the mi=
nimum interval for polling
> > >> the
> > >> =
+               OCC.
> > >&g=
t; +
> > >>  BMC - Host Communications
> > >=
;>  -------------------------
> > >>
> > &g=
t;> diff --git a/drivers/hwmon/occ/Makefile b/drivers/hwmon/occ/Makefile=

> > >> index 93cb52f..a6881f9 100644
> > >> =
--- a/drivers/hwmon/occ/Makefile
> > >> +++ b/drivers/hwmon/=
occ/Makefile
> > >> @@ -1 +1 @@
> > >> -obj-$=
(CONFIG=5FSENSORS=5FPPC=5FOCC) +=3D occ.o
> > >> +obj-$(CONF=
IG=5FSENSORS=5FPPC=5FOCC) +=3D occ.o occ=5Fsysfs.o
> > >> di=
ff --git a/drivers/hwmon/occ/occ=5Fsysfs.c
> > b/drivers/hwmon/occ=
/occ=5Fsysfs.c
> > >> new file mode 100644
> > >=
> index 0000000..2f20c40
> > >> --- /dev/null
> >=
; >> +++ b/drivers/hwmon/occ/occ=5Fsysfs.c
> > >> @@ -=
0,0 +1,271 @@
> > >> +/*
> > >> + * occ=5Fsys=
fs.c - OCC sysfs interface
> > >> + *
> > >> =
+ * This file contains the methods and data structures for implementing
=
> > >> the OCC
> > >> + * hwmon sysfs entries.> > >> + *
> > >> + * Copyright 2016 IBM Corp.=

> > >> + *
> > >> + * This program is free s=
oftware; you can redistribute it and/or modify
> > >> + * it=
 under the terms of the GNU General Public License as published by
> =
> >> + * the Free Software Foundation; either version 2 of the Lic=
ense, or
> > >> + * (at your option) any later version.
&=
gt; > >> + *
> > >> + * This program is distributed=
 in the hope that it will be useful,
> > >> + * but WITHOUT =
ANY WARRANTY; without even the implied warranty of
> > >> + =
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> >=
 >> + * GNU General Public License for more details.
> > >=
;> + */
> > >> +
> > >> +#include <linu=
x/delay.h>
> > >> +#include <linux/device.h>
>=
; > >> +#include <linux/err.h>
> > >> +#inclu=
de <linux/hwmon.h>
> > >> +#include <linux/hwmon-sy=
sfs.h>
> > >> +#include <linux/init.h>
> >=
 >> +#include <linux/jiffies.h>
> > >> +#include=
 <linux/kernel.h>
> > >> +#include <linux/module.h&=
gt;
> > >> +#include <linux/mutex.h>
> > >=
> +#include <linux/slab.h>
> > >> +#include "o=
cc.h"
> > >> +#include "occ=5Fsysfs.h"
>=
; > >> +
> > >> +#define RESP=5FRETURN=5FCMD=5FINVA=
L  0x13
> > >> +
> > >> +static int occ=
=5Fhwmon=5Fread(struct device *dev, enum hwmon=5Fsensor=5Ftypes
> >=
; >> type,
> > >> +          =
               u32 attr, int channel, lo=
ng *val)
> > >> +{
> > >> +     &nb=
sp; int rc =3D 0;
> > >
> > >
> > > Unn=
ecessary initialization.
> > 
> > Got it.
> > > > >
> > >
> > >> +     &n=
bsp; struct occ=5Fsysfs *driver =3D dev=5Fget=5Fdrvdata(dev);
> > =
>> +       struct occ *occ =3D driver->occ;
>=
 > >> +
> > >> +       switch (type)=
 {
> > >> +       case hwmon=5Fin:
> &g=
t; >> +               rc =3D occ=
=5Fget=5Fsensor=5Fvalue(occ, FREQ, channel, attr,
> > val);
>=
; > >> +               break;> > >> +       case hwmon=5Ftemp:
> >=
 >> +               rc =3D occ=5Fg=
et=5Fsensor=5Fvalue(occ, TEMP, channel, attr,
> > val);
> &g=
t; >> +               break;
&g=
t; > >> +       case hwmon=5Fpower:
> > &g=
t;> +               rc =3D occ=5Fget=
=5Fsensor=5Fvalue(occ, POWER, channel, attr,
> > val);
> >=
; >> +               break;
>=
; > >> +       default:
> > >> + &nb=
sp;             rc =3D -EOPNOTSUPP;
> &=
gt; >> +       }
> > >> +
> > =
>> +       return rc;
> > >> +}
>=
 > >> +
> > >> +static int occ=5Fhwmon=5Fread=5Fstr=
ing(struct device *dev,
> > >> +        =
                     &nbs=
p;  enum hwmon=5Fsensor=5Ftypes type, u32 attr,
> > >> =
+                     &nb=
sp;          int channel, char **str)
> >=
 >> +{
> > >> +       int rc;
> &=
gt; >> +       unsigned long val =3D 0;
> > &=
gt;> +
> > >> +       if (!((type =3D=3D h=
wmon=5Fin && attr =3D=3D hwmon=5Fin=5Flabel) ||
> > >&g=
t; +           (type =3D=3D hwmon=5Ftemp &&=
; attr =3D=3D hwmon=5Ftemp=5Flabel) ||
> > >> +    =
;       (type =3D=3D hwmon=5Fpower && attr =3D=3D hw=
mon=5Fpower=5Flabel)))
> > >> +         =
      return -EOPNOTSUPP;
> > >> +
> &g=
t; >> +       rc =3D occ=5Fhwmon=5Fread(dev, type, att=
r, channel, &val);
> > >> +       if (rc =
< 0)
> > >> +             &=
nbsp; return rc;
> > >> +
> > >> +   &nb=
sp;   rc =3D snprintf(*str, PAGE=5FSIZE - 1, "%ld", val);> > >
> > >
> > > val is unsigned long.> > >
> > > I am quite confused what this is for, th=
ough. The function is used for
> > > string attributes,
>=
 > > or in other words for labels. What is the benefit of having a la=
bel that
> > > returns the same
> > > data as the v=
alue attribute ? Is this maybe a misunderstanding ?
> > 
> &=
gt; So this does work, though I admit it's a bit confusing.
> > oc=
c=5Fget=5Fsensor=5Fvalue returns either the value or the occ sensor id
&=
gt; > depending on the "type" passed in. I'll rename occ=5Fget=
=5Fsensor=5Fvalue
> > and add a comment.
> > 
> >=
; >
> > >> +       if (rc > 0)
> =
> >> +               rc =3D 0;<=
br>> > >> +
> > >> +       return=
 rc;
> > >> +}
> > >> +
> > >>=
 +static int occ=5Fhwmon=5Fwrite(struct device *dev, enum hwmon=5Fsensor=5F=
types
> > >> type,
> > >> +     &nb=
sp;                    u3=
2 attr, int channel, long val)
> > >> +{
> > >&g=
t; +       int rc =3D 0;
> > >> +   &nbs=
p;   struct occ=5Fsysfs *driver =3D dev=5Fget=5Fdrvdata(dev);
> =
> >> +
> > >> +       if (type =3D=
=3D hwmon=5Fchip && attr =3D=3D hwmon=5Fchip=5Fupdate=5Finterval) {=

> > >> +               o=
cc=5Fset=5Fupdate=5Finterval(driver->occ, val);
> > >
>=
; > >
> > > As mentioned in the other patch, please drop.=
 This is not the intended
> > use
> > > case
> &=
gt; > for this attribute.
> > 
> > OK.
> > > > >
> > >
> > >> +     &n=
bsp;         return 0;
> > >> +   &=
nbsp;   } else if (type =3D=3D hwmon=5Fpower && attr =3D=3D hw=
mon=5Fpower=5Falarm) {
> > >> +         =
      rc =3D occ=5Fset=5Fuser=5Fpowercap(driver->occ, val=
);
> > >> +              =
 if (rc) {
> > >> +            =
;           if (rc =3D=3D RESP=5FRETURN=5FCMD=5FIN=
VAL) {
> > >> +             &n=
bsp;                 dev=5Ferr(dev,=

> > >> +               &=
nbsp;                    =
   "set invalid powercap value:
> > >> %ld\n"=
,
> > >> +               =
                     =
;   val);
> > >> +           &=
nbsp;                   return=
 -EINVAL;
> > >> +            =
           }
> > >> +
> > =
>> +                   &=
nbsp;   dev=5Ferr(dev, "set user powercap failed: 0x:%x
> &=
gt; \n",
> > >> rc);
> > >> +   &nb=
sp;                   return r=
c;
> > >> +              =
 }
> > >> +
> > >> +       &nb=
sp;       driver->user=5Fpowercap =3D val;
> > &=
gt;> +
> > >> +            =
   return rc;
> > >> +       }
> &=
gt; >> +
> > >> +       return -EOPNOTS=
UPP;
> > >> +}
> > >> +
> > >>=
 +static umode=5Ft occ=5Fis=5Fvisible(const void *data, enum hwmon=5Fsensor=
=5Ftypes
> > >> type,
> > >> +     =
                     =
;   u32 attr, int channel)
> > >> +{
> > >&=
gt; +       const struct occ=5Fsysfs *driver =3D data;
&g=
t; > >> +
> > >> +       switch (typ=
e) {
> > >> +       case hwmon=5Fchip:
>=
; > >> +               if (attr=
 =3D=3D hwmon=5Fchip=5Fupdate=5Finterval)
> > >> +   &n=
bsp;                   return =
S=5FIRUGO | S=5FIWUSR;
> > >> +         =
      break;
> > >> +       ca=
se hwmon=5Fin:
> > >> +           &=
nbsp;   if (BIT(attr) & driver->sensor=5Fhwmon=5Fconfigs[0])> > >> +               &nbs=
p;       return S=5FIRUGO;
> > >> +   &n=
bsp;           break;
> > >> + &nbs=
p;     case hwmon=5Ftemp:
> > >> +     &=
nbsp;         if (BIT(attr) & driver->sensor=5Fh=
wmon=5Fconfigs[1])
> > >> +         &nbs=
p;             return S=5FIRUGO;
> >=
 >> +               break;
>=
 > >> +       case hwmon=5Fpower:
> > >=
> +               /* user power limit=
 */
> > >> +              =
; if (attr =3D=3D hwmon=5Fpower=5Falarm)
> > >> +   &nb=
sp;                   return S=
=5FIRUGO | S=5FIWUSR;
> > >> +         &=
nbsp;     else if ((BIT(attr) & driver->sensor=5Fhwmon=5Fc=
onfigs[2])
> > ||
> > >> +       &nb=
sp;                (BIT(attr) &=
 driver->sensor=5Fhwmon=5Fconfigs[3]))
> > >> +   &n=
bsp;                   return =
S=5FIRUGO;
> > >> +            =
;   break;
> > >> +       default:
&g=
t; > >> +               return =
0;
> > >
> > >
> > > Might as well use =
break; here for consistency.
> > >
> > >
> &g=
t; >> +       }
> > >> +
> > &=
gt;> +       return 0;
> > >> +}
> &=
gt; >> +
> > >> +static const struct hwmon=5Fops occ=
=5Fhwmon=5Fops =3D {
> > >> +       .is=5Fvis=
ible =3D occ=5Fis=5Fvisible,
> > >> +       .=
read =3D occ=5Fhwmon=5Fread,
> > >> +       .=
read=5Fstring =3D occ=5Fhwmon=5Fread=5Fstring,
> > >> + &nbs=
p;     .write =3D occ=5Fhwmon=5Fwrite,
> > >> +};<=
br>> > >> +
> > >> +static const u32 occ=5Fchip=
=5Fconfig[] =3D {
> > >> +       HWMON=5FC=5F=
UPDATE=5FINTERVAL,
> > >> +       0
> &=
gt; >> +};
> > >> +
> > >> +static cons=
t struct hwmon=5Fchannel=5Finfo occ=5Fchip =3D {
> > >> + &n=
bsp;     .type =3D hwmon=5Fchip,
> > >> +   &=
nbsp;   .config =3D occ=5Fchip=5Fconfig
> > >> +};
&=
gt; > >> +
> > >> +static const enum hwmon=5Fsensor=
=5Ftypes
> > >> occ=5Fsensor=5Ftypes[MAX=5FOCC=5FSENSOR=5FTY=
PE] =3D {
> > >> +       hwmon=5Fin,
> =
> >> +       hwmon=5Ftemp,
> > >> + =
      hwmon=5Fpower,
> > >> +     &=
nbsp; hwmon=5Fpower
> > >> +};
> > >> +
&g=
t; > >> +struct occ=5Fsysfs *occ=5Fsysfs=5Fstart(struct device *de=
v, struct occ *occ,
> > >> +         &nb=
sp;                     &=
nbsp; const u32 *sensor=5Fhwmon=5Fconfigs,
> > >> +   &=
nbsp;                    =
         const char *name)
> > >> +{
=
> > >> +       bool master=5Focc =3D false;
&=
gt; > >> +       int rc, i, j, sensor=5Fnum, index =
=3D 0, id;
> > >> +       char *brk;
> =
> >> +       struct occ=5Fblocks *resp =3D NULL;> > >> +       u32 *sensor=5Fconfig;
> &g=
t; >> +       struct occ=5Fsysfs *hwmon =3D devm=5Fkza=
lloc(dev, sizeof(struct
> > >> occ=5Fsysfs),
> > &g=
t;> +                   &nb=
sp;                     &=
nbsp;    GFP=5FKERNEL);
> > >> +     &nb=
sp; if (!hwmon)
> > >> +           =
    return ERR=5FPTR(-ENOMEM);
> > >> +
> &g=
t; >> +       /* need space for null-termination and o=
cc chip */
> > >> +       hwmon->occ=5Fsen=
sors =3D
> > >> +             =
  devm=5Fkzalloc(dev, sizeof(struct hwmon=5Fchannel=5Finfo *) *
>=
; > >> +                 &=
nbsp;          (MAX=5FOCC=5FSENSOR=5FTYPE + 2), GF=
P=5FKERNEL);
> > >> +       if (!hwmon->oc=
c=5Fsensors)
> > >> +           &nb=
sp;   return ERR=5FPTR(-ENOMEM);
> > >> +
> > =
>> +       hwmon->occ =3D occ;
> > >>=
; +       hwmon->sensor=5Fhwmon=5Fconfigs =3D (u32 *)sens=
or=5Fhwmon=5Fconfigs;
> > >
> > >
> > >=
 Either this is a const or it isn't. If it isn't, it should not be passed> > in
> > > as const.
> > > If it is, it c=
an be declared const in struct occ=5Fsysfs.
> > 
> > It i=
s const, i'll fix.
> > 
> > >
> > >
>=
; > >> +       hwmon->occ=5Finfo.ops =3D &oc=
c=5Fhwmon=5Fops;
> > >> +       hwmon->occ=
=5Finfo.info =3D
> > >> +          =
     (const struct hwmon=5Fchannel=5Finfo **)hwmon->occ=5Fsens=
ors;
> > >
> > >
> > > Why is this type=
cast needed ?
> > 
> > struct hwmon=5Fchip=5Finfo {
&g=
t; >         const struct hwmon=5Fops *ops;
> =
>         const struct hwmon=5Fchannel=5Finfo **info=
;
> > };
> > 
> > My compiler throws an error wi=
thout a cast here.
> > 
> > occ=5Fsysfs.c:195:23: error: =
assignment from incompatible pointer type
> > [-Werror=3Dincompati=
ble-pointer-types]
> > |   hwmon->occ=5Finfo.info =3D hwmo=
n->occ=5Fsensors;
> > 
> > occ=5Fsensors is not const,=
 as it's dynamically zero-allocated and then
> > if we poll and ge=
t a sensor, we allocate a new hwmon=5Fchannel=5Finfo. Not
> > sure=
 what the best solution is here except to cast.
> > 
> 
&=
gt; Your statement makes me quite concerned. "if we poll and get a sen=
sor, we
> allocate a new hwmon=5Fchannel=5Finfo". Are you doing =
that before or after
> registering with the hwmon core ? Hopefully be=
fore.
All before registration. There is no changing of hwmo=
n attributes after we
probe the driver. But we need the first p=
oll response to allocate the
hwmon=5Fchannel=5Finfo structures =
and others to match the sensors found in the
first poll respons=
e.

> 
> > >
> > >
> > &=
gt;> +
> > >> +       occ=5Fget=5Fresponse=
=5Fblocks(occ, &resp);
> > >> +
> > >> + =
      for (i =3D 0; i < MAX=5FOCC=5FSENSOR=5FTYPE; ++i)> > >> +               res=
p->sensor=5Fblock=5Fid[i] =3D -1;
> > >> +
> > &=
gt;> +       /* read sensor data from occ */
> >=
 >> +       rc =3D occ=5Fupdate=5Fdevice(occ);
>=
 > >> +       if (rc) {
> > >> + &nb=
sp;             dev=5Ferr(dev, "cannot g=
et occ sensor data: %d\n", rc);
> > >> +     =
          return ERR=5FPTR(rc);
> > >&=
gt; +       }
> > >> +       i=
f (!resp->blocks)
> > >> +         &n=
bsp;     return ERR=5FPTR(-ENOMEM);
> > >> +
&g=
t; > >> +       master=5Focc =3D resp->sensor=5F=
block=5Fid[CAPS] >=3D 0;
> > >> +
> > >> +=
       for (i =3D 0; i < MAX=5FOCC=5FSENSOR=5FTYPE; i++) =
{
> > >> +               =
id =3D resp->sensor=5Fblock=5Fid[i];
> > >> +   &nbs=
p;           if (id < 0)
> > >> =
+                     &nb=
sp; continue;
> > >> +
> > >> +    =
           sensor=5Fnum =3D resp->blocks[id].he=
ader.sensor=5Fnum;
> > >> +         &nbs=
p;     /* need null-termination */
> > >> +  =
             sensor=5Fconfig =3D devm=5Fkzall=
oc(dev,
> > >> +             &=
nbsp;                    =
          sizeof(u32) * (sensor=5Fnum +
> &g=
t; >> 1),
> > >> +           =
                     =
;            GFP=5FKERNEL);
> > >=
> +               if (!sensor=5Fconfi=
g)
> > >> +              =
         return ERR=5FPTR(-ENOMEM);
> > >&g=
t; +
> > >> +             &nbs=
p; for (j =3D 0; j < sensor=5Fnum; j++)
> > >> +   &=
nbsp;                   sensor=
=5Fconfig[j] =3D sensor=5Fhwmon=5Fconfigs[i];
> > >> +
&g=
t; > >> +               hwmon-&=
gt;occ=5Fsensors[index] =3D
> > >> +       &n=
bsp;               devm=5Fkzalloc(dev, s=
izeof(struct
> > >> hwmon=5Fchannel=5Finfo),
> > &g=
t;> +                   &nb=
sp;                GFP=5FKERNEL);> > >> +               if =
(!hwmon->occ=5Fsensors[index])
> > >> +     &nb=
sp;                 return ERR=5FPT=
R(-ENOMEM);
> > >> +
> > >> +     &=
nbsp;         hwmon->occ=5Fsensors[index]->type =
=3D occ=5Fsensor=5Ftypes[i];
> > >> +       &=
nbsp;       hwmon->occ=5Fsensors[index]->config =3D se=
nsor=5Fconfig;
> > >> +           &=
nbsp;   index++;
> > >> +       }
>=
; > >
> > >
> > > I had the impression from p=
atch 1 that the number of sensors can change
> > at
> > &=
gt; runtime.
> > > If so, this doesn't really work well; the sy=
sfs attributes won't be
> > updated
> > > in this
&=
gt; > > case.
> > >
> > > Can it really happe=
n that the sensor information can change ?
> > 
> > Well,=
 the number of sensors won't change while we are running. But we
> &g=
t; don't know how many sensors we have when we start up. So this first
&=
gt; > poll will tell us how many we need and allow the driver to initial=
ize
> > the correct number of hwmon attributes. But yes, it should=
n't change while
> > we
> > are running.
> > > I'll have to look into the other patch again. I seem to recall that i=
t does
> handle changing number of sensors, but I may be wrong.
Yes, the core driver does handle varying sensors. But this was d=
esigned mainly
just to handle the first poll response. The core=
 doesn't know which poll
response is the first, so it made sens=
e to just handle any number of sensors
each time we poll.
Also, the OCC specification doesn't indicate that the poll
response *must* have the same number of sensors each time it's polle=
d... so
perhaps it may change in some conditions. But the hwmon=
 attributes won't change.
That's OK for us.

>=
; 
> > >
> > >> +
> > >> +  =
     /* only need one of these for any number of occs */
> =
> >> +       if (master=5Focc)
> > >>=
; +               hwmon->occ=5Fsensor=
s[index] =3D
> > >> +           &nb=
sp;           (struct hwmon=5Fchannel=5Finfo *)&am=
p;occ=5Fchip;
> > >
> > >
> > > Why thi=
s typecast ?
> > 
> > occ=5Fchip is const but my occ=5Fse=
nsors field isn't.
> > 
> Hmm .. I'll have to have a closer =
look. Maybe I need to make the core fields
> non-const. Having to use=
 typecasts isn't really desirable and defeats the
> purpose.
OK, agreed, thanks.
Eddie

> 
=
> Guenter
> 
> > >
> > >
> > >=
> +
> > >> +       /* search for bad chars=
 */
> > >> +       strncpy(hwmon->hwmon=5F=
name, name, OCC=5FHWMON=5FNAME=5FLENGTH);
> > >> +   &n=
bsp;   brk =3D strpbrk(hwmon->hwmon=5Fname, "-* \t\n");> > >> +       while (brk) {
> > >=
> +               *brk =3D '=5F';
=
> > >> +               brk =
=3D strpbrk(brk,  "-* \t\n");
> > >> +  =
     }
> > >> +
> > >> +   &nb=
sp;   hwmon->dev =3D devm=5Fhwmon=5Fdevice=5Fregister=5Fwith=5Finfo=
(dev,
> > >> +
> > >> hwmon->hwmon=5Fname,=

> > >> +               &=
nbsp;                    =
                     hwmo=
n,
> > >> +
> > >> &hwmon->occ=5Finfo,=

> > >> +               &=
nbsp;                    =
                     NULL=
);
> > >> +       if (IS=5FERR(hwmon->dev)=
) {
> > >> +              =
; dev=5Ferr(dev, "cannot register hwmon device %s: %ld\n",
>=
; > >> +                 &=
nbsp;     hwmon->hwmon=5Fname, PTR=5FERR(hwmon->dev));
&=
gt; > >> +               return=
 ERR=5FCAST(hwmon->dev);
> > >> +       }<=
br>> > >> +
> > >> +       return=
 hwmon;
> > >> +}
> > >> +EXPORT=5FSYMBOL(occ=
=5Fsysfs=5Fstart);
> > >> +
> > >> +MODULE=5F=
AUTHOR("Eddie James <eajames@us.ibm.com>");
> > &g=
t;> +MODULE=5FDESCRIPTION("OCC sysfs driver");
> > &g=
t;> +MODULE=5FLICENSE("GPL");
> > >> diff --git=
 a/drivers/hwmon/occ/occ=5Fsysfs.h
> > b/drivers/hwmon/occ/occ=5Fs=
ysfs.h
> > >> new file mode 100644
> > >> ind=
ex 0000000..7de92e7
> > >> --- /dev/null
> > >&g=
t; +++ b/drivers/hwmon/occ/occ=5Fsysfs.h
> > >> @@ -0,0 +1,4=
4 @@
> > >> +/*
> > >> + * occ=5Fsysfs.h - OC=
C sysfs interface
> > >> + *
> > >> + * This =
file contains the data structures and function prototypes for
> > =
the
> > >> OCC
> > >> + * hwmon sysfs entries=
.
> > >> + *
> > >> + * Copyright 2016 IBM Co=
rp.
> > >> + *
> > >> + * This program is fre=
e software; you can redistribute it and/or modify
> > >> + *=
 it under the terms of the GNU General Public License as published by
&g=
t; > >> + * the Free Software Foundation; either version 2 of the =
License, or
> > >> + * (at your option) any later version.> > >> + *
> > >> + * This program is distribu=
ted in the hope that it will be useful,
> > >> + * but WITHO=
UT ANY WARRANTY; without even the implied warranty of
> > >>=
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> &=
gt; >> + * GNU General Public License for more details.
> > =
>> + */
> > >> +
> > >> +#ifndef =5F=5F=
OCC=5FSYSFS=5FH=5F=5F
> > >> +#define =5F=5FOCC=5FSYSFS=5FH=
=5F=5F
> > >> +
> > >> +#include <linux/hw=
mon.h>
> > >> +
> > >> +struct occ;
>=
; > >> +struct device;
> > >> +
> > >&g=
t; +#define OCC=5FHWMON=5FNAME=5FLENGTH  32
> > >> +> > >> +struct occ=5Fsysfs {
> > >> +   &n=
bsp;   struct device *dev;
> > >> +      =
; struct occ *occ;
> > >> +
> > >> +   &=
nbsp;   char hwmon=5Fname[OCC=5FHWMON=5FNAME=5FLENGTH + 1];
> &g=
t; >> +       u32 *sensor=5Fhwmon=5Fconfigs;
> &=
gt; >> +       struct hwmon=5Fchannel=5Finfo **occ=5Fs=
ensors;
> > >> +       struct hwmon=5Fchip=5F=
info occ=5Finfo;
> > >> +       u16 user=5Fpo=
wercap;
> > >> +};
> > >
> > >
&g=
t; > > Is this information used outside the sysfs code ? If not, it s=
hould be
> > > defined
> > > in the source file and=
 not be available outside of it.
> > 
> > OK. It is used =
in p8=5Focc=5Fi2c.c but none of the members are accessed so
> > we=
 shouldn't need it defined there, just declared, you're right.
> >=
 
> > 
> > >
> > >
> > >> +=

> > >> +struct occ=5Fsysfs *occ=5Fsysfs=5Fstart(struct devi=
ce *dev, struct occ *occ,
> > >> +       &nbs=
p;                     &n=
bsp;   const u32 *sensor=5Fhwmon=5Fconfigs,
> > >> + &n=
bsp;                     =
          const char *name);
> > >>=
 +#endif /* =5F=5FOCC=5FSYSFS=5FH=5F=5F */
> > >>
> &g=
t; >
>