> 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.
Hi Linus,
thx for the review, some questions inline.
Regards,
Lorenzo
>
> 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?
ack, I will fix it in v5
>
> > +#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.
ack, I will remove it in v5
>
> (...)
>
> > +#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?
we need it for pinctrl_gpio_direction_output() and
pinctrl_gpio_direction_input() for direction_input and direction_output
callbacks.
>
> (...)
>
> > +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.
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?
>
> > +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()
>
> > +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.
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;
};
};
};
>
> > +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:
signature.asc
Description: PGP signature
