lkml.org 
[lkml]   [2021]   [Jan]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander
From
Date
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
>

\
 
 \ /
  Last update: 2021-01-10 00:14    [W:0.174 / U:2.900 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site