[PATCH v2 2/4] ARM: mediatek: Add Pinctrl/GPIO driver for mt8135.

On Tue, Sep 23, 2014 at 5:39 AM, Hongzhou.Yang
[off-list ref] wrote:

From: Hongzhou Yang <redacted>

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 and mt8135 part. It implements the pinctrl
part and gpio part.

Signed-off-by: Hongzhou Yang <redacted>

(...)

This should most certainly select GPIOLIB_IRQCHIP, I'm
pretty sure you can use the common chained irqchip handling.

(...)

+postcore_initcall(mtk_pinctrl_init);

Why? We prefer to use module_init() these days, and employ deferred
probe to order module probe order. Is there some problem with this?

(...)

+#include <linux/io.h>
+#include <linux/gpio.h>
+#include <linux/irqdomain.h>
+#include <linux/irqchip/chained_irq.h>

These two includes go away with GPIOLIB_IRQCHIP

+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/pinctrl/machine.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <dt-bindings/pinctrl/mt65xx.h>
+
+#include "../pinconf.h"
+#include "pinctrl-mtk-common.h"
+
+#define PINMUX_MAX_VAL 8
+#define MAX_GPIO_MODE_PER_REG 5
+#define GPIO_MODE_BITS        3
+
+static const char * const mt_gpio_functions[] = {
+       "func0", "func1", "func2", "func3",
+       "func4", "func5", "func6", "func7",
+};
+
+static void __iomem *mt_get_base_addr(struct mt_pinctrl *pctl,
+               unsigned long pin)
+{
+       if (pin >= pctl->devdata->type1_start && pin < pctl->devdata->type1_end)
+               return pctl->membase2;
+       return pctl->membase1;
+}
+
+static void mt_pctrl_write_reg(struct mt_pinctrl *pctl,
+               unsigned long pin,
+               u32 reg, u32 d)
+{
+       writel(d, mt_get_base_addr(pctl, pin) + reg);
+}

1) Don't you want to use writel_relaxed() and
2) What does this helper really buy you? I would prefer to
  inline this everywhere it's used. At the least tag this
  function as inline.

+static unsigned int mt_get_port(unsigned long pin)
+{
+       return ((pin >> 4) & 0xf) << 4;

Isn't that equivalent to
return pin & 0xf0;

Add a comment explaining what's going on here.

+static int mt_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
+                       struct pinctrl_gpio_range *range,
+                       unsigned offset,
+                       bool input)
+{
+       unsigned int reg_addr;
+       unsigned int bit;
+       struct mt_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+
+       reg_addr = mt_get_port(offset) + pctl->devdata->dir_offset;
+       bit = 1 << (offset & 0xf);

#include <linux/bitops.h>

bit = BIT(offset & 0xf);

+       if (input)
+               reg_addr += (4 << 1);

What is wrong with reg_add += 8;

+       else
+               reg_addr += 4;
+
+       writel(bit, pctl->membase1 + reg_addr);

Note: here you're not using mt_pctrl_write_reg().
And I think you should use writel_relaxed().

+static void mt_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+       unsigned int reg_addr;
+       unsigned int bit;
+       struct mt_pinctrl *pctl = dev_get_drvdata(chip->dev);
+
+       reg_addr = mt_get_port(offset) + pctl->devdata->dout_offset;
+       bit = 1 << (offset & 0xf);

BIT(offset & 0xf)

+       if (value)
+               writel(bit, pctl->membase1 + reg_addr + 4);
+       else
+               writel(bit, pctl->membase1 + reg_addr + (4 << 1));

+8

+static int mt_gpio_set_pull_conf(struct pinctrl_dev *pctldev,
+               unsigned long pin, enum pin_config_param param,
+               enum pin_config_param argument)
+{
+       unsigned int reg_pullen, reg_pullsel;
+       unsigned int bit;
+       struct mt_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+       int pullen = 0;
+       int pullsel = 0;
+
+       bit = 1 << (pin & 0xf);

BIT

+       switch (param) {
+       case PIN_CONFIG_BIAS_DISABLE:
+               pullen = 0;
+               pullsel = 0;
+               break;
+       case PIN_CONFIG_BIAS_PULL_UP:
+               mt_pmx_gpio_set_direction(pctldev, NULL, pin, 1);
+               pullen = 1;
+               pullsel = 1;
+               break;
+       case PIN_CONFIG_BIAS_PULL_DOWN:
+               mt_pmx_gpio_set_direction(pctldev, NULL, pin, 1);
+               pullen = 1;
+               pullsel = 0;
+               break;
+
+       case PIN_CONFIG_INPUT_ENABLE:
+               mt_pmx_gpio_set_direction(pctldev, NULL, pin, 1);
+               break;
+
+       case PIN_CONFIG_OUTPUT:
+               mt_pmx_gpio_set_direction(pctldev, NULL, pin, 0);
+               mt_gpio_set(pctl->chip, pin, argument);
+               return 0;
+
+       default:
+               return -EINVAL;
+       }

Cut the whitespace newlines above I think.

+       if (pullen)
+               reg_pullen = mt_get_port(pin) +
+                       pctl->devdata->pullen_offset + 4;
+       else
+               reg_pullen = mt_get_port(pin) +
+                       pctl->devdata->pullen_offset + (4 << 1);

+8

+
+       if (pullsel)
+               reg_pullsel = mt_get_port(pin) +
+                       pctl->devdata->pullsel_offset + 4;
+       else
+               reg_pullsel = mt_get_port(pin) +
+                       pctl->devdata->pullsel_offset + (4 << 1);

+8

+       mt_pctrl_write_reg(pctl, pin, reg_pullen, bit);
+       mt_pctrl_write_reg(pctl, pin, reg_pullsel, bit);
+       return 0;
+}

(...)

+static int mt_pctrl_is_function_valid(struct mt_pinctrl *pctl,
+               u32 pin_num, u32 fnum)

Return type should be bool.

+{
+       int i;
+
+       for (i = 0; i < pctl->devdata->npins; i++) {
+               const struct mt_desc_pin *pin = pctl->devdata->pins + i;
+
+               if (pin->pin.number == pin_num) {
+                       struct mt_desc_function *func = pin->functions + fnum;
+
+                       if (func->name)
+                               return 1;
+                       else
+                               return 0;
+               }
+       }
+
+       return 0;
+}

return true/false;

+static int mt_pctrl_dt_node_to_map_func(struct mt_pinctrl *pctl, u32 pin,
+               u32 fnum, struct pinctrl_map **maps)
+static int mt_pctrl_dt_node_to_map_config(struct mt_pinctrl *pctl, u32 pin,
+               unsigned long *configs, unsigned num_configs,
+               struct pinctrl_map **maps)
+static void mt_pctrl_dt_free_map(struct pinctrl_dev *pctldev,
+                                   struct pinctrl_map *map,
+                                   unsigned num_maps)
+static int mt_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
+                                     struct device_node *node,
+                                     struct pinctrl_map **map,
+                                     unsigned *num_maps)

