Re: hwmon driver with misc interface

[Eddie James <eajames@linux.vnet.ibm.com>]

On 07/08/2018 08:26 PM, Guenter Roeck wrote:
> On 07/08/2018 06:05 PM, Benjamin Herrenschmidt wrote:
>> On Sun, 2018-07-08 at 16:30 -0700, Guenter Roeck wrote:
>>>
>>> Trying to be be reasonable....
>>>
>>> Let's make some ground rules.
>>>
>>> - Do not attach foreign attributes (not related to hardware 
>>> monitoring) to
>>>     the hwmon device. Attach foreign attributes to its parent, eg 
>>> the platform
>>>     or i2c driver, or to a separate (misc ?) device if that is not 
>>> feasible
>>>     for some reason.
>>> - Avoid foreign subsystem drivers. If the chip has an input device, 
>>> a watchdog,
>>>     and a hardware monitor, there should be three drivers.
>>>     This is to some degree flexible; for example, PMBus drivers may 
>>> register
>>>     as power regulators, and some chips also have gpio support. But 
>>> what,
>>>     for example, the applesmc driver does is really not acceptable.
>>
>> This rule can be a bit nasty if the various "parts" of the chip need
>> tight interlock, share an interrupt etc... the solution to that is to
>> have most of the common code in a "parent" driver that creates child
>> devices with separate drivers that directly link onto the parent and
>> use exported functions, but it can easily bloat the driver
>> significantly for little benefit.
>>
>
> But that is what mfd drivers are for, or am I missing something ?
> After all, it has "multi-function device" right in its name.
>
> Sure, there is somebloat, but on the plus side it ensures that
> all bits and pieces are reviewed by the respective maintainers,
> and the cross-functional API is _forced_ to be clean.
>
>> That said, this is maybe not *too much* of an issue in the OCC case,
>> see below.
>>
>>> - Private hwmon attributes are acceptable as long as they are clearly
>>>     documented and explained as necessary. This is not a free ride; 
>>> you should
>>>     have good reasons for private attributes and be able to explain 
>>> that and why
>>>     you need them. In this context, "because the hardware provides 
>>> the information"
>>>     is not a valid reason. The use case is important, not the fact that
>>>     the hardware provides some random information.
>>>
>>> Can you work with that ?
>>
>> Anything is always possible :-) The main question for me here is
>> whether to keep what we do today:
>>
>>     * sbefifo (the transport driver)
>>       |
>>       * fsi-occ platform driver
>>              ("passes occ hwmon commands to sbefifo and adds /dev/occ")
>>               |
>>               * occ-hwmon
>>
>> Or can I collapse fsi-occ and occ-hwmon into one.
>>
>> Now /dev/occ is just a "raw" interface to send commands to the OCC, via
>> the same path occ-hwmon does. There's locking needed there between the
>> two so it currently happens in fsi-occ.
>>
>>> From what you are saying, you prefer that we keep it separate, which is
>> our current design. I find it a bit messy but it's not a huge deal
>> frankly, so let's do so.
>>
> Other drivers solve that with an API from parent to child, either with
> a direct function call or with a callback function provided to the child.
> Another option would be to handle it through regmap; That nowadays
> supports custom accesses implemented in the parent driver (see 
> regmap_read
> and regmap_write in struct regmap_config). The child driver gets the
> regmap pointer and uses the regmap API. I don't see that as messy.
>
>> The pre-requisite sbefifo driver is now in Greg's tree (and I'll have
>> some fixes for it in -next this week), so you should be able to at
>> least test build when Eddie resubmits.
>>
> Ok.
>
>> As for the various sysfs files for monitoring the base functionality of
>> the occ, Eddie, you can always move them to fsi-occ.
>>
>
> Yes, I think that would be more appropriate.

This still won't work, since then we wouldn't have those attributes 
available in the P8 version of the driver (which has no fsi-occ driver). 
In addition, how would the poll response data get from the hwmon driver 
to the fsi-occ driver? Yet another interface? Seems awkward.

How about debugfs? We don't really mind where the attributes are, just 
that the data is exposed somewhere...

Thanks,
Eddie

>
> Thanks,
> Guenter
>
>> Cheers,
>> Ben.
>>
>