LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Linus Walleij <linus.walleij@linaro.org>
To: Hongzhou Yang <hongzhou.yang@mediatek.com>
Cc: "Rob Herring" <robh+dt@kernel.org>,
	"Matthias Brugger" <matthias.bgg@gmail.com>,
	"Sascha Hauer" <kernel@pengutronix.de>,
	"Pawel Moll" <pawel.moll@arm.com>,
	"Mark Rutland" <mark.rutland@arm.com>,
	"Ian Campbell" <ijc+devicetree@hellion.org.uk>,
	"Kumar Gala" <galak@codeaurora.org>,
	"Russell King" <linux@arm.linux.org.uk>,
	"Grant Likely" <grant.likely@linaro.org>,
	"Joe.C" <yingjoe.chen@mediatek.com>,
	"Heiko Stübner" <heiko@sntech.de>,
	"Catalin Marinas" <catalin.marinas@arm.com>,
	"Vladimir Murzin" <vladimir.murzin@arm.com>,
	"Ashwin Chaugule" <ashwin.chaugule@linaro.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"huang eddie" <eddie.huang@mediatek.com>,
	dandan.he@mediatek.com, alan.cheng@mediatek.com,
	toby.liu@mediatek.com
Subject: Re: [PATCH v3 1/3] ARM: mediatek: Add Pinctrl/GPIO driver for mt8135.
Date: Thu, 27 Nov 2014 10:14:49 +0100	[thread overview]
Message-ID: <CACRpkdY5cen-GicF6tuDaXzJXGD3DZUYODQEgpKCWoLP-MDBsA@mail.gmail.com> (raw)
In-Reply-To: <1415709535-31515-2-git-send-email-hongzhou.yang@mediatek.com>

()On Tue, Nov 11, 2014 at 1:38 PM, Hongzhou Yang
<hongzhou.yang@mediatek.com> wrote:

> From: Hongzhou Yang <hongzhou.yang@mediatek.com>
>
> The mediatek SoCs have GPIO controller that handle both the muxing and GPIOs.
>
> The GPIO controller have pinmux, pull enable, pull select, direction and output high/low control.
>
> This driver include common driver and mt8135 part.
> The common driver include the pinctrl driver and GPIO driver.
> The mt8135 part contain its special device data.
>
> Signed-off-by: Hongzhou Yang <hongzhou.yang@mediatek.com>
(...)
>  menuconfig ARCH_MEDIATEK
>         bool "Mediatek MT65xx & MT81xx SoC" if ARCH_MULTI_V7
>         select ARM_GIC
> +       select PINCTRL

Should it not also select PINCTRL_MTK8135 for the right SoC?

(...)
> +++ b/drivers/pinctrl/mediatek/Kconfig
> @@ -0,0 +1,12 @@
> +if ARCH_MEDIATEK
> +
> +config PINCTRL_MTK_COMMON
> +       bool
> +       select PINMUX
> +       select GENERIC_PINCONF

This is also a GPIO driver but it fails to select GPIOLIB,
OF_GPIO and also possibly GPIOLIB_IRQCHIP.

(...)
> +static int mtk_pctrl_dt_node_to_map_config(struct mtk_pinctrl *pctl, u32 pin,
> +               unsigned long *configs, unsigned num_configs,
> +               struct pinctrl_map **map, unsigned *cnt_maps,
> +               unsigned *num_maps)
> +{
> +       struct mtk_pinctrl_group *grp;
> +       unsigned long *cfgs;
> +
> +       if (*num_maps == *cnt_maps)
> +               return -ENOSPC;
> +
> +       cfgs = kmemdup(configs, num_configs * sizeof(*cfgs),
> +                      GFP_KERNEL);
> +       if (cfgs == NULL)
> +               return -ENOMEM;
> +
> +       grp = mtk_pctrl_find_group_by_pin(pctl, pin);
> +       if (!grp) {
> +               dev_err(pctl->dev, "unable to match pin %d to group\n", pin);
> +               return -EINVAL;
> +       }
> +
> +       (*map)[*num_maps].type = PIN_MAP_TYPE_CONFIGS_GROUP;
> +       (*map)[*num_maps].data.configs.group_or_pin = grp->name;
> +       (*map)[*num_maps].data.configs.configs = cfgs;
> +       (*map)[*num_maps].data.configs.num_configs = num_configs;
> +       (*num_maps)++;
> +
> +       return 0;
> +}

