On Tue, Nov 19, 2013 at 6:00 PM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote: > On Tue, Nov 19, 2013 at 08:54:38AM -0800, Guenter Roeck wrote: >> On Tue, Nov 19, 2013 at 04:42:49PM +0000, Laszlo Papp wrote: >> > On Fri, Nov 15, 2013 at 7:52 PM, Marcus Folkesson >> > <marcus.folkesson@xxxxxxxxx> wrote: >> > > >> > >> This is just one use case of those, you could also use it for >> > >> non-generic gpio functionality, like alarm, "full-on", internal clock, >> > >> external clock, etc. I believe it is always a bit tricky with MFD. I >> > >> personally prefer to put it into the chip driver because this is not >> > >> clearly a generic gpio interface here, and I need to drive it >> > >> dynamically. >> > > >> > > I agree. >> > > >> > > I think the solution with expose the "GPIOs" in sysfs is the right way to >> > > go. >> > > The chip-function is of a dynamic nature and should therefor not be set in >> > > platform data / devicetree. >> > > >> > > As mentioned before, GPIOs should use the gpio subsystem whenever possible, >> > > but the the gpio-functionality is just a subset of >> > > functions these pins may be set to. >> > > >> > > Also, the I think the *real* reason why the entries is called "gpio" is that >> > > it is so the registers are are mentioned in the datasheet. >> > > Everyone that is working with the device will know what it is all about. >> > > I see it more as an register expose than a gpio interface... >> > > >> > > I agree that the entries does not really fit here. But they does not fit >> > > better elsewhere either. >> > > And I don't think they fit worse than the alarm-entries that is already in >> > > mainline. >> > > >> > > Anyway, I think the documentation file should mention what function each >> > > valid value represent. >> > >> > Yes, makes sense to make the documentation more comprehensive. Thanks. >> > >> > Any other issues from anyone before submitting a polished version? >> > >> You'll have to get feedback from Jean. I won't accept the patch. >> > To add to this: The datasheet clearly states that the pins are GPIO pins, > three of which can be configured as ALERT output, ALL_ON input, or clock > input/output. Which is inline with what we wrote before. Although, you probably meant FULL_ON rather than ALL_ON. There is no "ALL_ON" in this context. > GPIO pins should be made available to the kernel through > the GPIO subsystem; any clock configuration should be configured through the > clock subsystem if needed. I believe we will agree to disagree there. I find it more convenient (along with Markus, etc) to have clearly chip specific feature available for the chip in one place - especially when that follows the consistency - rather than distributed in several sub-spaces. Not that this is only a minor feature. Splitting them into even tinier is strange in my opinion. > The pin configuration as ALERT output/ALL_ON > input/clock is clearly board specific and should thus be provided as > platform data and/or devicetree data if needed. That is still static, not dynamic, hence inappropriate for the use case at hand. > The existing GPIO alarm attributes should be removed. The pin values should be > reported as GPIO pin values instead. Feel free to provide a change for review if you wish. I would like to note that I am not planning to rewrite an already existing and tested feature as of now. Not sure if I could find the motivation and time for doing that any soon. To me, it only looks personal taste nitpicking so far, and the feature would be more important to me. There are pro/cons for both sides, but if this feature cannot get it in with this design, there might be no feature like this for the posterity. So unless there are good arguments with modulo critical answers why it is unacceptably wrong as is for now, let us drop this change in upstream, then. _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
