On Sun, Oct 16, 2016 at 05:09:24PM +0100, Aidan Thornton wrote: > On Mon, Oct 10, 2016 at 8:56 PM, Grigori Goronzy <greg@xxxxxxxxxxxx> wrote: > > On 2016-10-08 23:07, Aidan Thornton wrote: > >> > >> On Fri, Oct 7, 2016 at 12:30 PM, Johan Hovold <johan@xxxxxxxxxx> wrote: > >>> > >>> Why is this change needed? I see no write to this register in the > >>> vendor driver. > >> > >> > >> In principle, it might not be because the value written to register > >> 0x18 is probably overwritten by ch341_init_set_baudrate anyway. It's > >> there because it's in Grigori's original patch, it looks superficially > >> reasonable (corresponds to ENABLE_RX|ENABLE_TX|CS8, as opposed to the > >> rather implausible ENABLE_TX|PAR_EVEN|CS5), and given that some > >> hardware revisions are apparently a little temperamental I was > >> reluctant to remove it without fully understanding why it was added in > >> the first place. As the vendor driver does without it might make sense > >> to delete the write altogether, but it does do quite a few things > >> differently from this driver. Depends what Grigori says and the > >> results of actual testing. Ok, thanks for the explanation. I think we should try to get rid of anything apparently redundant eventually, moving towards how the vendor driver does things. It would also be good to replace more of the magic constants in there to make the code more self-describing. As long this is done in steps that can be (more easily) rolled-back in case it turns out we do actually break support for some device in the process. > > Yes, I think I introduced it for consistency. The whole init sequence is a > > bit strange and doesn't make too much sense, but I tried to avoid modifying > > it because I don't know how the different hardware revisions react to that. > > In theory it shouldn't make a difference as the chip is reinitialized later > > on, though. Yeah, that init sequence looks a bit weird indeed. But you say you introduced it for consistency; with what? Given that the vendor driver lacks this write and all devices supposedly have been working with the current sequence, I mean. > > By the way, I tested the series against my CH341A board and it works just > > fine, as expected. Great, thanks. > Ah, OK. Thanks for the help and sorry for the late reply, this got > buried under a pile of other e-mails in my inbox and it took me a > while to see it. Thoughts on omitting the write to 0x2518 altogether > then? Doing so doesn't seem to cause any problems on the CH340G I'm > testing on, but obviously I can't guarantee this will always be the > case. I suggest either keeping what's currently there or dropping it entirely as part of this change, and then possibly clean up the rest of the init sequence in a separate patch. Thanks, Johan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
