Re: [PATCH] serial: samsung: add support for manual RTS setting

[Tomasz Figa <t.figa@samsung.com>]

On Tuesday 17 of September 2013 20:08:33 José Miguel Gonçalves wrote:
> On 17-09-2013 16:21, Tomasz Figa wrote:
> > I had the following scenario in mind:
> > 1) enable CRTSCTS,
> > 2) set RTS status using the IOCTL,
> > 3) disable CRTSCTS,
> > 4) do something,
> > 5) enable CRTSCTS again.
> > 
> > I would expect that the value set in point 2 would be still valid after
> > point 5, while it will be reset.
> 
> Maybe I'm missing something but, as I see it, as soon as you enable AFC
> in the UART controller the RTS pin is no more manually controllable, so
> after point 5 the pin is set automattically by the controller according
> with the Rx FIFO contents. Don't you want to say instead that the value
> set in 2) will be valid in 4)?

Ahh, right, I inverted CRTSCTS bit polarity in points 1, 3 and 5. It should 
be:
1) disable CRTSCTS,
2) set RTS status using the IOCTL,
3) enable CRTSCTS,
4) do something,
5) disable CRTSCTS again.

> >> Regarding port capability, if it's decided to validate it in
> >> s3c24xx_serial_get_mctrl() and s3c24xx_serial_set_mctrl() it should
> >> also
> >> be validated here. The question is how to validate for the full
> >> spectrum
> >> of SoCs that this driver supports?
> > 
> > Hmm, since the driver is already broken in this aspect, ignoring this
> > in
> > your patch might be fine for now. A follow up patch fixing this would
> > be
> > welcome, though. However I don't have any good idea how to implement
> > this at the moment.
> > 
> > First thing that comes to my mind is using the variant data structures
> > to store information about per port capability (different port FIFO
> > sizes are already handled like this), but this would imply splitting
> > some of the groups, as S5PC100 supports modem control for different
> > subset of ports than S3C64xx, while they both use the same variant
> > data.
> > 
> > Using device tree, this could be passed as an extra property, but some
> > of the platforms using samsung serial driver can be booted without
> > device tree, so it wouldn't cover all the cases.
> 
> So, as I understand, for now is enough to resubmit the patch with the
> UMCON register setting in s3c24xx_serial_set_termios() preserving the
> previous manual setting of the RTS pin, correct? I was thinking in
> something like this:
> 
>      umcon = rd_regb(port, S3C2410_UMCON);
>      if (termios->c_cflag & CRTSCTS) {
>          umcon |= S3C2410_UMCOM_AFC;
>          umcon &= ~S3C2412_UMCON_AFC_8; // Disable RTS when RX FIFO
> contains 63 bytes } else {
>          umcon &= ~S3C2410_UMCOM_AFC;
>      }
>      wr_regl(port, S3C2410_UMCON, umcon);

Yes, this looks fine.

Best regards,
Tomasz