Re: [PATCH] [PATCH v6] USB: serial: pl2303: Add new PID to support PL2303HXN (TYPE_HXN)

[Johan Hovold <johan@kernel.org>]

[ Please do not top post, reply inline and trim irrelevant context.
  Specifically do not copy context to the top of the mail.

  Reordering your reply below. ]

On Wed, Jul 03, 2019 at 12:22:45AM +0800, Charles Yeh wrote:
> Johan Hovold <johan@kernel.org> 於 2019年7月1日 週一 下午11:29寫道：
> > On Mon, Jul 01, 2019 at 11:11:02PM +0800, Charles Yeh wrote:

> > > >         } else if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
> > > > > +                     pl2303_vendor_write(serial, PL2303_HXN_RESET_CONTROL,
> > > > > +                             0);
> > > >
> > > > You again completely ignored my question about why you're wring 0
> > > > instead of 3 here.
> > > >
> > > > I'll ignore your patch until you explain.
> > >
> > > 3. In pl2303_open: Because TYPE_HXN is different from the instruction of reset
> > >    down/up stream used by TYPE_HX.
> > >    Therefore, we will also execute different instructions here.
> > >    The default of chip Reset Control is 0xFF(TYPE_HXN), therefore we will
> > >    write 0x00 to reset down/up stream(TYPE_HXN).
> >
> > I'm asking why you write the value 0 instead of 3 (or say, 0xfc)? Your
> > documentation said bit 0 and 1 are used to reset the up and downstream
> > pipes.
> >
> > To be more specific; what happens if I
> >
> >         1. set bit 0
> >         2. clear bit 0?
> >
> > and leave the other bits alone (write back the same value, e.g. 0xfe).

> You are right..
> set "1" is reset.
> set "0" is nothing.
> 
> I have used pl2303_update_reg instead pl2303_vendor_write which to reset
> the upstream and downstream pipe data

Ok, thanks for confirming. Note that I asked you about this back in
April (15 April).

In that very same mail thread I also pointed out that you must not
ignore review feedback. Even if for some reason disagree with it you
should at least explain why.

Johan