On Tue, Apr 26, 2016 at 03:24:53PM +0300, Maksim Salau wrote:
> Hi Johan,
>
> On Sun, 24 Apr 2016 15:36:35 +0200
> Johan Hovold <johan@xxxxxxxxxx> wrote:
>
> > On Mon, Feb 29, 2016 at 04:11:20PM +0300, Maksim Salau wrote:
> > > The adaptor can be found on development boards for 78k, RL78 and V850
> > > microcontrollers produced by Renesas Electronics Corporation.
> > >
> > > This is not a full-featured USB to serial converter, however it allows
> > > basic communication and simple control which is enough for programming of
> > > on-board flash and debugging through a debug monitor.
> > >
> > > uPD78F0730 is a USB-enabled microcontroller with USB-to-UART conversion
> > > implemented in firmware.
> > >
> > > This chip is also present in some debugging adaptors which use it for
> > > USB-to-SPI conversion as well. The present driver doesn't cover SPI,
> > > only USB-to-UART conversion is supported.
> > >
> > > Signed-off-by: Maksim Salau <maksim.salau@xxxxxxxxx>
> > > ---
> >
> > Thanks for submitting this driver, and sorry for the late review. The
> > code looks nice and clean, but I have some comments below.
> >
>
> Thank you for feedback.
>
> > > +/* Control signal bits in UPD78F0730_CMD_SET_DTR_RTS command */
> > > +#define UPD78F0730_RESET_RTS 0x01
> > > +#define UPD78F0730_RESET_DTR 0x02
> >
> > Why are these named RESET? Looks like you use these bits to assert the
> > signals when reading the datasheet.
>
> Indeed, it turned out that RTS and DTR signals are inverted:
> upd78f0730 has active-high levels, while others have active-low levels.
> To match others I added inversion. May be it'll be a good idea to make
> inversion optional by adding a module parameter.
No, we shouldn't be adding module parameters. Matching on device ID
would be the way to go if this was something that was device dependent,
but that doesn't seem to be the case here.
Can you verify that this does not conflict with how hardware flow
control has been implemented (in hw)?
> > > + if (cflag & CRTSCTS) {
> > > + dev_err(dev, "%s - hardware flow control is not supported\n",
> > > + __func__);
> >
> > dev_warn and clear the setting in termios.
> >
> > Looks like the device does support it though?
>
> There are means to enable it on protocol layer, but the CTS pin is not present.
> I doubt if any flow control will ever be used with the adaptor.
> I can add support for hardware flow control, but which flow control type should
> prevail if both are enabled? Only one flow control type can be enabled at a time
> according to the datasheet.
It seems reasonable to let hardware flow control take precedence over
software flow control if both are requested.
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
