From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 42893C433B4 for ; Mon, 17 May 2021 17:11:51 +0000 (UTC) Received: from desiato.infradead.org (desiato.infradead.org [90.155.92.199]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id BA0A7611CC for ; Mon, 17 May 2021 17:11:50 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org BA0A7611CC Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=crapouillou.net Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=desiato.20200630; h=Sender:Content-Type: Content-Transfer-Encoding:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:Message-Id:Cc:To :Subject:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=PYiSGRL2rLdbGvtTjHarqaMBt7sg+mYUvLhfxDDi8os=; b=MHMH/u9zSzTkYaC4qCg7Acf39 UlfhF68RY1UTnKLAlxbRI3OACLUPnwX/FFMyxTCuDip9wfNs72M9sjP7MKRjcTcK3ZJrWGp1fve7f rxRvvUj0Ml6QupalXeiNmw8ycFhAq9HdKqT68TzUHCYeIrhhyZ5enAz05EMGtbmujoADYXqhgMTgV Uwum9sA98Den2WNn3Rm5STeA+zGX7cz6A1oR9782nfM9mDGNwlMaky4dj2wh+RZOWtNupBEIfvmya ENYih2auf/CCW7u5AEDkNSWwATS5I2E/QH7GyT/sopefJK91KtNCjWJb2Bs9wzTwcHkalPfxpNGGU ae7bJzt8g==; Received: from localhost ([::1] helo=desiato.infradead.org) by desiato.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1ligkW-00FbDY-LL; Mon, 17 May 2021 17:09:49 +0000 Received: from bombadil.infradead.org ([2607:7c80:54:e::133]) by desiato.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1ligkT-00FbDG-GK for linux-arm-kernel@desiato.infradead.org; Mon, 17 May 2021 17:09:45 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20210309; h=Content-Transfer-Encoding: Content-Type:MIME-Version:References:In-Reply-To:Message-Id:Cc:To:Subject: From:Date:Sender:Reply-To:Content-ID:Content-Description; bh=IUjmo5NeuHJTowKx82DAVjiPw6rvxb0e1iHghQPVSaI=; b=IGktGq9S1r5yuKVq5na9nGYW+X M+9aWGRSlOmPqAC/cPotZPYzo9wBX+ZL1QeIy6kO64HdriMHKJeuEeX763BTSg1dep7lHFNDaugaL IElQ+/FtI00vZvMYgc+Yddj9xfYX1dAUaJ5IrHdwd2tDCgLtBhGlMDcJCtETdG6itZh/TTJiNoHeE VstGzrF1nRoHkJXOyHdQQ3xfGWM67Xd0OIi1puWM4B+okT8YeJF+M8oTQKpOE54+J+TXYWMIfJ0PV ResN8cQrnu8hOsmQTNbxbJ3qXwt4jrcJIF25tZf6L8CvmgJkKzQlL7hVbYizHWuZFNE9LV6DVNea5 mMIS/HJw==; Received: from aposti.net ([89.234.176.197]) by bombadil.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1ligkP-00E0FC-UH for linux-arm-kernel@lists.infradead.org; Mon, 17 May 2021 17:09:44 +0000 Date: Mon, 17 May 2021 18:09:26 +0100 From: Paul Cercueil Subject: Re: [PATCH 2/2] drm: xlnx: consolidate the functions which programming AUDIO_VIDEO_SELECT register To: quanyang.wang@windriver.com Cc: Hyun Kwon , Laurent Pinchart , David Airlie , Daniel Vetter , Michal Simek , dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Message-Id: In-Reply-To: <20210513114540.1241122-3-quanyang.wang@windriver.com> References: <20210513114540.1241122-1-quanyang.wang@windriver.com> <20210513114540.1241122-3-quanyang.wang@windriver.com> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210517_100942_321569_CA7C20B0 X-CRM114-Status: GOOD ( 26.02 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="iso-8859-1"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi, Le jeu., mai 13 2021 at 19:45:40 +0800, quanyang.wang@windriver.com a = =E9crit : > From: Quanyang Wang > = > For now, the functions zynqmp_disp_avbuf_enable/disable_audio and > zynqmp_disp_avbuf_enable/disable_video are all programming the = > register > AV_BUF_OUTPUT_AUDIO_VIDEO_SELECT to select the output for audio or = > video. > And in the future, many drm properties (like video_tpg, audio_tpg, > audio_pl, etc) also need to access it. So let's introduce some = > variables > of enum type and consolidate the code to unify handling this. > = > Signed-off-by: Quanyang Wang > --- > drivers/gpu/drm/xlnx/zynqmp_disp.c | 166 = > ++++++++++++++---------- > drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 15 +-- > 2 files changed, 101 insertions(+), 80 deletions(-) > = > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c = > b/drivers/gpu/drm/xlnx/zynqmp_disp.c > index c55e24412f8c..a82bc88a98aa 100644 > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c > @@ -102,12 +102,39 @@ enum zynqmp_disp_layer_id { > = > /** > * enum zynqmp_disp_layer_mode - Layer mode > - * @ZYNQMP_DISP_LAYER_NONLIVE: non-live (memory) mode > + * @ZYNQMP_DISP_LAYER_MEM: memory mode > * @ZYNQMP_DISP_LAYER_LIVE: live (stream) mode > + * @ZYNQMP_DISP_LAYER_TPG: tpg mode (only for video layer) > + * @ZYNQMP_DISP_LAYER_DISABLE: disable mode > */ > enum zynqmp_disp_layer_mode { > - ZYNQMP_DISP_LAYER_NONLIVE, > - ZYNQMP_DISP_LAYER_LIVE > + ZYNQMP_DISP_LAYER_MEM, > + ZYNQMP_DISP_LAYER_LIVE, > + ZYNQMP_DISP_LAYER_TPG, > + ZYNQMP_DISP_LAYER_DISABLE > +}; > + > +enum avbuf_vid_mode { > + VID_MODE_LIVE, > + VID_MODE_MEM, > + VID_MODE_TPG, > + VID_MODE_NONE > +}; > + > +enum avbuf_gfx_mode { > + GFX_MODE_DISABLE, > + GFX_MODE_MEM, > + GFX_MODE_LIVE, > + GFX_MODE_NONE > +}; > + > +enum avbuf_aud_mode { > + AUD1_MODE_LIVE, > + AUD1_MODE_MEM, > + AUD1_MODE_TPG, > + AUD1_MODE_DISABLE, > + AUD2_MODE_DISABLE, > + AUD2_MODE_ENABLE > }; > = > /** > @@ -542,92 +569,98 @@ static void = > zynqmp_disp_avbuf_disable_channels(struct zynqmp_disp_avbuf *avbuf) > } > = > /** > - * zynqmp_disp_avbuf_enable_audio - Enable audio > + * zynqmp_disp_avbuf_output_select - Select the buffer manager = > outputs > * @avbuf: Audio/video buffer manager > + * @layer: The layer > + * @mode: The mode for this layer > * > - * Enable all audio buffers with a non-live (memory) source. > + * Select the buffer manager outputs for @layer. > */ > -static void zynqmp_disp_avbuf_enable_audio(struct zynqmp_disp_avbuf = > *avbuf) > +static void zynqmp_disp_avbuf_output_select(struct zynqmp_disp_avbuf = > *avbuf, > + struct zynqmp_disp_layer *layer, u32 mode) You can put 'mode' on a new line to avoid getting over 80 characters. > { > - u32 val; > - > - val =3D zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT); > - val &=3D ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK; > - val |=3D ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MEM; > - val |=3D ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_EN; > - zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, val); > + u32 reg; > + > + reg =3D zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT); Empty line here (spacing before comment) > + /* Select audio mode when the layer is NULL */ > + if (layer =3D=3D NULL) { > + if (mode >=3D AUD2_MODE_DISABLE) { > + reg &=3D ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_MASK; > + reg |=3D (mode - AUD2_MODE_DISABLE) > + << ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_SHIFT; Please consider using the FIELD_PREP() macro from . = Then you can get rid of your *_SHIFT macros. > + } else { > + reg &=3D ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK; > + reg |=3D mode << ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_SHIFT; > + } > + } else if (is_layer_vid(layer)) { > + reg &=3D ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK; > + reg |=3D mode << ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_SHIFT; > + } else { > + reg &=3D ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK; > + reg |=3D mode << ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_SHIFT; > + } Empty line here (spacing after block) > + zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, reg); > } > = > /** > - * zynqmp_disp_avbuf_disable_audio - Disable audio > + * zynqmp_disp_avbuf_enable_audio - Enable audio > * @avbuf: Audio/video buffer manager > * > - * Disable all audio buffers. > + * Enable all audio buffers. > */ > -static void zynqmp_disp_avbuf_disable_audio(struct zynqmp_disp_avbuf = > *avbuf) > +static void zynqmp_disp_avbuf_enable_audio(struct zynqmp_disp_avbuf = > *avbuf) > { > - u32 val; > - > - val =3D zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT); > - val &=3D ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK; > - val |=3D ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_DISABLE; Same as above with FIELD_PREP(). > - val &=3D ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_EN; > - zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, val); > + zynqmp_disp_avbuf_output_select(avbuf, NULL, AUD1_MODE_MEM); > + zynqmp_disp_avbuf_output_select(avbuf, NULL, AUD2_MODE_ENABLE); > } > = > /** > - * zynqmp_disp_avbuf_enable_video - Enable a video layer > + * zynqmp_disp_avbuf_disable_audio - Disable audio > * @avbuf: Audio/video buffer manager > - * @layer: The layer > - * @mode: Operating mode of layer > * > - * Enable the video/graphics buffer for @layer. > + * Disable all audio buffers. > */ > -static void zynqmp_disp_avbuf_enable_video(struct zynqmp_disp_avbuf = > *avbuf, > - struct zynqmp_disp_layer *layer, > - enum zynqmp_disp_layer_mode mode) > +static void zynqmp_disp_avbuf_disable_audio(struct zynqmp_disp_avbuf = > *avbuf) > { > - u32 val; > - > - val =3D zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT); > - if (is_layer_vid(layer)) { > - val &=3D ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK; > - if (mode =3D=3D ZYNQMP_DISP_LAYER_NONLIVE) > - val |=3D ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MEM; > - else > - val |=3D ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_LIVE; > - } else { > - val &=3D ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK; > - val |=3D ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MEM; > - if (mode =3D=3D ZYNQMP_DISP_LAYER_NONLIVE) > - val |=3D ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MEM; > - else > - val |=3D ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_LIVE; > - } > - zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, val); > + zynqmp_disp_avbuf_output_select(avbuf, NULL, AUD1_MODE_DISABLE); > + zynqmp_disp_avbuf_output_select(avbuf, NULL, AUD2_MODE_DISABLE); > } > = > /** > - * zynqmp_disp_avbuf_disable_video - Disable a video layer > - * @avbuf: Audio/video buffer manager > + * zynqmp_disp_avbuf_set_layer_output -Set layer output You're missing a space after the dash character. > * @layer: The layer > + * @mode: The layer mode > * > - * Disable the video/graphics buffer for @layer. > + * Set output for @layer > */ > -static void zynqmp_disp_avbuf_disable_video(struct zynqmp_disp_avbuf = > *avbuf, > - struct zynqmp_disp_layer *layer) > +static void zynqmp_disp_avbuf_set_layer_output(struct = > zynqmp_disp_layer *layer, > + enum zynqmp_disp_layer_mode mode) > { > - u32 val; > - > - val =3D zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT); > - if (is_layer_vid(layer)) { > - val &=3D ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK; > - val |=3D ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_NONE; > - } else { > - val &=3D ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK; > - val |=3D ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_DISABLE; > + int val; > + struct zynqmp_disp *disp =3D layer->disp; I'd swap these two lines above - variables are usually defined in = "reverse christmas tree" order, the longest line first, the smallest = line last. No big deal though. > + > + switch (mode) { > + case ZYNQMP_DISP_LAYER_LIVE: > + val =3D is_layer_vid(layer) ? VID_MODE_LIVE : GFX_MODE_LIVE; > + break; > + case ZYNQMP_DISP_LAYER_MEM: > + val =3D is_layer_vid(layer) ? VID_MODE_MEM : GFX_MODE_MEM; > + break; > + case ZYNQMP_DISP_LAYER_TPG: > + if (!is_layer_vid(layer)) { > + dev_err(disp->dev, "gfx layer has no tpg mode\n"); > + return; > + } > + val =3D VID_MODE_TPG; > + break; > + case ZYNQMP_DISP_LAYER_DISABLE: > + val =3D is_layer_vid(layer) ? VID_MODE_NONE : GFX_MODE_DISABLE; > + break; > + default: > + dev_err(disp->dev, "invalid layer mode\n"); > + return; > } > - zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, val); While you're at it, you can add an empty line here (spacing after block) > + zynqmp_disp_avbuf_output_select(&disp->avbuf, layer, val); > } > = > /** > @@ -1030,11 +1063,10 @@ zynqmp_disp_layer_find_format(struct = > zynqmp_disp_layer *layer, > */ > static void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer) > { > - zynqmp_disp_avbuf_enable_video(&layer->disp->avbuf, layer, > - ZYNQMP_DISP_LAYER_NONLIVE); > + zynqmp_disp_avbuf_set_layer_output(layer, ZYNQMP_DISP_LAYER_MEM); > zynqmp_disp_blend_layer_enable(&layer->disp->blend, layer); > = > - layer->mode =3D ZYNQMP_DISP_LAYER_NONLIVE; > + layer->mode =3D ZYNQMP_DISP_LAYER_MEM; > } > = > /** > @@ -1051,7 +1083,7 @@ static void zynqmp_disp_layer_disable(struct = > zynqmp_disp_layer *layer) > for (i =3D 0; i < layer->drm_fmt->num_planes; i++) > dmaengine_terminate_sync(layer->dmas[i].chan); > = > - zynqmp_disp_avbuf_disable_video(&layer->disp->avbuf, layer); > + zynqmp_disp_avbuf_set_layer_output(layer, = > ZYNQMP_DISP_LAYER_DISABLE); > zynqmp_disp_blend_layer_disable(&layer->disp->blend, layer); > } > = > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h = > b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h > index f92a006d5070..dad3e356d9ab 100644 > --- a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h > @@ -120,23 +120,12 @@ > #define ZYNQMP_DISP_AV_BUF_OUTPUT 0x70 > #define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_SHIFT 0 > #define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK (0x3 << 0) > -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_LIVE (0 << 0) > -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MEM (1 << 0) > -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_PATTERN (2 << 0) > -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_NONE (3 << 0) > #define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_SHIFT 2 > #define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK (0x3 << 2) > -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_DISABLE (0 << 2) > -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MEM (1 << 2) > -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_LIVE (2 << 2) > -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_NONE (3 << 2) > #define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_SHIFT 4 > #define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK (0x3 << 4) > -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_PL (0 << 4) > -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MEM (1 << 4) > -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_PATTERN (2 << 4) > -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_DISABLE (3 << 4) > -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_EN BIT(6) > +#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_MASK (0x1 << 6) > +#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_SHIFT 6 Please use BIT() or GENMASK(). You don't need the _SHIFT macros if you = use FIELD_PREP() / FIELD_GET(). Cheers, -Paul > #define ZYNQMP_DISP_AV_BUF_HCOUNT_VCOUNT_INT0 0x74 > #define ZYNQMP_DISP_AV_BUF_HCOUNT_VCOUNT_INT1 0x78 > #define ZYNQMP_DISP_AV_BUF_PATTERN_GEN_SELECT 0x100 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel