One week passed since the initial submit. Any feedback from the maintainer who accepts patches for this? On Tue, Nov 19, 2013 at 7:43 PM, Laszlo Papp <lpapp@xxxxxxx> wrote: > 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
