[PATCH 2/2] OMAP2+: UART: Add mechanism to probe uart pins and configure rx wakeup

[Russ Dill]

On Tue, Apr 10, 2012 at 9:11 AM, Tony Lindgren <tony at atomide.com> wrote:
> * Govindraj.R <govindraj.raja at ti.com> [120410 06:44]:
>> --- a/arch/arm/mach-omap2/serial.c
>> +++ b/arch/arm/mach-omap2/serial.c
>> @@ -120,11 +120,63 @@ static void omap_uart_set_smartidle(struct platform_device *pdev) {}
>>  #endif /* CONFIG_PM */
>>
>>  #ifdef CONFIG_OMAP_MUX
>> -static void omap_serial_fill_default_pads(struct omap_board_data *bdata)
>> +
>> +#define OMAP_UART_DEFAULT_PAD_NAME_LEN       28
>> +static char rx_pad_name[OMAP_UART_DEFAULT_PAD_NAME_LEN],
>> +             tx_pad_name[OMAP_UART_DEFAULT_PAD_NAME_LEN] __initdata;
>> +static struct omap_device_pad default_omap_uart_pads[2] __initdata;
>
> Won't the default_omap_uart_pads get overwritten for each port?
> We need that information in the driver too for each port? Why don't you
> just allocate bdata.pads in omap_serial_board_init as we now assume it's
> needed for each port?

Because of the way the function is used, it doesn't. It does seem
somewhat fragile though, with non-obvious breakage possible in the
future if someone changes the way omap_serial_fill_default_pads is
called.

>> +static void  __init omap_serial_fill_default_pads(struct omap_board_data *bdata)
>>  {
>> +     struct omap_mux_partition *tx_partition = NULL, *rx_partition = NULL;
>> +     struct omap_mux *rx_mux = NULL, *tx_mux = NULL;
>> +
>> +     if (bdata->id != 2) {
>> +             snprintf(rx_pad_name, OMAP_UART_DEFAULT_PAD_NAME_LEN,
>> +                      "uart%d_rx.uart%d_rx", bdata->id + 1, bdata->id + 1);
>> +             snprintf(tx_pad_name, OMAP_UART_DEFAULT_PAD_NAME_LEN,
>> +                      "uart%d_tx.uart%d_tx", bdata->id + 1, bdata->id + 1);
>> +     } else {
>> +             snprintf(rx_pad_name, OMAP_UART_DEFAULT_PAD_NAME_LEN,
>> +                     "uart%d_rx_irrx.uart%d_rx_irrx", bdata->id + 1,
>> +                     bdata->id + 1);
>> +             snprintf(tx_pad_name, OMAP_UART_DEFAULT_PAD_NAME_LEN,
>> +                     "uart%d_tx_irtx.uart%d_tx_irtx", bdata->id + 1,
>> +                     bdata->id + 1);
>> +     }
>
> This would be simpler with something like this (untested):
>
>        char *rx_fmt, *tx_fmt;
>        int uart_nr = bdata->id + 1;
>
>        if (bdata->id == 2) {
>                rx_fmt = "uart%d_rx.uart%d_rx";
>                tx_fmt = "uart%d_tx.uart%d_tx";
>        } else {
>                rx_fmt = "uart%d_rx_irrx.uart%d_rx_irrx";
>                tx_fmt = "uart%d_tx_irtx.uart%d_tx_irtx";
>        }
>
>        snprintf(rx_pad_name, OMAP_UART_DEFAULT_PAD_NAME_LEN, rx_fmt,
>                        uart_nr, uart_nr);
>        snprintf(tx_pad_name, OMAP_UART_DEFAULT_PAD_NAME_LEN, tx_fmt,
>                        uart_nr, uart_nr);
>
>> +     if (omap_mux_get_by_name(rx_pad_name, &rx_partition, &rx_mux) >= 0 &&
>> +                     omap_mux_get_by_name
>> +                             (tx_pad_name, &tx_partition, &tx_mux) >= 0) {
>> +             u16 tx_mode, rx_mode;
>> +
>> +             tx_mode = omap_mux_read(tx_partition, tx_mux->reg_offset);
>> +             rx_mode = omap_mux_read(rx_partition, rx_mux->reg_offset);
>> +
>> +             /*
>> +              * Check if uart is used in default tx/rx mode i.e. in mux mode0
>> +              * if yes then configure rx pin for wake up capability
>> +              */
>> +             if (!(rx_mode & 0x07) && !(tx_mode & 0x07)) {
>> +                     default_omap_uart_pads[0].name = rx_pad_name;
>> +                     default_omap_uart_pads[0].flags  =
>> +                             OMAP_DEVICE_PAD_REMUX | OMAP_DEVICE_PAD_WAKEUP;
>> +                     default_omap_uart_pads[0].enable = OMAP_PIN_INPUT |
>> +                                                             OMAP_MUX_MODE0;
>> +                     default_omap_uart_pads[0].idle = OMAP_PIN_INPUT |
>> +                                                             OMAP_MUX_MODE0;
>> +
>> +                     default_omap_uart_pads[1].name = tx_pad_name;
>> +                     default_omap_uart_pads[1].enable = OMAP_PIN_OUTPUT |
>> +                                                             OMAP_MUX_MODE0;
>> +                     bdata->pads = default_omap_uart_pads;
>> +                     bdata->pads_cnt = ARRAY_SIZE(default_omap_uart_pads);
>> +             }
>> +     }
>>  }
>
> Then looking at omap_serial_board_init, it still looks wrong:
>
> void __init omap_serial_board_init(struct omap_uart_port_info *info)
> {
>        struct omap_uart_state *uart;
>        struct omap_board_data bdata;
>
>        list_for_each_entry(uart, &uart_list, node) {
>                bdata.id = uart->num;
>                bdata.flags = 0;
>                bdata.pads = NULL;
>                bdata.pads_cnt = 0;
>
>                if (cpu_is_omap44xx() || cpu_is_omap34xx())
>                        omap_serial_fill_default_pads(&bdata);
>
> Why don't you fill the pads for omap2? It has the same pads, although
> it does not have the serial wake-up events working.
>
> And why don't you exit if omap_serial_fill_default_pads fails for the
> no info case?
>
> Shouldn't it be something like:
>
>                if (!info) {
>                        res = omap_serial_fill_default_pads(&bdata);
>                        if (!res)
>                                return;
>                        port = NULL;
>                } else {
>                        port = &info[uart->num];
>                }
>                omap_serial_init_port(&bdata, port);
>
> Regards,
>
> Tony