Re: [PATCH] RS485: fix inconsistencies in the meaning of some variables

[Claudio Scordino]

Il 08/11/2011 10:30, Nicolas Ferre ha scritto:
> On 11/04/2011 09:19 AM, Claudio Scordino :
> > Hi Alan, Hi Greg,
> >
> > 	it seems that the crisv10.c and the atmel_serial.c serial
> > drivers interpret the fields of the serial_rs485 structure in a different
> > way.
> >
> > In particular, it seems that crisv10.c uses SER_RS485_RTS_AFTER_SEND and
> > SER_RS485_RTS_ON_SEND for the _logic value_ of the RTS pin;
> > atmel_serial.c, instead, uses these values to know if a _delay_ must be
> > set before and after sending.
>
> It seems sensible, but, on the other hand, I fear that this is a big
> change in the user interface: If people are already relying on this for
> their application, this can be difficult to understand the change. Can't
> we imagine an smoother migration path?
>
> It seems from de6f86ce5 that 16C950 may also use rs485 mode (with
> another signal that RTS BTW)...
>
> See comments online...
>
> > This patch makes the usage of these variables consistent across all
> > drivers and fixes the Documentation as well.
> > In particular, SER_RS485_RTS_AFTER_SEND and SER_RS485_RTS_ON_SEND will
> > be used to set the logic value of the RTS pin (as in the crisv10.c
> > driver); the delay is understood by looking only at the value of
> > delay_rts_before_send and delay_rts_after_send.
> >
> > Best regards,
> >
> > 	Claudio
> >
> >
> > Subject: RS485: fix inconsistencies in the meaning of some variables
> > From: Claudio Scordino<claudio@xxxxxxxxxxxxxxx>
> >
> > The crisv10.c and the atmel_serial.c serial drivers interpret the fields
> > of the serial_rs485 structure in a different way.
> > In particular, crisv10.c uses SER_RS485_RTS_AFTER_SEND and
> > SER_RS485_RTS_ON_SEND for the voltage of the RTS pin; atmel_serial.c, instead,
> > uses these values to know if a delay must be set before and after sending.
> > This patch makes the usage of these variables consistent across all drivers and
> > fixes the Documentation as well.
> > > From now on, SER_RS485_RTS_AFTER_SEND and SER_RS485_RTS_ON_SEND will be used to
> > set the voltage of the RTS pin (as in the crisv10.c driver); the delay will be
> > understood by looking only at the value of delay_rts_before_send and
> > delay_rts_after_send.
>
> Ok, but don't you think that the flags names are not so much
> self-explanatory for this new meaning?
>
> What about:
> SER_RS485_RTS_LEVEL_DURING_SEND
> SER_RS485_RTS_VALUE_DURING_SEND (maybe too vague?)
> SER_RS485_RTS_LOGICAL_VALUE_DURING_SEND (maybe too long?)
>
> Moreover, can't we just use one property for this? I mean, if RTS
> logical value is high during the sending of data, can it mean that RTS
> will be low before and after? And the other way around: if the signal is
> low during data send, will it be high before and after?
>
> Here again, changing the user interface is not a good idea, so I fear
> that it can be a show stopper.

Hi Nicolas,

	I understand, but honestly I do not agree.

The current state is inconsistent, and leaving the status quo can only 
bring to more issues in the future (because it is not clear if the 
interface should be used either as in the Cris or in the Atmel driver). 
That's why I think it should be fixed ASAP (before further drivers start 
using it).

The modifications that I have proposed are very minimal, and most 
user-space code should continue to work without any difference. Any Cris 
user-space code will continue to work, because we didn't change the 
behavior of the driver. For Atmel user-space code, instead, the behavior 
of the driver changes only if flags are not set and delay variables 
contain a value different than 0 (which, hopefully, is not a very common 
situation). That's the reason why I preferred to not change the names of 
the variables, even if better names would be desirable.

If you want, I can re-format the patch according to you suggestions, 
remove formatted lines and changing the names of the variables. But 
unfortunately, I cannot undertake the device tree bindings at the moment.

Best regards,

	Claudio
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/