Sergei Shtylyov-2 wrote: > > Hello. > > On 29-11-2010 10:13, Subhasish Ghosh wrote: > >> The patch adds support for the programmable realtime unit (PRU) >> available on OMAPL138. This defines the system resource >> requirements such as pin mux, clock, iomem, interrupt etc >> and registers the platform device as per the Linux driver model. > >> Signed-off-by: Subhasish Ghosh<[email protected]> > [...] > >> diff --git a/arch/arm/mach-davinci/board-da850-evm.c >> b/arch/arm/mach-davinci/board-da850-evm.c >> index f89b0b7..e907ef5 100644 >> --- a/arch/arm/mach-davinci/board-da850-evm.c >> +++ b/arch/arm/mach-davinci/board-da850-evm.c > > The board file should be in a patch of its own. > > >> diff --git a/arch/arm/mach-davinci/da850.c >> b/arch/arm/mach-davinci/da850.c >> index 63916b9..59a3638 100644 >> --- a/arch/arm/mach-davinci/da850.c >> +++ b/arch/arm/mach-davinci/da850.c >> @@ -238,6 +238,13 @@ static struct clk tptc2_clk = { >> .flags = ALWAYS_ENABLED, >> }; >> >> +static struct clk pru_clk = { >> + .name = "pru_ck", >> + .parent = &pll0_sysclk2, >> + .lpsc = DA8XX_LPSC0_DMAX, >> + .flags = ALWAYS_ENABLED, > > Why it's always enabled? > [SG] - As per the L138 Datasheet, this falls under "Always On" domain. > >> +}; >> + >> static struct clk uart0_clk = { >> .name = "uart0", >> .parent =&pll0_sysclk2, >> @@ -373,6 +380,7 @@ static struct clk_lookup da850_clks[] = { >> CLK(NULL, "tpcc1", &tpcc1_clk), >> CLK(NULL, "tptc2", &tptc2_clk), >> CLK(NULL, "uart0", &uart0_clk), >> + CLK(NULL, "pru_ck", &pru_clk), > > Don't use spaces to indent and align the initializer with the others > please. > >> @@ -390,12 +398,15 @@ static struct clk_lookup da850_clks[] = { >> CLK(NULL, NULL, NULL), >> }; >> >> -/* >> - * Device specific mux setup >> - * >> - * soc description mux mode mode mux dbg >> - * reg offset mask mode >> +/* soc -> DA850 >> + * desc -> Pin name, which evaluates to soc##_##desc. >> + * muxreg -> Pin Multiplexing Control n (PINMUXn) Register >> number. >> + * mode_offset -> Bit offset in the register PINMUXn. >> + * mode_mask -> Number of bits for Pin Multiplexing Control n. >> + * mux_mode -> Multiplexing mode to set. >> + * dbg -> debug on/off. > > Why are you changing this at all? > >> @@ -514,7 +525,7 @@ static const struct mux_config da850_pins[] = { >> MUX_CFG(DA850, EMA_A_5, 12, 8, 15, 1, false) >> MUX_CFG(DA850, EMA_A_6, 12, 4, 15, 1, false) >> MUX_CFG(DA850, EMA_A_7, 12, 0, 15, 1, false) >> - MUX_CFG(DA850, EMA_A_8, 11, 28, 15, 1, false) >> + MUX_CFG(DA850, EMA_A_8, 11, 28, 15, 1, false) > > Why touch this at all? > >> @@ -542,7 +553,12 @@ static const struct mux_config da850_pins[] = { >> MUX_CFG(DA850, EMA_CLK, 6, 0, 15, 1, false) >> MUX_CFG(DA850, EMA_WAIT_1, 6, 24, 15, 1, false) >> MUX_CFG(DA850, NEMA_CS_2, 7, 0, 15, 1, false) >> + /* PRU functions for soft can */ > > CAN. And please use tab to, not spaces. > >> + MUX_CFG(DA850, PRU0_R31_0, 7, 28, 15, 0, false) >> + MUX_CFG(DA850, PRU1_R30_15, 12, 0, 15, 4, false) >> + MUX_CFG(DA850, PRU1_R31_18, 11, 20, 15, 0, false) > > Please align with the other initializers. And are these all PRU pins? > [SG] - Yes, all of these are PRU pins. > >> /* GPIO function */ >> + MUX_CFG(DA850, GPIO2_0, 6, 28, 15, 8, false) >> MUX_CFG(DA850, GPIO2_6, 6, 4, 15, 8, false) >> MUX_CFG(DA850, GPIO2_8, 5, 28, 15, 8, false) >> MUX_CFG(DA850, GPIO2_15, 5, 0, 15, 8, false) >> @@ -557,6 +573,12 @@ const short da850_uart0_pins[] __initdata = { >> -1 >> }; >> >> +const short da850_pru_can_pins[] __initdata = { >> + DA850_GPIO2_0, DA850_PRU0_R31_0, DA850_PRU1_R30_15, >> + DA850_PRU1_R31_18, > > No, lists in da850.c are generic and associated GPIO pins shouldn't be > there -- this list should be placed into the board file. > >> diff --git a/arch/arm/mach-davinci/devices-da8xx.c >> b/arch/arm/mach-davinci/devices-da8xx.c >> index 9eec630..11a9b67 100644 >> --- a/arch/arm/mach-davinci/devices-da8xx.c >> +++ b/arch/arm/mach-davinci/devices-da8xx.c >> @@ -85,6 +86,42 @@ struct platform_device da8xx_serial_device = { > [...] >> +static struct platform_device omapl138_pru_can_device = { >> + .name = "davinci_pru_can", >> + .id = 1, > > Why 1? Is there other device? I don't see them... > >> diff --git a/include/linux/can/platform/ti_omapl_pru_can.h >> b/include/linux/can/platform/ti_omapl_pru_can.h >> new file mode 100644 >> index 0000000..0dea436 >> --- /dev/null >> +++ b/include/linux/can/platform/ti_omapl_pru_can.h >> @@ -0,0 +1,29 @@ > > I think this file belongs to the driver patch... > [SG] -- This header file is included in devices-da8xx.c. > > WBR, Sergei > _______________________________________________ > Davinci-linux-open-source mailing list > [email protected] > http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source > >
-- View this message in context: http://davinci-linux-open-source.1494791.n2.nabble.com/RFC-PATCH-0-1-da850-evm-Support-for-TI-s-PRU-CAN-Emulation-tp5783456p5784294.html Sent from the davinci-linux-open-source mailing list archive at Nabble.com. _______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
