This series implements minor improvements for Intel pinctrl driver.
The optimizations are as tested with gcc 7.5.0 with default -O2.
Changes since v2:
- Drop redundant patches.
- Update commit message.
Changes since v1:
- Update commit message.
- Specify compiler options used to measure optimizations.
- Drop redundant comments.
Raag Jadav (3):
pinctrl: intel: refine ->set_mux() hook
pinctrl: intel: refine ->irq_set_type() hook
pinctrl: intel: simplify exit path of ->gpio_request_enable() hook
drivers/pinctrl/intel/pinctrl-intel.c | 48 +++++++++++++--------------
1 file changed, 24 insertions(+), 24 deletions(-)
--
2.17.1
Utilize a temporary variable for common shift operation
in ->irq_set_type() hook and improve readability.
While at it, simplify if-else-if chain and save a few bytes.
add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-16 (-16)
Function old new delta
intel_gpio_irq_type 317 301 -16
Total: Before=10469, After=10453, chg -0.15%
Signed-off-by: Raag Jadav <[email protected]>
---
drivers/pinctrl/intel/pinctrl-intel.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index e8adf2580321..3f78066b1837 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -1128,8 +1128,8 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned int type)
struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
unsigned int pin = intel_gpio_to_pin(pctrl, irqd_to_hwirq(d), NULL, NULL);
unsigned long flags;
+ u32 value, rxevcfg;
void __iomem *reg;
- u32 value;
reg = intel_get_padcfg(pctrl, pin, PADCFG0);
if (!reg)
@@ -1150,23 +1150,24 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned int type)
intel_gpio_set_gpio_mode(reg);
value = readl(reg);
-
value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);
if ((type & IRQ_TYPE_EDGE_BOTH) == IRQ_TYPE_EDGE_BOTH) {
- value |= PADCFG0_RXEVCFG_EDGE_BOTH << PADCFG0_RXEVCFG_SHIFT;
+ rxevcfg = PADCFG0_RXEVCFG_EDGE_BOTH;
} else if (type & IRQ_TYPE_EDGE_FALLING) {
- value |= PADCFG0_RXEVCFG_EDGE << PADCFG0_RXEVCFG_SHIFT;
- value |= PADCFG0_RXINV;
+ rxevcfg = PADCFG0_RXEVCFG_EDGE;
} else if (type & IRQ_TYPE_EDGE_RISING) {
- value |= PADCFG0_RXEVCFG_EDGE << PADCFG0_RXEVCFG_SHIFT;
+ rxevcfg = PADCFG0_RXEVCFG_EDGE;
} else if (type & IRQ_TYPE_LEVEL_MASK) {
- if (type & IRQ_TYPE_LEVEL_LOW)
- value |= PADCFG0_RXINV;
+ rxevcfg = PADCFG0_RXEVCFG_LEVEL;
} else {
- value |= PADCFG0_RXEVCFG_DISABLED << PADCFG0_RXEVCFG_SHIFT;
+ rxevcfg = PADCFG0_RXEVCFG_DISABLED;
}
+ if (type == IRQ_TYPE_EDGE_FALLING || type == IRQ_TYPE_LEVEL_LOW)
+ value |= PADCFG0_RXINV;
+
+ value |= rxevcfg << PADCFG0_RXEVCFG_SHIFT;
writel(value, reg);
if (type & IRQ_TYPE_EDGE_BOTH)
--
2.17.1
Simplify exit path of ->gpio_request_enable() hook and save a few bytes.
add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-36 (-36)
Function old new delta
intel_gpio_request_enable 186 150 -36
Total: Before=10453, After=10417, chg -0.34%
Signed-off-by: Raag Jadav <[email protected]>
---
drivers/pinctrl/intel/pinctrl-intel.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 3f78066b1837..43e17bc177d0 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -485,20 +485,19 @@ static int intel_gpio_request_enable(struct pinctrl_dev *pctldev,
struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
void __iomem *padcfg0;
unsigned long flags;
+ int ret = 0;
padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0);
raw_spin_lock_irqsave(&pctrl->lock, flags);
if (!intel_pad_owned_by_host(pctrl, pin)) {
- raw_spin_unlock_irqrestore(&pctrl->lock, flags);
- return -EBUSY;
+ ret = -EBUSY;
+ goto out_unlock;
}
- if (!intel_pad_is_unlocked(pctrl, pin)) {
- raw_spin_unlock_irqrestore(&pctrl->lock, flags);
- return 0;
- }
+ if (!intel_pad_is_unlocked(pctrl, pin))
+ goto out_unlock;
/*
* If pin is already configured in GPIO mode, we assume that
@@ -506,16 +505,15 @@ static int intel_gpio_request_enable(struct pinctrl_dev *pctldev,
* potential glitches on the pin. Otherwise, for the pin in
* alternative mode, consumer has to supply respective flags.
*/
- if (intel_gpio_get_gpio_mode(padcfg0) == PADCFG0_PMODE_GPIO) {
- raw_spin_unlock_irqrestore(&pctrl->lock, flags);
- return 0;
- }
+ if (intel_gpio_get_gpio_mode(padcfg0) == PADCFG0_PMODE_GPIO)
+ goto out_unlock;
intel_gpio_set_gpio_mode(padcfg0);
+out_unlock:
raw_spin_unlock_irqrestore(&pctrl->lock, flags);
- return 0;
+ return ret;
}
static int intel_gpio_set_direction(struct pinctrl_dev *pctldev,
--
2.17.1
On Tue, Jun 13, 2023 at 02:20:54PM +0530, Raag Jadav wrote:
> Simplify exit path of ->gpio_request_enable() hook and save a few bytes.
>
> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-36 (-36)
> Function old new delta
> intel_gpio_request_enable 186 150 -36
> Total: Before=10453, After=10417, chg -0.34%
I believe Mika's comment against patch 3 of v2 of this series applies here
as well, meaning this patch won't be accepted.
But let Mika express himself.
--
With Best Regards,
Andy Shevchenko
On Tue, Jun 13, 2023 at 02:20:51PM +0530, Raag Jadav wrote:
> This series implements minor improvements for Intel pinctrl driver.
>
> The optimizations are as tested with gcc 7.5.0 with default -O2.
>
> Changes since v2:
> - Drop redundant patches.
> - Update commit message.
>
> Changes since v1:
> - Update commit message.
> - Specify compiler options used to measure optimizations.
> - Drop redundant comments.
>
> Raag Jadav (3):
> pinctrl: intel: refine ->set_mux() hook
> pinctrl: intel: refine ->irq_set_type() hook
> pinctrl: intel: simplify exit path of ->gpio_request_enable() hook
For the series,
Acked-by: Mika Westerberg <[email protected]>
On Tue, Jun 13, 2023 at 02:20:53PM +0530, Raag Jadav wrote:
> Utilize a temporary variable for common shift operation
> in ->irq_set_type() hook and improve readability.
> While at it, simplify if-else-if chain and save a few bytes.
>
> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-16 (-16)
> Function old new delta
> intel_gpio_irq_type 317 301 -16
> Total: Before=10469, After=10453, chg -0.15%
...
> value = readl(reg);
> -
> value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);
>
> if ((type & IRQ_TYPE_EDGE_BOTH) == IRQ_TYPE_EDGE_BOTH) {
> - value |= PADCFG0_RXEVCFG_EDGE_BOTH << PADCFG0_RXEVCFG_SHIFT;
> + rxevcfg = PADCFG0_RXEVCFG_EDGE_BOTH;
> } else if (type & IRQ_TYPE_EDGE_FALLING) {
> - value |= PADCFG0_RXEVCFG_EDGE << PADCFG0_RXEVCFG_SHIFT;
> - value |= PADCFG0_RXINV;
> + rxevcfg = PADCFG0_RXEVCFG_EDGE;
> } else if (type & IRQ_TYPE_EDGE_RISING) {
> - value |= PADCFG0_RXEVCFG_EDGE << PADCFG0_RXEVCFG_SHIFT;
> + rxevcfg = PADCFG0_RXEVCFG_EDGE;
> } else if (type & IRQ_TYPE_LEVEL_MASK) {
> - if (type & IRQ_TYPE_LEVEL_LOW)
> - value |= PADCFG0_RXINV;
> + rxevcfg = PADCFG0_RXEVCFG_LEVEL;
> } else {
> - value |= PADCFG0_RXEVCFG_DISABLED << PADCFG0_RXEVCFG_SHIFT;
> + rxevcfg = PADCFG0_RXEVCFG_DISABLED;
> }
>
> + if (type == IRQ_TYPE_EDGE_FALLING || type == IRQ_TYPE_LEVEL_LOW)
> + value |= PADCFG0_RXINV;
> +
> + value |= rxevcfg << PADCFG0_RXEVCFG_SHIFT;
> writel(value, reg);
Looking at this I realized that entire temporary variable assignments can be
done outside of spin lock. You probably would need another one for keeping
rxinv value.
Will it give us any memory reduction in comparison to the current code?
--
With Best Regards,
Andy Shevchenko
> On Tue, Jun 13, 2023 at 02:20:53PM +0530, Raag Jadav wrote:
> > Utilize a temporary variable for common shift operation in
> > ->irq_set_type() hook and improve readability.
> > While at it, simplify if-else-if chain and save a few bytes.
> >
> > add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-16 (-16)
> > Function old new delta
> > intel_gpio_irq_type 317 301 -16
> > Total: Before=10469, After=10453, chg -0.15%
>
> ...
>
> > value = readl(reg);
> > -
> > value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);
> >
> > if ((type & IRQ_TYPE_EDGE_BOTH) == IRQ_TYPE_EDGE_BOTH) {
> > - value |= PADCFG0_RXEVCFG_EDGE_BOTH <<
> PADCFG0_RXEVCFG_SHIFT;
> > + rxevcfg = PADCFG0_RXEVCFG_EDGE_BOTH;
> > } else if (type & IRQ_TYPE_EDGE_FALLING) {
> > - value |= PADCFG0_RXEVCFG_EDGE <<
> PADCFG0_RXEVCFG_SHIFT;
> > - value |= PADCFG0_RXINV;
> > + rxevcfg = PADCFG0_RXEVCFG_EDGE;
> > } else if (type & IRQ_TYPE_EDGE_RISING) {
> > - value |= PADCFG0_RXEVCFG_EDGE <<
> PADCFG0_RXEVCFG_SHIFT;
> > + rxevcfg = PADCFG0_RXEVCFG_EDGE;
> > } else if (type & IRQ_TYPE_LEVEL_MASK) {
> > - if (type & IRQ_TYPE_LEVEL_LOW)
> > - value |= PADCFG0_RXINV;
> > + rxevcfg = PADCFG0_RXEVCFG_LEVEL;
> > } else {
> > - value |= PADCFG0_RXEVCFG_DISABLED <<
> PADCFG0_RXEVCFG_SHIFT;
> > + rxevcfg = PADCFG0_RXEVCFG_DISABLED;
> > }
> >
> > + if (type == IRQ_TYPE_EDGE_FALLING || type ==
> IRQ_TYPE_LEVEL_LOW)
> > + value |= PADCFG0_RXINV;
> > +
> > + value |= rxevcfg << PADCFG0_RXEVCFG_SHIFT;
> > writel(value, reg);
>
> Looking at this I realized that entire temporary variable assignments can be
> done outside of spin lock. You probably would need another one for keeping
> rxinv value.
Something like this?
u32 value, rxevcfg;
u32 rxinv = 0;
if ((type & IRQ_TYPE_EDGE_BOTH) == IRQ_TYPE_EDGE_BOTH) {
rxevcfg = PADCFG0_RXEVCFG_EDGE_BOTH;
} else if (type & IRQ_TYPE_EDGE_FALLING) {
rxevcfg = PADCFG0_RXEVCFG_EDGE;
} else if (type & IRQ_TYPE_EDGE_RISING) {
rxevcfg = PADCFG0_RXEVCFG_EDGE;
} else if (type & IRQ_TYPE_LEVEL_MASK) {
rxevcfg = PADCFG0_RXEVCFG_LEVEL;
} else {
rxevcfg = PADCFG0_RXEVCFG_DISABLED;
}
if (type == IRQ_TYPE_EDGE_FALLING || type == IRQ_TYPE_LEVEL_LOW)
rxinv = PADCFG0_RXINV;
raw_spin_lock_irqsave(&pctrl->lock, flags);
intel_gpio_set_gpio_mode(reg);
value = readl(reg);
value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);
value |= rxinv;
value |= rxevcfg << PADCFG0_RXEVCFG_SHIFT;
writel(value, reg);
> Will it give us any memory reduction in comparison to the current code?
add/remove: 0/0 grow/shrink: 1/0 up/down: 4/0 (4)
Function old new delta
intel_gpio_irq_type 317 321 +4
Total: Before=10469, After=10473, chg +0.04%
Unfortunately gcc doesn't seem to consider this as best of the sequence,
and I'm not entirely sure why.
On Thu, Jun 15, 2023 at 09:48:12AM +0000, Jadav, Raag wrote:
> > On Tue, Jun 13, 2023 at 02:20:53PM +0530, Raag Jadav wrote:
> > > Utilize a temporary variable for common shift operation in
> > > ->irq_set_type() hook and improve readability.
> > > While at it, simplify if-else-if chain and save a few bytes.
> > >
> > > add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-16 (-16)
> > > Function old new delta
> > > intel_gpio_irq_type 317 301 -16
> > > Total: Before=10469, After=10453, chg -0.15%
> >
> > ...
> >
> > > value = readl(reg);
> > > -
> > > value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);
> > >
> > > if ((type & IRQ_TYPE_EDGE_BOTH) == IRQ_TYPE_EDGE_BOTH) {
> > > - value |= PADCFG0_RXEVCFG_EDGE_BOTH <<
> > PADCFG0_RXEVCFG_SHIFT;
> > > + rxevcfg = PADCFG0_RXEVCFG_EDGE_BOTH;
> > > } else if (type & IRQ_TYPE_EDGE_FALLING) {
> > > - value |= PADCFG0_RXEVCFG_EDGE <<
> > PADCFG0_RXEVCFG_SHIFT;
> > > - value |= PADCFG0_RXINV;
> > > + rxevcfg = PADCFG0_RXEVCFG_EDGE;
> > > } else if (type & IRQ_TYPE_EDGE_RISING) {
> > > - value |= PADCFG0_RXEVCFG_EDGE <<
> > PADCFG0_RXEVCFG_SHIFT;
> > > + rxevcfg = PADCFG0_RXEVCFG_EDGE;
> > > } else if (type & IRQ_TYPE_LEVEL_MASK) {
> > > - if (type & IRQ_TYPE_LEVEL_LOW)
> > > - value |= PADCFG0_RXINV;
> > > + rxevcfg = PADCFG0_RXEVCFG_LEVEL;
> > > } else {
> > > - value |= PADCFG0_RXEVCFG_DISABLED <<
> > PADCFG0_RXEVCFG_SHIFT;
> > > + rxevcfg = PADCFG0_RXEVCFG_DISABLED;
> > > }
> > >
> > > + if (type == IRQ_TYPE_EDGE_FALLING || type ==
> > IRQ_TYPE_LEVEL_LOW)
> > > + value |= PADCFG0_RXINV;
> > > +
> > > + value |= rxevcfg << PADCFG0_RXEVCFG_SHIFT;
> > > writel(value, reg);
> >
> > Looking at this I realized that entire temporary variable assignments can be
> > done outside of spin lock. You probably would need another one for keeping
> > rxinv value.
>
> Something like this?
>
> u32 value, rxevcfg;
> u32 rxinv = 0;
>
> if ((type & IRQ_TYPE_EDGE_BOTH) == IRQ_TYPE_EDGE_BOTH) {
> rxevcfg = PADCFG0_RXEVCFG_EDGE_BOTH;
> } else if (type & IRQ_TYPE_EDGE_FALLING) {
> rxevcfg = PADCFG0_RXEVCFG_EDGE;
> } else if (type & IRQ_TYPE_EDGE_RISING) {
> rxevcfg = PADCFG0_RXEVCFG_EDGE;
> } else if (type & IRQ_TYPE_LEVEL_MASK) {
> rxevcfg = PADCFG0_RXEVCFG_LEVEL;
> } else {
> rxevcfg = PADCFG0_RXEVCFG_DISABLED;
> }
>
> if (type == IRQ_TYPE_EDGE_FALLING || type == IRQ_TYPE_LEVEL_LOW)
> rxinv = PADCFG0_RXINV;
>
> raw_spin_lock_irqsave(&pctrl->lock, flags);
>
> intel_gpio_set_gpio_mode(reg);
>
> value = readl(reg);
>
> value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);
> value |= rxinv;
> value |= rxevcfg << PADCFG0_RXEVCFG_SHIFT;
>
> writel(value, reg);
This one looks better.
> > Will it give us any memory reduction in comparison to the current code?
>
> add/remove: 0/0 grow/shrink: 1/0 up/down: 4/0 (4)
> Function old new delta
> intel_gpio_irq_type 317 321 +4
> Total: Before=10469, After=10473, chg +0.04%
>
> Unfortunately gcc doesn't seem to consider this as best of the sequence,
> and I'm not entirely sure why.
It's fine as is, readability counts more than few bytes here.
On Thu, Jun 15, 2023 at 12:55:17PM +0300, [email protected] wrote:
> On Thu, Jun 15, 2023 at 09:48:12AM +0000, Jadav, Raag wrote:
> > > On Tue, Jun 13, 2023 at 02:20:53PM +0530, Raag Jadav wrote:
...
> > > Looking at this I realized that entire temporary variable assignments can be
> > > done outside of spin lock. You probably would need another one for keeping
> > > rxinv value.
> >
> > Something like this?
Almost, see below.
> > u32 value, rxevcfg;
> > u32 rxinv = 0;
No assignment here.
u32 rxinv, rxevcfg;
u32 value;
> > if ((type & IRQ_TYPE_EDGE_BOTH) == IRQ_TYPE_EDGE_BOTH) {
> > rxevcfg = PADCFG0_RXEVCFG_EDGE_BOTH;
> > } else if (type & IRQ_TYPE_EDGE_FALLING) {
> > rxevcfg = PADCFG0_RXEVCFG_EDGE;
> > } else if (type & IRQ_TYPE_EDGE_RISING) {
> > rxevcfg = PADCFG0_RXEVCFG_EDGE;
> > } else if (type & IRQ_TYPE_LEVEL_MASK) {
> > rxevcfg = PADCFG0_RXEVCFG_LEVEL;
> > } else {
> > rxevcfg = PADCFG0_RXEVCFG_DISABLED;
> > }
Now, if it's fully included in the diff (even with --patience parameter),
then you may drop {}.
> > if (type == IRQ_TYPE_EDGE_FALLING || type == IRQ_TYPE_LEVEL_LOW)
> > rxinv = PADCFG0_RXINV;
else
rxinv = 0;
> > raw_spin_lock_irqsave(&pctrl->lock, flags);
> >
> > intel_gpio_set_gpio_mode(reg);
> >
> > value = readl(reg);
> >
> > value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);
> > value |= rxinv;
> > value |= rxevcfg << PADCFG0_RXEVCFG_SHIFT;
And I would rewrite these to the standard patterns:
value = (value & ~PADCFG0_RXINV) | rxinv;
value = (value & ~PADCFG0_RXEVCFG_MASK) | (rxevcfg << PADCFG0_RXEVCFG_SHIFT);
And looking at this, perhaps do shift also outside the lock:
} else {
rxevcfg = PADCFG0_RXEVCFG_DISABLED;
}
rxevcfg <<= PADCFG0_RXEVCFG_SHIFT;
But, taking into account scope of the _RXEVCFG_*, I would add shift directly to
the definitions and kill that SHIFT entirely:
#define PADCFG0_RXEVCFG_LEVEL (0 << 25)
#define PADCFG0_RXEVCFG_EDGE (1 << 25)
#define PADCFG0_RXEVCFG_DISABLED (2 << 25)
#define PADCFG0_RXEVCFG_EDGE_BOTH (3 << 25)
...
value = (value & ~PADCFG0_RXINV) | rxinv;
value = (value & ~PADCFG0_RXEVCFG_MASK) | rxevcfg;
Try that one and look if it looks better. It might even save bytes after all.
> > writel(value, reg);
>
> This one looks better.
>
> > > Will it give us any memory reduction in comparison to the current code?
> >
> > add/remove: 0/0 grow/shrink: 1/0 up/down: 4/0 (4)
> > Function old new delta
> > intel_gpio_irq_type 317 321 +4
> > Total: Before=10469, After=10473, chg +0.04%
> >
> > Unfortunately gcc doesn't seem to consider this as best of the sequence,
> > and I'm not entirely sure why.
>
> It's fine as is, readability counts more than few bytes here.
--
With Best Regards,
Andy Shevchenko
> On Thu, Jun 15, 2023 at 12:55:17PM +0300,
> [email protected] wrote:
> > On Thu, Jun 15, 2023 at 09:48:12AM +0000, Jadav, Raag wrote:
> > > > On Tue, Jun 13, 2023 at 02:20:53PM +0530, Raag Jadav wrote:
>
> ...
>
> > > > Looking at this I realized that entire temporary variable
> > > > assignments can be done outside of spin lock. You probably would
> > > > need another one for keeping rxinv value.
> > >
> > > Something like this?
>
> Almost, see below.
>
> > > u32 value, rxevcfg;
> > > u32 rxinv = 0;
>
> No assignment here.
>
> u32 rxinv, rxevcfg;
> u32 value;
>
> > > if ((type & IRQ_TYPE_EDGE_BOTH) == IRQ_TYPE_EDGE_BOTH) {
> > > rxevcfg = PADCFG0_RXEVCFG_EDGE_BOTH;
> > > } else if (type & IRQ_TYPE_EDGE_FALLING) {
> > > rxevcfg = PADCFG0_RXEVCFG_EDGE;
> > > } else if (type & IRQ_TYPE_EDGE_RISING) {
> > > rxevcfg = PADCFG0_RXEVCFG_EDGE;
> > > } else if (type & IRQ_TYPE_LEVEL_MASK) {
> > > rxevcfg = PADCFG0_RXEVCFG_LEVEL;
> > > } else {
> > > rxevcfg = PADCFG0_RXEVCFG_DISABLED;
> > > }
>
> Now, if it's fully included in the diff (even with --patience parameter), then
> you may drop {}.
>
> > > if (type == IRQ_TYPE_EDGE_FALLING || type ==
> IRQ_TYPE_LEVEL_LOW)
> > > rxinv = PADCFG0_RXINV;
>
> else
> rxinv = 0;
>
> > > raw_spin_lock_irqsave(&pctrl->lock, flags);
> > >
> > > intel_gpio_set_gpio_mode(reg);
> > >
> > > value = readl(reg);
> > >
> > > value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);
> > > value |= rxinv;
> > > value |= rxevcfg << PADCFG0_RXEVCFG_SHIFT;
>
> And I would rewrite these to the standard patterns:
>
> value = (value & ~PADCFG0_RXINV) | rxinv;
> value = (value & ~PADCFG0_RXEVCFG_MASK) | (rxevcfg <<
> PADCFG0_RXEVCFG_SHIFT);
>
> And looking at this, perhaps do shift also outside the lock:
>
> } else {
> rxevcfg = PADCFG0_RXEVCFG_DISABLED;
> }
> rxevcfg <<= PADCFG0_RXEVCFG_SHIFT;
>
> But, taking into account scope of the _RXEVCFG_*, I would add shift directly
> to the definitions and kill that SHIFT entirely:
>
> #define PADCFG0_RXEVCFG_LEVEL (0 << 25)
> #define PADCFG0_RXEVCFG_EDGE (1 << 25)
> #define PADCFG0_RXEVCFG_DISABLED (2 << 25)
> #define PADCFG0_RXEVCFG_EDGE_BOTH (3 << 25)
>
> ...
>
> value = (value & ~PADCFG0_RXINV) | rxinv;
> value = (value & ~PADCFG0_RXEVCFG_MASK) | rxevcfg;
>
> Try that one and look if it looks better. It might even save bytes after all.
Should I add all of this in original patch or send this as a separate patch
on top this series?
On Thu, Jun 15, 2023 at 11:08:38AM +0000, Jadav, Raag wrote:
...
> Should I add all of this in original patch or send this as a separate patch
> on top this series?
Always base the changes on the respective subsystem tree, don't forget to use
--base when formatting patch with Git tools. Then send it separately. The 3rd
patch in the series is questionable to me. I would like to look into it later
on separately.
(The first implies that there is no changes as per this series in that
function).
--
With Best Regards,
Andy Shevchenko
On Tue, Jun 13, 2023 at 02:20:54PM +0530, Raag Jadav wrote:
> Simplify exit path of ->gpio_request_enable() hook and save a few bytes.
> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-36 (-36)
> Function old new delta
> intel_gpio_request_enable 186 150 -36
> Total: Before=10453, After=10417, chg -0.34%
While it seems a good footprint improvement, it looks like we do not use goto
in entire drivers/pinctrl/intel. So, I would keep it that way.
--
With Best Regards,
Andy Shevchenko