Can't this use pinctrl_utils_add_map_configs()?

> +static void mtk_pctrl_dt_free_map(struct pinctrl_dev *pctldev,
> +                                   struct pinctrl_map *map,
> +                                   unsigned num_maps)
> +{
> +       int i;
> +
> +       for (i = 0; i < num_maps; i++) {
> +               if (map[i].type == PIN_MAP_TYPE_CONFIGS_GROUP)
> +                       kfree(map[i].data.configs.configs);
> +       }
> +
> +       kfree(map);
> +}

Use pinctrl_utils_dt_free_map() instead.

> +static int mtk_dt_cnt_map(struct pinctrl_map **map, unsigned *cnt_maps,
> +               unsigned *num_maps, unsigned cnt)
> +{
> +       unsigned old_num = *cnt_maps;
> +       unsigned new_num = *num_maps + cnt;
> +       struct pinctrl_map *new_map;
> +
> +       if (old_num >= new_num)
> +               return 0;
> +
> +       new_map = krealloc(*map, sizeof(*new_map) * new_num, GFP_KERNEL);
> +       if (!new_map)
> +               return -ENOMEM;
> +
> +       memset(new_map + old_num, 0, (new_num - old_num) * sizeof(*new_map));
> +
> +       *map = new_map;
> +       *cnt_maps = new_num;
> +
> +       return 0;
> +}

Use pinctrl_utils_reserve_map() instead.

