| Message ID | 20181005150951.22129-1-chris.brandt@renesas.com |
|---|---|
| Headers | show |
| Series | pinctrl: Add RZ/A2 pin and gpio driver | expand |
Hi Chris, thanks for the patches. On Fri, Oct 05, 2018 at 10:09:50AM -0500, Chris Brandt wrote: > Adds support for the pin and gpio controller found in R7S9210 (RZ/A2) SoCs. > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> > --- > drivers/pinctrl/Kconfig | 11 + > drivers/pinctrl/Makefile | 1 + > drivers/pinctrl/pinctrl-rza2.c | 519 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 531 insertions(+) > create mode 100644 drivers/pinctrl/pinctrl-rza2.c > > diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig > index 978b2ed4d014..5819e0c9c3a8 100644 > --- a/drivers/pinctrl/Kconfig > +++ b/drivers/pinctrl/Kconfig > @@ -195,6 +195,17 @@ config PINCTRL_RZA1 > help > This selects pinctrl driver for Renesas RZ/A1 platforms. > > +config PINCTRL_RZA2 > + bool "Renesas RZ/A2 gpio and pinctrl driver" > + depends on OF > + depends on ARCH_R7S9210 || COMPILE_TEST > + select GPIOLIB > + select GENERIC_PINCTRL_GROUPS > + select GENERIC_PINMUX_FUNCTIONS > + select GENERIC_PINCONF > + help > + This selects pinctrl driver for Renesas RZ/A2 platforms. > + > config PINCTRL_SINGLE > tristate "One-register-per-pin type device tree based pinctrl driver" > depends on OF > diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile > index 8e127bd8610f..9354f0c2044c 100644 > --- a/drivers/pinctrl/Makefile > +++ b/drivers/pinctrl/Makefile > @@ -27,6 +27,7 @@ obj-$(CONFIG_PINCTRL_PIC32) += pinctrl-pic32.o > obj-$(CONFIG_PINCTRL_PISTACHIO) += pinctrl-pistachio.o > obj-$(CONFIG_PINCTRL_ROCKCHIP) += pinctrl-rockchip.o > obj-$(CONFIG_PINCTRL_RZA1) += pinctrl-rza1.o > +obj-$(CONFIG_PINCTRL_RZA2) += pinctrl-rza2.o > obj-$(CONFIG_PINCTRL_SINGLE) += pinctrl-single.o > obj-$(CONFIG_PINCTRL_SIRF) += sirf/ > obj-$(CONFIG_PINCTRL_SX150X) += pinctrl-sx150x.o > diff --git a/drivers/pinctrl/pinctrl-rza2.c b/drivers/pinctrl/pinctrl-rza2.c > new file mode 100644 > index 000000000000..db8d5a3cf2ea > --- /dev/null > +++ b/drivers/pinctrl/pinctrl-rza2.c > @@ -0,0 +1,519 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Combined GPIO and pin controller support for Renesas RZ/A2 (R7S72100) SoC R7S9210 > + * > + * Copyright (C) 2018 Chris Brandt > + */ > + > +/* > + * This pin controller/gpio combined driver supports Renesas devices of RZ/A2 > + * family. > + */ > + > +#include <linux/gpio.h> > +#include <linux/bitops.h> Headers sorted please :) > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/pinctrl/pinmux.h> > + > +#include "core.h" > +#include "pinmux.h" > + > +#define DRIVER_NAME "pinctrl-rza2" > + > +#define RZA2_NPORTS (PM + 1) Why not making this part of the enum itself? > +#define RZA2_PINS_PER_PORT 8 > +#define RZA2_NPINS (RZA2_PINS_PER_PORT * RZA2_NPORTS) > +#define RZA2_PIN_ID_TO_PORT(id) ((id) / RZA2_PINS_PER_PORT) > +#define RZA2_PIN_ID_TO_PIN(id) ((id) % RZA2_PINS_PER_PORT) > + > +/* > + * Use 16 lower bits [15:0] for pin identifier > + * Use 16 higher bits [31:16] for pin mux function > + */ > +#define MUX_PIN_ID_MASK GENMASK(15, 0) > +#define MUX_FUNC_MASK GENMASK(31, 16) > +#define MUX_FUNC_OFFS 16 > +#define MUX_FUNC(pinconf) ((pinconf & MUX_FUNC_MASK) >> MUX_FUNC_OFFS) > +#define MUX_FUNC(pinconf) ((pinconf & MUX_FUNC_MASK) >> MUX_FUNC_OFFS) Seems like the same line to me. Also, double empty line. > + > + > +enum pfc_pin_port_name {P0, P1, P2, P3, P4, P5, P6, P7, P8, P9, PA, PB, PC, PD, > + PE, PF, PG, PH, PJ, PK, PL, PM}; weird spacing ^^^^ > +static const char port_names[] = "0123456789ABCDEFGHJKLM"; > + > +struct rza2_pinctrl_priv { > + struct device *dev; > + void __iomem *base; > + > + struct pinctrl_pin_desc *pins; > + struct pinctrl_desc desc; > + struct pinctrl_dev *pctl; > +}; > + > +#define PDR_BASE(b) (b + 0x0000) /* 16-bit, 2 bytes apart */ () around macros parameters when used: ((b) + 0x0000) which is just (b) by the way :) Also, please prefix all defines with RZA2_ not to pollute the global symbols space. > +#define PODR_BASE(b) (b + 0x0040) /* 8-bit, 1 byte apart */ > +#define PIDR_BASE(b) (b + 0x0060) /* 8-bit, 1 byte apart */ > +#define PMR_BASE(b) (b + 0x0080) /* 8-bit, 1 byte apart */ > +#define DSCR_BASE(b) (b + 0x0140) /* 16-bit, 2 bytes apart */ > +#define PFS_BASE(b) (b + 0x0200) /* 8-bit, 8 bytes apart */ Here you define registers address bases, but instead of computing everytime the port-pin dedicated register address (I'm thinking about PFS specifically but it applies to others), would you consider providing accessors helpers to write the offset computations process just once here? Eg. #define RZA2_PFS(_b, _port, _pin) (RZA2_PFS_BASE(_b) + (_port) * 8 + (_pin)) Same for other register which needs offset calculation based on port and pin values. > +#define PWPR(b) (b + 0x02FF) /* 8-bit */ > +#define PFENET(b) (b + 0x0820) /* 8-bit */ > +#define PPOC(b) (b + 0x0900) /* 32-bit */ > +#define PHMOMO(b) (b + 0x0980) /* 32-bit */ > +#define PMODEPFS(b) (b + 0x09C0) /* 32-bit */ > +#define PCKIO(b) (b + 0x09D0) /* 8-bit */ > + > +void rza2_set_pin_function(void __iomem *pfc_base, u8 port, u8 pin, u8 func) static ? > +{ > + u8 reg8; > + u16 reg16; > + u16 mask16; > + > + /* Set pin to 'Non-use (Hi-z input protection)' */ > + reg16 = readw(PDR_BASE(pfc_base) + (port * 2)); as in example, reg16 = readw(RZA2_PDR(pfc_base), port)); it's nicer and isolates the offset calculations, which differs from one register to another (the 'x bytes apart' comment you added in the registers definitions represents this, right? ) > + mask16 = 0x03 << (pin * 2); > + reg16 &= ~mask16; > + writew(reg16, PDR_BASE((pfc_base)) + port * 2); > + > + /* Temporarily switch to GPIO */ > + reg8 = readb(PMR_BASE(pfc_base) + port); > + reg8 &= ~BIT(pin); > + writeb(reg8, PMR_BASE(pfc_base) + port); > + > + /* PFS Register Write Protect : OFF */ > + writeb(0x00, PWPR(pfc_base)); /* B0WI=0, PFSWE=0 */ > + writeb(0x40, PWPR(pfc_base)); /* B0WI=0, PFSWE=1 */ > + > + /* Set Pin function (interrupt disabled, ISEL=0) */ > + writeb(func, PFS_BASE(pfc_base) + (port * 8) + pin); > + > + /* PFS Register Write Protect : ON */ > + writeb(0x00, PWPR(pfc_base)); /* B0WI=0, PFSWE=0 */ > + writeb(0x80, PWPR(pfc_base)); /* B0WI=1, PFSWE=0 */ > + > + /* Port Mode : Peripheral module pin functions */ > + reg8 = readb(PMR_BASE(pfc_base) + port); > + reg8 |= BIT(pin); > + writeb(reg8, PMR_BASE(pfc_base) + port); > +} > + > +static void rza2_pin_to_gpio(void __iomem *pfc_base, u8 port, u8 pin, u8 dir) > +{ > + u16 reg16; > + u16 mask16; > + > + reg16 = readw(PDR_BASE(pfc_base) + (port * 2)); > + mask16 = 0x03 << (pin * 2); > + reg16 &= ~mask16; > + > + if (dir == GPIOF_DIR_IN) > + reg16 |= 2 << (pin * 2); // pin as input C++ comments style! > + else > + reg16 |= 3 << (pin * 2); // pin as output Also, you are using those '2' and '3' values here and there in the code, as they represent the pin 'status. Would you consider making a define for them? Actually, for all of them #define RZA2_PIN_STATE_HIGHZ 0x00 #define RZA2_PIN_STATE_INPUT 0x02 #define RZA2_PIN_STATE_OUTPUT 0x03 > + > + writew(reg16, PDR_BASE((pfc_base)) + port * 2); > +} > + > +static int rza2_chip_get_direction(struct gpio_chip *chip, unsigned int offset) > +{ > + struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip); > + u8 port = offset / 8; > + u8 pin = offset % 8; > + u16 reg16; > + > + reg16 = readw(PDR_BASE(priv->base) + (port * 2)); > + reg16 = (reg16 >> (pin * 2)) & 0x03; > + > + if (reg16 == 3) > + return GPIOF_DIR_OUT; > + > + if (reg16 == 2) > + return GPIOF_DIR_IN; > + > + /* > + * This GPIO controller has a default Hi-Z state that is not input or > + * output, so force the pin to input now. > + */ I wonder if it is fine for the .get_direction callback to change the pin state. > + rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_IN); Empty line before return is nice (here and in other places). I'll stop here with comments on the driver, as I should have a look at bindings before. I'll comment there too and the get back to this. Thanks j > + return GPIOF_DIR_IN; > +} > + > +static int rza2_chip_direction_input(struct gpio_chip *chip, > + unsigned int offset) > +{ > + struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip); > + u8 port = offset / 8; > + u8 pin = offset % 8; > + > + rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_IN); > + > + return 0; > +} > + > +static int rza2_chip_direction_output(struct gpio_chip *chip, > + unsigned int offset, int val) > +{ > + struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip); > + u8 port = offset / 8; > + u8 pin = offset % 8; > + > + rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_OUT); > + > + return 0; > +} > + > +static int rza2_chip_get(struct gpio_chip *chip, unsigned int offset) > +{ > + struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip); > + u8 port = offset / 8; > + u8 pin = offset % 8; > + > + return (readb(PIDR_BASE(priv->base) + port) >> pin) & 1; > +} > + > +static void rza2_chip_set(struct gpio_chip *chip, unsigned int offset, > + int value) > +{ > + struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip); > + u8 port = offset / 8; > + u8 pin = offset % 8; > + u8 new_value = readb(PODR_BASE(priv->base) + port); > + > + if (value) > + new_value |= (1 << pin); > + else > + new_value &= ~(1 << pin); > + > + writeb(new_value, PODR_BASE(priv->base) + port); > +} > + > +static const char * const rza2_gpio_names[] = { > + "P0_0", "P0_1", "P0_2", "P0_3", "P0_4", "P0_5", "P0_6", "P0_7", > + "P1_0", "P1_1", "P1_2", "P1_3", "P1_4", "P1_5", "P1_6", "P1_7", > + "P2_0", "P2_1", "P2_2", "P2_3", "P2_4", "P2_5", "P2_6", "P2_7", > + "P3_0", "P3_1", "P3_2", "P3_3", "P3_4", "P3_5", "P3_6", "P3_7", > + "P4_0", "P4_1", "P4_2", "P4_3", "P4_4", "P4_5", "P4_6", "P4_7", > + "P5_0", "P5_1", "P5_2", "P5_3", "P5_4", "P5_5", "P5_6", "P5_7", > + "P6_0", "P6_1", "P6_2", "P6_3", "P6_4", "P6_5", "P6_6", "P6_7", > + "P7_0", "P7_1", "P7_2", "P7_3", "P7_4", "P7_5", "P7_6", "P7_7", > + "P8_0", "P8_1", "P8_2", "P8_3", "P8_4", "P8_5", "P8_6", "P8_7", > + "P9_0", "P9_1", "P9_2", "P9_3", "P9_4", "P9_5", "P9_6", "P9_7", > + "PA_0", "PA_1", "PA_2", "PA_3", "PA_4", "PA_5", "PA_6", "PA_7", > + "PB_0", "PB_1", "PB_2", "PB_3", "PB_4", "PB_5", "PB_6", "PB_7", > + "PC_0", "PC_1", "PC_2", "PC_3", "PC_4", "PC_5", "PC_6", "PC_7", > + "PD_0", "PD_1", "PD_2", "PD_3", "PD_4", "PD_5", "PD_6", "PD_7", > + "PE_0", "PE_1", "PE_2", "PE_3", "PE_4", "PE_5", "PE_6", "PE_7", > + "PF_0", "PF_1", "PF_2", "PF_3", "P0_4", "PF_5", "PF_6", "PF_7", > + "PG_0", "PG_1", "PG_2", "P0_3", "PG_4", "PG_5", "PG_6", "PG_7", > + "PH_0", "PH_1", "PH_2", "PH_3", "PH_4", "PH_5", "PH_6", "PH_7", > + /* port I does not exist */ > + "PJ_0", "PJ_1", "PJ_2", "PJ_3", "PJ_4", "PJ_5", "PJ_6", "PJ_7", > + "PK_0", "PK_1", "PK_2", "PK_3", "PK_4", "PK_5", "PK_6", "PK_7", > + "PL_0", "PL_1", "PL_2", "PL_3", "PL_4", "PL_5", "PL_6", "PL_7", > + "PM_0", "PM_1", "PM_2", "PM_3", "PM_4", "PM_5", "PM_6", "PM_7", > +}; > + > +static struct gpio_chip chip = { > + .names = rza2_gpio_names, > + .base = -1, > + .ngpio = RZA2_NPINS, > + .get_direction = rza2_chip_get_direction, > + .direction_input = rza2_chip_direction_input, > + .direction_output = rza2_chip_direction_output, > + .get = rza2_chip_get, > + .set = rza2_chip_set, > +}; > + > +struct pinctrl_gpio_range gpio_range; > + > +static int rza2_gpio_register(struct rza2_pinctrl_priv *priv) > +{ > + struct device_node *np = priv->dev->of_node; > + struct of_phandle_args of_args; > + int ret; > + > + if (!of_property_read_bool(np, "gpio-controller")) { > + dev_info(priv->dev, "No gpio controller registered\n"); > + return 0; > + } > + > + chip.label = devm_kasprintf(priv->dev, GFP_KERNEL, "%pOFn", np); > + chip.of_node = np; > + chip.parent = priv->dev; > + > + ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, > + &of_args); > + if (ret) { > + dev_err(priv->dev, "Unable to parse gpio-ranges\n"); > + return ret; > + } > + > + gpio_range.id = of_args.args[0]; > + gpio_range.name = chip.label; > + gpio_range.pin_base = gpio_range.base = of_args.args[1]; > + gpio_range.npins = of_args.args[2]; > + gpio_range.gc = &chip; > + > + /* Register our gpio chip with gpiolib */ > + ret = devm_gpiochip_add_data(priv->dev, &chip, priv); > + if (ret) > + return ret; > + > + /* Register pin range with pinctrl core */ > + pinctrl_add_gpio_range(priv->pctl, &gpio_range); > + > + dev_dbg(priv->dev, "Registered gpio controller\n"); > + > + return 0; > +} > + > +static int rza2_pinctrl_register(struct rza2_pinctrl_priv *priv) > +{ > + struct pinctrl_pin_desc *pins; > + unsigned int i; > + int ret; > + > + pins = devm_kcalloc(priv->dev, RZA2_NPINS, sizeof(*pins), GFP_KERNEL); > + if (!pins) > + return -ENOMEM; > + > + priv->pins = pins; > + priv->desc.pins = pins; > + priv->desc.npins = RZA2_NPINS; > + > + for (i = 0; i < RZA2_NPINS; i++) { > + unsigned int pin = RZA2_PIN_ID_TO_PIN(i); > + unsigned int port = RZA2_PIN_ID_TO_PORT(i); > + > + pins[i].number = i; > + pins[i].name = devm_kasprintf(priv->dev, GFP_KERNEL, "P%c_%u", > + port_names[port], pin); > + } > + > + ret = devm_pinctrl_register_and_init(priv->dev, &priv->desc, priv, > + &priv->pctl); > + if (ret) { > + dev_err(priv->dev, "pinctrl registration failed\n"); > + return ret; > + } > + > + ret = pinctrl_enable(priv->pctl); > + if (ret) { > + dev_err(priv->dev, "pinctrl enable failed\n"); > + return ret; > + } > + > + ret = rza2_gpio_register(priv); > + if (ret) { > + dev_err(priv->dev, "GPIO registration failed\n"); > + return ret; > + } > + > + return 0; > +} > + > +/* > + * For each DT node, create a single pin mapping. That pin mapping will only > + * contain a single group of pins, and that group of pins will only have a > + * single function that can be selected. > + */ > +static int rza2_dt_node_to_map(struct pinctrl_dev *pctldev, > + struct device_node *np, > + struct pinctrl_map **map, > + unsigned int *num_maps) > +{ > + struct rza2_pinctrl_priv *priv = pinctrl_dev_get_drvdata(pctldev); > + struct property *of_pins; > + int i; > + unsigned int *pins; > + unsigned int *psel_val; > + const char **pin_fn; > + int ret, npins; > + int gsel, fsel; > + > + /* Find out how many pins to map */ > + of_pins = of_find_property(np, "pinmux", NULL); > + if (!of_pins) { > + dev_info(priv->dev, "Missing pinmux property\n"); > + return -ENOENT; > + } > + npins = of_pins->length / sizeof(u32); > + > + pins = devm_kcalloc(priv->dev, npins, sizeof(*pins), GFP_KERNEL); > + psel_val = devm_kcalloc(priv->dev, npins, sizeof(*psel_val), > + GFP_KERNEL); > + pin_fn = devm_kzalloc(priv->dev, sizeof(*pin_fn), GFP_KERNEL); > + if (!pins || !psel_val || !pin_fn) > + return -ENOMEM; > + > + /* Collect pin locations and mux settings from DT properties */ > + for (i = 0; i < npins; ++i) { > + u32 value; > + > + ret = of_property_read_u32_index(np, "pinmux", i, &value); > + if (ret) > + return ret; > + pins[i] = value & MUX_PIN_ID_MASK; > + psel_val[i] = MUX_FUNC(value); > + } > + > + /* Register a single pin group listing all the pins we read from DT */ > + gsel = pinctrl_generic_add_group(pctldev, np->name, pins, npins, NULL); > + if (gsel < 0) > + return gsel; > + > + /* > + * Register a single group function where the 'data' is an array PSEL > + * register values read from DT. > + */ > + pin_fn[0] = np->name; > + fsel = pinmux_generic_add_function(pctldev, np->name, pin_fn, 1, > + psel_val); > + if (fsel < 0) { > + ret = fsel; > + goto remove_group; > + } > + > + dev_dbg(priv->dev, "Parsed %s with %d pins\n", np->name, npins); > + > + /* Create map where to retrieve function and mux settings from */ > + *num_maps = 0; > + *map = kzalloc(sizeof(**map), GFP_KERNEL); > + if (!*map) { > + ret = -ENOMEM; > + goto remove_function; > + } > + > + (*map)->type = PIN_MAP_TYPE_MUX_GROUP; > + (*map)->data.mux.group = np->name; > + (*map)->data.mux.function = np->name; > + *num_maps = 1; > + > + return 0; > + > +remove_function: > + pinmux_generic_remove_function(pctldev, fsel); > + > +remove_group: > + pinctrl_generic_remove_group(pctldev, gsel); > + > + dev_info(priv->dev, "Unable to parse DT node %s\n", np->name); > + > + return ret; > +} > + > +static void rza2_dt_free_map(struct pinctrl_dev *pctldev, > + struct pinctrl_map *map, unsigned int num_maps) > +{ > + kfree(map); > +} > + > +static const struct pinctrl_ops rza2_pinctrl_ops = { > + .get_groups_count = pinctrl_generic_get_group_count, > + .get_group_name = pinctrl_generic_get_group_name, > + .get_group_pins = pinctrl_generic_get_group_pins, > + .dt_node_to_map = rza2_dt_node_to_map, > + .dt_free_map = rza2_dt_free_map, > +}; > + > +static int rza2_set_mux(struct pinctrl_dev *pctldev, unsigned int selector, > + unsigned int group) > +{ > + struct rza2_pinctrl_priv *priv = pinctrl_dev_get_drvdata(pctldev); > + struct function_desc *func; > + struct group_desc *grp; > + int i; > + unsigned int *psel_val; > + > + grp = pinctrl_generic_get_group(pctldev, group); > + if (!grp) > + return -EINVAL; > + > + func = pinmux_generic_get_function(pctldev, selector); > + if (!func) > + return -EINVAL; > + > + psel_val = func->data; > + > + for (i = 0; i < grp->num_pins; ++i) { > + dev_dbg(priv->dev, "Setting P%c_%d to PSEL=%d\n", > + port_names[RZA2_PIN_ID_TO_PORT(grp->pins[i])], > + RZA2_PIN_ID_TO_PIN(grp->pins[i]), > + psel_val[i]); > + rza2_set_pin_function( > + priv->base, > + RZA2_PIN_ID_TO_PORT(grp->pins[i]), > + RZA2_PIN_ID_TO_PIN(grp->pins[i]), > + psel_val[i]); > + } > + > + return 0; > +} > + > +static const struct pinmux_ops rza2_pinmux_ops = { > + .get_functions_count = pinmux_generic_get_function_count, > + .get_function_name = pinmux_generic_get_function_name, > + .get_function_groups = pinmux_generic_get_function_groups, > + .set_mux = rza2_set_mux, > + .strict = true, > +}; > + > +static int rza2_pinctrl_probe(struct platform_device *pdev) > +{ > + struct rza2_pinctrl_priv *priv; > + struct resource *res; > + int ret; > + > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->dev = &pdev->dev; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + priv->base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(priv->base)) > + return PTR_ERR(priv->base); > + > + platform_set_drvdata(pdev, priv); > + > + priv->desc.name = DRIVER_NAME; > + priv->desc.pctlops = &rza2_pinctrl_ops; > + priv->desc.pmxops = &rza2_pinmux_ops; > + priv->desc.owner = THIS_MODULE; > + > + ret = rza2_pinctrl_register(priv); > + if (ret) > + return ret; > + > + pr_info("RZ/A2 pin controller registered\n"); > + > + return 0; > +} > + > +static const struct of_device_id rza2_pinctrl_of_match[] = { > + { > + .compatible = "renesas,r7s9210-pinctrl", > + }, > + { /* sentinel */ } > +}; > + > +static struct platform_driver rza2_pinctrl_driver = { > + .driver = { > + .name = DRIVER_NAME, > + .of_match_table = rza2_pinctrl_of_match, > + }, > + .probe = rza2_pinctrl_probe, > +}; > + > +static int __init rza2_pinctrl_init(void) > +{ > + return platform_driver_register(&rza2_pinctrl_driver); > +} > +core_initcall(rza2_pinctrl_init); > + > + > +MODULE_AUTHOR("Chris Brandt <chris.brandt@renesas.com>"); > +MODULE_DESCRIPTION("Pin and gpio controller driver for RZ/A2 SoC"); > +MODULE_LICENSE("GPL v2"); > -- > 2.16.1 >
Hi Jacopo, > thanks for the patches. Thanks for the review! On Thursday, October 18, 2018, jacopo mondi wrote: > > + * Combined GPIO and pin controller support for Renesas RZ/A2 > (R7S72100) SoC > > R7S9210 Thanks. hmm, I wonder what pinctrl driver I copied that from... ;) > > +#include <linux/gpio.h> > > +#include <linux/bitops.h> > > Headers sorted please :) OK, sorted. > > +#define RZA2_NPORTS (PM + 1) > > Why not making this part of the enum itself? Good idea. > > +#define MUX_FUNC(pinconf) ((pinconf & MUX_FUNC_MASK) >> MUX_FUNC_OFFS) > > +#define MUX_FUNC(pinconf) ((pinconf & MUX_FUNC_MASK) >> MUX_FUNC_OFFS) > > Seems like the same line to me. > Also, double empty line. Oops, thanks. > > +enum pfc_pin_port_name {P0, P1, P2, P3, P4, P5, P6, P7, P8, P9, PA, PB, > PC, PD, > > + PE, PF, PG, PH, PJ, PK, PL, PM}; > weird spacing ^^^^ I was doing that to point out that there is no "I" port (they skipped over it). But if you didn't realize that, then it is not obvious, so I'll just format it normally. > > +static const char port_names[] = "0123456789ABCDEFGHJKLM"; > > + > > +struct rza2_pinctrl_priv { > > + struct device *dev; > > + void __iomem *base; > > + > > + struct pinctrl_pin_desc *pins; > > + struct pinctrl_desc desc; > > + struct pinctrl_dev *pctl; > > +}; > > + > > +#define PDR_BASE(b) (b + 0x0000) /* 16-bit, 2 bytes apart */ > > () around macros parameters when used: > ((b) + 0x0000) > which is just (b) by the way :) OK. The "+ 0x0000" was more for documentation reason. > Also, please prefix all defines with RZA2_ not to pollute the global > symbols space. OK. > > +#define PODR_BASE(b) (b + 0x0040) /* 8-bit, 1 byte apart */ > > +#define PIDR_BASE(b) (b + 0x0060) /* 8-bit, 1 byte apart */ > > +#define PMR_BASE(b) (b + 0x0080) /* 8-bit, 1 byte apart */ > > +#define DSCR_BASE(b) (b + 0x0140) /* 16-bit, 2 bytes apart */ > > +#define PFS_BASE(b) (b + 0x0200) /* 8-bit, 8 bytes apart */ > > Here you define registers address bases, but instead of computing > everytime the port-pin dedicated register address (I'm thinking about > PFS specifically but it applies to others), would you consider providing > accessors helpers to write the offset computations process just once here? > > Eg. > #define RZA2_PFS(_b, _port, _pin) (RZA2_PFS_BASE(_b) + (_port) * 8 + > (_pin)) > > Same for other register which needs offset calculation based on port > and pin values. I like that idea. Thanks. > > +#define PWPR(b) (b + 0x02FF) /* 8-bit */ > > +#define PFENET(b) (b + 0x0820) /* 8-bit */ > > +#define PPOC(b) (b + 0x0900) /* 32-bit */ > > +#define PHMOMO(b) (b + 0x0980) /* 32-bit */ > > +#define PMODEPFS(b) (b + 0x09C0) /* 32-bit */ > > +#define PCKIO(b) (b + 0x09D0) /* 8-bit */ > > > + > > +void rza2_set_pin_function(void __iomem *pfc_base, u8 port, u8 pin, u8 > func) > static ? I actually left that as global for a reason (because for some testing, I need a way to set pin directly in a non-standard way). But, I might as well make it static and just use a different hack for my testing. > > + /* Set pin to 'Non-use (Hi-z input protection)' */ > > + reg16 = readw(PDR_BASE(pfc_base) + (port * 2)); > > as in example, > > reg16 = readw(RZA2_PDR(pfc_base), port)); > > it's nicer and isolates the offset calculations, which differs > from one register to another (the 'x bytes apart' comment you added in > the registers definitions represents this, right? ) Yup, that's what my code looks like now after the change :) > > + if (dir == GPIOF_DIR_IN) > > + reg16 |= 2 << (pin * 2); // pin as input > > C++ comments style! OK, I changed it. However.... I can't find anywhere it says that // is not proper. For example, just look at checkpatch.pl: $ grep -m1 allow_c99_comments scripts/checkpatch.pl my $allow_c99_comments = 1; Also every file has this as the very first line: // SPDX-License-Identifier: GPL-2.0 ;) > > + else > > + reg16 |= 3 << (pin * 2); // pin as output > > Also, you are using those '2' and '3' values here and there in the > code, as they represent the pin 'status. Would you consider making > a define for them? Actually, for all of them > > #define RZA2_PIN_STATE_HIGHZ 0x00 > #define RZA2_PIN_STATE_INPUT 0x02 > #define RZA2_PIN_STATE_OUTPUT 0x03 OK. > > + /* > > + * This GPIO controller has a default Hi-Z state that is not input > or > > + * output, so force the pin to input now. > > + */ > I wonder if it is fine for the .get_direction callback to change the > pin state. I see no other option. I can only return "input" or "output", but Hi-Z is neither one of those. The .get_direction is the first one to get called after assigning a pin. If I return that it's an 'input', it will think it does not have to call .direction_input, and then the pin doesn't work. > > + rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_IN); > > Empty line before return is nice (here and in other places). True. I added it. > I'll stop here with comments on the driver, as I should have a look at > bindings before. I'll comment there too and the get back to this. OK. I've made all these changes. I see just sent some more comments, so I'll have a look at them first before I send a V2. Thanks, Chris