On Tue, Apr 10, 2012 at 9:11 AM, Tony Lindgren <tony@xxxxxxxxxxx> wrote:
> * Govindraj.R <govindraj.raja@xxxxxx> [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
��.n��������+%������w��{.n�����{�������ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
