| Message ID | 20240911-en7581-pinctrl-v4-0-60ac93d760bb@kernel.org |
|---|---|
| Headers | show |
| Series | Add mfd, pinctrl and pwm support to EN7581 SoC | expand |
On Wed, Sep 11, 2024 at 09:50:00PM +0200, Lorenzo Bianconi wrote: > Introduce airoha-mfd driver in order to load pinctrl and pwm drivers for > EN7581 SoC. airoha-mfd is needed since both pinctrl and pwm drivers > needs to access the same memory block (gpio memory region) to configure > {gio,irq}_chip and pwm functionalities respectively, so model them as > childs of a parent mfd driver. > Current EN7581 pinctrl driver supports the following functionalities: > - pin multiplexing via chip_scu syscon > - pin pull-up, pull-down, open-drain, current strength, > {input,output}_enable, output_{low,high} via chip_scu syscon > - gpio controller > - irq controller > > --- > Changes in v4: > - add 'Limitation' description in pwm driver > - fix comments in pwm driver > - rely on mfd->base __iomem pointer in pwm driver, modify register > offsets according to it and get rid of sgpio_cfg, flash_cfg and > cycle_cfg pointers > - simplify register utility routines in pwm driver > - use 'generator' instead of 'waveform' suffix for pwm routines > - fix possible overflow calculating duty cycle in pwm driver > - do not modify pwm state in free callback in pwm driver > - cap the maximum period in pwm driver > - do not allow inverse polarity in pwm driver > - do not set of_xlate callback in the pwm driver and allow the stack to > do it > - fix MAINTAINERS file for airoha pinctrl driver > - fix undefined reference to __ffsdi2 in pinctrl driver > - simplify airoha,en7581-gpio-sysctl.yam binding > - Link to v3: https://lore.kernel.org/r/20240831-en7581-pinctrl-v3-0-98eebfb4da66@kernel.org > > Changes in v3: > - introduce airoha-mfd driver > - add pwm driver to the same series > - model pinctrl and pwm drivers as childs of a parent mfd driver. > - access chip-scu memory region in pinctrl driver via syscon > - introduce a single airoha,en7581-gpio-sysctl.yaml binding and get rid > of dedicated bindings for pinctrl and pwm > - add airoha,en7581-chip-scu.yaml binding do the series > - Link to v2: https://lore.kernel.org/r/20240822-en7581-pinctrl-v2-0-ba1559173a7f@kernel.org > > Changes in v2: > - Fix compilation errors > - Collapse some register mappings for gpio and irq controllers > - update dt-bindings according to new register mapping > - fix some dt-bindings errors > - Link to v1: https://lore.kernel.org/all/cover.1723392444.git.lorenzo@kernel.org/ > > --- > Benjamin Larsson (1): > pwm: airoha: Add support for EN7581 SoC > > Christian Marangi (2): > dt-bindings: mfd: Add support for Airoha EN7581 GPIO System Controller > mfd: airoha: Add support for Airoha EN7581 MFD > > Lorenzo Bianconi (2): > dt-bindings: arm: airoha: Add the chip-scu node for EN7581 SoC > pinctrl: airoha: Add support for EN7581 SoC > > .../bindings/arm/airoha,en7581-chip-scu.yaml | 42 + > .../bindings/mfd/airoha,en7581-gpio-sysctl.yaml | 433 +++ > MAINTAINERS | 7 + > drivers/mfd/Kconfig | 8 + > drivers/mfd/Makefile | 2 + > drivers/mfd/airoha-en7581-gpio-mfd.c | 72 + > drivers/pinctrl/mediatek/Kconfig | 16 +- > drivers/pinctrl/mediatek/Makefile | 1 + > drivers/pinctrl/mediatek/pinctrl-airoha.c | 2964 ++++++++++++++++++++ > drivers/pwm/Kconfig | 10 + > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-airoha.c | 414 +++ > include/linux/mfd/airoha-en7581-mfd.h | 9 + > 13 files changed, 3978 insertions(+), 1 deletion(-) > --- > base-commit: 264c13114bd71ddfd7b25c7b94f6cda4587eca25 > change-id: 20240818-en7581-pinctrl-1bf120154be0 > prerequisite-change-id: 20240705-for-6-11-bpf-a349efc08df8:v2 > > Hi, any news with this? Rob reviewed the DT schemas and he is ok with them. Any other comments for the MFD driver and/or the pinctrl or PWM driver?
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@kernel.org> 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@genexis.eu> > Co-developed-by: Benjamin Larsson <benjamin.larsson@genexis.eu> > Signed-off-by: Benjamin Larsson <benjamin.larsson@genexis.eu> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> (...) > +#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
> 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@kernel.org> 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@genexis.eu> > > Co-developed-by: Benjamin Larsson <benjamin.larsson@genexis.eu> > > Signed-off-by: Benjamin Larsson <benjamin.larsson@genexis.eu> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > (...) > > > +#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
[...] > > 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
On Mon, 23 Sep 2024, Christian Marangi wrote: > On Wed, Sep 11, 2024 at 09:50:00PM +0200, Lorenzo Bianconi wrote: > > Introduce airoha-mfd driver in order to load pinctrl and pwm drivers for > > EN7581 SoC. airoha-mfd is needed since both pinctrl and pwm drivers > > needs to access the same memory block (gpio memory region) to configure > > {gio,irq}_chip and pwm functionalities respectively, so model them as > > childs of a parent mfd driver. > > Current EN7581 pinctrl driver supports the following functionalities: > > - pin multiplexing via chip_scu syscon > > - pin pull-up, pull-down, open-drain, current strength, > > {input,output}_enable, output_{low,high} via chip_scu syscon > > - gpio controller > > - irq controller > > > > --- > > Changes in v4: > > - add 'Limitation' description in pwm driver > > - fix comments in pwm driver > > - rely on mfd->base __iomem pointer in pwm driver, modify register > > offsets according to it and get rid of sgpio_cfg, flash_cfg and > > cycle_cfg pointers > > - simplify register utility routines in pwm driver > > - use 'generator' instead of 'waveform' suffix for pwm routines > > - fix possible overflow calculating duty cycle in pwm driver > > - do not modify pwm state in free callback in pwm driver > > - cap the maximum period in pwm driver > > - do not allow inverse polarity in pwm driver > > - do not set of_xlate callback in the pwm driver and allow the stack to > > do it > > - fix MAINTAINERS file for airoha pinctrl driver > > - fix undefined reference to __ffsdi2 in pinctrl driver > > - simplify airoha,en7581-gpio-sysctl.yam binding > > - Link to v3: https://lore.kernel.org/r/20240831-en7581-pinctrl-v3-0-98eebfb4da66@kernel.org > > > > Changes in v3: > > - introduce airoha-mfd driver > > - add pwm driver to the same series > > - model pinctrl and pwm drivers as childs of a parent mfd driver. > > - access chip-scu memory region in pinctrl driver via syscon > > - introduce a single airoha,en7581-gpio-sysctl.yaml binding and get rid > > of dedicated bindings for pinctrl and pwm > > - add airoha,en7581-chip-scu.yaml binding do the series > > - Link to v2: https://lore.kernel.org/r/20240822-en7581-pinctrl-v2-0-ba1559173a7f@kernel.org > > > > Changes in v2: > > - Fix compilation errors > > - Collapse some register mappings for gpio and irq controllers > > - update dt-bindings according to new register mapping > > - fix some dt-bindings errors > > - Link to v1: https://lore.kernel.org/all/cover.1723392444.git.lorenzo@kernel.org/ > > > > --- > > Benjamin Larsson (1): > > pwm: airoha: Add support for EN7581 SoC > > > > Christian Marangi (2): > > dt-bindings: mfd: Add support for Airoha EN7581 GPIO System Controller > > mfd: airoha: Add support for Airoha EN7581 MFD > > > > Lorenzo Bianconi (2): > > dt-bindings: arm: airoha: Add the chip-scu node for EN7581 SoC > > pinctrl: airoha: Add support for EN7581 SoC > > > > .../bindings/arm/airoha,en7581-chip-scu.yaml | 42 + > > .../bindings/mfd/airoha,en7581-gpio-sysctl.yaml | 433 +++ > > MAINTAINERS | 7 + > > drivers/mfd/Kconfig | 8 + > > drivers/mfd/Makefile | 2 + > > drivers/mfd/airoha-en7581-gpio-mfd.c | 72 + > > drivers/pinctrl/mediatek/Kconfig | 16 +- > > drivers/pinctrl/mediatek/Makefile | 1 + > > drivers/pinctrl/mediatek/pinctrl-airoha.c | 2964 ++++++++++++++++++++ > > drivers/pwm/Kconfig | 10 + > > drivers/pwm/Makefile | 1 + > > drivers/pwm/pwm-airoha.c | 414 +++ > > include/linux/mfd/airoha-en7581-mfd.h | 9 + > > 13 files changed, 3978 insertions(+), 1 deletion(-) > > --- > > base-commit: 264c13114bd71ddfd7b25c7b94f6cda4587eca25 > > change-id: 20240818-en7581-pinctrl-1bf120154be0 > > prerequisite-change-id: 20240705-for-6-11-bpf-a349efc08df8:v2 > > > > > > Hi, > > any news with this? Rob reviewed the DT schemas and he is ok with them. > > Any other comments for the MFD driver and/or the pinctrl or PWM driver? Note that the merge-window is still open. Some maintainers, myself included, use this lull to prioritise other things. This set is on my list and will be dealt with shortly.
On Wed, Sep 25, 2024 at 10:47:38AM +0100, Lee Jones wrote: > On Mon, 23 Sep 2024, Christian Marangi wrote: > > > On Wed, Sep 11, 2024 at 09:50:00PM +0200, Lorenzo Bianconi wrote: > > > Introduce airoha-mfd driver in order to load pinctrl and pwm drivers for > > > EN7581 SoC. airoha-mfd is needed since both pinctrl and pwm drivers > > > needs to access the same memory block (gpio memory region) to configure > > > {gio,irq}_chip and pwm functionalities respectively, so model them as > > > childs of a parent mfd driver. > > > Current EN7581 pinctrl driver supports the following functionalities: > > > - pin multiplexing via chip_scu syscon > > > - pin pull-up, pull-down, open-drain, current strength, > > > {input,output}_enable, output_{low,high} via chip_scu syscon > > > - gpio controller > > > - irq controller > > > > > > --- > > > Changes in v4: > > > - add 'Limitation' description in pwm driver > > > - fix comments in pwm driver > > > - rely on mfd->base __iomem pointer in pwm driver, modify register > > > offsets according to it and get rid of sgpio_cfg, flash_cfg and > > > cycle_cfg pointers > > > - simplify register utility routines in pwm driver > > > - use 'generator' instead of 'waveform' suffix for pwm routines > > > - fix possible overflow calculating duty cycle in pwm driver > > > - do not modify pwm state in free callback in pwm driver > > > - cap the maximum period in pwm driver > > > - do not allow inverse polarity in pwm driver > > > - do not set of_xlate callback in the pwm driver and allow the stack to > > > do it > > > - fix MAINTAINERS file for airoha pinctrl driver > > > - fix undefined reference to __ffsdi2 in pinctrl driver > > > - simplify airoha,en7581-gpio-sysctl.yam binding > > > - Link to v3: https://lore.kernel.org/r/20240831-en7581-pinctrl-v3-0-98eebfb4da66@kernel.org > > > > > > Changes in v3: > > > - introduce airoha-mfd driver > > > - add pwm driver to the same series > > > - model pinctrl and pwm drivers as childs of a parent mfd driver. > > > - access chip-scu memory region in pinctrl driver via syscon > > > - introduce a single airoha,en7581-gpio-sysctl.yaml binding and get rid > > > of dedicated bindings for pinctrl and pwm > > > - add airoha,en7581-chip-scu.yaml binding do the series > > > - Link to v2: https://lore.kernel.org/r/20240822-en7581-pinctrl-v2-0-ba1559173a7f@kernel.org > > > > > > Changes in v2: > > > - Fix compilation errors > > > - Collapse some register mappings for gpio and irq controllers > > > - update dt-bindings according to new register mapping > > > - fix some dt-bindings errors > > > - Link to v1: https://lore.kernel.org/all/cover.1723392444.git.lorenzo@kernel.org/ > > > > > > --- > > > Benjamin Larsson (1): > > > pwm: airoha: Add support for EN7581 SoC > > > > > > Christian Marangi (2): > > > dt-bindings: mfd: Add support for Airoha EN7581 GPIO System Controller > > > mfd: airoha: Add support for Airoha EN7581 MFD > > > > > > Lorenzo Bianconi (2): > > > dt-bindings: arm: airoha: Add the chip-scu node for EN7581 SoC > > > pinctrl: airoha: Add support for EN7581 SoC > > > > > > .../bindings/arm/airoha,en7581-chip-scu.yaml | 42 + > > > .../bindings/mfd/airoha,en7581-gpio-sysctl.yaml | 433 +++ > > > MAINTAINERS | 7 + > > > drivers/mfd/Kconfig | 8 + > > > drivers/mfd/Makefile | 2 + > > > drivers/mfd/airoha-en7581-gpio-mfd.c | 72 + > > > drivers/pinctrl/mediatek/Kconfig | 16 +- > > > drivers/pinctrl/mediatek/Makefile | 1 + > > > drivers/pinctrl/mediatek/pinctrl-airoha.c | 2964 ++++++++++++++++++++ > > > drivers/pwm/Kconfig | 10 + > > > drivers/pwm/Makefile | 1 + > > > drivers/pwm/pwm-airoha.c | 414 +++ > > > include/linux/mfd/airoha-en7581-mfd.h | 9 + > > > 13 files changed, 3978 insertions(+), 1 deletion(-) > > > --- > > > base-commit: 264c13114bd71ddfd7b25c7b94f6cda4587eca25 > > > change-id: 20240818-en7581-pinctrl-1bf120154be0 > > > prerequisite-change-id: 20240705-for-6-11-bpf-a349efc08df8:v2 > > > > > > > > > > Hi, > > > > any news with this? Rob reviewed the DT schemas and he is ok with them. > > > > Any other comments for the MFD driver and/or the pinctrl or PWM driver? > > Note that the merge-window is still open. Some maintainers, myself > included, use this lull to prioritise other things. This set is on my > list and will be dealt with shortly. > Wasn't aware the merge window was still open and sorry for the noise. Thanks for the feedback!
Hi Lorenzo, so these comments: On Tue, Sep 24, 2024 at 12:12 PM Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > > +#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. I looked it over again and it looks good, I was just confused. > > > + 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: I was mainly thinking that the airoha_pinctrl_gpio_get_direction() is limited to pins that are used for GPIO. The callback should be usable on any pins, no matter if they can be muxed to GPIO or not? > &mfd { > ... > pio: pinctrl { > ... > pwm_gpio18_idx10_pins: pwm-gpio18-idx10-pins { > function = "pwm"; > pins = "gpio18"; > output-enable; > }; > }; > }; Like this one. Which I think works. It's the name of the function which confuses me: airoha_pinctrl_gpio_get_direction() and anything else that is used directly from the airoha_pinconf_set() function doesn't really care if the pin is used as GPIO or not does it? Can you rename the functions just e.g. airoha_pinctrl_get_direction() because it has nothing to do with GPIO. It's jus pin control. Also some defines are confusing this way: + /* set output enable */ + mask = BIT(gpio % AIROHA_GPIO_BANK_SIZE); + index = gpio / AIROHA_GPIO_BANK_SIZE; + airoha_pinctrl_rmw(pinctrl, pinctrl->gpiochip.out[index], + mask, !input ? mask : 0); Variables named "gpio" and AIROHA_GPIO_BANK_SIZE despite it is used for pins that are not (in the Linux sense) GPIO all the time. This is a big confusion for the mind. Can you rename the variable from "gpio" to "pin" or so and the AIROHA_GPIO_BANK_SIZE to AIROHA_PIN_BANK_SIZE etc so it is clear what is going on? I understand that the datasheet might be talking about "GPIO this and GPIO that" but what hardware engineers mean with GPIO is something else than what Linux mean: for them it means "it can be muxed so it is kinda-general-purpose-kinda" but in Linux this has a strict meaning: it can be used by the gpiolib to control individual lines. I think this would make it easier for me (and possibly others) ton understand the driver. Yours, Linus Walleij
> Hi Lorenzo, > > so these comments: > > On Tue, Sep 24, 2024 at 12:12 PM Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > > > > +#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. > > I looked it over again and it looks good, I was just confused. ack, no worries. > > > > > + 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: > > I was mainly thinking that the > airoha_pinctrl_gpio_get_direction() is limited to pins that are > used for GPIO. > > The callback should be usable on any pins, no matter if they > can be muxed to GPIO or not? > > > &mfd { > > ... > > pio: pinctrl { > > ... > > pwm_gpio18_idx10_pins: pwm-gpio18-idx10-pins { > > function = "pwm"; > > pins = "gpio18"; > > output-enable; > > }; > > }; > > }; > > Like this one. > > Which I think works. > > It's the name of the function which confuses me: > airoha_pinctrl_gpio_get_direction() and anything else that > is used directly from the airoha_pinconf_set() function > doesn't really care if the pin is used as GPIO or not does > it? > > Can you rename the functions just e.g. airoha_pinctrl_get_direction() > because it has nothing to do with GPIO. It's jus pin control. ack, I will do in v6 > > Also some defines are confusing this way: > > + /* set output enable */ > + mask = BIT(gpio % AIROHA_GPIO_BANK_SIZE); > + index = gpio / AIROHA_GPIO_BANK_SIZE; > + airoha_pinctrl_rmw(pinctrl, pinctrl->gpiochip.out[index], > + mask, !input ? mask : 0); > > Variables named "gpio" and AIROHA_GPIO_BANK_SIZE despite > it is used for pins that are not (in the Linux sense) GPIO all the time. > This is a big confusion for the mind. > > Can you rename the variable from "gpio" to "pin" or so > and the AIROHA_GPIO_BANK_SIZE to AIROHA_PIN_BANK_SIZE > etc so it is clear what is going on? ack, I will do in v6 > > I understand that the datasheet might be talking about > "GPIO this and GPIO that" but what hardware engineers mean > with GPIO is something else than what Linux mean: for them > it means "it can be muxed so it is kinda-general-purpose-kinda" > but in Linux this has a strict meaning: it can be used by the > gpiolib to control individual lines. > > I think this would make it easier for me (and possibly others) > ton understand the driver. ack. Regards, Lorenzo > > Yours, > Linus Walleij
Introduce airoha-mfd driver in order to load pinctrl and pwm drivers for EN7581 SoC. airoha-mfd is needed since both pinctrl and pwm drivers needs to access the same memory block (gpio memory region) to configure {gio,irq}_chip and pwm functionalities respectively, so model them as childs of a parent mfd driver. Current EN7581 pinctrl driver supports the following functionalities: - pin multiplexing via chip_scu syscon - pin pull-up, pull-down, open-drain, current strength, {input,output}_enable, output_{low,high} via chip_scu syscon - gpio controller - irq controller --- Changes in v4: - add 'Limitation' description in pwm driver - fix comments in pwm driver - rely on mfd->base __iomem pointer in pwm driver, modify register offsets according to it and get rid of sgpio_cfg, flash_cfg and cycle_cfg pointers - simplify register utility routines in pwm driver - use 'generator' instead of 'waveform' suffix for pwm routines - fix possible overflow calculating duty cycle in pwm driver - do not modify pwm state in free callback in pwm driver - cap the maximum period in pwm driver - do not allow inverse polarity in pwm driver - do not set of_xlate callback in the pwm driver and allow the stack to do it - fix MAINTAINERS file for airoha pinctrl driver - fix undefined reference to __ffsdi2 in pinctrl driver - simplify airoha,en7581-gpio-sysctl.yam binding - Link to v3: https://lore.kernel.org/r/20240831-en7581-pinctrl-v3-0-98eebfb4da66@kernel.org Changes in v3: - introduce airoha-mfd driver - add pwm driver to the same series - model pinctrl and pwm drivers as childs of a parent mfd driver. - access chip-scu memory region in pinctrl driver via syscon - introduce a single airoha,en7581-gpio-sysctl.yaml binding and get rid of dedicated bindings for pinctrl and pwm - add airoha,en7581-chip-scu.yaml binding do the series - Link to v2: https://lore.kernel.org/r/20240822-en7581-pinctrl-v2-0-ba1559173a7f@kernel.org Changes in v2: - Fix compilation errors - Collapse some register mappings for gpio and irq controllers - update dt-bindings according to new register mapping - fix some dt-bindings errors - Link to v1: https://lore.kernel.org/all/cover.1723392444.git.lorenzo@kernel.org/ --- Benjamin Larsson (1): pwm: airoha: Add support for EN7581 SoC Christian Marangi (2): dt-bindings: mfd: Add support for Airoha EN7581 GPIO System Controller mfd: airoha: Add support for Airoha EN7581 MFD Lorenzo Bianconi (2): dt-bindings: arm: airoha: Add the chip-scu node for EN7581 SoC pinctrl: airoha: Add support for EN7581 SoC .../bindings/arm/airoha,en7581-chip-scu.yaml | 42 + .../bindings/mfd/airoha,en7581-gpio-sysctl.yaml | 433 +++ MAINTAINERS | 7 + drivers/mfd/Kconfig | 8 + drivers/mfd/Makefile | 2 + drivers/mfd/airoha-en7581-gpio-mfd.c | 72 + drivers/pinctrl/mediatek/Kconfig | 16 +- drivers/pinctrl/mediatek/Makefile | 1 + drivers/pinctrl/mediatek/pinctrl-airoha.c | 2964 ++++++++++++++++++++ drivers/pwm/Kconfig | 10 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-airoha.c | 414 +++ include/linux/mfd/airoha-en7581-mfd.h | 9 + 13 files changed, 3978 insertions(+), 1 deletion(-) --- base-commit: 264c13114bd71ddfd7b25c7b94f6cda4587eca25 change-id: 20240818-en7581-pinctrl-1bf120154be0 prerequisite-change-id: 20240705-for-6-11-bpf-a349efc08df8:v2 Best regards,