Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758974AbdCVK1J (ORCPT ); Wed, 22 Mar 2017 06:27:09 -0400 Received: from mail-it0-f67.google.com ([209.85.214.67]:36799 "EHLO mail-it0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758808AbdCVK1B (ORCPT ); Wed, 22 Mar 2017 06:27:01 -0400 MIME-Version: 1.0 In-Reply-To: <1490026491-21742-2-git-send-email-jacopo+renesas@jmondi.org> References: <1490026491-21742-1-git-send-email-jacopo+renesas@jmondi.org> <1490026491-21742-2-git-send-email-jacopo+renesas@jmondi.org> From: Geert Uytterhoeven Date: Wed, 22 Mar 2017 11:26:58 +0100 X-Google-Sender-Auth: XYPsX_Uvo6HCXTq2eCNogliblIg Message-ID: Subject: Re: [PATCH v2 1/7] pinctrl: Renesas RZ/A1 pin and gpio controller To: Jacopo Mondi Cc: Geert Uytterhoeven , Laurent Pinchart , Chris Brandt , Linus Walleij , Rob Herring , Mark Rutland , Russell King , Linux-Renesas , "linux-gpio@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 12756 Lines: 414 Hi Jacopo, On Mon, Mar 20, 2017 at 5:14 PM, Jacopo Mondi wrote: > Add combined gpio and pin controller driver for Renesas RZ/A1 > r7s72100 SoC. > > Signed-off-by: Jacopo Mondi Thanks for your patch! > --- /dev/null > +++ b/drivers/pinctrl/pinctrl-rza1.c > +/* > + * This pincontroller/gpio combined driver support Renesas devices of RZ/A1 > + * family. > + * This includes SoCs which are sub- or super- sets of this particular line, > + * as RZ/A1H (r7s721000), RZ/A1M (r7s721001) and RZ/A1L (r7s721002) are. RZ/A1M = r7s721010, RZ/A1L = r7s721020 > +#define RZA1_ADDR(mem, reg, port) ((mem) + (reg) + ((port) * 4)) > + > +#define RZA1_NPORTS 12 > +#define RZA1_PINS_PER_PORT 16 > +#define RZA1_NPINS (RZA1_PINS_PER_PORT * RZA1_NPORTS) > +#define RZA1_PIN_TO_PORT(pin) ((pin) / RZA1_PINS_PER_PORT) > +#define RZA1_PIN_TO_OFFSET(pin) ((pin) % RZA1_PINS_PER_PORT) > + > +/* > + * Be careful here: the pin configuration subnodes in device tree enumerates enumerate > + * alternate functions from 1 to 8; subtract 1 before using macros so to match > + * registers configuration which expects numbers from 0 to 7 instead. register configuration > + */ > +#define MUX_FUNC_OFFS 3 > +#define MUX_FUNC_MASK (BIT(MUX_FUNC_OFFS) - 1) > +#define MUX_FUNC_PFC_MASK BIT(0) > +#define MUX_FUNC_PFCE_MASK BIT(1) > +#define MUX_FUNC_PFCEA_MASK BIT(2) > +#define MUX_CONF_BIDIR BIT(0) > +#define MUX_CONF_SWIO_INPUT BIT(1) > +#define MUX_CONF_SWIO_OUTPUT BIT(2) > + > +/** > + * rza1_pin_conf - describes a pin position, id, mux config and output value > + * > + * Use uint32_t to match types used in of_device nodes argument lists. > + * > + * @id: the pin identifier from 0 to RZA1_NPINS > + * @port: the port where pin sits on > + * @offset: pin offset in the port > + * @mux: alternate function configuration settings > + * @value: output value to set the pin to > + */ > +struct rza1_pin_conf { > + uint32_t id; > + uint32_t port; > + uint32_t offset; > + uint32_t mux_conf; > + uint32_t value; In the kernel, we tend to use u32 instead of uint32_t. But many of these fields can be smaller than 32 bits, right, saving some memory (important when running with built-in RAM only). > +/** > + * rza1_pinctrl - RZ pincontroller device > + * > + * @dev: parent device structure > + * @mutex: protect [pinctrl|pinmux]_generic functions > + * @base: logical address base > + * @nports: number of pin controller ports > + * @ports: pin controller banks > + * @ngpiochips: number of gpio chips > + * @gpio_ranges: gpio ranges for pinctrl core > + * @pins: pin array for pinctrl core > + * @desc: pincontroller desc for pinctrl core > + * @pctl: pinctrl device > + */ > +struct rza1_pinctrl { > + struct device *dev; > + > + struct mutex mutex; > + > + void __iomem *base; > + > + unsigned int nport; > + struct rza1_port *ports; > + > + unsigned int ngpiochips; > + > + struct pinctrl_gpio_range *gpio_ranges; > + struct pinctrl_pin_desc *pins; > + struct pinctrl_desc desc; > + struct pinctrl_dev *pctl; It's a good idea not to mix pointers and integers, as the pointers will be 64-bit on 64-bit platforms, leading to gaps due to alignment rules. Not super-important here, as (for now) this driver is meant for 32-bit SoCs. > +/** > + * rza1_set_bit() - un-locked set/clear a single bit in pin configuration > + * registers > + */ > +static inline void rza1_set_bit(const struct rza1_port *port, > + unsigned int reg, unsigned int offset, > + bool set) I think "reg" and "set" still fit on the previous lines (many more cases in other functions). I'd call "offset" "bit" (and "reg" "offset"?) > +{ > + void __iomem *mem = RZA1_ADDR(port->base, reg, port->id); I think this would be easier to read without using the RZA1_ADDR() macro. > + u16 val = ioread16(mem); > + > + if (set) > + val |= BIT(offset); > + else > + val &= ~BIT(offset); > + > + iowrite16(val, mem); > +} > + > +static inline int rza1_get_bit(struct rza1_port *port, > + unsigned int reg, unsigned int offset) > +{ > + void __iomem *mem = RZA1_ADDR(port->base, reg, port->id); > + > + return ioread16(mem) & BIT(offset); Same comments as for rza1_set_bit(). > +} > + > +/** > + * rza1_pin_reset() - reset a pin to default initial state > + * > + * Reset pin state disabling input buffer and bi-directional control, > + * and configure it as input port. > + * Note that pin is now configured with direction as input but with input > + * buffer disabled. This implies the pin value cannot be read in this state. > + * > + * @port: port where pin sits on > + * @offset: pin offset > + */ > +static void rza1_pin_reset(struct rza1_port *port, > + unsigned int offset) The above fits on a single line. "pin" instead of "offset"? > +{ > + spin_lock(&port->lock); spin_lock_irqsave()? (everywhere) > + rza1_set_bit(port, PIBC_REG, offset, 0); > + rza1_set_bit(port, PBDC_REG, offset, 0); > + > + rza1_set_bit(port, PM_REG, offset, 1); > + rza1_set_bit(port, PMC_REG, offset, 0); > + rza1_set_bit(port, PIPC_REG, offset, 0); > + spin_unlock(&port->lock); spin_unlock_irqrestore()? (everywhere) > +} > + > +static inline int rza1_pin_get_direction(struct rza1_port *port, > + int offset) The above fits on a single line. "pin" instead of "offset" (many more below)? > +/** > + * rza1_pin_conf_validate() - make sure a single bit it set in mux_conf mask is set > + */ > +static inline int rza1_pin_conf_validate(u8 mux_conf) > +{ > + do { > + if (mux_conf & BIT(0)) > + break; > + } while ((mux_conf >>= 1)); > + > + return (mux_conf >> 1); Please study to find a better way to do this ;-) > +/** > + * rza1_gpio_get() - read a gpio pin value > + * > + * Read gpio pin value through PPR register. > + * Requires bi-directional mode to work when reading value of a pin the value > + * in output mode > +/** > + * rza1_pinmux_set() - retrieve pins from a group and apply them mux settings their > + * > + * @pctldev: pin controller device > + * @selector: function selector > + * @group: group selector > + */ > +static int rza1_pinmux_set(struct pinctrl_dev *pctldev, unsigned int selector, > + unsigned int group) > +{ > + int i; > + struct group_desc *grp; > + struct function_desc *func; > + struct rza1_pin_conf *pin_confs; > + struct rza1_pinctrl *rza1_pctl = pinctrl_dev_get_drvdata(pctldev); Reverse Christmas tree ordering (longest line first)? > +/** > + * rza1_parse_pmx_function() - parse and register a pin mux function > + * > + * Pins for RZ SoC pin controller described by "renesas-pins" property. > + * > + * First argument in the list identifies the pin, while the second one > + * describes the requested alternate function number and additional > + * configuration parameter to be applied to the selected function. > + * > + * @rza1_pctl: RZ/A1 pin controller device > + * @np: of pmx sub-node > + */ > +static int rza1_parse_pmx_function(struct rza1_pinctrl *rza1_pctl, > + struct device_node *np) > +{ > + int ret; > + int of_pins; npins? > + unsigned int i; > + unsigned int *grpins; > + const char *grpname; > + const char **fngrps; > + struct rza1_pin_conf *pin_confs; > + struct pinctrl_dev *pctldev = rza1_pctl->pctl; > + char const *prop_name = "renesas,pins"; const char *prop_name Reverse Christmas tree ordering (longest line first)? (more below) > + > + of_pins = pinctrl_count_index_with_args(np, prop_name); > + if (of_pins <= 0) { > + dev_err(rza1_pctl->dev, "Missing %s property\n", prop_name); > + return -ENOENT; > + } > + > + /* > + * Functions are made of 1 group only; > + * in facts, functions and groups are identical for this pin controller in fact > + * except that functions carry an array of per-pin configurations configuration > + * settings. > + */ > + pin_confs = devm_kcalloc(rza1_pctl->dev, of_pins, sizeof(*pin_confs), > + GFP_KERNEL); > + grpins = devm_kcalloc(rza1_pctl->dev, of_pins, sizeof(*grpins), > + GFP_KERNEL); > + fngrps = devm_kzalloc(rza1_pctl->dev, sizeof(*fngrps), GFP_KERNEL); > + > + if (!pin_confs || !grpins || !fngrps) > + return -ENOMEM; > + > + /* Collect pin positions and mux settings to store them in function */ > + for (i = 0; i < of_pins; ++i) { > + struct rza1_pin_conf *pin_conf = &pin_confs[i]; > + struct of_phandle_args of_pins_args; > + > + ret = pinctrl_parse_index_with_args(np, prop_name, i, > + &of_pins_args); > + if (ret) > + return ret; > + > + if (of_pins_args.args_count < RZA1_PINMUX_OF_ARGS) { > + dev_err(rza1_pctl->dev, > + "Wrong arguments number for %s property\n", number of arguments > +/** > + * rza1_parse_gpiochip() - parse and register a gpio chip and pin range > + * > + * The gpio controller subnode shall provide a "gpio-ranges" list property as > + * defined by gpio device tree binding documentation. > + * Gpio chips and pin ranges are here collected, but ranges are registered > + * later, after pin controller has been registered too. Only gpiochips are after the pin controller > + * registered here. > + * > + * @rza1_pctl: RZ/A1 pin controller device > + * @np: of gpio-controller node > + * @chip: gpio chip to register to gpiolib > + * @range: pin range to register to pinctrl core > + */ > +static int rza1_parse_gpiochip(struct rza1_pinctrl *rza1_pctl, > + struct device_node *np, > + struct gpio_chip *chip, > + struct pinctrl_gpio_range *range) > +{ > + int ret; > + u32 pinctrl_base; > + unsigned int gpioport; > + struct of_phandle_args of_args; > + const char *list_name = "gpio-ranges"; > + > + of_parse_phandle_with_fixed_args(np, list_name, 3, 0, &of_args); This function can fail. > + > + /* > + * Find out on which port this gpio-chip maps to inspecting the second by inspecting > + * argument of "gpio-ranges" property. the "gpio-ranges" property. > + */ > + pinctrl_base = of_args.args[1]; > + gpioport = RZA1_PIN_TO_PORT(pinctrl_base); > + if (gpioport > RZA1_NPORTS) { > + dev_err(rza1_pctl->dev, > + "Invalid values in property %s\n", list_name); > + return -EINVAL; > + } > + > + *chip = rza1_gpiochip_template; > + chip->base = -1; > + chip->label = kasprintf(GFP_KERNEL, "%s-%d", np->name, gpioport); devm_kasprintf()? "%s-%u" > + chip->ngpio = of_args.args[2]; > + chip->of_node = np; > + chip->parent = rza1_pctl->dev; > + > + range->id = gpioport; > + range->name = kasprintf(GFP_KERNEL, "%s-%d", np->name, gpioport); Reuse chip->label? > +/** > + * rza1_pinctrl_register() - Enumerate pins, ports, gpiochips and functions and > + * register to pinctrl and gpio cores > + * > + * @rza1_pctl: RZ/A1 pin controller device > + */ > +static int rza1_pinctrl_register(struct rza1_pinctrl *rza1_pctl) > +{ > + for (i = 0; i < RZA1_NPINS; ++i) { > + unsigned int port = RZA1_PIN_TO_PORT(i); > + unsigned int offset = RZA1_PIN_TO_OFFSET(i); > + > + pins[i].number = i; > + pins[i].name = kasprintf(GFP_KERNEL, "P%u-%u", port, offset); devm_kasprintf() Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds