[RFC PATCH linux 6/9] hwmon: occ: Add hwmon implementation for the P8 OCC

[Andrew Jeffery]

On Tue, 2016-11-29 at 17:00 -0600, Eddie James wrote:
> > +
> > +static int occ_p8_probe(struct i2c_client *client,
> > +                        const struct i2c_device_id *id)
> > +{
> > +       struct occ *occ;
> > +       struct occ_sysfs *hwmon;
> > +
> > +       occ = occ_p8_start(&client->dev, client, p8_bus_ops);
> > +       if (IS_ERR(occ))
> > +               return PTR_ERR(occ);
> > +
> > +       hwmon = occ_sysfs_start(&client->dev, occ,
> &p8_sysfs_config);
> 
> I see why you defined those here. But I guess I don't see why
> occ_sysfs_config has to be separate from occ_config structure. It
> could be populated in occ_p8_start and then passed to occ_sysfs_start
> here.

I admit getting to this point was a little tricky, and I'm not
completely satisfied by what I've come up with. Here's my reasoning:

I felt that only half of your struct occ_protocol dealt with SCOM
protocol problems, specifically the command_addr and response_addr
members. num_caps_fields and caps_names were only used in the sysfs
code where they are relevant for interpretation of the payload, but are
not in transacting SCOMs. Therefore I removed them from struct
occ_protocol.

I also renamed struct occ_protocol to occ_config. I could live with
either name but my feeling is it's more obvious that the consumer of
the API needs to populate a struct with 'config' in its name.

Given I'd removed num_caps_fields and caps_names I had to put the
information somewhere. One place might be in struct occ as I've
interpreted your suggestion above. However, I was trying to keep struct
occ opaque so in occ.c could fully encapsulate its behaviour. As struct
occ is opaque, occ_sysfs.c has no visibility into its members and thus
can't access anything we stash in it, e.g. num_caps_fields and
caps_names.

That leaves us with needing some other solution, one of which I've
implemented above. It achieves the goals I was outlining above and
keeps the sysfs code free of any implementation specifics, at the cost
of describing the information in a slightly weird spot.

An alternative might be to define struct occ_sysfs_config in occ_p8.c
and add an extern to occ_p8.h. That bakes sysfs knowledge into the p8
header which to me feels more unfortunate than my current approach. Or,
define say p8_caps_names and p8_num_caps in occ_p8.c and extern those
in occ_p8.h and use them to define a struct occ_sysfs_config here. What
do you think?

Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20161130/6e9d4298/attachment.sig>