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

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 10267 bytes --]

[...]
> 
> I am fine to switch to regmap but I guess we need to enable fast_io
> since it can run even in interrupt context. Btw, I figured out even
> airoha_pinctrl_rmw needs to grab a spin_lock since we can exec a led
> trigger (like timer) where we run airoha_pinctrl_rmw in interrupt context
> (so it should be fine to use a single regmap for it).
> However, I guess we need to keep the spin_lock in airoha_pinctrl_gpiochip
> since we need to grab it in airoha_pinctrl_gpio_irq_unmask() and
> airoha_pinctrl_gpio_irq_type() (we access irq_type array there).
> A possible solution would be to keep the local spin_lock and set
> disable_locking. What do you think? Do you prefer to switch to regmap or
> keep the current implementation using 'guard(spinlock_irqsave)' instead?

I reworked the code using regmap APIs and setting disable_locking flag
in order to keep the spinlock local (I switch to guard(spinlock) as
well). Thx for pointing this out, the code is simpler and more readable,
I will add it in v5.

> 
> > 
> > > +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:
> 
> it is used in airoha_pinconf_get()/airoha_pinconf_set()
> 
> > 

[...]

> > > +       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.
> 
> Do you mean to get rid of PIN_CONFIG_OUTPUT_ENABLE, PIN_CONFIG_INPUT_ENABLE
> (and even PIN_CONFIG_OUTPUT in airoha_pinconf_set()) here?
> E.g. we need PIN_CONFIG_OUTPUT_ENABLE to enable pwm for pwm-leds:
> 
> &mfd {
> 	...
> 	pio: pinctrl {
> 		...
> 		pwm_gpio18_idx10_pins: pwm-gpio18-idx10-pins {
> 			function = "pwm";
> 			pins = "gpio18";
> 			output-enable;
> 		};
> 	};
> };

I reworked the code here in order to not explicitly use gpio value in
airoha_pinconf_get/airoha_pinconf_set routines. However, we need to switch
from pin to gpio configuring data/direction/out hw registers since:
- not all pins can be used as gpio (actually we can configure just pins in the
  range [13, 59])
- data, dir and out hw register are indexed using gpio id and not pin one.
  (e.g BIT(0) in data[0] refers to GPIO0 and not to PIN0).

Regards,
Lorenzo

> 
> > 
> > > +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.
> 
> ack, I will fix it in v5.
> 
> > 
> > > +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...
> 
> Naming is always hard, I will try to improve :)
> 
> > 
> > > +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);
> 
> ack, I will fix it in v5.
> 
> Regards,
> Lorenzo
> 
> > 
> > Yours,
> > Linus Walleij



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]