Messages in this thread |  | | Subject | Re: [PATCH 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander | From | AngeloGioacchino Del Regno <> | Date | Sun, 10 Jan 2021 00:11:57 +0100 |
| |
Il 09/01/21 23:11, Linus Walleij ha scritto: > On Sat, Jan 9, 2021 at 3:02 PM AngeloGioacchino Del Regno > <angelogioacchino.delregno@somainline.org> wrote: > >> The Awinic AW9523(B) is a multi-function I2C gpio expander in a >> TQFN-24L package, featuring PWM (max 37mA per pin, or total max >> power 3.2Watts) for LED driving capability. >> >> It has two ports with 8 pins per port (for a total of 16 pins), >> configurable as either PWM with 1/256 stepping or GPIO input/output, >> 1.8V logic input; each GPIO can be configured as input or output >> independently from each other. >> >> This IC also has an internal interrupt controller, which is capable >> of generating an interrupt for each GPIO, depending on the >> configuration, and will raise an interrupt on the INTN pin to >> advertise this to an external interrupt controller. >> >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> > > Okay! > > Overall this driver is in good shape. > > The major review comment is that it'd be nice if you look into > using regmaps register cache instead of rolling your own, > and also possibly using regmaps locking rather than your own > as a result of that. > Actually, I really tried to use regmap's FLAT register cache and after many, many tries... I had to give up. I just couldn't get it working. :(
>> +config PINCTRL_AW9523 >> + bool "Awinic AW9523/AW9523B I2C GPIO expander pinctrl driver" >> + depends on OF && I2C >> + select PINMUX >> + select PINCONF >> + select GENERIC_PINCONF >> + select GPIOLIB >> + select GPIOLIB_IRQCHIP >> + select REGMAP >> + help >> + The Awinic AW9523/AW9523B is a multi-function I2C GPIO >> + expander with PWM functionality. This driver bundles a >> + pinctrl driver to select the function muxing and a GPIO >> + driver to handle GPIO, when the GPIO function is selected. >> + >> + Say yes to enable pinctrl and GPIO support for the AW9523(B). > > This: > > + DECLARE_BITMAP(old_masked[AW9523_NUM_PORTS], AW9523_PINS_PER_PORT); > + DECLARE_BITMAP(masked[AW9523_NUM_PORTS], AW9523_PINS_PER_PORT) > (...) > + DECLARE_BITMAP(direction_in[AW9523_NUM_PORTS], AW9523_PINS_PER_PORT); > > And this looks like a reimplementation of the existing register cache > in regmap. So use regmaps regcache instead. (More notes on that > below.) > > This looks good. Right dependencies and helpers. > >> + int hw_pin = pin % AW9523_PINS_PER_PORT; > > This makes me a bit wary. > > Is that really the "hardware pin" as it looks? It looks more like > the bit number 0..7 in the register for that port. I would just name these > "regbit" or just "n" like you do in the irq code. > Yes this is the bit number 0..7, you've understood that right. I guess renaming it to "regbit" is a good choice, makes it more understandable!
>> +/* >> + * __aw9523_gpio_get_direction - Get pin direction >> + * @regmap: Regmap structure >> + * @pin: gpiolib pin number >> + * @hwp: pin index in port register >> + * >> + * Return: Pin direction for success or negative number for error >> + */ >> +static int __aw9523_gpio_get_direction(struct regmap *regmap, u8 pin, u8 hwp) > > Nitpick: I kind of dislike __underscore functions because they have > ambiguous semantics. Sometimes it is a compiler thing. Sometimes > it is an inner function from something wrapped, i.e. it depends on > context what these underscores > mean. What about finding a better name that says what the function > is doing? > My initial idea was aw9523_get_pin_direction... then I changed it to include the word "gpio" in an attempt to make it less confusing. Let's go for the initial one then!
>> +static int __aw9523_get_port_state(struct regmap *regmap, u8 pin, >> + u8 hw_pin, unsigned int *state) > > Same. > ...And here I had another function without __prefix, which was then merged into another one as having it separated made no sense, then I forgot to remove the underscores. Oops! Removed!
>> +static int aw9523_gpio_irq_type(struct irq_data *d, unsigned int type) >> +{ >> + switch (type) { >> + case IRQ_TYPE_NONE: >> + case IRQ_TYPE_LEVEL_MASK: >> + case IRQ_TYPE_LEVEL_HIGH: >> + case IRQ_TYPE_LEVEL_LOW: >> + case IRQ_TYPE_EDGE_BOTH: >> + case IRQ_TYPE_EDGE_RISING: >> + case IRQ_TYPE_EDGE_FALLING: >> + return 0; > > Does this hardware really support all these edge types without any > software configuration whatsoever. That looks weird. > And it would indeed be weird: I've rechecked the datasheet again and only LEVEL interrupts are supported. As stated there: "When AW9523B detect port change, any input state from high-level to low-level or from
low-level to high-level will generate interrupt after 8us internal deglitch." I wonder what happened with my brain, there...
>> +static irqreturn_t aw9523_irq_thread_func(int irq, void *dev_id) >> +{ >> + struct aw9523 *awi = (struct aw9523 *)dev_id; >> + unsigned long n, val = 0; >> + unsigned long changed_gpio; >> + unsigned int tmp, port_pin, i, ret; >> + >> + for (i = 0; i < AW9523_NUM_PORTS; i++) { >> + port_pin = i * AW9523_PINS_PER_PORT; >> + ret = regmap_read(awi->regmap, >> + AW9523_REG_IN_STATE(port_pin), >> + &tmp); >> + if (ret) >> + return ret; >> + >> + val |= (u8)tmp << (i * 8); >> + } > > Can you convince me that these are not just consecutive registers > that could be read in one go with regmap_bulk_read()? > (I could not unwind the macros in my head, and you have the > datasheet I suppose.) > I cannot and I would never convince you of something wrong: yes, this is a read of two (and only two) consecutive registers. Here, I didn't go for regmap_bulk_read in favor of a "paranoid" performance optimization of this operation: in regmap_bulk_read we have 2 if branches, 1 if-else branch, plus another "implicit" (regmap_get_offset) if-else branch, and a switch. That's exactly what I'm avoiding with this for loop... for 1.5 times.
...And that's the full story: all about keeping overhead as minimal as possible. However, if it's really necessary to get that (even if very small) overhead, I can switch that to a regmap_bulk_read call... but from my perspective, having less instructions is better for many reasons. A typical case of "less is more", I guess?
>> +/* >> + * aw9523_irq_bus_sync_unlock - Synchronize state and unlock >> + * @d: irq data >> + * >> + * Writes the interrupt mask bits (found in the bit map) to the >> + * hardware, then unlocks the bus. >> + */ >> +static void aw9523_irq_bus_sync_unlock(struct irq_data *d) >> +{ >> + struct aw9523 *awi = gpiochip_get_data(irq_data_get_irq_chip_data(d)); >> + int i; >> + >> + for (i = 0; i < AW9523_NUM_PORTS; i++) { >> + if (bitmap_equal(awi->irq->masked[i], awi->irq->old_masked[i], >> + AW9523_PINS_PER_PORT)) >> + continue; >> + regmap_write(awi->regmap, >> + AW9523_REG_INTR_DIS(AW9523_PINS_PER_PORT * i), >> + *awi->irq->masked[i]); >> + bitmap_copy(awi->irq->old_masked[i], awi->irq->masked[i], >> + AW9523_PINS_PER_PORT); >> + } >> + mutex_unlock(&awi->irq->lock); >> +} > > These copies in the state that you write out at sync unlock. > > Can this not be done using the async facility in regmap? > > regmap_write_async()/regcache_mark_dirty() in all the IRQ > config etc functions, followed by a simple > regcache_sync() here makes it unnecessary to keep your > own register cache I believe? > > At least that is how I always thought it was supposed to be > used. > As I wrote earlier, unfortunately I tried hard... but I couldn't succeed...
>> +static int aw9523_direction_input(struct gpio_chip *chip, unsigned int offset) >> +{ >> + struct aw9523 *awi = gpiochip_get_data(chip); >> + u8 hw_pin = offset % AW9523_PINS_PER_PORT; >> + int port = AW9523_PIN_TO_PORT(offset); >> + >> + set_bit(offset, awi->direction_in[port]); > > This direction_in state seems to be another reimplementation of regmaps > register cache. > >> +static int aw9523_hw_reset(struct aw9523 *awi) >> +{ >> + int ret, max_retries = 2; >> + >> + /* Sometimes the chip needs more than one reset cycle */ >> + do { >> + ret = __aw9523_hw_reset(awi); > > Please give a better name to the inner function. Like > aw9523_drive_reset_gpio() or so. > I like it. aw9523_drive_reset_gpio it is!
>> + for (i = 0; i < AW9523_NUM_PORTS; i++) { >> + bitmap_fill(awi->irq->masked[i], AW9523_PINS_PER_PORT); >> + bitmap_fill(awi->irq->old_masked[i], AW9523_PINS_PER_PORT); >> + } > > This is another of these complications of reimplementing regmaps > register cache. > >> +static const struct regmap_config aw9523_regmap = { >> + .reg_bits = 8, >> + .val_bits = 8, >> + >> + .cache_type = REGCACHE_NONE, > > By using some elaborate caching here instead of implementing > your own, the driver can be simplified. > >> + .disable_locking = true, > > Are you sure you are not just reimplementing this locking > with your mutex? > Yes, I am using more specialized locking, which results in less lock-unlock operations in many cases, bringing *a lot* less overhead. Using the regmap locking, my keyboard matrix was a lot slower: I really had the need to optimize this driver's performance as much as possible.
>> +static struct i2c_driver aw9523_driver = { >> + .driver = { >> + .name = "aw9523-pinctrl", >> + .of_match_table = of_aw9523_i2c_match, >> + }, >> + .probe = aw9523_probe, > > A lot of people (especially on Qualcomm platforms, which is used in the > DT binding example) are working to modularize pin controllers. > > This controller on a slow bus should be able to support .remove() I > think? > > You should even be able to insmod/rmmod it at runtime for testing. > Actually, yes. I will add a .remove callback. You will get a V2 of this driver tomorrow!
-- Angelo > Yours, > Linus Walleij >
|  |