Messages in this thread | | | Date | Wed, 24 Oct 2012 14:31:18 +0200 | From | Davide Ciminaghi <> | Subject | Re: [PATCH 02/10] drivers/mfd/sta2x11-mfd: add regmap support |
| |
On Tue, Oct 23, 2012 at 06:18:38PM +0100, Mark Brown wrote: > On Mon, Oct 22, 2012 at 04:50:33PM +0200, ciminaghi@gnudd.com wrote: > > > +static bool sta2x11_sctl_writeable_reg(struct device *dev, unsigned int reg) > > +{ > > + return !__reg_within_range(reg, SCTL_SCPCIECSBRST, SCTL_SCRSTSTA); > > +} > > This and most of your other readable/writable things look like a > framework feature waiting to be written - something data driven which > takes a table of register ranges and goes and does the > __reg_within_range() check on them. Seems like it'd be really useful > for devices like this. > I was looking at other drivers with regmap support, and it actually looks like this __reg_within_range (or similar) thing is fairly common. For instance sound/soc/tegra/tegra30_ahub.c:
#define REG_IN_ARRAY(reg, name) \ ((reg >= TEGRA30_AHUB_##name) && \ (reg <= LAST_REG(name) && \ (!((reg - TEGRA30_AHUB_##name) % TEGRA30_AHUB_##name##_STRIDE))))
also used for precious and volatile registers.
sound/soc/tegra/tegra20_das.c:
static bool tegra20_das_wr_rd_reg(struct device *dev, unsigned int reg) { if ((reg >= TEGRA20_DAS_DAP_CTRL_SEL) && (reg <= LAST_REG(DAP_CTRL_SEL))) return true; if ((reg >= TEGRA20_DAS_DAC_INPUT_DATA_CLK_SEL) && (reg <= LAST_REG(DAC_INPUT_DATA_CLK_SEL))) return true;
return false; }
My opinion is that passing function pointers for readable/writeable/precious/volatile methods could still be useful when registers' features or access properties can change at runtime (for instance a given register is readable/writeable in working mode X and becomes read-only when the device switches to mode Y). Other than that, four tables could just be passed via struct regmap_config. regmap_writeable/readable/precious/volatile would then invoke a (regmap private) _regmap_reg_in_ranges() function which would do the check based on the correct range table. Things would work just like now in case of NULL table pointers.
I am planning to submit a regmap patch in the next days (actually I've already written something, but it is completely untested). Since sta2x11-mfd is blocking the rest of our work on the Connext chip, though, I think the best thing would be for me to keep things as they are now, and then doing this improvement later on, if you agree.
> > +static bool sta2x11_apb_soc_regs_writeable_reg(struct device *dev, > > + unsigned int reg) > > +{ > > + if (!sta2x11_apb_soc_regs_readable_reg(dev, reg)) > > + return false; > > + return (!__reg_within_range(reg, PCIE_PM_STATUS_0_PORT_0_4, > > + PCIE_PM_STATUS_7_0_EP4) && > > + reg != PCIE_COMMON_CLOCK_CONFIG_0_4_0 && > > + !__reg_within_range(reg, PCIE_SoC_INT_ROUTER_STATUS0_REG, > > + PCIE_SoC_INT_ROUTER_STATUS3_REG) && > > + reg != SYSTEM_CONFIG_STATUS_REG && > > + reg != COMPENSATION_REG1); > > For this I'd write a switch statement with the range checks in the > default: case. Actually you could just use the GCC switch range > feature: > > case PCIE_PM_STATUS_0_PORT_0_4..PCIE_PM_STATUS_7_0_EP4: > > Either of these would increase readability. > ok, will do that immediately.
> but generally > > Reviewed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
Thanks a lot for your time.
Regards Davide
| |