> +static int mtk_pctrl_dt_subnode_to_map(struct pinctrl_dev *pctldev,
> +                                     struct device_node *node,
> +                                     struct pinctrl_map **map,
> +                                     unsigned *num_maps, unsigned *cnt_maps)
> +{
> +       struct property *pins;
> +       u32 pinfunc, pin, func;
> +       int num_pins, num_funcs, maps_per_pin;
> +       unsigned long *configs;
> +       unsigned int num_configs;
> +       bool has_config = 0;
> +       int i, err;
> +       unsigned cnt = 0;
> +       struct mtk_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
> +
> +       pins = of_find_property(node, "mediatek,pins", NULL);
> +       if (!pins) {
> +               dev_err(pctl->dev, "missing mediatek,pins property in node %s .\n",
> +                               node->name);
> +               return -EINVAL;
> +       }
> +
> +       err = pinconf_generic_parse_dt_config(node, &configs, &num_configs);
> +       if (num_configs)
> +               has_config = 1;

I'd prefer we first identify if it's a config or mux subnode, then go on to
parse it as mux or config. See comments on patch 2/3.

> +
> +       num_pins = pins->length / sizeof(u32);
> +       num_funcs = num_pins;
> +       maps_per_pin = 0;
> +       if (num_funcs)
> +               maps_per_pin++;
> +       if (has_config && num_pins >= 1)
> +               maps_per_pin++;
> +
> +       if (!num_pins || !maps_per_pin)
> +               return -EINVAL;
> +
> +       cnt = num_pins * maps_per_pin;
> +
> +       err = mtk_dt_cnt_map(map, cnt_maps, num_maps, cnt);
> +       if (err < 0)
> +               goto fail;
> +
> +       for (i = 0; i < num_pins; i++) {
> +               err = of_property_read_u32_index(node, "mediatek,pins",
> +                               i, &pinfunc);

As mentioned use just "pins" and let's figure out how to handle
this in a generic way.

> +static int mtk_gpio_request(struct gpio_chip *chip, unsigned offset)
> +{
> +       return pinctrl_request_gpio(chip->base + offset);
> +}
> +
> +static void mtk_gpio_free(struct gpio_chip *chip, unsigned offset)
> +{
> +       pinctrl_free_gpio(chip->base + offset);
> +}
> +
> +static int mtk_gpio_direction_input(struct gpio_chip *chip,
> +                                       unsigned offset)
> +{
> +       return pinctrl_gpio_direction_input(chip->base + offset);
> +}
> +
> +static int mtk_gpio_direction_output(struct gpio_chip *chip,
> +                                       unsigned offset, int value)
> +{
> +       mtk_gpio_set(chip, offset, value);
> +       return pinctrl_gpio_direction_output(chip->base + offset);
> +}

This is kinda nice!

> +static int mtk_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> +{
> +       struct mtk_pinctrl *pctl = dev_get_drvdata(chip->dev);
> +       struct mtk_pinctrl_group *g = pctl->groups + offset;
> +       struct mtk_desc_function *desc =
> +                       mtk_pctrl_desc_find_irq_function_from_name(
> +                                       pctl, g->name);
> +       if (!desc)
> +               return -EINVAL;
> +
> +       return desc->irqnum;
> +}

I don't quite get this still. Does this mean every single GPIO line
potentially has it's own unique IRQ line?

Yours,
Linus Walleij

      parent reply	other threads:[~2014-11-27  9:15 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <1415709535-31515-1-git-send-email-hongzhou.yang@mediatek.com>
2014-11-18 16:24 ` [PATCH v3 0/3] Add Mediatek SoC Pinctrl/GPIO driver for MT8135 Sascha Hauer
     [not found] ` <1415709535-31515-3-git-send-email-hongzhou.yang@mediatek.com>
2014-11-27  8:44   ` [PATCH v3 2/3] dt-bindings: Add pinctrl bindings for mt65xx/mt81xx Linus Walleij
2014-11-27 10:18     ` Sascha Hauer
2014-11-28 16:12       ` Linus Walleij
2014-12-02 13:55         ` Sascha Hauer
2015-01-10 21:33           ` Linus Walleij
2015-01-12 12:22             ` Sascha Hauer
2015-01-13 10:05               ` Linus Walleij
2015-01-13 16:16                 ` Sascha Hauer
2015-01-13 16:24                   ` Jean-Christophe PLAGNIOL-VILLARD
2015-01-16  9:53                   ` Linus Walleij
2015-01-16 10:23                     ` Yingjoe Chen
2015-01-20  9:45                       ` Linus Walleij
2015-01-26 15:57                         ` Sascha Hauer
2015-01-27 14:07                           ` Linus Walleij
     [not found] ` <1415709535-31515-2-git-send-email-hongzhou.yang@mediatek.com>
2014-11-27  9:14   ` Linus Walleij [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CACRpkdY5cen-GicF6tuDaXzJXGD3DZUYODQEgpKCWoLP-MDBsA@mail.gmail.com \
    --to=linus.walleij@linaro.org \
    --cc=alan.cheng@mediatek.com \
    --cc=ashwin.chaugule@linaro.org \
    --cc=catalin.marinas@arm.com \
    --cc=dandan.he@mediatek.com \
    --cc=devicetree@vger.kernel.org \
    --cc=eddie.huang@mediatek.com \
    --cc=galak@codeaurora.org \
    --cc=grant.likely@linaro.org \
    --cc=heiko@sntech.de \
    --cc=hongzhou.yang@mediatek.com \
    --cc=ijc+devicetree@hellion.org.uk \
    --cc=kernel@pengutronix.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@arm.linux.org.uk \
    --cc=mark.rutland@arm.com \
    --cc=matthias.bgg@gmail.com \
    --cc=pawel.moll@arm.com \
    --cc=robh+dt@kernel.org \
    --cc=toby.liu@mediatek.com \
    --cc=vladimir.murzin@arm.com \
    --cc=yingjoe.chen@mediatek.com \
    --subject='Re: [PATCH v3 1/3] ARM: mediatek: Add Pinctrl/GPIO driver for mt8135.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).