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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id A8911C4332F for ; Thu, 20 Oct 2022 16:39:36 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=qpVJ5Y1FmXCF9RBfv6gRKZKjEgnHyL1WZe9t8qZXiog=; b=Z/ShSk09YxEl+R BtLBtecC44jBubgpNhsFsVcpj+PVsUNKI03kmlLRJnM7qVUl2eVmG5eRELgjyvA/HwwGCU1sTwxOo W9TUUhWesoR9+6vWtGtsYofXm0ik1EIegE9iBxor7IWEmD01cjszmc2JALoRYdYhVy8iW7qXYgFxb dcC4l4sLIAMIwfyFSNA8CW9SOMldroadotAjLue5eCl14vonJKhU3uYyCt0KBp0FR6ZIk9xEw0uvG YgQ3GBjGXacdK55ft6oBtap3slxKrQblECrOCRfURzPeMRUNEVMphg0hBd17pHOLuQkxkNqu8Go89 VCEEOy3MD+0Z/hPZnSBw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1olYYi-000RBI-KI; Thu, 20 Oct 2022 16:38:17 +0000 Received: from madras.collabora.co.uk ([2a00:1098:0:82:1000:25:2eeb:e5ab]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1olYTw-000NWS-BT; Thu, 20 Oct 2022 16:33:22 +0000 Received: from notapiano (unknown [IPv6:2600:4041:5b1a:cd00:524d:e95d:1a9c:492a]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: nfraprado) by madras.collabora.co.uk (Postfix) with ESMTPSA id 5C056660252A; Thu, 20 Oct 2022 17:33:14 +0100 (BST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1666283595; bh=Q6u2GbH3PGV2qnNkRIJ/Fn4pm+KSrq6IxNUNSYEyuFY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=HrvyrFKm+YQPD31b/I/zrMo5s4g3vbGwhHZXD9Ufc/Pg/y7i650rPbKDnJIG1VUp8 M2Tyxtgn+uKnhvC0Mu+s5VmAoBE27ikd2mXu3diJWF0yTaaAbSdlwZZ1qRdrZ197tR NA7dlUGIyrWt9IWpPHc1F4nLKZ1WXBzFlC3jdJBoNPlg8ba7auPTaHQoWejaI2X0N1 UiHUX1QcJgYwceHrOw8ORtuKug1p/T92+mF0AlvbzQ8kP6RLAVT47bEeer5nqQeBfY FSmlosRyu4VUA8L52cHjyMdpGk0PYNjHZ3fMSuaAPGTP0bND2dOA3OyRa0Z9aS3omy 0lWp6oiNXPRqg== Date: Thu, 20 Oct 2022 12:33:10 -0400 From: =?utf-8?B?TsOtY29sYXMgRi4gUi4gQS4=?= Prado To: xinlei.lee@mediatek.com Cc: matthias.bgg@gmail.com, rex-bc.chen@mediatek.com, angelogioacchino.delregno@collabora.com, jason-jh.lin@mediatek.com, chunkuang.hu@kernel.org, p.zabel@pengutronix.de, airlied@linux.ie, daniel@ffwll.ch, dri-devel@lists.freedesktop.org, linux-mediatek@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Project_Global_Chrome_Upstream_Group@mediatek.com Subject: Re: [PATCH v12,1/3] soc: mediatek: Add all settings to mtk_mmsys_ddp_dpi_fmt_config func Message-ID: <20221020163310.tpkjvr7pe5vhwylh@notapiano> References: <1666147936-27368-1-git-send-email-xinlei.lee@mediatek.com> <1666147936-27368-2-git-send-email-xinlei.lee@mediatek.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1666147936-27368-2-git-send-email-xinlei.lee@mediatek.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221020_093320_796184_132409A1 X-CRM114-Status: GOOD ( 25.80 ) 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-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi, On Wed, Oct 19, 2022 at 10:52:14AM +0800, xinlei.lee@mediatek.com wrote: > From: Xinlei Lee > = > The difference between MT8186 and other ICs is that when modifying the > output format, we need to modify the mmsys_base+0x400 register to take > effect. > So when setting the dpi output format, we need to call mmsys_func to set mmsys_func isn't something that exists in the code. Instead mention the act= ual function name: mtk_mmsys_ddp_dpi_fmt_config. > it to MT8186 synchronously. Here, before saying that the commit adds all the settings for dpi, you could have mentioned that the previous commit lacked those, to make it clearer: Commit a071e52f75d1 ("soc: mediatek: Add mmsys func to adapt to dpi output = for MT8186") lacked some of the possible output formats and also had a wrong bitmask. > Adding mmsys all the settings that need to be modified with dpi are for > mt8186. This sentence I would change to the following one: Add the missing output formats and fix the bitmask. Finally, you're also making the function more HW-agnostic (although in my opinion this could've been a future separate commit), so it's worth mention= ing it here: While at it, also update mtk_mmsys_ddp_dpi_fmt_config() to use generic form= ats, so that it is slightly easier to extend for other platforms. > = > Fixes: a071e52f75d1 ("soc: mediatek: Add mmsys func to adapt to dpi > output for MT8186") The fixes tag should be kept in a single line, without wrapping. > = > Signed-off-by: Xinlei Lee > Reviewed-by: AngeloGioacchino Del Regno > Reviewed-by: CK Hu > --- > drivers/soc/mediatek/mt8186-mmsys.h | 8 +++++--- > drivers/soc/mediatek/mtk-mmsys.c | 27 ++++++++++++++++++++------ > include/linux/soc/mediatek/mtk-mmsys.h | 7 +++++++ > 3 files changed, 33 insertions(+), 9 deletions(-) > = > diff --git a/drivers/soc/mediatek/mt8186-mmsys.h b/drivers/soc/mediatek/m= t8186-mmsys.h > index 09b1ccbc0093..035aec1eb616 100644 > --- a/drivers/soc/mediatek/mt8186-mmsys.h > +++ b/drivers/soc/mediatek/mt8186-mmsys.h > @@ -5,9 +5,11 @@ > = > /* Values for DPI configuration in MMSYS address space */ > #define MT8186_MMSYS_DPI_OUTPUT_FORMAT 0x400 > -#define DPI_FORMAT_MASK 0x1 > -#define DPI_RGB888_DDR_CON BIT(0) > -#define DPI_RGB565_SDR_CON BIT(1) > +#define DPI_FORMAT_MASK GENMASK(1, 0) > +#define DPI_RGB888_SDR_CON 0 > +#define DPI_RGB888_DDR_CON 1 > +#define DPI_RGB565_SDR_CON 2 > +#define DPI_RGB565_DDR_CON 3 These defines should all have a MT8186_ prefix. This will avoid confusions = now that mtk_mmsys_ddp_dpi_fmt_config() is being made more platform-agnostic. > = > #define MT8186_MMSYS_OVL_CON 0xF04 > #define MT8186_MMSYS_OVL0_CON_MASK 0x3 > diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-= mmsys.c > index d2c7a87aab87..205f6de45851 100644 > --- a/drivers/soc/mediatek/mtk-mmsys.c > +++ b/drivers/soc/mediatek/mtk-mmsys.c > @@ -238,12 +238,27 @@ static void mtk_mmsys_update_bits(struct mtk_mmsys = *mmsys, u32 offset, u32 mask, > = > void mtk_mmsys_ddp_dpi_fmt_config(struct device *dev, u32 val) > { > - if (val) > - mtk_mmsys_update_bits(dev_get_drvdata(dev), MT8186_MMSYS_DPI_OUTPUT_FO= RMAT, > - DPI_RGB888_DDR_CON, DPI_FORMAT_MASK); > - else > - mtk_mmsys_update_bits(dev_get_drvdata(dev), MT8186_MMSYS_DPI_OUTPUT_FO= RMAT, > - DPI_RGB565_SDR_CON, DPI_FORMAT_MASK); > + struct mtk_mmsys *mmsys =3D dev_get_drvdata(dev); > + > + switch (val) { > + case MTK_DPI_RGB888_SDR_CON: > + mtk_mmsys_update_bits(mmsys, MT8186_MMSYS_DPI_OUTPUT_FORMAT, > + DPI_FORMAT_MASK, DPI_RGB888_SDR_CON); > + break; > + case MTK_DPI_RGB565_SDR_CON: > + mtk_mmsys_update_bits(mmsys, MT8186_MMSYS_DPI_OUTPUT_FORMAT, > + DPI_FORMAT_MASK, DPI_RGB565_SDR_CON); > + break; > + case MTK_DPI_RGB565_DDR_CON: > + mtk_mmsys_update_bits(mmsys, MT8186_MMSYS_DPI_OUTPUT_FORMAT, > + DPI_FORMAT_MASK, DPI_RGB565_DDR_CON); > + break; > + case MTK_DPI_RGB888_DDR_CON: > + default: > + mtk_mmsys_update_bits(mmsys, MT8186_MMSYS_DPI_OUTPUT_FORMAT, > + DPI_FORMAT_MASK, DPI_RGB888_DDR_CON); > + break; > + } To be honest I don't really see the point of making the function slightly m= ore platform-agnostic like this. With a single platform making use of it it's j= ust an unneeded extra abstraction, and it could easily be done when a second platform starts requiring this as well... In any case, Reviewed-by: N=EDcolas F. R. A. Prado Thanks, N=EDcolas > } [..] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel