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
Re: [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
- Subject: Re: [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs
- From: Linus Walleij <linus.walleij@xxxxxxxxxx>
- Date: Wed, 20 Sep 2023 13:58:37 +0200
- In-reply-to: <20230919103815.16818-4-kabel@kernel.org>
- References: <20230919103815.16818-1-kabel@kernel.org> <20230919103815.16818-4-kabel@kernel.org>
- Follow-Ups:
- Prev by Date: Re: [PATCH v3 0/7] Add pinctrl driver support for Realtek DHC SoCs
- Next by Date: Re: [PATCH 2/2] pinctrl: renesas: rzg2l: Enable noise filter for GPIO interrupt input
- Previous by thread: Re: [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs
- Next by thread: Re: [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs
- Index(es):
 |