| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CACRpkdY5cen-GicF6tuDaXzJXGD3DZUYODQEgpKCWoLP-MDBsA@mail.gmail.com>
Date: Thu, 27 Nov 2014 10:14:49 +0100
From: Linus Walleij <linus.walleij@...aro.org>
To: Hongzhou Yang <hongzhou.yang@...iatek.com>
Cc: Rob Herring <robh+dt@...nel.org>,
Matthias Brugger <matthias.bgg@...il.com>,
Sascha Hauer <kernel@...gutronix.de>,
Pawel Moll <pawel.moll@....com>,
Mark Rutland <mark.rutland@....com>,
Ian Campbell <ijc+devicetree@...lion.org.uk>,
Kumar Gala <galak@...eaurora.org>,
Russell King <linux@....linux.org.uk>,
Grant Likely <grant.likely@...aro.org>,
"Joe.C" <yingjoe.chen@...iatek.com>,
Heiko Stübner <heiko@...ech.de>,
Catalin Marinas <catalin.marinas@....com>,
Vladimir Murzin <vladimir.murzin@....com>,
Ashwin Chaugule <ashwin.chaugule@...aro.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
huang eddie <eddie.huang@...iatek.com>, dandan.he@...iatek.com,
alan.cheng@...iatek.com, toby.liu@...iatek.com
Subject: Re: [PATCH v3 1/3] ARM: mediatek: Add Pinctrl/GPIO driver for mt8135.
()On Tue, Nov 11, 2014 at 1:38 PM, Hongzhou Yang
<hongzhou.yang@...iatek.com> wrote:
> From: Hongzhou Yang <hongzhou.yang@...iatek.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@...iatek.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists