On Wed, Oct 27, 2021 at 5:28 AM Joey Gouly <joey.gouly@xxxxxxx> wrote:
>
> This driver adds support for the pinctrl / GPIO hardware found
> on some Apple SoCs.
...
> +#include <dt-bindings/pinctrl/apple.h>
bits.h is missed
> +#include <linux/gpio/driver.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
...
> +struct regmap_config regmap_config = {
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = 4,
> + .cache_type = REGCACHE_FLAT,
> + .max_register = 512 * sizeof(u32),
> + .num_reg_defaults_raw = 512,
> + .use_relaxed_mmio = true
Would be good to have a comma.
> +};
...
> +// No locking needed to mask/unmask IRQs as the interrupt mode is per pin-register.
Wrong style.
...
> +static uint32_t apple_gpio_get_reg(struct apple_gpio_pinctrl *pctl,
> + unsigned int pin)
> +{
> + unsigned int val = 0;
> +
> + regmap_read(pctl->map, REG_GPIO(pin), &val);
> + return val;
Better
unsigned int val;
int ret;
ret = regmap_read(...);
if (ret)
return 0;
return val;
> +}
...
> + ret = of_property_count_u32_elems(node, "pinmux");
> + if (ret <= 0) {
> + dev_err(pctl->dev,
> + "missing or empty pinmux property in node %pOFn.\n",
> + node);
> + return ret;
This is incorrect. It always happens when somebody is in hurry :-)
> + }
...
> + apple_gpio_set_reg(
> + pctl, group, REG_GPIOx_PERIPH | REG_GPIOx_INPUT_ENABLE,
> + FIELD_PREP(REG_GPIOx_PERIPH, func) | REG_GPIOx_INPUT_ENABLE);
Strange indentation.
...
> + return (FIELD_GET(REG_GPIOx_MODE, reg) == REG_GPIOx_OUT) ?
> + GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;
Easier to read with
if ()
return X
return Y
...
> + apple_gpio_set_reg(pctl, offset, REG_GPIOx_DATA,
> + value ? REG_GPIOx_DATA : 0);
One line?
> +}
...
> + struct apple_gpio_pinctrl *pctl =
> + gpiochip_get_data(irq_data_get_irq_chip_data(data));
One line?
> + unsigned int irqgrp =
> + FIELD_GET(REG_GPIOx_GRP, apple_gpio_get_reg(pctl, data->hwirq));
Ditto.
> + writel(BIT(data->hwirq & 31),
% 32 can also do and be more specific.
> + pctl->base + REG_IRQ(irqgrp, data->hwirq));
One line?
...
> +static void apple_gpio_irq_mask(struct irq_data *data)
> +{
> + struct apple_gpio_pinctrl *pctl =
> + gpiochip_get_data(irq_data_get_irq_chip_data(data));
Missed blank line.
> + apple_gpio_set_reg(pctl, data->hwirq, REG_GPIOx_MODE,
> + FIELD_PREP(REG_GPIOx_MODE, REG_GPIOx_IN_IRQ_OFF));
> +}
...
> + if (!of_property_read_bool(pctl->dev->of_node, "gpio-controller"))
> + return dev_err_probe(pctl->dev, -ENODEV,
> + "No gpio-controller property\n");
Still not sure why we need this check.
...
> + pctl->gpio_chip.of_node = pctl->dev->of_node;
It should be done by the OF GPIO library.
> + for (i = 0; i < girq->num_parents; i++) {
> + ret = platform_get_irq(to_platform_device(pctl->dev),
> + i);
This looks ugly even if you take the 80 char rule to your heart (it
has exceptions and here is one of them).
...
> + ret = devm_gpiochip_add_data(pctl->dev, &pctl->gpio_chip, pctl);
> +out:
out_free_irq_data:
(or alike)
> + kfree(girq->parents);
> + kfree(irq_data);
...
> + static const char* pinmux_functions[] = {
> + "gpio", "periph1", "periph2", "periph3"
> + };
...
I see it seems pending, so some of the above may be addressed with
follow up, some may be left unconsidered.
--
With Best Regards,
Andy Shevchenko
