Hi Marek, thanks for your patch! This is a very interesting hardware. On Tue, Sep 19, 2023 at 12:38 PM Marek Behún <kabel@xxxxxxxxxx> wrote: > Add support for GPIOs connected to the MCU on the Turris Omnia board. > > This include: > - front button pin > - enable pins for USB regulators > - MiniPCIe / mSATA card presence pins in MiniPCIe port 0 > - LED output pins from WAN ethernet PHY, LAN switch and MiniPCIe ports > - on board revisions 32+ also various peripheral resets and another > voltage regulator enable pin > > Signed-off-by: Marek Behún <kabel@xxxxxxxxxx> OK all pretty straight-forward devices built on top of GPIO in something like device tree or ACPI etc. > --- a/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu > +++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu > @@ -1,3 +1,19 @@ > +What: /sys/bus/i2c/devices/<mcu_device>/front_button_mode > +Date: August 2023 > +KernelVersion: 6.6 > +Contact: Marek Behún <kabel@xxxxxxxxxx> > +Description: (RW) The front button on the Turris Omnia router can be > + configured either to change the intensity of all the LEDs on the > + front panel, or to send the press event to the CPU as an > + interrupt. > + > + This file switches between these two modes: > + - "mcu" makes the button press event be handled by the MCU to > + change the LEDs panel intensity. > + - "cpu" makes the button press event be handled by the CPU. > + > + Format: %s. I understand what this does, but why does this have to be configured at runtime from userspace sysfs? Why can't this just be a property in device tree or ACPI (etc)? I don't imagine a user is going to change this back and forth are they? They likely want one or another. > --- a/drivers/platform/cznic/turris-omnia-mcu-base.c > +++ b/drivers/platform/cznic/turris-omnia-mcu-base.c > @@ -222,9 +222,20 @@ static int omnia_mcu_probe(struct i2c_client *client) > return -ENODATA; > } > > + ret = omnia_mcu_register_gpiochip(mcu); > + if (ret) > + return ret; I guess it's OK to have a GPIOchip with the rest of the stuff, there are a few precedents, such as drivers/bcma. > +static const char * const omnia_mcu_gpio_names[64] = { I would name these _templates since it is not the final names that will be used. Very nice with all debug names though! > +#define _DEF_GPIO(_cmd, _ctl_cmd, _bit, _ctl_bit, _int_bit, _feat, _feat_mask) \ > + { \ > + .cmd = _cmd, \ > + .ctl_cmd = _ctl_cmd, \ > + .bit = _bit, \ > + .ctl_bit = _ctl_bit, \ > + .int_bit = _int_bit, \ > + .feat = _feat, \ > + .feat_mask = _feat_mask, \ > + } > +#define _DEF_GPIO_STS(_name) \ > + _DEF_GPIO(CMD_GET_STATUS_WORD, 0, STS_ ## _name, 0, INT_ ## _name, 0, 0) > +#define _DEF_GPIO_CTL(_name) \ > + _DEF_GPIO(CMD_GET_STATUS_WORD, CMD_GENERAL_CONTROL, STS_ ## _name, \ > + CTL_ ## _name, 0, 0, 0) > +#define _DEF_GPIO_EXT_STS(_name, _feat) \ > + _DEF_GPIO(CMD_GET_EXT_STATUS_DWORD, 0, EXT_STS_ ## _name, 0, \ > + INT_ ## _name, FEAT_ ## _feat | FEAT_EXT_CMDS, \ > + FEAT_ ## _feat | FEAT_EXT_CMDS) > +#define _DEF_GPIO_EXT_STS_LED(_name, _ledext) \ > + _DEF_GPIO(CMD_GET_EXT_STATUS_DWORD, 0, EXT_STS_ ## _name, 0, \ > + INT_ ## _name, FEAT_LED_STATE_ ## _ledext, \ > + FEAT_LED_STATE_EXT_MASK) > +#define _DEF_GPIO_EXT_STS_LEDALL(_name) \ > + _DEF_GPIO(CMD_GET_EXT_STATUS_DWORD, 0, EXT_STS_ ## _name, 0, \ > + INT_ ## _name, FEAT_LED_STATE_EXT_MASK, 0) > +#define _DEF_GPIO_EXT_CTL(_name, _feat) \ > + _DEF_GPIO(CMD_GET_EXT_CONTROL_STATUS, CMD_EXT_CONTROL, \ > + EXT_CTL_ ## _name, EXT_CTL_ ## _name, 0, \ > + FEAT_ ## _feat | FEAT_EXT_CMDS, \ > + FEAT_ ## _feat | FEAT_EXT_CMDS) > +#define _DEF_INT(_name) \ > + _DEF_GPIO(0, 0, 0, 0, INT_ ## _name, 0, 0) Ugh that's really hard to read and understand, but I can't think of anything better so I guess we live with it. > +static const struct omnia_gpio { > + u8 cmd, ctl_cmd; > + u32 bit, ctl_bit; > + u32 int_bit; If they are really just ONE bit I would actually use an int, encode the bit number rather than the mask, and then use BIT( ..->int_bit) from <linux/bits.h> everywhere I need a mask. No strong preference though. > +/* mapping from interrupts to indexes of GPIOs in the omnia_gpios array */ > +static const unsigned int omnia_int_to_gpio_idx[32] = { > + [ilog2(INT_CARD_DET)] = 4, > + [ilog2(INT_MSATA_IND)] = 5, > + [ilog2(INT_USB30_OVC)] = 6, ilog2 is a bit unituitive for a programmer, are you a schooled mathematician Marek? ;) It is also a consequence of using a bitmask rather than a bit number in the struct, all ilog2 does is fls-1. So what about just putting the bit number in the struct and then all of these assignments will look natural as well. > +/* index of PHY_SFP GPIO in the omnia_gpios array */ > +enum { > + OMNIA_GPIO_PHY_SFP_OFFSET = 54, > +}; I would just use a define for this, the enum isn't very useful for singular values. > +static int omnia_ctl_cmd(struct omnia_mcu *mcu, u8 cmd, u16 val, u16 mask) > +{ > + int err; > + > + mutex_lock(&mcu->lock); > + err = omnia_ctl_cmd_unlocked(mcu, cmd, val, mask); We just discussed this on an unrelated topic: *_unlocked means it should be called without need for the appropriate mutex to be locked, since it clearly requires &mcu->lock to be taken, this should be omnia_ctl_cmd_locked() as in "call this with the lock held". I did a quick grep _locked to convince myself about this. Obvious exampled include wait_event_interruptible_locked_irq() or wake_up_locked() in the core kernel infrastructure that clearly states (include/linux/wait.h for wait_event_interruptible_locked(): "It must be called with wq.lock being held." > +static int omnia_gpio_get(struct gpio_chip *gc, unsigned int offset) > +{ (...) > + if (gpio->cmd == CMD_GET_STATUS_WORD && > + !(mcu->features & FEAT_NEW_INT_API)) > + return !!(mcu->last_status & gpio->bit); So I expect something like return !!(mcu->last_status & BIT(gpio->bit)); > +static int omnia_gpio_get_multiple(struct gpio_chip *gc, unsigned long *mask, > + unsigned long *bits) > +{ > + mutex_lock(&mcu->lock); (...) > + if (err) > + goto err_unlock; (...) > + if (err) > + goto err_unlock; (...) > + if (err) > + goto err_unlock; > + > + mutex_unlock(&mcu->lock); This function is kind of a good example of where using scoped guards from <linux/cleanup.h> can simplify the code. After the initial lock you can just return on error and let the cleanup handler unlock the mutex, just guard(mutex)(&mcu->lock); In the beginning and then it's done, the lock will be held until the function is exited. > +static void omnia_gpio_set_multiple(struct gpio_chip *gc, unsigned long *mask, > + unsigned long *bits) > +{ (...) same here. > +static int omnia_gpio_init_valid_mask(struct gpio_chip *gc, > + unsigned long *valid_mask, > + unsigned int ngpios) > +{ > + struct omnia_mcu *mcu = gpiochip_get_data(gc); > + > + bitmap_zero(valid_mask, ngpios); > + > + for (int i = 0; i < ngpios; ++i) { > + const struct omnia_gpio *gpio = &omnia_gpios[i]; > + > + if (!gpio->cmd && !gpio->int_bit) > + continue; > + > + if (omnia_gpio_available(mcu, gpio)) > + set_bit(i, valid_mask); Speaking of locks, isn't this where you should use __set_bit() rather than set_bit()? __set_bit() is incomprehensible shorthand for "set_bit_nonatomic". > +static void omnia_irq_shutdown(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct omnia_mcu *mcu = gpiochip_get_data(gc); > + irq_hw_number_t hwirq = irqd_to_hwirq(d); > + u32 bit = omnia_gpios[hwirq].int_bit; > + > + mcu->rising &= ~bit; > + mcu->falling &= ~bit; Yeah so here it would be BIT(bit) etc. > + if (type & IRQ_TYPE_EDGE_RISING) > + mcu->rising |= bit; > + else > + mcu->rising &= ~bit; And with bits in place of bitmasks these would be if () __set_bit(bit, mcu->rising); else __clear_bit(bit, mcu->rising); etc > +static void omnia_irq_init_valid_mask(struct gpio_chip *gc, > + unsigned long *valid_mask, > + unsigned int ngpios) > +{ > + struct omnia_mcu *mcu = gpiochip_get_data(gc); > + > + bitmap_zero(valid_mask, ngpios); > + > + for (int i = 0; i < ngpios; ++i) { > + const struct omnia_gpio *gpio = &omnia_gpios[i]; > + > + if (!gpio->int_bit) > + continue; > + > + if (omnia_gpio_available(mcu, gpio)) > + set_bit(i, valid_mask); __set_bit() > +static bool omnia_irq_read_pending_new(struct omnia_mcu *mcu, > + unsigned long *pending) > +{ > + u32 rising, falling; > + u8 reply[8] = {}; > + size_t len; > + int err; > + > + /* > + * Determine how many bytes we need to read from the reply to the > + * CMD_GET_INT_AND_CLEAR command in order to retrieve all unmasked > + * interrupts. > + */ > + len = max(((ilog2(mcu->rising & mcu->mask) >> 3) << 1) + 1, > + ((ilog2(mcu->falling & mcu->mask) >> 3) << 1) + 2); Really hard to read, and again easier if we store the bit number rather than the bitmask. > + mutex_lock(&mcu->lock); Scoped guard? > +static void button_release_emul_fn(struct work_struct *work) > +{ > + struct omnia_mcu *mcu = container_of(to_delayed_work(work), > + struct omnia_mcu, > + button_release_emul_work); > + > + mcu->button_pressed_emul = false; > + generic_handle_irq_safe(mcu->client->irq); > +} (...) > + /* > + * The old firmware triggers an interrupt whenever status word changes, > + * but does not inform about which bits rose or fell. We need to compute > + * this here by comparing with the last status word value. > + * > + * The STS_BUTTON_PRESSED bit needs special handling, because the old > + * firmware clears the STS_BUTTON_PRESSED bit on successful completion > + * of the CMD_GET_STATUS_WORD command, resulting in another interrupt: > + * - first we get an interrupt, we read the status word where > + * STS_BUTTON_PRESSED is present, > + * - MCU clears the STS_BUTTON_PRESSED bit because we read the status > + * word, > + * - we get another interrupt because the status word changed again > + * (STS_BUTTON_PRESSED was cleared). > + * > + * The gpiolib-cdev, gpiolib-sysfs and gpio-keys input driver all call > + * the gpiochip's .get() method after an edge event on a requested GPIO > + * occurs. > + * > + * We ensure that the .get() method reads 1 for the button GPIO for some > + * time. > + */ > + > + if (status & STS_BUTTON_PRESSED) { > + mcu->button_pressed_emul = true; > + mod_delayed_work(system_wq, &mcu->button_release_emul_work, > + msecs_to_jiffies(50)); > + } else if (mcu->button_pressed_emul) { > + status |= STS_BUTTON_PRESSED; > + } I feel a bit icky about this, would be best if Dmitry Torokhov review such input subsystem-related things. It looks like a reimplementation of drivers/input/keyboard/gpio_keys.c so why not just connect a gpio-keys device using either device tree or a software node? (Dmitry knows all about how to create proper software nodes for stuff like this, DT is self-evident.) > + mcu->gc.names = omnia_mcu_gpio_names; Do we really support format strings in these names? I don't think so, or I'm wrong about my own code... (Which wouldn't be the first time.) > #include <asm/byteorder.h> > +#include <linux/gpio/driver.h> > #include <linux/i2c.h> > +#include <linux/log2.h> I think we want to get rid of log2.h from this driver. Yours, Linus Walleij