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

[Eddie James]

On Tue, Nov 29, 2016 at 7:00 PM, Andrew Jeffery <andrew at aj.id.au> wrote:
> 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.

Makes sense.

>
> 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?

I think your solution is fine, I was just a bit confused as to why you
decided to do that. Let's stick with your solution, it's better than
putting it in the headers.

Thanks,
Eddie