[ Adding Martyn who implemented the cp2105 gpio support. ]
[ Karoly, please avoid top-posting and trim unneeded context instead of
copying context to the top.
Also set your mailer to wrap lines at 72 columns or so that I don't
have to reflow your mail manually when responding. ]
On Wed, Jun 20, 2018 at 09:45:05AM +0000, Karoly Pados wrote:
> OK, though if you allow me a few comments and explanations:
>
> > You're gonna need to unify a lot of this with the current
> > implementation for cp2105 however.
> >
> This also relates to your later points about unifying the cp2105 an
> cp2102n: The implementation for the cp2102n ended up pretty different.
> Both in initializaiton as well as normal operation, partially due to
> the better config/input/alt.function support for the cp2102n. At this
> point I though it is much more readable if I just create separate code
> hierarchies than having to pollute everything with ifs and
> switch-cases.
>
> And maybe most importantly, I cannot test on the cp2105, so I cannot
> check if I've broken anything by mistake, and since it is the odd bird
> anyway among most devices, I though it is pretty logical not to touch
> its code but instead create a new skeleton for others.
>
> Of course you're the boss and whatever you say I will do, but I wanted
> to explain my reasons (or at least let you know my arguments).
If it really was all that different it may make sense, but judging from
vendor driver there are a lot of similarities (e.g. 2 byte mask + value
for set and 1 byte value for get, open-source or push-pull, alternate
function, ...).
As I already mentioned, parsing the configuration will need to be
different, but it seems like a lot of the implementation can be shared
(without much clutter).
> > I was going to comment on the odd coding style above, when I noticed
> > that you've copied this implementation from a silabs forum post. Not
> > good at all (as there was no indication of any license).
> >
> Not quite. It is not from a forum post, but from a SiLabs Knowledge
> Base article
> (https://www.silabs.com/community/interface/knowledge-base.entry.html/2017/06/12/fletcher_checksumfo-TeDF)
Yeah, that's the one I was referring to.
> That article states explicitly that the code was taken from Wikipedia,
> so it is CC-SA, which is to the best of my knowledge 1) compatible
> with GPL, and 2) does not require attribution if the original material
> is missing it, and it does. So AFAICT we are clear on the licensing
> front.
First of all I can't seem to find that code snippet on the wiki page it
does refer to, so I'm still not convinced.
Second, this should have been high-lighted in your submission somehow.
> Do you still want me to remove the checksum check?
Yes.
> > So for cp2105 we decided against implementing an input mode. We at least
> > need to be consistent here, so I suggest starting with dropping those
> >
> Again, you're the boss, but I do find this odd. These are different
> devices and thankfully now there'd be better support for the cp2102n,
> and you're asking me to remove input support for consistency reasons.
> To remove a feature for one device just because it wasn't correctly
> implemented in another and wanting to stay consistent with the missing
> support is very odd for me tbh.
> I know I'm in no position, but please let me ask you once to please
> reconsider this.
We're not removing anything from the kernel since nothing has yet been
merged. I'm asking you to separate it into a follow up patch, which if
we find it useful and correct would allow the same functionality for
cp2105 as well.
The gpio pins on cp2102n and cp2105 share the same electrical
characteristics and there's no good reason why we should treat them
differently.
> The way it was done for the cp2105 where we make everything and
> everyone on the system think that a pin is "out" but in reality is an
> input is quirky to say the least, while at the same time it also does
> not make sure that an input pin (which is of course not marked as
> such) is really an input. Why amputate the cp2102n like this if it
> has been already implemented correctly? If we wanted to stay
> consistent with half-developed and incomplete drivers then there'd be
> no improvements to Linux.
We do not want to have two competing and incompatible implementations of
essentially the same thing.
> > Please be more specific here; what do you mean by not corresponding to
> > reality.
> >
> It means that I've found GPIO4-6 occupying bits which were documented
> doing completely other things, and those other things occupying
> reserved bits even though they were documented occupying the places
> where GPIOs 4-6 were.
Ok, but try to explain your findings regarding this in the commit
message and/or a comment instead of the current formulation. You can be
more specific in the comment, without getting into every detail too
("not corresponding to reality" is too vague).
But to clarify, this has to do with the configuration of the pins,
right? You'd still use the same mechanisms to set and retrieve the latch
register?
> > No need to check for this in every callback; move to
> > cp210x_gpio_request().
> >
> Wouldn't this prevent the disabled GPIOs from being created in the
> first place? I very much want to create them in every case, because
> otherwise it becomes a PITA for both the driver and consumers:
>
> - For the driver a PITA, because the disabled GPIOs can be any and
> non-continuous, so the module would need to dynamically and
> arbitrarily remap logical GPIOs to hardware GPIO numbers.
>
> - For consumers a PITA, because it would mean GPIOs can change names
> depending on the current configuration of the device. On one device
> GPIO.3 would be called GPIO.2, on another GPIO.1 etc. My intention
> here is that applications should be able to use the real ID of the
> GPIO and expect that GPIO.3 (base+offset 3) is actually GPIO.3 and
> not something else.
No, that shouldn't be problem (unless something has changed in gpiolib
recently). All pins would be registered, but we simply deny requests for
unavailable pins.
Thanks,
Johan
On Wed, 2018-06-20 at 12:52 +0200, Johan Hovold wrote:
> > > [ Adding Martyn who implemented the cp2105 gpio support. ]
> > > So for cp2105 we decided against implementing an input mode. We
> > > at least
> > > need to be consistent here, so I suggest starting with dropping
> > > those
> > >
> >
> > Again, you're the boss, but I do find this odd. These are different
> > devices and thankfully now there'd be better support for the
> > cp2102n,
> > and you're asking me to remove input support for consistency
> > reasons.
> > To remove a feature for one device just because it wasn't correctly
> > implemented in another and wanting to stay consistent with the
> > missing
> > support is very odd for me tbh.
> > I know I'm in no position, but please let me ask you once to please
> > reconsider this.
>
> We're not removing anything from the kernel since nothing has yet
> been
> merged. I'm asking you to separate it into a follow up patch, which
> if
> we find it useful and correct would allow the same functionality for
> cp2105 as well.
>
> The gpio pins on cp2102n and cp2105 share the same electrical
> characteristics and there's no good reason why we should treat them
> differently.
>
> > The way it was done for the cp2105 where we make everything and
> > everyone on the system think that a pin is "out" but in reality is
> > an
> > input is quirky to say the least, while at the same time it also
> > does
> > not make sure that an input pin (which is of course not marked as
> > such) is really an input. Why amputate the cp2102n like this if it
> > has been already implemented correctly? If we wanted to stay
> > consistent with half-developed and incomplete drivers then there'd
> > be
> > no improvements to Linux.
>
The rationale for the pins being permanently configured as output pins
is that these pins (at least on the cp2105) do not appear to provide a
true input mode. They offer a "push-pull" mode (where the voltage is
pulled directly to ground or supply rail) and an "open-drain" mode
(where the pin is weakly pulled high by an internal resistor and can be
pulled to ground). Unless I missed something, there is no tristate/high
impedance mode typically associated with a pin being used as input.
Sure, you can use the open-drain mode as input as long as you
understand that the permanent pull up in the cp2015 might have an
impact on what you are reading. For example, if you have a signal that
is externally weakly pulled down, it's going to depend on the relative
resistances of the internal and external resistors as to what voltage
the line rests at and therefore what state the line is considered to be
in. This could stop things working if you naively think the cp2105 is
acting as a typical high-impedance input.
So, with that in mind, we came to the decision that as you *can* read
the state of the output when in open-drain mode, doing it that way may
avoid such potential issues. I didn't do an exhaustive search of other
GPIO devices at the time, so if it can be shown that the majority of
other GPIO drivers with such limitations can be placed into an input
state, I'd be happy for it to be changed so as to remain consistent.
Martyn
Johan Hovold <[email protected]> writes:
>> Not quite. It is not from a forum post, but from a SiLabs Knowledge
>> Base article
>> (https://www.silabs.com/community/interface/knowledge-base.entry.html/2017/06/12/fletcher_checksumfo-TeDF)
>
> Yeah, that's the one I was referring to.
>
>> That article states explicitly that the code was taken from Wikipedia,
>> so it is CC-SA, which is to the best of my knowledge 1) compatible
>> with GPL, and 2) does not require attribution if the original material
>> is missing it, and it does. So AFAICT we are clear on the licensing
>> front.
>
> First of all I can't seem to find that code snippet on the wiki page it
> does refer to, so I'm still not convinced.
It was there in older versions of the article. See for example:
https://en.wikipedia.org/w/index.php?title=Fletcher%27s_checksum&oldid=730327006
> Second, this should have been high-lighted in your submission somehow.
Definitely. All code has an original author who deserves credit. And if
you cannot find the original author, then there is always a risk than
someone along the line stole the code... Maybe long before it ended up
in Wikipedia. But that doesn't matter.
Doesn't seem worth the risk for a simple checksum algorithm which
probably has lots of GPL implementations.
Bjørn
On Mon, Jul 02, 2018 at 08:04:32PM +0200, Bj?rn Mork wrote:
> Johan Hovold <[email protected]> writes:
>
> >> Not quite. It is not from a forum post, but from a SiLabs Knowledge
> >> Base article
> >> (https://www.silabs.com/community/interface/knowledge-base.entry.html/2017/06/12/fletcher_checksumfo-TeDF)
> >
> > Yeah, that's the one I was referring to.
> >
> >> That article states explicitly that the code was taken from Wikipedia,
> >> so it is CC-SA, which is to the best of my knowledge 1) compatible
> >> with GPL, and 2) does not require attribution if the original material
> >> is missing it, and it does. So AFAICT we are clear on the licensing
> >> front.
> >
> > First of all I can't seem to find that code snippet on the wiki page it
> > does refer to, so I'm still not convinced.
>
> It was there in older versions of the article. See for example:
> https://en.wikipedia.org/w/index.php?title=Fletcher%27s_checksum&oldid=730327006
Ah, thanks for digging that out.
> > Second, this should have been high-lighted in your submission somehow.
>
> Definitely. All code has an original author who deserves credit. And if
> you cannot find the original author, then there is always a risk than
> someone along the line stole the code... Maybe long before it ended up
> in Wikipedia. But that doesn't matter.
>
> Doesn't seem worth the risk for a simple checksum algorithm which
> probably has lots of GPL implementations.
Right.
Johan