Re: [PATCH 2/4] pwm: berlin: Put channel config into driver data

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 2575 bytes --]

On Mon, Nov 08, 2021 at 01:09:42PM +0100, Thierry Reding wrote:
> On Tue, May 04, 2021 at 03:25:35PM +0200, Uwe Kleine-König wrote:
> > Instead of allocating extra data in .request() provide the needed memory
> > in struct berlin_pwm_chip. This reduces the number of allocations. A side
> > effect is that on suspend and resume the state for all four channels is
> > always saved and restored. This is easier (and probably quicker) than
> > looking up the matching pwm_device and checking its PWMF_REQUESTED bit.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/pwm/pwm-berlin.c | 37 ++++++-------------------------------
> >  1 file changed, 6 insertions(+), 31 deletions(-)
> 
> This doesn't look like a worthwhile change to me. The per-PWM channel
> data was originally introduced to support exactly this type of use-case,
> so I don't see why we shouldn't keep using it here.

My reasons to not use the per-channel data are:

 - up to 5 smaller mallocs instead of a single bigger one. A single
   allocation is more effective (AFAIK) regarding memory management
   overhead, memory management overhead, and cache locality

 - bad naming, this isn't per-chip data as the function name
   "pwm_set_chip_data" suggests, but per-channel ("pwm"?) data.

 - maybe subjectively the data model is easier to understand if all
   required data (clk, channel state etc) is in a single data structure

 - With an arm allmodconfig bloat-o-meter reports for my patch:

	add/remove: 0/2 grow/shrink: 0/2 up/down: 0/-376 (-376)
	Function                                     old     new   delta
	berlin_pwm_free                               44       -     -44
	berlin_pwm_suspend                           364     256    -108
	berlin_pwm_resume                            444     332    -112
	berlin_pwm_request                           112       -    -112
	Total: Before=3712, After=3336, chg -10.13%

   So code size (and probably also run time) is improved.

If "back then we considered per-channel data a good idea, so let's use
it" is really your best argument to keep the code as is, I ask you to
reconsider that.

I checked the other drivers that make use of pwm_set_chip_data, and some
of them have a really strange usage pattern. Will prepare patches for
these and let you judge if these are worthwile.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]