diff mbox series

counter: ti-eqep: Use container_of instead of struct counter_device::priv

Message ID 20211213114312.1406562-1-u.kleine-koenig@pengutronix.de (mailing list archive)
State Accepted
Headers show
Series counter: ti-eqep: Use container_of instead of struct counter_device::priv | expand

Commit Message

Uwe Kleine-König Dec. 13, 2021, 11:43 a.m. UTC 
Using counter->priv is a memory read and so more expensive than
container_of which is only an addition. (In this case even a noop
because the offset is 0.)

So container_of is expected to be a tad faster, it's type-safe, and
produces smaller code (ARCH=arm allmodconfig):

	$ source/scripts/bloat-o-meter drivers/counter/ti-eqep.o-pre drivers/counter/ti-eqep.o
	add/remove: 0/0 grow/shrink: 0/9 up/down: 0/-108 (-108)
	Function                                     old     new   delta
	ti_eqep_position_enable_write                132     120     -12
	ti_eqep_position_enable_read                 260     248     -12
	ti_eqep_position_ceiling_write               132     120     -12
	ti_eqep_position_ceiling_read                236     224     -12
	ti_eqep_function_write                       220     208     -12
	ti_eqep_function_read                        372     360     -12
	ti_eqep_count_write                          312     300     -12
	ti_eqep_count_read                           236     224     -12
	ti_eqep_action_read                          664     652     -12
	Total: Before=4598, After=4490, chg -2.35%

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/counter/ti-eqep.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)


base-commit: fa55b7dcdc43c1aa1ba12bca9d2dd4318c2a0dbf

Comments

David Lechner Dec. 13, 2021, 3:50 p.m. UTC | #1 
On 12/13/21 5:43 AM, Uwe Kleine-König wrote:
> Using counter->priv is a memory read and so more expensive than
> container_of which is only an addition. (In this case even a noop
> because the offset is 0.)
> 
> So container_of is expected to be a tad faster, it's type-safe, and
> produces smaller code (ARCH=arm allmodconfig):
> 
> 	$ source/scripts/bloat-o-meter drivers/counter/ti-eqep.o-pre drivers/counter/ti-eqep.o
> 	add/remove: 0/0 grow/shrink: 0/9 up/down: 0/-108 (-108)
> 	Function                                     old     new   delta
> 	ti_eqep_position_enable_write                132     120     -12
> 	ti_eqep_position_enable_read                 260     248     -12
> 	ti_eqep_position_ceiling_write               132     120     -12
> 	ti_eqep_position_ceiling_read                236     224     -12
> 	ti_eqep_function_write                       220     208     -12
> 	ti_eqep_function_read                        372     360     -12
> 	ti_eqep_count_write                          312     300     -12
> 	ti_eqep_count_read                           236     224     -12
> 	ti_eqep_action_read                          664     652     -12
> 	Total: Before=4598, After=4490, chg -2.35%
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---

Acked-by: David Lechner <david@lechnology.com>
William Breathitt Gray Dec. 16, 2021, 5:25 a.m. UTC | #2 
On Mon, Dec 13, 2021 at 12:43:12PM +0100, Uwe Kleine-König wrote:
> Using counter->priv is a memory read and so more expensive than
> container_of which is only an addition. (In this case even a noop
> because the offset is 0.)
> 
> So container_of is expected to be a tad faster, it's type-safe, and
> produces smaller code (ARCH=arm allmodconfig):
> 
> 	$ source/scripts/bloat-o-meter drivers/counter/ti-eqep.o-pre drivers/counter/ti-eqep.o
> 	add/remove: 0/0 grow/shrink: 0/9 up/down: 0/-108 (-108)
> 	Function                                     old     new   delta
> 	ti_eqep_position_enable_write                132     120     -12
> 	ti_eqep_position_enable_read                 260     248     -12
> 	ti_eqep_position_ceiling_write               132     120     -12
> 	ti_eqep_position_ceiling_read                236     224     -12
> 	ti_eqep_function_write                       220     208     -12
> 	ti_eqep_function_read                        372     360     -12
> 	ti_eqep_count_write                          312     300     -12
> 	ti_eqep_count_read                           236     224     -12
> 	ti_eqep_action_read                          664     652     -12
> 	Total: Before=4598, After=4490, chg -2.35%
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

