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.
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
