Re: [PATCH v5 4/5] pinctrl: add pinctrl/GPIO driver for Apple SoCs

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

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