I'll pick this up and submit it with the rest of the Counter changes for
this cycle.

Thanks,

William Breathitt Gray

> ---
>  drivers/counter/ti-eqep.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
> index 09817c953f9a..9e0e46bca4c2 100644
> --- a/drivers/counter/ti-eqep.c
> +++ b/drivers/counter/ti-eqep.c
> @@ -87,10 +87,15 @@ struct ti_eqep_cnt {
>  	struct regmap *regmap16;
>  };
>  
> +static struct ti_eqep_cnt *ti_eqep_count_from_counter(struct counter_device *counter)
> +{
> +	return container_of(counter, struct ti_eqep_cnt, counter);
> +}
> +
>  static int ti_eqep_count_read(struct counter_device *counter,
>  			      struct counter_count *count, u64 *val)
>  {
> -	struct ti_eqep_cnt *priv = counter->priv;
> +	struct ti_eqep_cnt *priv = ti_eqep_count_from_counter(counter);
>  	u32 cnt;
>  
>  	regmap_read(priv->regmap32, QPOSCNT, &cnt);
> @@ -102,7 +107,7 @@ static int ti_eqep_count_read(struct counter_device *counter,
>  static int ti_eqep_count_write(struct counter_device *counter,
>  			       struct counter_count *count, u64 val)
>  {
> -	struct ti_eqep_cnt *priv = counter->priv;
> +	struct ti_eqep_cnt *priv = ti_eqep_count_from_counter(counter);
>  	u32 max;
>  
>  	regmap_read(priv->regmap32, QPOSMAX, &max);
> @@ -116,7 +121,7 @@ static int ti_eqep_function_read(struct counter_device *counter,
>  				 struct counter_count *count,
>  				 enum counter_function *function)
>  {
> -	struct ti_eqep_cnt *priv = counter->priv;
> +	struct ti_eqep_cnt *priv = ti_eqep_count_from_counter(counter);
>  	u32 qdecctl;
>  
>  	regmap_read(priv->regmap16, QDECCTL, &qdecctl);
> @@ -143,7 +148,7 @@ static int ti_eqep_function_write(struct counter_device *counter,
>  				  struct counter_count *count,
>  				  enum counter_function function)
>  {
> -	struct ti_eqep_cnt *priv = counter->priv;
> +	struct ti_eqep_cnt *priv = ti_eqep_count_from_counter(counter);
>  	enum ti_eqep_count_func qsrc;
>  
>  	switch (function) {
> @@ -173,7 +178,7 @@ static int ti_eqep_action_read(struct counter_device *counter,
>  			       struct counter_synapse *synapse,
>  			       enum counter_synapse_action *action)
>  {
> -	struct ti_eqep_cnt *priv = counter->priv;
> +	struct ti_eqep_cnt *priv = ti_eqep_count_from_counter(counter);
>  	enum counter_function function;
>  	u32 qdecctl;
>  	int err;
> @@ -245,7 +250,7 @@ static int ti_eqep_position_ceiling_read(struct counter_device *counter,
>  					 struct counter_count *count,
>  					 u64 *ceiling)
>  {
> -	struct ti_eqep_cnt *priv = counter->priv;
> +	struct ti_eqep_cnt *priv = ti_eqep_count_from_counter(counter);
>  	u32 qposmax;
>  
>  	regmap_read(priv->regmap32, QPOSMAX, &qposmax);
> @@ -259,7 +264,7 @@ static int ti_eqep_position_ceiling_write(struct counter_device *counter,
>  					  struct counter_count *count,
>  					  u64 ceiling)
>  {
> -	struct ti_eqep_cnt *priv = counter->priv;
> +	struct ti_eqep_cnt *priv = ti_eqep_count_from_counter(counter);
>  
>  	if (ceiling != (u32)ceiling)
>  		return -ERANGE;
> @@ -272,7 +277,7 @@ static int ti_eqep_position_ceiling_write(struct counter_device *counter,
>  static int ti_eqep_position_enable_read(struct counter_device *counter,
>  					struct counter_count *count, u8 *enable)
>  {
> -	struct ti_eqep_cnt *priv = counter->priv;
> +	struct ti_eqep_cnt *priv = ti_eqep_count_from_counter(counter);
>  	u32 qepctl;
>  
>  	regmap_read(priv->regmap16, QEPCTL, &qepctl);
> @@ -285,7 +290,7 @@ static int ti_eqep_position_enable_read(struct counter_device *counter,
>  static int ti_eqep_position_enable_write(struct counter_device *counter,
>  					 struct counter_count *count, u8 enable)
>  {
> -	struct ti_eqep_cnt *priv = counter->priv;
> +	struct ti_eqep_cnt *priv = ti_eqep_count_from_counter(counter);
>  
>  	regmap_write_bits(priv->regmap16, QEPCTL, QEPCTL_PHEN, enable ? -1 : 0);
>  
> 
> base-commit: fa55b7dcdc43c1aa1ba12bca9d2dd4318c2a0dbf
> -- 
> 2.30.2
>
Uwe Kleine-König Dec. 16, 2021, 10:49 a.m. UTC | #3 
hello William,

On Thu, Dec 16, 2021 at 02:25:03PM +0900, William Breathitt Gray wrote:
> On Mon, Dec 13, 2021 at 12:43:12PM +0100, Uwe Kleine-König wrote:
> > Using counter->priv is a memory read and so more expensive than
> > container_of which is only an addition. (In this case even a noop
> > because the offset is 0.)
> > 
> > So container_of is expected to be a tad faster, it's type-safe, and
> > produces smaller code (ARCH=arm allmodconfig):
> > 
> > 	$ source/scripts/bloat-o-meter drivers/counter/ti-eqep.o-pre drivers/counter/ti-eqep.o
> > 	add/remove: 0/0 grow/shrink: 0/9 up/down: 0/-108 (-108)
> > 	Function                                     old     new   delta
> > 	ti_eqep_position_enable_write                132     120     -12
> > 	ti_eqep_position_enable_read                 260     248     -12
> > 	ti_eqep_position_ceiling_write               132     120     -12
> > 	ti_eqep_position_ceiling_read                236     224     -12
> > 	ti_eqep_function_write                       220     208     -12
> > 	ti_eqep_function_read                        372     360     -12
> > 	ti_eqep_count_write                          312     300     -12
> > 	ti_eqep_count_read                           236     224     -12
> > 	ti_eqep_action_read                          664     652     -12
> > 	Total: Before=4598, After=4490, chg -2.35%
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> I'll pick this up and submit it with the rest of the Counter changes for
> this cycle.

This is great. The same transformation could be done for the other
drivers using the priv pointer. Then priv could be removed from struct
counter_device. Good idea?

A quick prototype patch yields:

drivers/counter/104-quad-8.o
  add/remove: 0/0 grow/shrink: 5/17 up/down: 76/-172 (-96)
  Total: Before=11802, After=11706, chg -0.81%
drivers/counter/ftm-quaddec.o
  add/remove: 0/0 grow/shrink: 0/5 up/down: 0/-60 (-60)
  Total: Before=5096, After=5036, chg -1.18%
drivers/counter/intel-qep.o
  add/remove: 0/0 grow/shrink: 0/10 up/down: 0/-116 (-116)
  Total: Before=4867, After=4751, chg -2.38%
drivers/counter/interrupt-cnt.o
  add/remove: 0/0 grow/shrink: 0/6 up/down: 0/-60 (-60)
  Total: Before=2841, After=2781, chg -2.11%
drivers/counter/microchip-tcb-capture.o
  add/remove: 0/0 grow/shrink: 1/6 up/down: 12/-68 (-56)
  Total: Before=5920, After=5864, chg -0.95%
drivers/counter/stm32-lptimer-cnt.o
  add/remove: 0/0 grow/shrink: 0/10 up/down: 0/-140 (-140)
  Total: Before=6458, After=6318, chg -2.17%
drivers/counter/stm32-timer-cnt.o
  add/remove: 0/0 grow/shrink: 0/11 up/down: 0/-132 (-132)
  Total: Before=5504, After=5372, chg -2.40%
drivers/counter/ti-eqep.o
  add/remove: 0/0 grow/shrink: 0/10 up/down: 0/-120 (-120)
  Total: Before=4598, After=4478, chg -2.61%

(the ti-eqep object file got a bit smaller, probably because I removed
the priv member from struct counter_device.)

Best regards
Uwe
William Breathitt Gray Dec. 16, 2021, 11:31 p.m. UTC | #4 
On Thu, Dec 16, 2021 at 11:49:15AM +0100, Uwe Kleine-König wrote:
> hello William,
> 
> On Thu, Dec 16, 2021 at 02:25:03PM +0900, William Breathitt Gray wrote:
> > On Mon, Dec 13, 2021 at 12:43:12PM +0100, Uwe Kleine-König wrote:
> > > Using counter->priv is a memory read and so more expensive than
> > > container_of which is only an addition. (In this case even a noop
> > > because the offset is 0.)
> > > 
> > > So container_of is expected to be a tad faster, it's type-safe, and
> > > produces smaller code (ARCH=arm allmodconfig):
> > > 
> > > 	$ source/scripts/bloat-o-meter drivers/counter/ti-eqep.o-pre drivers/counter/ti-eqep.o
> > > 	add/remove: 0/0 grow/shrink: 0/9 up/down: 0/-108 (-108)
> > > 	Function                                     old     new   delta
> > > 	ti_eqep_position_enable_write                132     120     -12
> > > 	ti_eqep_position_enable_read                 260     248     -12
> > > 	ti_eqep_position_ceiling_write               132     120     -12
> > > 	ti_eqep_position_ceiling_read                236     224     -12
> > > 	ti_eqep_function_write                       220     208     -12
> > > 	ti_eqep_function_read                        372     360     -12
> > > 	ti_eqep_count_write                          312     300     -12
> > > 	ti_eqep_count_read                           236     224     -12
> > > 	ti_eqep_action_read                          664     652     -12
> > > 	Total: Before=4598, After=4490, chg -2.35%
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > 
> > I'll pick this up and submit it with the rest of the Counter changes for
> > this cycle.
> 
> This is great. The same transformation could be done for the other
> drivers using the priv pointer. Then priv could be removed from struct
> counter_device. Good idea?

Sure, that sounds reasonable. Create a patchset with the changes, CC the
respective driver maintainers, and we'll see if we can merge those too.

William Breathitt Gray

> 
> A quick prototype patch yields:
> 
> drivers/counter/104-quad-8.o
>   add/remove: 0/0 grow/shrink: 5/17 up/down: 76/-172 (-96)
>   Total: Before=11802, After=11706, chg -0.81%
> drivers/counter/ftm-quaddec.o
>   add/remove: 0/0 grow/shrink: 0/5 up/down: 0/-60 (-60)
>   Total: Before=5096, After=5036, chg -1.18%
> drivers/counter/intel-qep.o
>   add/remove: 0/0 grow/shrink: 0/10 up/down: 0/-116 (-116)
>   Total: Before=4867, After=4751, chg -2.38%
> drivers/counter/interrupt-cnt.o
>   add/remove: 0/0 grow/shrink: 0/6 up/down: 0/-60 (-60)
>   Total: Before=2841, After=2781, chg -2.11%
> drivers/counter/microchip-tcb-capture.o
>   add/remove: 0/0 grow/shrink: 1/6 up/down: 12/-68 (-56)
>   Total: Before=5920, After=5864, chg -0.95%
> drivers/counter/stm32-lptimer-cnt.o
>   add/remove: 0/0 grow/shrink: 0/10 up/down: 0/-140 (-140)
>   Total: Before=6458, After=6318, chg -2.17%
> drivers/counter/stm32-timer-cnt.o
>   add/remove: 0/0 grow/shrink: 0/11 up/down: 0/-132 (-132)
>   Total: Before=5504, After=5372, chg -2.40%
> drivers/counter/ti-eqep.o
>   add/remove: 0/0 grow/shrink: 0/10 up/down: 0/-120 (-120)
>   Total: Before=4598, After=4478, chg -2.61%
> 
> (the ti-eqep object file got a bit smaller, probably because I removed
> the priv member from struct counter_device.)
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |
diff mbox series

Patch

diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
index 09817c953f9a..9e0e46bca4c2 100644
--- a/drivers/counter/ti-eqep.c
+++ b/drivers/counter/ti-eqep.c
@@ -87,10 +87,15 @@  struct ti_eqep_cnt {
 	struct regmap *regmap16;
 };
 
+static struct ti_eqep_cnt *ti_eqep_count_from_counter(struct counter_device *counter)
+{
+	return container_of(counter, struct ti_eqep_cnt, counter);
+}
+
 static int ti_eqep_count_read(struct counter_device *counter,
 			      struct counter_count *count, u64 *val)
 {
-	struct ti_eqep_cnt *priv = counter->priv;
+	struct ti_eqep_cnt *priv = ti_eqep_count_from_counter(counter);
 	u32 cnt;
 
 	regmap_read(priv->regmap32, QPOSCNT, &cnt);
@@ -102,7 +107,7 @@  static int ti_eqep_count_read(struct counter_device *counter,
 static int ti_eqep_count_write(struct counter_device *counter,
 			       struct counter_count *count, u64 val)
 {
-	struct ti_eqep_cnt *priv = counter->priv;
+	struct ti_eqep_cnt *priv = ti_eqep_count_from_counter(counter);
 	u32 max;
 
 	regmap_read(priv->regmap32, QPOSMAX, &max);
@@ -116,7 +121,7 @@  static int ti_eqep_function_read(struct counter_device *counter,
 				 struct counter_count *count,
 				 enum counter_function *function)
 {
-	struct ti_eqep_cnt *priv = counter->priv;
+	struct ti_eqep_cnt *priv = ti_eqep_count_from_counter(counter);
 	u32 qdecctl;
 
 	regmap_read(priv->regmap16, QDECCTL, &qdecctl);
@@ -143,7 +148,7 @@  static int ti_eqep_function_write(struct counter_device *counter,
 				  struct counter_count *count,
 				  enum counter_function function)
 {
-	struct ti_eqep_cnt *priv = counter->priv;
+	struct ti_eqep_cnt *priv = ti_eqep_count_from_counter(counter);
 	enum ti_eqep_count_func qsrc;
 
 	switch (function) {
@@ -173,7 +178,7 @@  static int ti_eqep_action_read(struct counter_device *counter,
 			       struct counter_synapse *synapse,
 			       enum counter_synapse_action *action)
 {
-	struct ti_eqep_cnt *priv = counter->priv;
+	struct ti_eqep_cnt *priv = ti_eqep_count_from_counter(counter);
 	enum counter_function function;
 	u32 qdecctl;
 	int err;
@@ -245,7 +250,7 @@  static int ti_eqep_position_ceiling_read(struct counter_device *counter,
 					 struct counter_count *count,
 					 u64 *ceiling)
 {
-	struct ti_eqep_cnt *priv = counter->priv;
+	struct ti_eqep_cnt *priv = ti_eqep_count_from_counter(counter);
 	u32 qposmax;
 
 	regmap_read(priv->regmap32, QPOSMAX, &qposmax);
@@ -259,7 +264,7 @@  static int ti_eqep_position_ceiling_write(struct counter_device *counter,
 					  struct counter_count *count,
 					  u64 ceiling)
 {
-	struct ti_eqep_cnt *priv = counter->priv;
+	struct ti_eqep_cnt *priv = ti_eqep_count_from_counter(counter);
 
 	if (ceiling != (u32)ceiling)
 		return -ERANGE;
@@ -272,7 +277,7 @@  static int ti_eqep_position_ceiling_write(struct counter_device *counter,
 static int ti_eqep_position_enable_read(struct counter_device *counter,
 					struct counter_count *count, u8 *enable)
 {
-	struct ti_eqep_cnt *priv = counter->priv;
+	struct ti_eqep_cnt *priv = ti_eqep_count_from_counter(counter);
 	u32 qepctl;
 
 	regmap_read(priv->regmap16, QEPCTL, &qepctl);
@@ -285,7 +290,7 @@  static int ti_eqep_position_enable_read(struct counter_device *counter,
 static int ti_eqep_position_enable_write(struct counter_device *counter,
 					 struct counter_count *count, u8 enable)
 {
-	struct ti_eqep_cnt *priv = counter->priv;
+	struct ti_eqep_cnt *priv = ti_eqep_count_from_counter(counter);
 
 	regmap_write_bits(priv->regmap16, QEPCTL, QEPCTL_PHEN, enable ? -1 : 0);