On Wed, Oct 27, 2021 at 5:28 AM Joey Gouly [off-list ref] 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