Il 09/01/21 23:11, Linus Walleij ha scritto:
On Sat, Jan 9, 2021 at 3:02 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
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
