| Message ID | 20201026141839.28536-6-brgl@bgdev.pl |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gpio: exar: refactor the driver | expand |
On Mon, Oct 26, 2020 at 4:23 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Provide and use helpers for calculating the register address and bit > offset instead of hand coding it in every function. Can you check code generation on x86, for example? Sometimes compilers are eager to use idiv assembly instruction which does simultaneously / and %. I dunno if a) it's used for / 8 and % 8 since 8 is 2^3, b) splitting to functions makes the above optimisation impossible. > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > --- > drivers/gpio/gpio-exar.c | 40 ++++++++++++++++++++++++++++------------ > 1 file changed, 28 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c > index db366d85b6b4..629f4dad6919 100644 > --- a/drivers/gpio/gpio-exar.c > +++ b/drivers/gpio/gpio-exar.c > @@ -33,6 +33,26 @@ struct exar_gpio_chip { > unsigned int first_pin; > }; > > +static unsigned int > +exar_offset_to_sel_addr(struct exar_gpio_chip *exar_gpio, unsigned int offset) > +{ > + return (offset + exar_gpio->first_pin) / 8 ? EXAR_OFFSET_MPIOSEL_HI > + : EXAR_OFFSET_MPIOSEL_LO; > +} > + > +static unsigned int > +exar_offset_to_lvl_addr(struct exar_gpio_chip *exar_gpio, unsigned int offset) > +{ > + return (offset + exar_gpio->first_pin) / 8 ? EXAR_OFFSET_MPIOLVL_HI > + : EXAR_OFFSET_MPIOLVL_LO; > +} > + > +static unsigned int > +exar_offset_to_bit(struct exar_gpio_chip *exar_gpio, unsigned int offset) > +{ > + return (offset + exar_gpio->first_pin) % 8; > +} > + > static void exar_update(struct gpio_chip *chip, unsigned int reg, int val, > unsigned int offset) > { > @@ -52,9 +72,8 @@ static int exar_set_direction(struct gpio_chip *chip, int direction, > unsigned int offset) > { > struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip); > - unsigned int addr = (offset + exar_gpio->first_pin) / 8 ? > - EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO; > - unsigned int bit = (offset + exar_gpio->first_pin) % 8; > + unsigned int addr = exar_offset_to_sel_addr(exar_gpio, offset); > + unsigned int bit = exar_offset_to_bit(exar_gpio, offset); > > exar_update(chip, addr, direction, bit); > return 0; > @@ -75,9 +94,8 @@ static int exar_get(struct gpio_chip *chip, unsigned int reg) > static int exar_get_direction(struct gpio_chip *chip, unsigned int offset) > { > struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip); > - unsigned int addr = (offset + exar_gpio->first_pin) / 8 ? > - EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO; > - unsigned int bit = (offset + exar_gpio->first_pin) % 8; > + unsigned int addr = exar_offset_to_sel_addr(exar_gpio, offset); > + unsigned int bit = exar_offset_to_bit(exar_gpio, offset); > > if (exar_get(chip, addr) & BIT(bit)) > return GPIO_LINE_DIRECTION_IN; > @@ -88,9 +106,8 @@ static int exar_get_direction(struct gpio_chip *chip, unsigned int offset) > static int exar_get_value(struct gpio_chip *chip, unsigned int offset) > { > struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip); > - unsigned int addr = (offset + exar_gpio->first_pin) / 8 ? > - EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO; > - unsigned int bit = (offset + exar_gpio->first_pin) % 8; > + unsigned int addr = exar_offset_to_lvl_addr(exar_gpio, offset); > + unsigned int bit = exar_offset_to_bit(exar_gpio, offset); > > return !!(exar_get(chip, addr) & BIT(bit)); > } > @@ -99,9 +116,8 @@ static void exar_set_value(struct gpio_chip *chip, unsigned int offset, > int value) > { > struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip); > - unsigned int addr = (offset + exar_gpio->first_pin) / 8 ? > - EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO; > - unsigned int bit = (offset + exar_gpio->first_pin) % 8; > + unsigned int addr = exar_offset_to_sel_addr(exar_gpio, offset); > + unsigned int bit = exar_offset_to_bit(exar_gpio, offset); > > exar_update(chip, addr, value, bit); > } > -- > 2.29.1 > -- With Best Regards, Andy Shevchenko
On Mon, Oct 26, 2020 at 3:51 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Oct 26, 2020 at 4:23 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > Provide and use helpers for calculating the register address and bit > > offset instead of hand coding it in every function. > > Can you check code generation on x86, for example? > > Sometimes compilers are eager to use idiv assembly instruction which > does simultaneously / and %. > I dunno if a) it's used for / 8 and % 8 since 8 is 2^3, b) splitting > to functions makes the above optimisation impossible. > Is this optimization really needed though? It's not like it's a hot path if it's protected by a mutex anyway. I prefer cleaner code here. Bartosz
On Mon, Oct 26, 2020 at 4:23 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: ... > +static unsigned int > +exar_offset_to_sel_addr(struct exar_gpio_chip *exar_gpio, unsigned int offset) > +{ > + return (offset + exar_gpio->first_pin) / 8 ? EXAR_OFFSET_MPIOSEL_HI > + : EXAR_OFFSET_MPIOSEL_LO; > +} > + > +static unsigned int > +exar_offset_to_lvl_addr(struct exar_gpio_chip *exar_gpio, unsigned int offset) > +{ > + return (offset + exar_gpio->first_pin) / 8 ? EXAR_OFFSET_MPIOLVL_HI > + : EXAR_OFFSET_MPIOLVL_LO; > +} > + > +static unsigned int > +exar_offset_to_bit(struct exar_gpio_chip *exar_gpio, unsigned int offset) > +{ > + return (offset + exar_gpio->first_pin) % 8; > +} Answering to your question... It can be done line this: static unsigned int exar_offset_to_bank_and_bit(..., *bit) { *bit = (offset + exar_gpio->first_pin) % 8; return (offset + exar_gpio->first_pin) / 8; } static unsigned int exar_offset_to_lvl_addr_and_bit(, *bit) { return exar_offset_to_bank_and_bit(..., bit) ? EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO; } ... > + unsigned int addr = exar_offset_to_lvl_addr(exar_gpio, offset); > + unsigned int bit = exar_offset_to_bit(exar_gpio, offset); unsigned int addr, bit; addr = exar_offset_to_lvl_addr_and_bit(..., &bit); -- With Best Regards, Andy Shevchenko
From: Andy Shevchenko > Sent: 02 November 2020 10:59 > > On Mon, Oct 26, 2020 at 4:23 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > ... > > > +static unsigned int > > +exar_offset_to_sel_addr(struct exar_gpio_chip *exar_gpio, unsigned int offset) > > +{ > > + return (offset + exar_gpio->first_pin) / 8 ? EXAR_OFFSET_MPIOSEL_HI > > + : EXAR_OFFSET_MPIOSEL_LO; > > +} > > + > > +static unsigned int > > +exar_offset_to_lvl_addr(struct exar_gpio_chip *exar_gpio, unsigned int offset) > > +{ > > + return (offset + exar_gpio->first_pin) / 8 ? EXAR_OFFSET_MPIOLVL_HI > > + : EXAR_OFFSET_MPIOLVL_LO; > > +} > > + > > +static unsigned int > > +exar_offset_to_bit(struct exar_gpio_chip *exar_gpio, unsigned int offset) > > +{ > > + return (offset + exar_gpio->first_pin) % 8; > > +} > > Answering to your question... > > It can be done line this: > > static unsigned int exar_offset_to_bank_and_bit(..., *bit) > { > *bit = (offset + exar_gpio->first_pin) % 8; > return (offset + exar_gpio->first_pin) / 8; > } That is likely to require the compiler reload exar_gpio->first_pin after the write to *bit. > static unsigned int exar_offset_to_lvl_addr_and_bit(, *bit) > { > return exar_offset_to_bank_and_bit(..., bit) ? > EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO; > } Gah why is it using divide then ?: ? AFAICT (from the above) there are at most 16 pins. Much better would be using: tmp = offset + exar_gpio->first_pin; *bit = tmp & 7; return tmp & 8; Inlined the compiler may well compute: exar_offset_to_bank_and_bit() ? HI : LO; as: LO + (HI - LO) * exar_offset_to_bank_and_bit(). The latter term is likely to be just (tmp & 8) >> n. I also bet the code actually wants (1 << bit). David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c index db366d85b6b4..629f4dad6919 100644 --- a/drivers/gpio/gpio-exar.c +++ b/drivers/gpio/gpio-exar.c @@ -33,6 +33,26 @@ struct exar_gpio_chip { unsigned int first_pin; }; +static unsigned int +exar_offset_to_sel_addr(struct exar_gpio_chip *exar_gpio, unsigned int offset) +{ + return (offset + exar_gpio->first_pin) / 8 ? EXAR_OFFSET_MPIOSEL_HI + : EXAR_OFFSET_MPIOSEL_LO; +} + +static unsigned int +exar_offset_to_lvl_addr(struct exar_gpio_chip *exar_gpio, unsigned int offset) +{ + return (offset + exar_gpio->first_pin) / 8 ? EXAR_OFFSET_MPIOLVL_HI + : EXAR_OFFSET_MPIOLVL_LO; +} + +static unsigned int +exar_offset_to_bit(struct exar_gpio_chip *exar_gpio, unsigned int offset) +{ + return (offset + exar_gpio->first_pin) % 8; +} + static void exar_update(struct gpio_chip *chip, unsigned int reg, int val, unsigned int offset) { @@ -52,9 +72,8 @@ static int exar_set_direction(struct gpio_chip *chip, int direction, unsigned int offset) { struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip); - unsigned int addr = (offset + exar_gpio->first_pin) / 8 ? - EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO; - unsigned int bit = (offset + exar_gpio->first_pin) % 8; + unsigned int addr = exar_offset_to_sel_addr(exar_gpio, offset); + unsigned int bit = exar_offset_to_bit(exar_gpio, offset); exar_update(chip, addr, direction, bit); return 0; @@ -75,9 +94,8 @@ static int exar_get(struct gpio_chip *chip, unsigned int reg) static int exar_get_direction(struct gpio_chip *chip, unsigned int offset) { struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip); - unsigned int addr = (offset + exar_gpio->first_pin) / 8 ? - EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO; - unsigned int bit = (offset + exar_gpio->first_pin) % 8; + unsigned int addr = exar_offset_to_sel_addr(exar_gpio, offset); + unsigned int bit = exar_offset_to_bit(exar_gpio, offset); if (exar_get(chip, addr) & BIT(bit)) return GPIO_LINE_DIRECTION_IN; @@ -88,9 +106,8 @@ static int exar_get_direction(struct gpio_chip *chip, unsigned int offset) static int exar_get_value(struct gpio_chip *chip, unsigned int offset) { struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip); - unsigned int addr = (offset + exar_gpio->first_pin) / 8 ? - EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO; - unsigned int bit = (offset + exar_gpio->first_pin) % 8; + unsigned int addr = exar_offset_to_lvl_addr(exar_gpio, offset); + unsigned int bit = exar_offset_to_bit(exar_gpio, offset); return !!(exar_get(chip, addr) & BIT(bit)); } @@ -99,9 +116,8 @@ static void exar_set_value(struct gpio_chip *chip, unsigned int offset, int value) { struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip); - unsigned int addr = (offset + exar_gpio->first_pin) / 8 ? - EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO; - unsigned int bit = (offset + exar_gpio->first_pin) % 8; + unsigned int addr = exar_offset_to_sel_addr(exar_gpio, offset); + unsigned int bit = exar_offset_to_bit(exar_gpio, offset); exar_update(chip, addr, value, bit); }