Re: [alsa-devel] [PATCH] OMAP: Exporting functions doing common register access
-----Original Message----- From: Paul Walmsley [mailto:paul@pwsan.com] Sent: Tuesday, November 17, 2009 2:46 AM To: Aggarwal, Anuj Cc: linux-omap@vger.kernel.org Subject: Re: [PATCH] OMAP: Exporting functions doing common register access
Hello Anuj,
On Mon, 16 Nov 2009, Anuj Aggarwal wrote:
These functions need to be exported so that drivers (e.g. McBSP) can be configured as modules.
McBSP driver gets built as a module when ASoC driver for OMAP3 EVM is configured as module. McBSP driver uses functions like omap_ctrl_readl/omap_ctrl_writel, which are defined in control.c file but not exported. Without that, McBSP driver fails to build as a module.
Signed-off-by: Anuj Aggarwal anuj.aggarwal@ti.com
This will prevent the McBSP driver from being usable across chips. For example, if a future OMAP comes out with McBSP clock source switching implemented in a different way, or if a DaVinci chip comes out with a McBSP block but no SCM block, this either won't build or won't run.
A better approach would be to move the OMAP2/3-specific functions from the McBSP driver into the OMAP McBSP integration code, arch/arm/*omap*/mcbsp.c, and to define function pointers in struct platform_data for these functions, and to pass them from the integration code into the device driver code. That will make these drivers independent of chip integration details.
Sorry to confuse you, I was talking about the platform specific ASoC-McBSP code for OMAP (file - sound/soc/omap/omap-mcbsp.c) platform. The McBSP driver - both platform and machine specific - doesn't use the functions exported by the patch.
Including alsa-devel list too...
- Paul
-----Original Message----- From: linux-omap-owner@vger.kernel.org [mailto:linux-omap- owner@vger.kernel.org] On Behalf Of Aggarwal, Anuj Sent: Tuesday, November 17, 2009 11:28 AM To: Paul Walmsley Cc: linux-omap@vger.kernel.org; alsa-devel@alsa-project.org Subject: RE: [PATCH] OMAP: Exporting functions doing common register access
-----Original Message----- From: Paul Walmsley [mailto:paul@pwsan.com] Sent: Tuesday, November 17, 2009 2:46 AM To: Aggarwal, Anuj Cc: linux-omap@vger.kernel.org Subject: Re: [PATCH] OMAP: Exporting functions doing common register access
Hello Anuj,
On Mon, 16 Nov 2009, Anuj Aggarwal wrote:
These functions need to be exported so that drivers (e.g. McBSP) can be configured as modules.
McBSP driver gets built as a module when ASoC driver for OMAP3 EVM is configured as module. McBSP driver uses functions like omap_ctrl_readl/omap_ctrl_writel, which are defined in control.c file but not exported. Without that, McBSP driver fails to build as a module.
Signed-off-by: Anuj Aggarwal anuj.aggarwal@ti.com
This will prevent the McBSP driver from being usable across chips. For example, if a future OMAP comes out with McBSP clock source switching implemented in a different way, or if a DaVinci chip comes out with a McBSP block but no SCM block, this either won't build or won't run.
A better approach would be to move the OMAP2/3-specific functions from
the
McBSP driver into the OMAP McBSP integration code, arch/arm/*omap*/mcbsp.c, and to define function pointers in struct platform_data for these functions, and to pass them from the integration code into the device driver code. That will make these drivers independent of chip integration details.
Sorry to confuse you, I was talking about the platform specific ASoC-McBSP code for OMAP (file - sound/soc/omap/omap-mcbsp.c) platform. The McBSP driver - both platform and machine specific - doesn't use the functions exported by the patch.
Including alsa-devel list too...
[Aggarwal, Anuj] Any comments on this patch? Do I have to re-work this?
- Paul
-- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 18 Nov 2009, Aggarwal, Anuj wrote:
[Aggarwal, Anuj] Any comments on this patch? Do I have to re-work this?
Yes. omap_ctrl_*() should not be exported, as mentioned earlier.
A lot of rework will be needed at some point in sound/soc/omap/ to get all of the chip- and board-integration details out of these files. DMA channels, IRQs, board data, etc. should all go into arch/arm/*omap*. plat-omap/mcbsp.c needs to be moved to somewhere under drivers/ at some point, but the platform_device setup for it should be kept under arch/arm/*omap*.
In the interim, I would suggest that you remove the the clock source and receiver source change functions from omap-mcbsp.c, split them into OMAP1 and OMAP2/3 variants, and place them into arch/arm/mach-omap*/mcbsp.c. Expand struct omap_mcbsp_ops to add function pointers to those functions. Call those from soc/omap-mcbsp.c via mcbsp->pdata->ops. That way you won't need those exports.
I don't understand how this code compiled on OMAP1 in any case, since it doesn't have a System Control Module.
- Paul
On Wed, 18 Nov 2009 03:40:40 -0700 (MST) Paul Walmsley paul@pwsan.com wrote:
In the interim, I would suggest that you remove the the clock source and receiver source change functions from omap-mcbsp.c, split them into OMAP1 and OMAP2/3 variants, and place them into arch/arm/mach-omap*/mcbsp.c. Expand struct omap_mcbsp_ops to add function pointers to those functions. Call those from soc/omap-mcbsp.c via mcbsp->pdata->ops. That way you won't need those exports.
Paul: What's your opinnion, would it be possible or would it be wise to handle these McBSP clock route setups with the clock framework instead?
Functions omap_mcbsp_dai_set_clks_src and omap_mcbsp_dai_set_rcvr_src are basically just setting up the input clock for McBSP SRG or McBSP1 receiver.
I don't understand how this code compiled on OMAP1 in any case, since it doesn't have a System Control Module.
OMAP1 can include control.h as well :-)
Access is anyway protected with the cpu_class_is_omap1().
Hi Jarkko,
On Wed, 18 Nov 2009, Jarkko Nikula wrote:
On Wed, 18 Nov 2009 03:40:40 -0700 (MST) Paul Walmsley paul@pwsan.com wrote:
In the interim, I would suggest that you remove the the clock source and receiver source change functions from omap-mcbsp.c, split them into OMAP1 and OMAP2/3 variants, and place them into arch/arm/mach-omap*/mcbsp.c. Expand struct omap_mcbsp_ops to add function pointers to those functions. Call those from soc/omap-mcbsp.c via mcbsp->pdata->ops. That way you won't need those exports.
Paul: What's your opinnion, would it be possible or would it be wise to handle these McBSP clock route setups with the clock framework instead?
Functions omap_mcbsp_dai_set_clks_src and omap_mcbsp_dai_set_rcvr_src are basically just setting up the input clock for McBSP SRG or McBSP1 receiver.
Sure. There already should be some support for it in clock34xx.h, but I doubt anyone's tested it:
static struct clk mcbsp1_fck = { .name = "mcbsp_fck", .ops = &clkops_omap2_dflt_wait, .id = 1, .init = &omap2_init_clksel_parent, .enable_reg = OMAP_CM_REGADDR(CORE_MOD, CM_FCLKEN1), .enable_bit = OMAP3430_EN_MCBSP1_SHIFT, .clksel_reg = OMAP343X_CTRL_REGADDR(OMAP2_CONTROL_DEVCONF0), .clksel_mask = OMAP2_MCBSP1_CLKS_MASK, .clksel = mcbsp_15_clksel, .clkdm_name = "core_l4_clkdm", .recalc = &omap2_clksel_recalc, };
etc. Some similar entries would need to be added to the clock24xx.h file.
I don't understand how this code compiled on OMAP1 in any case, since it doesn't have a System Control Module.
OMAP1 can include control.h as well :-)
Yeah, I guess it is this stuff in control.h that makes it work:
#define omap_ctrl_base_get() 0 #define omap_ctrl_readb(x) 0 #define omap_ctrl_readw(x) 0 #define omap_ctrl_readl(x) 0 #define omap_ctrl_writeb(x, y) WARN_ON(1) #define omap_ctrl_writew(x, y) WARN_ON(1) #define omap_ctrl_writel(x, y) WARN_ON(1)
Would be nice to get rid of that at some point.
- Paul
In the interim, I would suggest that you remove the the clock source
and
receiver source change functions from omap-mcbsp.c, split them into
OMAP1
and OMAP2/3 variants, and place them into arch/arm/mach-omap*/mcbsp.c. Expand struct omap_mcbsp_ops to add function pointers to those
functions.
Call those from soc/omap-mcbsp.c via mcbsp->pdata->ops. That way you
won't
need those exports.
Paul: What's your opinnion, would it be possible or would it be wise to handle these McBSP clock route setups with the clock framework instead?
Functions omap_mcbsp_dai_set_clks_src and omap_mcbsp_dai_set_rcvr_src are basically just setting up the input clock for McBSP SRG or McBSP1 receiver.
Sure. There already should be some support for it in clock34xx.h, but I doubt anyone's tested it:
static struct clk mcbsp1_fck = { .name = "mcbsp_fck", .ops = &clkops_omap2_dflt_wait, .id = 1, .init = &omap2_init_clksel_parent, .enable_reg = OMAP_CM_REGADDR(CORE_MOD, CM_FCLKEN1), .enable_bit = OMAP3430_EN_MCBSP1_SHIFT, .clksel_reg = OMAP343X_CTRL_REGADDR(OMAP2_CONTROL_DEVCONF0), .clksel_mask = OMAP2_MCBSP1_CLKS_MASK, .clksel = mcbsp_15_clksel, .clkdm_name = "core_l4_clkdm", .recalc = &omap2_clksel_recalc, };
etc. Some similar entries would need to be added to the clock24xx.h file.
[Aggarwal, Anuj] One problem which I found in trying the above discussed approach is that I am not able to get mcbsp clock handles in omap-mcbsp.c file. To call clk_set_parent() with the new parent clock as "mcbsp_clks" (in case of an external clock), I need the mcbsp_fck clock handle. But this handle is not shared with the omap_mcbsp_dai[].
What should be the right method of getting this handle in omap_mcbsp_dai_set_dai_sysclk(), in case I understood the concept properly. Basically, we want to get rid of the low level stuff in this function and instead use standard clock framework APIs?
-----Original Message----- From: linux-omap-owner@vger.kernel.org [mailto:linux-omap- owner@vger.kernel.org] On Behalf Of Aggarwal, Anuj Sent: Thursday, January 07, 2010 7:40 PM To: Paul Walmsley; Jarkko Nikula Cc: linux-omap@vger.kernel.org; alsa-devel@alsa-project.org; Lohithakshan, Ranjith Subject: RE: [PATCH] OMAP: Exporting functions doing common register access
In the interim, I would suggest that you remove the the clock source
and
receiver source change functions from omap-mcbsp.c, split them into
OMAP1
and OMAP2/3 variants, and place them into arch/arm/mach-
omap*/mcbsp.c.
Expand struct omap_mcbsp_ops to add function pointers to those
functions.
Call those from soc/omap-mcbsp.c via mcbsp->pdata->ops. That way you
won't
need those exports.
Paul: What's your opinnion, would it be possible or would it be wise
to
handle these McBSP clock route setups with the clock framework
instead?
Functions omap_mcbsp_dai_set_clks_src and omap_mcbsp_dai_set_rcvr_src are basically just setting up the input clock for McBSP SRG or McBSP1 receiver.
Sure. There already should be some support for it in clock34xx.h, but I doubt anyone's tested it:
static struct clk mcbsp1_fck = { .name = "mcbsp_fck", .ops = &clkops_omap2_dflt_wait, .id = 1, .init = &omap2_init_clksel_parent, .enable_reg = OMAP_CM_REGADDR(CORE_MOD, CM_FCLKEN1), .enable_bit = OMAP3430_EN_MCBSP1_SHIFT, .clksel_reg = OMAP343X_CTRL_REGADDR(OMAP2_CONTROL_DEVCONF0), .clksel_mask = OMAP2_MCBSP1_CLKS_MASK, .clksel = mcbsp_15_clksel, .clkdm_name = "core_l4_clkdm", .recalc = &omap2_clksel_recalc, };
etc. Some similar entries would need to be added to the clock24xx.h
file. [Aggarwal, Anuj] One problem which I found in trying the above discussed approach is that I am not able to get mcbsp clock handles in omap-mcbsp.c file. To call clk_set_parent() with the new parent clock as "mcbsp_clks" (in case of an external clock), I need the mcbsp_fck clock handle. But this handle is not shared with the omap_mcbsp_dai[].
What should be the right method of getting this handle in omap_mcbsp_dai_set_dai_sysclk(), in case I understood the concept properly. Basically, we want to get rid of the low level stuff in this function and instead use standard clock framework APIs?
Paul / Jarkko,
Any comments/pointers please?
-- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Anuj,
On Thu, 7 Jan 2010, Aggarwal, Anuj wrote:
In the interim, I would suggest that you remove the the clock source
and
receiver source change functions from omap-mcbsp.c, split them into
OMAP1
and OMAP2/3 variants, and place them into arch/arm/mach-omap*/mcbsp.c. Expand struct omap_mcbsp_ops to add function pointers to those
functions.
Call those from soc/omap-mcbsp.c via mcbsp->pdata->ops. That way you
won't
need those exports.
Paul: What's your opinnion, would it be possible or would it be wise to handle these McBSP clock route setups with the clock framework instead?
Functions omap_mcbsp_dai_set_clks_src and omap_mcbsp_dai_set_rcvr_src are basically just setting up the input clock for McBSP SRG or McBSP1 receiver.
Sure. There already should be some support for it in clock34xx.h, but I doubt anyone's tested it:
static struct clk mcbsp1_fck = { .name = "mcbsp_fck", .ops = &clkops_omap2_dflt_wait, .id = 1, .init = &omap2_init_clksel_parent, .enable_reg = OMAP_CM_REGADDR(CORE_MOD, CM_FCLKEN1), .enable_bit = OMAP3430_EN_MCBSP1_SHIFT, .clksel_reg = OMAP343X_CTRL_REGADDR(OMAP2_CONTROL_DEVCONF0), .clksel_mask = OMAP2_MCBSP1_CLKS_MASK, .clksel = mcbsp_15_clksel, .clkdm_name = "core_l4_clkdm", .recalc = &omap2_clksel_recalc, };
etc. Some similar entries would need to be added to the clock24xx.h file.
[Aggarwal, Anuj] One problem which I found in trying the above discussed approach is that I am not able to get mcbsp clock handles in omap-mcbsp.c file.
Why not? Looks like there are init and exit functions in there. Those would seem like logical places for clk_get()/clk_put().
To call clk_set_parent() with the new parent clock as "mcbsp_clks" (in case of an external clock), I need the mcbsp_fck clock handle. But this handle is not shared with the omap_mcbsp_dai[].
Have you considered static variable(s)?
What should be the right method of getting this handle in omap_mcbsp_dai_set_dai_sysclk(), in case I understood the concept properly. Basically, we want to get rid of the low level stuff in this function and instead use standard clock framework APIs?
Yep, the contents of both the omap_mcbsp_dai_set_clks_src() function and the omap_mcbsp_dai_set_rcvr_src() should be implementable with standard clock framework APIs.
- Paul
participants (3)
-
Aggarwal, Anuj -
Jarkko Nikula -
Paul Walmsley