I'm worried about the escalating number of custom function->group
and pin config bindings, so I have submitted patches to fix this up and
allow for all-generic bindings to be used.

See:
http://marc.info/?l=devicetree&m=141223584006648&w=2
http://marc.info/?l=devicetree&m=141223586106655&w=2

+       pins = of_find_property(node, "mediatek,pinfunc", NULL);

So I want to get rid of "mediatek,pinfunc" and just use "function"
for this.

+       err = pinconf_generic_parse_dt_config(node, &configs, &num_configs);
+       if (num_configs)
+               has_config = 1;

This implicitly uses "pins", which is nice.

+static int mt_gpio_set_mode(struct pinctrl_dev *pctldev,
+               unsigned long pin, unsigned long mode)
+{
+       unsigned int reg_addr;
+       unsigned char bit;
+       unsigned int val;
+       unsigned long flags;
+       unsigned int mask = (1L << GPIO_MODE_BITS) - 1;
+       struct mt_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+
+       reg_addr = ((pin / 5) << 4) + pctl->devdata->pinmux_offset;

That 5 and 4 needs some explanation, or atleast being #defined
for readability.

+       spin_lock_irqsave(&pctl->lock, flags);
+       val = readl(pctl->membase1 + reg_addr);

readl_relaxed()?

+static struct mt_desc_function *
+mt_pctrl_desc_find_irq_by_name(struct mt_pinctrl *pctl,
+                                        const char *pin_name)

Why is it called *find_irq_by_name if it returns a
function? Seems more like find_function_from_pin_name
really.

And I don't know if that is such a good idea.

+{
+       int i, j;
+
+       for (i = 0; i < pctl->devdata->npins; i++) {
+               const struct mt_desc_pin *pin = pctl->devdata->pins + i;
+
+               if (!strcmp(pin->pin.name, pin_name)) {
+                       struct mt_desc_function *func = pin->functions;
+
+                       for (j = 0; j < PINMUX_MAX_VAL; j++) {
+                               if (func->irqnum != 255)

So why does it end at 255? Seems pretty arbitrary.

+                                       return func;
+
+                               func++;
+                       }
+               }
+       }
+
+       return NULL;
+}

+static int mt_pmx_enable(struct pinctrl_dev *pctldev,
+                           unsigned function,
+                           unsigned group)

This is typically named pmx_set_mux() nowadays, please
use the pin control development tree at this point, the change will
be upstream in v3.18.

+static const struct pinmux_ops mt_pmx_ops = {
+       .get_functions_count    = mt_pmx_get_funcs_cnt,
+       .get_function_name      = mt_pmx_get_func_name,
+       .get_function_groups    = mt_pmx_get_func_groups,
+       .enable                 = mt_pmx_enable,

.set_mux =...

+static int mt_gpio_request(struct gpio_chip *chip, unsigned offset)
+{
+       return pinctrl_request_gpio(chip->base + offset);
+}
+
+static void mt_gpio_free(struct gpio_chip *chip, unsigned offset)
+{
+       pinctrl_free_gpio(chip->base + offset);
+}

+static int mt_gpio_direction_input(struct gpio_chip *chip,
+                                       unsigned offset)
+{
+       return pinctrl_gpio_direction_input(chip->base + offset);
+}
+
+static int mt_gpio_direction_output(struct gpio_chip *chip,
+                                       unsigned offset, int value)
+{
+       return pinctrl_gpio_direction_output(chip->base + offset);
+}

This is nice.

+static int mt_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
+{
+       unsigned int reg_addr;
+       unsigned int bit;
+       unsigned int read_val = 0;
+
+       struct mt_pinctrl *pctl = dev_get_drvdata(chip->dev);
+
+       reg_addr =  mt_get_port(offset) + pctl->devdata->dir_offset;
+       bit = 1 << (offset & 0xf);

bit = BIT(offset & 0xf);

+       read_val = readl(pctl->membase1 + reg_addr);
+       return ((read_val & bit) != 0) ? 1 : 0;

No do it like this:

return !!(read_val & bit);

+static int mt_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+       unsigned int reg_addr;
+       unsigned int bit;
+       unsigned int read_val = 0;
+       struct mt_pinctrl *pctl = dev_get_drvdata(chip->dev);
+
+       if (mt_gpio_get_direction(chip, offset))
+               reg_addr = mt_get_port(offset) + pctl->devdata->dout_offset;
+       else
+               reg_addr = mt_get_port(offset) + pctl->devdata->din_offset;
+
+       bit = 1 << (offset & 0xf);
+       read_val = readl(pctl->membase1 + reg_addr);
+       return ((read_val & bit) != 0) ? 1 : 0;
+}

Same comments on this function.

+static int mt_gpio_of_xlate(struct gpio_chip *gc,
+                               const struct of_phandle_args *gpiospec,
+                               u32 *flags)
+{
+       int pin;
+
+       pin = gpiospec->args[0];
+
+       if (pin > (gc->base + gc->ngpio))
+               return -EINVAL;
+
+       if (flags)
+               *flags = gpiospec->args[1];
+
+       return pin;
+}

Why do you need your own xlate function to do this?

What is wrong with of_gpio_simple_xlate() which seems to do
the same thing?

Incidentally that is what you will get if you just leave this
pointer in the vtable as unassigned.

+static int mt_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
+{
+       struct mt_pinctrl *pctl = dev_get_drvdata(chip->dev);
+       struct mt_pinctrl_group *g = pctl->groups + offset;
+       struct mt_desc_function *desc = mt_pctrl_desc_find_irq_by_name(
+                       pctl, g->name);
+       if (!desc)
+               return -EINVAL;
+
+       mt_gpio_set_mode(pctl->pctl_dev, g->pin, desc->muxval);

No mode setting in the .to_irq() function, that makes the irqchip
not orthogonal to the gpio_chip.

+       return desc->irqnum;
+}

By the way use GPIOLIB_IRQCHIP for this and get rid
of .to_irq altogether.

(...)

+static int mt_pctrl_build_state(struct platform_device *pdev)
+{
+       struct mt_pinctrl *pctl = platform_get_drvdata(pdev);
+       int i;
+
+       pctl->ngroups = pctl->devdata->npins;
+
+       pctl->groups = devm_kzalloc(&pdev->dev,
+                                   pctl->ngroups * sizeof(*pctl->groups),
+                                   GFP_KERNEL);
+       if (!pctl->groups)
+               return -ENOMEM;
+
+       pctl->grp_names = devm_kzalloc(&pdev->dev,
+                                   pctl->ngroups * sizeof(*pctl->grp_names),
+                                   GFP_KERNEL);
+       if (!pctl->grp_names)
+               return -ENOMEM;
+
+       for (i = 0; i < pctl->devdata->npins; i++) {
+               const struct mt_desc_pin *pin = pctl->devdata->pins + i;
+               struct mt_pinctrl_group *group = pctl->groups + i;
+               const char **func_grp;
+
+               group->name = pin->pin.name;
+               group->pin = pin->pin.number;
+
+               func_grp = pctl->grp_names;
+               while (*func_grp)
+                       func_grp++;
+
+               *func_grp = pin->pin.name;
+       }
+
+       return 0;
+}

I don't understand what this function is doing so atleast it need
and explanation in kerneldoc above it.

(...)

+       ret = gpiochip_add(pctl->chip);
+       if (ret) {
+               ret = -EINVAL;
+               goto pctrl_error;
+       }

Here  you should be using gpiochip_irqchip_add()
followed by gpiochip_set_chained_irqchip().

+       for (i = 0; i < pctl->devdata->npins; i++) {
+               const struct mt_desc_pin *pin = pctl->devdata->pins + i;
+
+               ret = gpiochip_add_pin_range(pctl->chip, dev_name(&pdev->dev),
+                                            pin->pin.number,
+                                            pin->pin.number, 1);
+               if (ret) {
+                       ret = -EINVAL;
+                       goto chip_error;
+               }

Seems complicated but I don't know how complicated
your GPIO ranges are indeed.

+chip_error:
+       if (gpiochip_remove(pctl->chip))

We have removed the return value from gpiochip_remove() so rebase
to upstream here. No if(..)

(...)

+struct mt_desc_pin {
+       struct pinctrl_pin_desc pin;
+       const char *chip;
+       struct mt_desc_function *functions;

Why does a pin need to know about functions...

+};

Don't invent custom pin container structures. Look:

/**
 * struct pinctrl_pin_desc - boards/machines provide information on their
 * pins, pads or other muxable units in this struct
 * @number: unique pin number from the global pin number space
 * @name: a name for this pin
 * @drv_data: driver-defined per-pin data. pinctrl core does not touch this
 */
struct pinctrl_pin_desc {
        unsigned number;
        const char *name;
        void *drv_data;
};

You add your stuff to drv_data() rather than including this struct
into your own struct.

+#define MT_PIN(_pin, _pad, _chip, ...)                         \
+       {                                                       \
+               .pin = _pin,                                    \
+               .chip = _chip,                                  \
+               .functions = (struct mt_desc_function[]){       \
+                       __VA_ARGS__, { } },                     \
+       }
+
+#define MT_FUNCTION(_val, _name)                               \
+       {                                                       \
+               .name = _name,                                  \
+               .muxval = _val,                                 \
+               .irqnum = 255,                                  \

255 eh?

+       }
+
+#define MT_FUNCTION_IRQ(_val, _name, _irq)                     \
+       {                                                       \
+               .name = _name,                                  \
+               .muxval = _val,                                 \
+               .irqnum = _irq,                                 \
+       }
+
+struct mt_pinctrl_group {
+       const char      *name;
+       unsigned long   config;
+       unsigned        pin;
+};
+
+struct mt_gpio_devdata {
+       const struct mt_desc_pin *pins;
+       int npins;
+       unsigned int dir_offset;
+       unsigned int ies_offset;
+       unsigned int pullen_offset;
+       unsigned int pullsel_offset;
+       unsigned int drv_offset;
+       unsigned int invser_offset;
+       unsigned int dout_offset;
+       unsigned int din_offset;
+       unsigned int pinmux_offset;
+       unsigned short type1_start;
+       unsigned short type1_end;
+};

Add some kerneldoc for this struct as it't not apparently
self-evident.

+static const struct mt_desc_pin mt_pins_mt8135[] = {
+       MT_PIN(
+               PINCTRL_PIN(0, "MSDC0_DAT7"),
+               "D21", "mt8135",
+               MT_FUNCTION(0, "GPIO0"),
+               MT_FUNCTION(1, "MSDC0_DAT7"),
+               MT_FUNCTION_IRQ(2, "EINT49", 49),
+               MT_FUNCTION(3, "I2SOUT_DAT"),
+               MT_FUNCTION(4, "DAC_DAT_OUT"),
+               MT_FUNCTION(5, "PCM1_DO"),
+               MT_FUNCTION(6, "SPI1_MO"),
+               MT_FUNCTION(7, "NALE")
+       ),

I don't think this is a good idea and to encode all functions in a pin,
rather the revers is custom: define all functions and collect arrays
of pin numbers in the definitions of pin groups, then map the functions
and groups of pins together.

Look at other drivers for examples..

I don't like the device tree bindings for the very same reason: it moves
all this numeric encoding of pin-functions into the device tree instead
of combining group+function strings like most drivers do.

Is there some special reason to why you're turning this on its head?

Yours,
Linus Walleij