On Thu, Jun 09, 2022 at 02:52:46PM +0900, Tetsuo Handa wrote:
> This driver defines dcon_set_source() and dcon_set_source_sync(), where
> the latter calls flush_scheduled_work() which we want to remove.
>
> Since "struct dcon_priv" is per a device struct, I assume that
> dcon_set_source_sync() needs to wait for only one work associated with
> that device. Therefore, add bool argument to dcon_set_source() and
> wait for only one work using flush_work().
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> ---
> Please see commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue()
> using a macro") for background.
>
> drivers/staging/olpc_dcon/olpc_dcon.c | 23 ++++++++++-------------
> 1 file changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/staging/olpc_dcon/olpc_dcon.c b/drivers/staging/olpc_dcon/olpc_dcon.c
> index 7284cb4ac395..e0b1c91bb1a7 100644
> --- a/drivers/staging/olpc_dcon/olpc_dcon.c
> +++ b/drivers/staging/olpc_dcon/olpc_dcon.c
> @@ -369,21 +369,18 @@ static void dcon_source_switch(struct work_struct *work)
> dcon->curr_src = source;
> }
>
> -static void dcon_set_source(struct dcon_priv *dcon, int arg)
> +static void dcon_set_source(struct dcon_priv *dcon, int arg, bool sync)
> {
> if (dcon->pending_src == arg)
> - return;
> + goto flush;
>
> dcon->pending_src = arg;
>
> if (dcon->curr_src != arg)
> schedule_work(&dcon->switch_source);
> -}
> -
> -static void dcon_set_source_sync(struct dcon_priv *dcon, int arg)
> -{
> - dcon_set_source(dcon, arg);
> - flush_scheduled_work();
> +flush:
> + if (sync)
> + flush_work(&dcon->switch_source);
> }
>
> static ssize_t dcon_mode_show(struct device *dev,
> @@ -459,13 +456,13 @@ static ssize_t dcon_freeze_store(struct device *dev,
>
> switch (output) {
> case 0:
> - dcon_set_source(dcon, DCON_SOURCE_CPU);
> + dcon_set_source(dcon, DCON_SOURCE_CPU, false);
Ick, no, please don't add random boolean flags to parameters, it's now
impossible to remember what that means without having to go and dig up
the function itself.
Leave the _sync() call, just have it call dcon_set_source() and then do
the sync and that should make a much smaller patch, right?
thanks,
greg k-h
