Re: [lm-sensors] [PATCH] hwmon: (max6650) Add support for gpiodef

[Laszlo Papp]

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