[PATCH v3 1/7] mfd: add pruss mfd driver.
From: Subhasish Ghosh <hidden>
Date: 2011-03-30 07:13:42
Also in:
lkml
quoted
+s32 pruss_disable(struct device *dev, u8 pruss_num) +{ + struct da8xx_pruss *pruss = dev_get_drvdata(dev->parent); + struct da8xx_prusscore_regs *h_pruss; + struct pruss_map *pruss_mmap = (struct pruss_map *)pruss->ioaddr; + u32 temp_reg; + u32 delay_cnt;Can you explain the significance of pruss_num? As far as I can tell, you always pass constants in here, so it should be possible to determine the number from the device.
SG - The number is not programmed in the device, I need something to decide which PRU to disable or enable.
quoted
+ ~DA8XX_PRUCORE_CONTROL_COUNTENABLE_MASK) | + ((DA8XX_PRUCORE_CONTROL_COUNTENABLE_DISABLE << + DA8XX_PRUCORE_CONTROL_COUNTENABLE_SHIFT) & + DA8XX_PRUCORE_CONTROL_COUNTENABLE_MASK); + __raw_writel(temp_reg, &h_pruss->CONTROL);Better use readl/writel, the __raw_ variants are not reliable in general.
SG - Have changed this to iowrite32 and variants.
quoted
+ + /* Reset PRU0 */ + for (delay_cnt = 0x10000; delay_cnt > 0; delay_cnt--) + __raw_writel(DA8XX_PRUCORE_CONTROL_RESETVAL, + &h_pruss->CONTROL);Why do you need to reset it 65536 times? Is once not enough?
SG - I am not sure why this was done, but I have removed it now.
quoted
+ } + spin_unlock(&pruss->lock);This is almost the exact same code as for the DA8XX_PRUCORE_0 case. Please be a little more creative in order to avoid such code duplication.
SG - Ok, cleanup done.
quoted
+ + for (loop = 0; loop < bytestowrite; loop++) + __raw_writeb(*pdatatowrite++, paddresstowrite++); + + return 0; +} +EXPORT_SYMBOL(pruss_writeb);I would recommend providing a simpler variant of your all I/O accessors, which write a single word. Most of the users of these simply pass bytestowrite=1 anyway, so the caller can become more readable.
SG - At some sections in the code I am using upto 8 bytescount.
If its ok, I would want to keep it as is.
Also, my comments about __raw_* and Marc's comments about the type cast apply to all of these.
SG - Changed to iowrite32 variants and also used make C=1
quoted
+ err = mfd_add_devices(dev, 0, &cell, 1, NULL, 0); + if (err) { + dev_err(dev, "cannot add mfd cells\n"); + return err; + } + dev_info(dev, "mfd: added %s device\n", + dev_data[count].dev_name); + } + + return err; +}This would get much simpler if you just replaced the da8xx_pruss_devices array with an mfd_cell array.
SG - Will do.