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:2.853 / U:0.060 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site