[U-Boot,v1,3/5] dm: serial: Add setparity

Message ID 1526561446-29004-4-git-send-email-patrice.chotard@st.com
State Accepted
Commit eae4764f1ac2f3968a5c87a17b2e894a5fe05d0b
Delegated to: Tom Rini
Headers show
Series
  • Update STM32 serial driver
Related show

Commit Message

Patrice CHOTARD May 17, 2018, 12:50 p.m.
From: Patrick Delaunay <patrick.delaunay@st.com>

Implements serial setparity ops to allow uart parity change.
It allows to select ODD, EVEN or NONE parity.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
---

 include/serial.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Simon Glass May 18, 2018, 1:37 a.m. | #1
Hi Patrick,

On 17 May 2018 at 06:50, Patrice Chotard <patrice.chotard@st.com> wrote:
> From: Patrick Delaunay <patrick.delaunay@st.com>
>
> Implements serial setparity ops to allow uart parity change.
> It allows to select ODD, EVEN or NONE parity.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
>
>  include/serial.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/include/serial.h b/include/serial.h
> index 384df94ed0b3..b9ef6d91c9c5 100644
> --- a/include/serial.h
> +++ b/include/serial.h
> @@ -67,6 +67,12 @@ extern int usbtty_tstc(void);
>
>  struct udevice;
>
> +enum serial_par {
> +       SERIAL_PAR_NONE,
> +       SERIAL_PAR_ODD,
> +       SERIAL_PAR_EVEN
> +};
> +
>  /**
>   * struct struct dm_serial_ops - Driver model serial operations
>   *
> @@ -143,6 +149,16 @@ struct dm_serial_ops {
>          */
>         int (*loop)(struct udevice *dev, int on);
>  #endif
> +       /**
> +        * setparity() - Set up the parity
> +        *
> +        * Set up a new parity for this device.
> +        *
> +        * @dev: Device pointer
> +        * @parity: parity to use
> +        * @return 0 if OK, -ve on error
> +        */
> +       int (*setparity)(struct udevice *dev, enum serial_par parity);

To me it seems that changing parity while in the middle of operation
might be tricky. I suppose this follows along with setbrg() so fair
enough.

But I worry about adding more operations here. The next thing to come
is presumably the length (7 bits, 8 bits, ...). Perhaps we should have
a more generic setconfig() which change change speed, format and
parity all at once? For format and parity, we could have a 'default'
parameter value.

