[PATCH v2 01/13] mfd: pruss mfd driver.

On 02/22/2011 11:31 AM, Samuel Ortiz wrote:

Hi Subhasish,

On Tue, Feb 22, 2011 at 11:13:38AM +0530, Subhasish Ghosh wrote:

quoted

Thank you for your comments.

No problem.

quoted

quoted

quoted

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index fd01836..6c437df 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig

@@ -81,6 +81,16 @@ config MFD_DM355EVM_MSP
  boards.  MSP430 firmware manages resets and power sequencing,
  inputs from buttons and the IR remote, LEDs, an RTC, and more.

+config MFD_DA8XX_PRUSS
+ tristate "Texas Instruments DA8XX PRUSS support"
+ depends on ARCH_DAVINCI && ARCH_DAVINCI_DA850

Why are we depending on those ?

SG -- The PRUSS core in only available within DA850 and DA830,
           DA830 support is not yet implemented.

Sure, but if there are no actual code dependencies, I'd like to get rid of
those depends.

quoted

quoted

quoted

+u32 pruss_disable(struct device *dev, u8 pruss_num)
+{
+ struct da8xx_pruss *pruss = dev_get_drvdata(dev->parent);
+ da8xx_prusscore_regs h_pruss;
+ u32 temp_reg;
+
+ if (pruss_num == DA8XX_PRUCORE_0) {
+ /* Disable PRU0  */
+ h_pruss = (da8xx_prusscore_regs)
+ ((u32) pruss->ioaddr + 0x7000);

So it seems you're doing this in several places, and I have a few
comments:

- You don't need the da8xx_prusscore_regs at all.
- Define the register map through a set of #define in your header file.
- Use a static routine that takes the core number and returns the
register map
offset.

Then routines like this one will look a lot more readable.

SG -- There are a huge number of PRUSS registers. A lot of them are
reserved and are expected to change as development on the
           controller is still ongoing. 

First of all, from what I read in your patch you're only using the CONTROL
offset.

quoted

If we use #defines to plot
all the registers, then first, there are too many array type
registers which will need to be duplicated.

What I'm expecting is a small set of defines for the register offsets. You
have 13 fields in your da8xx_prusscore_regs, you only need to define 13
register offsets.

So, if you have a:

static u32 reg_offset(struct device *dev, u8 pru_num)
{
	struct da8xx_pruss *pru = dev_get_drvdata(dev->parent);

	switch (pru_num) {
	case DA8XX_PRUCORE_0:
		return (u32) pru->ioaddr + 0x7000;
	case DA8XX_PRUCORE_1:
		return (u32) pru->ioaddr + 0x7800;
	default:
		return 0;
}


then routines like pruss_enable (which should return an int, btw) would look
like:

int pruss_enable(struct device *dev, u8 pruss_num)
{
	u32 offset = reg_offset(dev, pruss_num);

	if (offset == 0)
		return -EINVAL;

	__raw_writel(DA8XX_PRUCORE_CONTROL_RESETVAL,
			offset + PRU_CORE_CONTROL);

	return 0;
}

All registers are memory mapped and could nicely be described by
structures (and sub-structures). Therefore we asked to considerer
structs, at least for the Pruss SocketCAN drivers. That would result in
much much clearer and better readable code. The code above would shrink to:

	__raw_writel(DA8XX_PRUCORE_CONTROL_RESETVAL,
		     &prucore[pruss_num].control);

And proper type checking would be ensured as well.

Wolfgang.