Messages in this thread | | | Date | Thu, 13 Feb 2014 17:59:14 +0100 | From | Thomas Petazzoni <> | Subject | Re: [PATCH v3 00/13] pinctrl: mvebu: restructure resource allocation |
| |
Dear Sebastian Hesselbarth,
On Thu, 13 Feb 2014 17:41:02 +0100, Sebastian Hesselbarth wrote:
> > Thanks again for working on this! I have boot tested this successfully > > on an Armada XP platform, and it seems to behave normally, the debugfs > > pinctrl contents make sense. > > I guess this is a Tested-by ?
Yes. My tests were admittedly fairly light, but I believe good enough :)
> > I am not sure what you mean here in terms of the ordering for the > > patches. I'm attaching several patches, and the first three patches > > adapt your patch series to also cover 375 and 38x, assuming the pinctrl > > support for 375 and 38x is merged before your patch series. > > Right. If 375/38x pinctrl goes in first (what I expect), I'd have to add > corresponding patches. You already sent them, I'll pick them up.
Ok, cool. Hopefully we can sort out the merging of those two patch series for 3.15 with Linus Walleij.
> > I must say I dislike quite a bit this unnamed mpp controls mechanism. > > Why isn't the name statically defined in the source code by the > > MPP_MODE macro, which already takes as first argument the pin number? > > Honestly, the unnamed mpp control thing is a bit odd. But if you tell > me how to create ~60 statically defined one pin groups out of a > single-line macro, we can change that easily. > > Back when that unnamed mpp control thing was invented, I must have been > to lazy to write e.g. > > MPP_FUNC_CTRL(0, 0, "mpp0", armada_xp_mpp_ctrl), > MPP_FUNC_CTRL(1, 1, "mpp1", armada_xp_mpp_ctrl), > MPP_FUNC_CTRL(2, 2, "mpp2", armada_xp_mpp_ctrl), > ... > MPP_FUNC_CTRL(66, 66, "mpp66", armada_xp_mpp_ctrl), > > instead of > > MPP_FUNC_CTRL(0, 66, NULL, armada_xp_mpp_ctrl), > > and generate the 66 names dynamically.
Right. But what I meant is that we already have a place where we have one macro call for each pin: when defining the MPP modes. So I was thinking of simplifying the whole stuff by "merging" the notion of MPP control with the notion of MPP mode. This way, when you do:
MPP_MODE(0, MPP_FUNCTION(...), MPP_FUNCTION(...)), MPP_MODE(1, MPP_FUNCTION(...), MPP_FUNCTION(...)), MPP_MODE(2, MPP_FUNCTION(...), MPP_FUNCTION(...)), [...] MPP_MODE(65, MPP_FUNCTION(...), MPP_FUNCTION(...)),
You can take this opportunity to generate:
{ "mpp0", ... }, { "mpp1", ... }, { "mpp2", ... }, ... { "mpp65", ... },
> > This is definitely good, but I'm wondering why the core cannot provide > > helper functions for the generic case where we have 4 bits per pin in > > contiguous registers. This would avoid duplicating the helper function > > six times (you have four in your patch series, and we'll need two more > > for A375 and A38x). > > I thought about it too, but we would need a soc specific callback > anyway as you'll have to pass the base address somehow (and that is now > known by soc specific stub only). My quick rule of thumb was that the > amount of code replication would be almost the same.
In pinctrl-mvebu.h, we could have:
static inline int default_mpp_ctrl_get(void __iomem *base, unsigned int pid, unsigned long *config) { unsigned off = (pid / MVEBU_MPPS_PER_REG) * MVEBU_MPP_BITS; unsigned shift = (pid % MVEBU_MPPS_PER_REG) * MVEBU_MPP_BITS;
*config = (readl(base + off) >> shift) & MVEBU_MPP_MASK;
return 0; }
static inline int default_mpp_ctrl_set(void __iomem *base, unsigned int pid, unsigned long config) { unsigned off = (pid / MVEBU_MPPS_PER_REG) * MVEBU_MPP_BITS; unsigned shift = (pid % MVEBU_MPPS_PER_REG) * MVEBU_MPP_BITS; unsigned long reg;
reg = readl(base + off) & ~(MVEBU_MPP_MASK << shift); writel(reg | (config << shift), base + off);
return 0; }
which would slightly reduce the per-SoC code to:
static int armada_370_mpp_ctrl_get(unsigned pid, unsigned long *config) { return default_mpp_ctrl_get(mpp_base, pid, config); }
static int armada_370_mpp_ctrl_set(unsigned pid, unsigned long config) { return default_mpp_ctrl_set(mpp_base, pid, config); }
but we admittedly cannot completely remove the per-SoC function, since the mpp_base is now only known to each per-SoC driver.
> > Feel free to squash these patches into the appropriate patches. > > Yep, thanks for these! I'll squash them in and send an updated v4 as > soon as the discussion here stalls.
Thanks!
Thomas -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
| |