Also, there should be a corresponding function in serial-uclass.c and
ideal a call from some sandbox test (although I see at present we
don't have test/dm/serial.c)

Regards,
Simon
Patrice CHOTARD May 21, 2018, 7:12 a.m. | #2
Hi Simon

On 05/18/2018 03:37 AM, Simon Glass wrote:
> Hi Patrick,
> 
> On 17 May 2018 at 06:50, Patrice Chotard <patrice.chotard@st.com> wrote:
>> From: Patrick Delaunay <patrick.delaunay@st.com>
>>
>> Implements serial setparity ops to allow uart parity change.
>> It allows to select ODD, EVEN or NONE parity.
>>
>> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>> ---
>>
>>   include/serial.h | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/include/serial.h b/include/serial.h
>> index 384df94ed0b3..b9ef6d91c9c5 100644
>> --- a/include/serial.h
>> +++ b/include/serial.h
>> @@ -67,6 +67,12 @@ extern int usbtty_tstc(void);
>>
>>   struct udevice;
>>
>> +enum serial_par {
>> +       SERIAL_PAR_NONE,
>> +       SERIAL_PAR_ODD,
>> +       SERIAL_PAR_EVEN
>> +};
>> +
>>   /**
>>    * struct struct dm_serial_ops - Driver model serial operations
>>    *
>> @@ -143,6 +149,16 @@ struct dm_serial_ops {
>>           */
>>          int (*loop)(struct udevice *dev, int on);
>>   #endif
>> +       /**
>> +        * setparity() - Set up the parity
>> +        *
>> +        * Set up a new parity for this device.
>> +        *
>> +        * @dev: Device pointer
>> +        * @parity: parity to use
>> +        * @return 0 if OK, -ve on error
>> +        */
>> +       int (*setparity)(struct udevice *dev, enum serial_par parity);
> 
> To me it seems that changing parity while in the middle of operation
> might be tricky. I suppose this follows along with setbrg() so fair
> enough.
> 
> But I worry about adding more operations here. The next thing to come
> is presumably the length (7 bits, 8 bits, ...). Perhaps we should have
> a more generic setconfig() which change change speed, format and
> parity all at once? For format and parity, we could have a 'default'
> parameter value.

Ok that make sense, i will resubmit a v2

> 
> Also, there should be a corresponding function in serial-uclass.c and
> ideal a call from some sandbox test (although I see at present we
> don't have test/dm/serial.c)

Ok

Patrice

> 
> Regards,
> Simon
>
Tom Rini May 28, 2018, 7:13 p.m. | #3
On Thu, May 17, 2018 at 02:50:44PM +0200, Patrice Chotard wrote:

> From: Patrick Delaunay <patrick.delaunay@st.com>
> 
> Implements serial setparity ops to allow uart parity change.
> It allows to select ODD, EVEN or NONE parity.
> 
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>

Applied to u-boot/master, thanks!
Patrice CHOTARD May 29, 2018, 7:13 a.m. | #4
Hi Tom

On 05/28/2018 09:13 PM, Tom Rini wrote:
> On Thu, May 17, 2018 at 02:50:44PM +0200, Patrice Chotard wrote:
> 
>> From: Patrick Delaunay <patrick.delaunay@st.com>
>>
>> Implements serial setparity ops to allow uart parity change.
>> It allows to select ODD, EVEN or NONE parity.
>>
>> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> 
> Applied to u-boot/master, thanks!
> 

After Simon's review, i expected to send a v2 of this series but you was 
too fast ;-).

2 solutions:
  _ i submit a new patchset including Simon's remarks on top of current 
master branch
  _ you revert this series and i submit the v2

What do you prefer ?

Thanks

Patrice
Tom Rini May 29, 2018, 11:14 a.m. | #5
On Tue, May 29, 2018 at 07:13:36AM +0000, Patrice CHOTARD wrote:
> Hi Tom
> 
> On 05/28/2018 09:13 PM, Tom Rini wrote:
> > On Thu, May 17, 2018 at 02:50:44PM +0200, Patrice Chotard wrote:
> > 
> >> From: Patrick Delaunay <patrick.delaunay@st.com>
> >>
> >> Implements serial setparity ops to allow uart parity change.
> >> It allows to select ODD, EVEN or NONE parity.
> >>
> >> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> >> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> > 
> > Applied to u-boot/master, thanks!
> > 
> 
> After Simon's review, i expected to send a v2 of this series but you was 
> too fast ;-).
> 
> 2 solutions:
>   _ i submit a new patchset including Simon's remarks on top of current 
> master branch
>   _ you revert this series and i submit the v2
> 
> What do you prefer ?

Ugh, oops.  I swear it was good when I checked, but maybe I was too
quick.  A follow-up series please, thanks!

Patch

diff --git a/include/serial.h b/include/serial.h
index 384df94ed0b3..b9ef6d91c9c5 100644
--- a/include/serial.h
+++ b/include/serial.h
@@ -67,6 +67,12 @@  extern int usbtty_tstc(void);
 
 struct udevice;
 
+enum serial_par {
+	SERIAL_PAR_NONE,
+	SERIAL_PAR_ODD,
+	SERIAL_PAR_EVEN
+};
+
 /**
  * struct struct dm_serial_ops - Driver model serial operations
  *
@@ -143,6 +149,16 @@  struct dm_serial_ops {
 	 */
 	int (*loop)(struct udevice *dev, int on);
 #endif
+	/**
+	 * setparity() - Set up the parity
+	 *
+	 * Set up a new parity for this device.
+	 *
+	 * @dev: Device pointer
+	 * @parity: parity to use
+	 * @return 0 if OK, -ve on error
+	 */
+	int (*setparity)(struct udevice *dev, enum serial_par parity);
 };
 
 /**