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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 19 Nov 2013 19:43:43 +0000
From:	Laszlo Papp <lpapp@....org>
To:	Guenter Roeck <linux@...ck-us.net>
Cc:	Marcus Folkesson <marcus.folkesson@...il.com>, hjk@...sjkoch.de,
	linux-kernel@...r.kernel.org, lm-sensors@...sensors.org
Subject: Re: [lm-sensors] [PATCH] hwmon: (max6650) Add support for gpiodef

On Tue, Nov 19, 2013 at 6:00 PM, Guenter Roeck <linux@...ck-us.net> 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@...il.com> 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/