> (...)config PINCTRL_MVEBU
bool "Marvell SoC pin controller drivers"
- depends on ARCH_MVEBU
+ depends on ARCH_MVEBU || ARCH_DOVE
Hmmmm.
Shouldn't this thing rather be:
depends on PLAT_ORION
else you end up lining up all Marvell machines here...
+#define DOVE_AU0_AC97_SEL (1<< 16)
It is way cleaner to say:
#include<linux/bitops.h>
#define DOVE_AU0_AC97_SEL BIT(16)
Same comments about the magic numbers as in the review of the first
patch.
+ if (ctrl->pid == 24)
+ mask = DOVE_CAM_GPIO_SEL;
+ else if (ctrl->pid == 40)
+ mask = DOVE_SD0_GPIO_SEL;
+ else if (ctrl->pid == 46)
+ mask = DOVE_SD1_GPIO_SEL;
+ else if (ctrl->pid == 58)
+ mask = DOVE_SPI_GPIO_SEL;
+ else if (ctrl->pid == 62)
+ mask = DOVE_UART1_GPIO_SEL;
+ else
+ return -EINVAL;
Magic numbers, atleast provide a comment on what is so special about
these pins, and can you use a switch() statement instead?
+static int dove_audio1_ctrl_gpio_req(struct mvebu_mpp_ctrl *ctrl, u8 pid)
+{
+ unsigned long config;
+
+ dove_audio1_ctrl_get(ctrl,&config);
+
+ switch (config) {
+ /* mode 0x00 i2s1/spdifo : no gpio */
+ /* mode 0x0c ssp/spdifo : no gpio */
+ /* mode 0x0f ssp/twsi : no gpio */
+ case 0x00:
+ case 0x0c:
+ case 0x0f:
+ return -ENOTSUPP;
Nice with the comments (also before the function),
now I understand what's happening, this is pretty OK,
I would have put the comments on the same
line as the magic number but whatever.
However you could just delete these lines and let them
fall through to -ENOTSUPP at the bottom.
+/* mpp[52:57] has gpio pins capable of in and out */
+static int dove_audio1_ctrl_gpio_dir(struct mvebu_mpp_ctrl *ctrl, u8 pid,
+ bool input)
+{
+ return 0;
+}
So should this not return a -ENOTSUPP for
pin< 52 || pin> 57?
+ /*
+ * General MPP Configuration Register is part of pdma registers.
+ * grab clk to make sure it is clocked.
+ */
+ pdma_clk = clk_get_sys("dove-pdma", NULL);
First there is something really weird with the name of this
clock. Atleast call the variable holding the clk something
apropriate like just "clk".
Second why are you fetching a sys clock instead of using the clkdev
device binding in the clock tree so you could say:
clk = clk_get(&pdev->dev, NULL);
+static int __devexit dove_pinctrl_remove(struct platform_device *pdev)
+{
+ int ret;
+
+ ret = mvebu_pinctrl_remove(pdev);
+ if (!IS_ERR(pdma_clk)) {
+ clk_disable_unprepare(pdma_clk);
+ clk_put(pdma_clk);
+ }
You don't need this (!IS_ERR()) clause, look in yout probe function:
you bail out if the clock is not found.