[U-Boot] [PATCH v2 2/3] serial: stm32: Replace setparity by setconfig

[Patrice CHOTARD]

Hi Simon

On 08/02/2018 06:57 PM, Simon Glass wrote:
> Hi Patrice,
> 
> On 1 August 2018 at 09:58, Patrice Chotard <patrice.chotard at st.com> wrote:
>> Replace stm32_serial_setparity by stm32_serial_setconfig
>> which allows to set serial bits number, parity and stop
>> bits number.
>> Only parity setting is implemented.
>>
>> Signed-off-by: Patrick Delaunay <patrick.delaunay at st.com>
>> Signed-off-by: Patrice Chotard <patrice.chotard at st.com>
>> ---
>>
>> Changes in v2:
>>    - Update stm32_serial_setconfig prototype
>>
>>   drivers/serial/serial_stm32.c | 21 +++++++++++++++------
>>   1 file changed, 15 insertions(+), 6 deletions(-)
> 
> Reviewed-by: Simon Glass <sjg at chromium.org>
> 
> Please see below.
> 
> Also I worry about bisectability. U-Boot should always build every
> commit and I worry that your previous patch will break things, fixed
> by this patch.

Yes you are right, i will rework the series to avoid bissectability 
breakdown

> 
>>
>> diff --git a/drivers/serial/serial_stm32.c b/drivers/serial/serial_stm32.c
>> index f26234549c3e..53866a23e6fc 100644
>> --- a/drivers/serial/serial_stm32.c
>> +++ b/drivers/serial/serial_stm32.c
>> @@ -47,20 +47,28 @@ static int stm32_serial_setbrg(struct udevice *dev, int baudrate)
>>          return 0;
>>   }
>>
>> -static int stm32_serial_setparity(struct udevice *dev, enum serial_par parity)
>> +static int stm32_serial_setconfig(struct udevice *dev, uint serial_config)
>>   {
>>          struct stm32x7_serial_platdata *plat = dev_get_platdata(dev);
>>          bool stm32f4 = plat->uart_info->stm32f4;
>>          u8 uart_enable_bit = plat->uart_info->uart_enable_bit;
>>          u32 cr1 = plat->base + CR1_OFFSET(stm32f4);
>>          u32 config = 0;
>> -
>> -       if (stm32f4)
>> -               return -EINVAL; /* not supported in driver*/
>> +       u8 parity = SERIAL_GET_PARITY(serial_config);
>> +       u8 bits = SERIAL_GET_BITS(serial_config);
>> +       u8 stop = SERIAL_GET_STOP(serial_config);
> 
> I don't see the benefit of using u8 here. Isn't uint good enough?

I simply forgot to update these types

Thanks

Patrice

> 
>> +
>> +       /*
>> +        * only parity config is implemented, check if other serial settings
>> +        * are the default one.
>> +        * (STM32F4 serial IP didn't support parity setting)
>> +        */
>> +       if (bits != SERIAL_8_BITS || stop != SERIAL_ONE_STOP || stm32f4)
>> +               return -ENOTSUPP; /* not supported in driver*/
>>
>>          clrbits_le32(cr1, USART_CR1_RE | USART_CR1_TE | BIT(uart_enable_bit));
>>          /* update usart configuration (uart need to be disable)
>> -        * PCE: parity check control
>> +        * PCE: parity check enable
>>           * PS : '0' : Even / '1' : Odd
>>           * M[1:0] = '00' : 8 Data bits
>>           * M[1:0] = '01' : 9 Data bits with parity
>> @@ -77,6 +85,7 @@ static int stm32_serial_setparity(struct udevice *dev, enum serial_par parity)
>>                  config = USART_CR1_PCE | USART_CR1_M0;
>>                  break;
>>          }
>> +
>>          clrsetbits_le32(cr1,
>>                          USART_CR1_PCE | USART_CR1_PS | USART_CR1_M1 |
>>                          USART_CR1_M0,
>> @@ -210,7 +219,7 @@ static const struct dm_serial_ops stm32_serial_ops = {
>>          .pending = stm32_serial_pending,
>>          .getc = stm32_serial_getc,
>>          .setbrg = stm32_serial_setbrg,
>> -       .setparity = stm32_serial_setparity
>> +       .setconfig = stm32_serial_setconfig
>>   };
>>
>>   U_BOOT_DRIVER(serial_stm32) = {
>> --
>> 1.9.1
>>
> 
> Regards,
> Simon
>