Hi Sekhar,
Thanks for reviewing.
On Fri, Apr 26, 2013 at 12:31:53, Nori, Sekhar wrote:
> Hi Prakash,
>
> On 4/9/2013 6:01 PM, Manjunathappa, Prakash wrote:
> > For modules having single clock, clk_get should be done with dev_id.
> > But current davinci implementation handles multiple instances
> > of the UART devices with single platform_device_register. Hence clk_get
> > is based on con_id rather than dev_id, this is not correct. Do
> > platform_device_register for each instance and clk_get on dev_id.
> >
> > Signed-off-by: Manjunathappa, Prakash <prakash.pm@xxxxxx>
> > ---
> > arch/arm/mach-davinci/da830.c | 8 ++--
> > arch/arm/mach-davinci/da850.c | 8 ++--
> > arch/arm/mach-davinci/devices-da8xx.c | 40 ++++++++++++++++---
> > arch/arm/mach-davinci/devices-tnetv107x.c | 35 ++++++++++++++---
> > arch/arm/mach-davinci/dm355.c | 48 ++++++++++++++++++-----
> > arch/arm/mach-davinci/dm365.c | 35 ++++++++++++----
> > arch/arm/mach-davinci/dm644x.c | 48 ++++++++++++++++++-----
> > arch/arm/mach-davinci/dm646x.c | 49 +++++++++++++++++++-----
> > arch/arm/mach-davinci/include/mach/da8xx.h | 2 +-
> > arch/arm/mach-davinci/include/mach/tnetv107x.h | 2 +-
> > arch/arm/mach-davinci/serial.c | 19 ++++++---
> > arch/arm/mach-davinci/tnetv107x.c | 8 ++--
> > 12 files changed, 230 insertions(+), 72 deletions(-)
>
> [...]
>
> > diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-davinci/devices-da8xx.c
> > index fc50243..eec7132 100644
> > --- a/arch/arm/mach-davinci/devices-da8xx.c
> > +++ b/arch/arm/mach-davinci/devices-da8xx.c
> > @@ -67,7 +67,7 @@
> > void __iomem *da8xx_syscfg0_base;
> > void __iomem *da8xx_syscfg1_base;
> >
> > -static struct plat_serial8250_port da8xx_serial_pdata[] = {
> > +static struct plat_serial8250_port da8xx_serial_pdata0[] = {
>
> da8xx_serial0_pdata is more appropriate. Likewise for other entries below.
>
True, will change them.
> > {
> > .mapbase = DA8XX_UART0_BASE,
> > .irq = IRQ_DA8XX_UARTINT0,
> > @@ -77,6 +77,11 @@ static struct plat_serial8250_port da8xx_serial_pdata[] = {
> > .regshift = 2,
> > },
> > {
> > + .flags = 0,
> > + },
>
> No need of trailing ',' on sentinel. No need of the zero initialization.
> Here and other places below.
>
Ok. I will remove them.
> > +};
> > +static struct plat_serial8250_port da8xx_serial_pdata1[] = {
> > + {
> > .mapbase = DA8XX_UART1_BASE,
> > .irq = IRQ_DA8XX_UARTINT1,
> > .flags = UPF_BOOT_AUTOCONF | UPF_SKIP_TEST |
> > @@ -85,6 +90,11 @@ static struct plat_serial8250_port da8xx_serial_pdata[] = {
> > .regshift = 2,
> > },
> > {
> > + .flags = 0,
> > + },
> > +};
> > +static struct plat_serial8250_port da8xx_serial_pdata2[] = {
> > + {
> > .mapbase = DA8XX_UART2_BASE,
> > .irq = IRQ_DA8XX_UARTINT2,
> > .flags = UPF_BOOT_AUTOCONF | UPF_SKIP_TEST |
> > @@ -97,11 +107,29 @@ static struct plat_serial8250_port da8xx_serial_pdata[] = {
> > },
> > };
> >
> > -struct platform_device da8xx_serial_device = {
> > - .name = "serial8250",
> > - .id = PLAT8250_DEV_PLATFORM,
> > - .dev = {
> > - .platform_data = da8xx_serial_pdata,
> > +struct platform_device da8xx_serial_device[] = {
> > + {
> > + .name = "serial8250",
> > + .id = PLAT8250_DEV_PLATFORM,
> > + .dev = {
> > + .platform_data = da8xx_serial_pdata0,
> > + },
> > + },
> > + {
> > + .name = "serial8250",
> > + .id = PLAT8250_DEV_PLATFORM1,
> > + .dev = {
> > + .platform_data = da8xx_serial_pdata1,
> > + },
> > + },
> > + {
> > + .name = "serial8250",
> > + .id = PLAT8250_DEV_PLATFORM2,
> > + .dev = {
> > + .platform_data = da8xx_serial_pdata2,
> > + },
> > + },
> > + {
> > },
> > };
>
> [...]
>
> > diff --git a/arch/arm/mach-davinci/serial.c b/arch/arm/mach-davinci/serial.c
> > index f262581..57e6150 100644
> > --- a/arch/arm/mach-davinci/serial.c
> > +++ b/arch/arm/mach-davinci/serial.c
> > @@ -76,7 +76,7 @@ int __init davinci_serial_setup_clk(unsigned instance, unsigned int *rate)
> > char name[16];
> > struct clk *clk;
> > struct davinci_soc_info *soc_info = &davinci_soc_info;
> > - struct device *dev = &soc_info->serial_dev->dev;
> > + struct device *dev = &soc_info->serial_dev[instance].dev;
> >
> > sprintf(name, "uart%d", instance);
> > clk = clk_get(dev, name);
>
> Why not pass con_id = NULL now?
>
Correct, I will change it.
> > @@ -96,19 +96,25 @@ int __init davinci_serial_setup_clk(unsigned instance, unsigned int *rate)
> >
> > int __init davinci_serial_init(struct davinci_uart_config *info)
> > {
> > - int i, ret;
> > + int i, ret = 0;
> > struct davinci_soc_info *soc_info = &davinci_soc_info;
> > - struct device *dev = &soc_info->serial_dev->dev;
> > - struct plat_serial8250_port *p = dev->platform_data;
> > + struct device *dev;
> > + struct plat_serial8250_port *p;
> >
> > /*
> > * Make sure the serial ports are muxed on at this point.
> > * You have to mux them off in device drivers later on if not needed.
> > */
> > - for (i = 0; p->flags; i++, p++) {
> > + for (i = 0; soc_info->serial_dev[i].dev.platform_data != NULL; i++) {
> > + dev = &soc_info->serial_dev[i].dev;
> > + p = dev->platform_data;
> > if (!(info->enabled_uarts & (1 << i)))
> > continue;
> >
> > + ret = platform_device_register(&soc_info->serial_dev[i]);
> > + if (ret)
> > + continue;
> > +
> > ret = davinci_serial_setup_clk(i, &p->uartclk);
> > if (ret)
> > continue;
> > @@ -125,6 +131,5 @@ int __init davinci_serial_init(struct davinci_uart_config *info)
> > if (p->membase && p->type != PORT_AR7)
> > davinci_serial_reset(p);
> > }
> > -
> > - return platform_device_register(soc_info->serial_dev);
> > + return ret;
> > }
>
> Now that we are overhauling this part of code, some improvements can be
> done. First, get rid of struct davinci_uart_config. None of the board
> files use it meaningfully and we know we are not going to have more
> board files. Second, make davinci_serial_init() take pointer to serial
> platform device directly. This eliminates need for serial_dev in the
> soc_info structure (yay!). You might also find that
> davinci_serial_setup_clk() can be eliminated as well since there is not
> much to do there now.
>
Ok I will cleanup this and submit v2.
Thanks,
Prakash
��.n��������+%������w��{.n�����{��ǫ����{ay� ʇڙ���f���h������_�(�階�ݢj"��������G����?���&��
