Re: [PATCH 02/11] pinctrl: mvebu: dove pinctrl driver

[Linus Walleij <linus.walleij@linaro.org>]

On Sat, Aug 11, 2012 at 2:56 PM, Sebastian Hesselbarth
<sebastian.hesselbarth@gmail.com> wrote:

(...)
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> @@ -147,7 +147,7 @@ config PINCTRL_COH901
>
>  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...

>         select PINMUX
>         select PINCONF
>         help
> @@ -155,6 +155,13 @@ config PINCTRL_MVEBU
>           This is only the driver core and additionally needs a SoC specific
>           driver.
>
> +config PINCTRL_DOVE
> +       bool "Support for Marvell Dove SoCs"
> +       depends on PINCTRL_MVEBU

Shouldn't this rather be:

depends on ARCH_DOVE
select PINCTRL_MVEBU

since you call down into the mvebu driver, it definatley needs
to select that.

(...)
> diff --git a/drivers/pinctrl/pinctrl-dove.c b/drivers/pinctrl/pinctrl-dove.c

> +#define  DOVE_AU0_AC97_SEL             (1 << 16)

It is way cleaner to say:

#include <linux/bitops.h>

#define  DOVE_AU0_AC97_SEL             BIT(16)

(...)
> +static int dove_pmu_mpp_ctrl_get(struct mvebu_mpp_ctrl *ctrl,
> +                                unsigned long *config)
> +{
> +       unsigned off = (ctrl->pid / 8) * 4;
> +       unsigned shift = (ctrl->pid % 8) * 4;
> +       unsigned long pmu = readl(DOVE_PMU_MPP_GENERAL_CTRL);
> +       unsigned long mpp = readl(DOVE_MPP_VIRT_BASE + off);
> +
> +       if (pmu & (1 << ctrl->pid))
> +               *config = 0x10;
> +       else
> +               *config = (mpp >> shift) & 0xf;
> +       return 0;
> +}

Same comments about the magic numbers as in the review of the first
patch.

> +static int dove_pmu_mpp_ctrl_set(struct mvebu_mpp_ctrl *ctrl,
> +                                unsigned long config)
> +{
> +       unsigned off = (ctrl->pid / 8) * 4;
> +       unsigned shift = (ctrl->pid % 8) * 4;
> +       unsigned long pmu = readl(DOVE_PMU_MPP_GENERAL_CTRL);
> +       unsigned long mpp = readl(DOVE_MPP_VIRT_BASE + off);
> +
> +       if (config == 0x10)
> +               writel(pmu | (1 << ctrl->pid), DOVE_PMU_MPP_GENERAL_CTRL);
> +       else {
> +               writel(pmu & ~(1 << ctrl->pid), DOVE_PMU_MPP_GENERAL_CTRL);
> +               mpp &= ~(0xf << shift);
> +               mpp |= config << shift;
> +               writel(mpp, DOVE_MPP_VIRT_BASE + off);
> +       }
> +       return 0;
> +}

Dito.

> +static int dove_mpp4_ctrl_get(struct mvebu_mpp_ctrl *ctrl,
> +                             unsigned long *config)
> +{
> +       unsigned long mpp4 = readl(DOVE_MPP_CTRL4_VIRT_BASE);
> +       unsigned long mask;
> +
> +       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_mpp4_ctrl_set(struct mvebu_mpp_ctrl *ctrl,
> +                             unsigned long config)
> +{
> +       unsigned long mpp4 = readl(DOVE_MPP_CTRL4_VIRT_BASE);
> +       unsigned long mask;
> +
> +       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;

Dito.

(...)
> +static int dove_audio1_ctrl_get(struct mvebu_mpp_ctrl *ctrl,
> +                               unsigned long *config)
> +{
> +       unsigned long mpp4 = readl(DOVE_MPP_CTRL4_VIRT_BASE);
> +       unsigned long sspc1 = readl(DOVE_SSP_CTRL_STATUS_1);
> +       unsigned long gmpp = readl(DOVE_MPP_GENERAL_VIRT_BASE);
> +       unsigned long gcfg2 = readl(DOVE_GLOBAL_CONFIG_2);
> +
> +       *config = 0;
> +       if (mpp4 & DOVE_AU1_GPIO_SEL)
> +               *config |= 0x8;
> +       if (sspc1 & DOVE_SSP_ON_AU1)
> +               *config |= 0x4;
> +       if (gmpp & DOVE_AU1_SPDIFO_GPIO_EN)
> +               *config |= 0x2;
> +       if (gcfg2 & DOVE_TWSI_OPTION3_GPIO)
> +               *config |= 0x1;

More magic, but the flags make it clear what the set
bits are all about, so OK.

But I would have:
#include <linux/bitops.h>

*config |= BIT(0);

etc.

> +       /* SSP/TWSI only if I2S1 not set*/
> +       if ((*config & 0x8) == 0)
> +               *config &= ~(0x4 | 0x1);
> +       /* TWSI only if SPDIFO not set*/
> +       if ((*config & 0x2) == 0)
> +               *config &= ~(0x1);
> +       return 0;

And that applies to these.

> +static int dove_audio1_ctrl_set(struct mvebu_mpp_ctrl *ctrl,
> +                               unsigned long config)
> +{
> +       unsigned long mpp4 = readl(DOVE_MPP_CTRL4_VIRT_BASE);
> +       unsigned long sspc1 = readl(DOVE_SSP_CTRL_STATUS_1);
> +       unsigned long gmpp = readl(DOVE_MPP_GENERAL_VIRT_BASE);
> +       unsigned long gcfg2 = readl(DOVE_GLOBAL_CONFIG_2);
> +
> +       if (config & 0x1)
> +               gcfg2 |= DOVE_TWSI_OPTION3_GPIO;
> +       if (config & 0x2)
> +               gmpp |= DOVE_AU1_SPDIFO_GPIO_EN;
> +       if (config & 0x4)
> +               sspc1 |= DOVE_SSP_ON_AU1;
> +       if (config & 0x8)
> +               mpp4 |= DOVE_AU1_GPIO_SEL;

Dito.

> +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.

> +
> +       /* mode 0x02 i2s1 : gpio[56:57] */
> +       /* mode 0x0e ssp  : gpio[56:57] */
> +       case 0x02:
> +       case 0x0e:
> +               if (pid >= 56)
> +                       return 0;
> +
> +       /* mode 0x08 spdifo : gpio[52:55] */
> +       /* mode 0x0b twsi   : gpio[52:55] */
> +       case 0x08:
> +       case 0x0b:
> +               if (pid <= 55)
> +                       return 0;
> +
> +       /* mode 0x0a/gpio : all gpio */
> +       case 0x0a:
> +               return 0;
> +       }
> +
> +       return -ENOTSUPP;
> +}

> +/* 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?

(...)
> +static int __devinit dove_pinctrl_probe(struct platform_device *pdev)
> +{
> +       const struct of_device_id *match =
> +               of_match_device(dove_pinctrl_of_match, &pdev->dev);
> +       int ret;
> +
> +       if (match)
> +               pdev->dev.platform_data = match->data;
> +
> +       /*
> +        * 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);

> +       clk_prepare_enable(pdma_clk);

Good!

> +       ret = mvebu_pinctrl_probe(pdev);
> +       if (!ret)
> +               return 0;
> +
> +       clk_disable_unprepare(pdma_clk);
> +       clk_put(pdma_clk);
> +       return ret;
> +}
> +
> +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.

The rest looks good... atleast now, but I'm low on coffee FTM.

Yours,
Linus Walleij