Hi Jacopo,
On Mon, Mar 20, 2017 at 5:14 PM, Jacopo Mondi <jacopo+renesas@xxxxxxxxxx> wrote:
> Add combined gpio and pin controller driver for Renesas RZ/A1
> r7s72100 SoC.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
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 <linux/bitops.h> 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@xxxxxxxxxxxxxx
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
