Re: [PATCH v4 4/5] pinctrl: airoha: Add support for EN7581 SoC

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Lorenzo / Benjamin,

thanks for your patch!

This is a real nice driver, I like the design of the pin database to support
this pretty complex pin controller.

Some comments and nits:

On Wed, Sep 11, 2024 at 9:51 PM Lorenzo Bianconi <lorenzo@xxxxxxxxxx> wrote:

> Introduce pinctrl driver for EN7581 SoC. Current EN7581 pinctrl driver
> supports the following functionalities:
> - pin multiplexing
> - pin pull-up, pull-down, open-drain, current strength,
>   {input,output}_enable, output_{low,high}
> - gpio controller
> - irq controller
>
> Tested-by: Benjamin Larsson <benjamin.larsson@xxxxxxxxxx>
> Co-developed-by: Benjamin Larsson <benjamin.larsson@xxxxxxxxxx>
> Signed-off-by: Benjamin Larsson <benjamin.larsson@xxxxxxxxxx>
> Signed-off-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx>

(...)

> +#include <dt-bindings/pinctrl/mt65xx.h>
> +#include <linux/bitfield.h>

Isn't just <linux/bits.h> enough for what you're using?

> +#include <linux/gpio/driver.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/kernel.h>

What do you use from kernel.h? We usually use more fingrained
headers these days.

(...)

> +#include <linux/mfd/airoha-en7581-mfd.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/pinctrl/consumer.h>

Why do you need the consumer header?

(...)

> +static u32 airoha_pinctrl_rmw_unlock(void __iomem *addr, u32 mask, u32 val)
> +{
> +       val |= (readl(addr) & ~mask);
> +       writel(val, addr);
> +
> +       return val;
> +}
> +
> +#define airoha_pinctrl_set_unlock(addr, val)                                   \
> +       airoha_pinctrl_rmw_unlock((addr), 0, (val))
> +#define airoha_pinctrl_clear_unlock(addr, mask)                                        \
> +       airoha_pinctrl_rmw_unlock((addr), (mask), (0))
> +
> +static u32 airoha_pinctrl_rmw(struct airoha_pinctrl *pinctrl,
> +                             void __iomem *addr, u32 mask, u32 val)
> +{
> +       mutex_lock(&pinctrl->mutex);
> +       val = airoha_pinctrl_rmw_unlock(addr, mask, val);
> +       mutex_unlock(&pinctrl->mutex);
> +
> +       return val;
> +}

Thus looks like a reimplementation of regmap-mmio, can't you just use
regmap MMIO? You use it for the SCU access already...

If you persist with this solution, please use a guard:

#include <linux/cleanup.h>

guard(mutex)(&pinctrl->mutex);

And the lock will be released when you exit the function.

> +static int airoha_pinctrl_get_gpio_from_pin(struct pinctrl_dev *pctrl_dev,
> +                                           int pin)
> +{
> +       struct pinctrl_gpio_range *range;
> +       int gpio;
> +
> +       range = pinctrl_find_gpio_range_from_pin_nolock(pctrl_dev, pin);
> +       if (!range)
> +               return -EINVAL;
> +
> +       gpio = pin - range->pin_base;
> +       if (gpio < 0)
> +               return -EINVAL;
> +
> +       return gpio;
> +}

This function is just used here:

> +static int airoha_pinconf_get(struct pinctrl_dev *pctrl_dev,
> +                             unsigned int pin, unsigned long *config)
> +{
> +       struct airoha_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctrl_dev);
> +       enum pin_config_param param = pinconf_to_config_param(*config);
> +       u32 arg;
> +
> +       switch (param) {
> +       case PIN_CONFIG_BIAS_PULL_DOWN:
> +       case PIN_CONFIG_BIAS_DISABLE:
> +       case PIN_CONFIG_BIAS_PULL_UP: {
> +               u32 pull_up, pull_down;
> +
> +               if (airoha_pinctrl_get_pullup_conf(pinctrl, pin, &pull_up) ||
> +                   airoha_pinctrl_get_pulldown_conf(pinctrl, pin, &pull_down))
> +                       return -EINVAL;
> +
> +               if (param == PIN_CONFIG_BIAS_PULL_UP &&
> +                   !(pull_up && !pull_down))
> +                       return -EINVAL;
> +               else if (param == PIN_CONFIG_BIAS_PULL_DOWN &&
> +                        !(pull_down && !pull_up))
> +                       return -EINVAL;
> +               else if (pull_up || pull_down)
> +                       return -EINVAL;
> +
> +               arg = 1;
> +               break;
> +       }
> +       case PIN_CONFIG_DRIVE_STRENGTH: {
> +               u32 e2, e4;
> +
> +               if (airoha_pinctrl_get_drive_e2_conf(pinctrl, pin, &e2) ||
> +                   airoha_pinctrl_get_drive_e4_conf(pinctrl, pin, &e4))
> +                       return -EINVAL;
> +
> +               arg = e4 << 1 | e2;
> +               break;
> +       }
> +       case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> +               if (airoha_pinctrl_get_pcie_rst_od_conf(pinctrl, pin, &arg))
> +                       return -EINVAL;
> +               break;
> +       case PIN_CONFIG_OUTPUT_ENABLE:
> +       case PIN_CONFIG_INPUT_ENABLE: {
> +               int gpio = airoha_pinctrl_get_gpio_from_pin(pctrl_dev, pin);
> +
> +               if (gpio < 0)
> +                       return gpio;
> +
> +               arg = airoha_pinctrl_gpio_get_direction(pinctrl, gpio);

I don't see why a pin would have to exist in a GPIO range in order to
be set as output or input?

Can't you just set up the pin as requested and not care whether
it has a corresponding GPIO range?

Is it over-reuse of the GPIO code? I'd say just set up the pin instead.

> +static int airoha_pinconf_set(struct pinctrl_dev *pctrl_dev,
> +                             unsigned int pin, unsigned long *configs,
> +                             unsigned int num_configs)
> +{
> +       struct airoha_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctrl_dev);
> +       int i;
> +
> +       for (i = 0; i < num_configs; i++) {
> +               u32 param = pinconf_to_config_param(configs[i]);
> +               u32 arg = pinconf_to_config_argument(configs[i]);
> +
> +               switch (param) {
> +               case PIN_CONFIG_BIAS_DISABLE:
> +                       airoha_pinctrl_set_pulldown_conf(pinctrl, pin, 0);
> +                       airoha_pinctrl_set_pullup_conf(pinctrl, pin, 0);
> +                       break;
> +               case PIN_CONFIG_BIAS_PULL_UP:
> +                       airoha_pinctrl_set_pulldown_conf(pinctrl, pin, 0);
> +                       airoha_pinctrl_set_pullup_conf(pinctrl, pin, 1);
> +                       break;
> +               case PIN_CONFIG_BIAS_PULL_DOWN:
> +                       airoha_pinctrl_set_pulldown_conf(pinctrl, pin, 1);
> +                       airoha_pinctrl_set_pullup_conf(pinctrl, pin, 0);
> +                       break;
> +               case PIN_CONFIG_DRIVE_STRENGTH: {
> +                       u32 e2 = 0, e4 = 0;
> +
> +                       switch (arg) {
> +                       case MTK_DRIVE_2mA:
> +                               break;
> +                       case MTK_DRIVE_4mA:
> +                               e2 = 1;
> +                               break;
> +                       case MTK_DRIVE_6mA:
> +                               e4 = 1;
> +                               break;
> +                       case MTK_DRIVE_8mA:
> +                               e2 = 1;
> +                               e4 = 1;
> +                               break;
> +                       default:
> +                               return -EINVAL;
> +                       }
> +
> +                       airoha_pinctrl_set_drive_e2_conf(pinctrl, pin, e2);
> +                       airoha_pinctrl_set_drive_e4_conf(pinctrl, pin, e4);
> +                       break;
> +               }
> +               case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> +                       airoha_pinctrl_set_pcie_rst_od_conf(pinctrl, pin, !!arg);
> +                       break;
> +               case PIN_CONFIG_OUTPUT_ENABLE:
> +               case PIN_CONFIG_INPUT_ENABLE:
> +               case PIN_CONFIG_OUTPUT: {
> +                       int gpio = airoha_pinctrl_get_gpio_from_pin(pctrl_dev, pin);
> +                       bool input = param == PIN_CONFIG_INPUT_ENABLE;
> +
> +                       if (gpio < 0)
> +                               return gpio;
> +
> +                       airoha_pinctrl_gpio_set_direction(pinctrl, gpio, input);
> +                       if (param == PIN_CONFIG_OUTPUT)
> +                               airoha_pinctrl_gpio_set_value(pinctrl, gpio, !!arg);
> +                       break;

Dito. No need to reuse the GPIO set direction function. Make a helper
that just work on the pin instead, and perhaps the GPIO set direction
can use that instead.

> +static int airoha_pinctrl_gpio_direction_output(struct gpio_chip *chip,
> +                                               unsigned int gpio, int value)
> +{
> +       int err;
> +
> +       err = pinctrl_gpio_direction_output(chip, gpio);
> +       if (err)
> +               return err;
> +
> +       airoha_pinctrl_gpio_set(chip, gpio, value);

Hm I get a bit confused by the similarly named helpers I guess...

> +static void airoha_pinctrl_gpio_irq_unmask(struct irq_data *data)
> +{
> +       u8 offset = data->hwirq % AIROHA_REG_GPIOCTRL_NUM_GPIO;
> +       u8 index = data->hwirq / AIROHA_REG_GPIOCTRL_NUM_GPIO;
> +       u32 mask = GENMASK(2 * offset + 1, 2 * offset);
> +       struct airoha_pinctrl_gpiochip *gpiochip;
> +       u32 val = BIT(2 * offset);
> +       unsigned long flags;
> +
> +       gpiochip = irq_data_get_irq_chip_data(data);
> +       if (WARN_ON_ONCE(data->hwirq >= ARRAY_SIZE(gpiochip->irq_type)))
> +               return;
> +
> +       spin_lock_irqsave(&gpiochip->lock, flags);

Use a scoped guard here

guard(spinlock_irqsave)(&gpiochip->lock);

> +static void airoha_pinctrl_gpio_irq_mask(struct irq_data *data)
> +{
> +       u8 offset = data->hwirq % AIROHA_REG_GPIOCTRL_NUM_GPIO;
> +       u8 index = data->hwirq / AIROHA_REG_GPIOCTRL_NUM_GPIO;
> +       u32 mask = GENMASK(2 * offset + 1, 2 * offset);
> +       struct airoha_pinctrl_gpiochip *gpiochip;
> +       unsigned long flags;
> +
> +       gpiochip = irq_data_get_irq_chip_data(data);
> +
> +       spin_lock_irqsave(&gpiochip->lock, flags);

Dito

> +static int airoha_pinctrl_gpio_irq_type(struct irq_data *data,
> +                                       unsigned int type)
> +{
> +       struct airoha_pinctrl_gpiochip *gpiochip;
> +       unsigned long flags;
> +
> +       gpiochip = irq_data_get_irq_chip_data(data);
> +       if (data->hwirq >= ARRAY_SIZE(gpiochip->irq_type))
> +               return -EINVAL;
> +
> +       spin_lock_irqsave(&gpiochip->lock, flags);

Dito

> +       girq->chip = devm_kzalloc(dev, sizeof(*girq->chip), GFP_KERNEL);
> +       if (!girq->chip)
> +               return -ENOMEM;
> +
> +       girq->chip->name = dev_name(dev);
> +       girq->chip->irq_unmask = airoha_pinctrl_gpio_irq_unmask;
> +       girq->chip->irq_mask = airoha_pinctrl_gpio_irq_mask;
> +       girq->chip->irq_mask_ack = airoha_pinctrl_gpio_irq_mask;
> +       girq->chip->irq_set_type = airoha_pinctrl_gpio_irq_type;
> +       girq->chip->flags = IRQCHIP_SET_TYPE_MASKED | IRQCHIP_IMMUTABLE;
> +       girq->default_type = IRQ_TYPE_NONE;
> +       girq->handler = handle_simple_irq;

If the irqchip is immutable it is const and there is no point to malloc it.

Just

static const struct irq_chip airoha_gpio_irq_chip = {...

And assign it:

girq = &g->gc.irq;
gpio_irq_chip_set_chip(girq, &airoha_gpio_irq_chip);

Yours,
Linus Walleij





[Index of Archives]     [Linux SPI]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux