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

Eddie James eajames.ibm at gmail.com
Thu Dec 1 03:17:22 AEDT 2016


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


More information about the openbmc mailing list