lkml.org 
[lkml]   [2012]   [Aug]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH 02/11] pinctrl: mvebu: dove pinctrl driver
From
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


\
 
 \ /
  Last update: 2012-08-20 16:21    [W:0.458 / U:28.960 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site