From mboxrd@z Thu Jan 1 00:00:00 1970 From: hongzhou.yang@mediatek.com (hongzhou yang) Date: Fri, 28 Nov 2014 13:06:58 +0800 Subject: [PATCH v3 1/3] ARM: mediatek: Add Pinctrl/GPIO driver for mt8135. In-Reply-To: References: <1415709535-31515-1-git-send-email-hongzhou.yang@mediatek.com> <1415709535-31515-2-git-send-email-hongzhou.yang@mediatek.com> Message-ID: <1417151218.25975.12.camel@mhfsdcap03> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thu, 2014-11-27 at 10:14 +0100, Linus Walleij wrote: > ()On Tue, Nov 11, 2014 at 1:38 PM, Hongzhou Yang > wrote: > > > From: Hongzhou Yang > > > > 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 > (...) > > 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? Ok, I will add it in next version.Thanks. > (...) > > +++ 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. Ok, I will add it in next version. Thanks. > (...) > > +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()? Yes, it can use it, I will use it, thanks. > > +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. Ok, thanks. > > +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. Ok, thanks. > > +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. I think this need more discuss, thanks. > > + > > + 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. Ok, I will modify it, thanks. > > +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? For this question, we will add irq support in another patch, then we can explain it, thanks. Regards, Hongzhou