Message ID | 20230310110542.6649-1-lujianhua000@gmail.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:5915:0:0:0:0:0 with SMTP id v21csp810202wrd; Fri, 10 Mar 2023 03:17:29 -0800 (PST) X-Google-Smtp-Source: AK7set8GWcC/8ZP0rQOmKZB7qJGHo0nBw31uG8SAkxmiQh2fVBVy1inlQeKj/X8FbbJUzP36JIO1 X-Received: by 2002:a05:6a20:29a9:b0:cd:c79:514b with SMTP id f41-20020a056a2029a900b000cd0c79514bmr20885815pzh.2.1678447049638; Fri, 10 Mar 2023 03:17:29 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1678447049; cv=none; d=google.com; s=arc-20160816; b=x/iQcHURjYWl2k3HN4xtoyUB10B5jousu8ECXqDhC/3sxSTM3sSFTMwccJ0QTK2CNT YwUNVn9Rnl9YcXZPSuObuVrKIdW0IV9+uVDPkB9EBf1J81AzDnOxd7qahf/FyYSOXS1I +tpcGS+cEN1IYHRJh+N0IXXI0ZuQQ1y07b7QkNkIkPMg8Oq2L3yDSqNTsAkUgcWRBsLw rx2DMWhqPVJR5+4jnJ6Kc/CZ0gH4slhCxGZZQtiCsUhtfWuFrNsCWBrp2mP+5LbZK0Uz +yMmCtbMlNyP4PGKYDOLhPugRPW8hU2w9jeoMLryXZDilw0KV8eUL/JgahqnpKuCCfA6 X5ow== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=mGDXehSv6v2/D0JVf9XtETXZgmT2wS6LdL23tIL4g10=; b=papN3H21ncLmV7L1poVBSNtbJDfAXyD4o6rXOYzk25BCKSrek0iUf1rlYmqlR+7l9o IjD2mT5pfqaUweH9K/sXOuZrlluEu0W9rC05t3WV2xww6djpvAkKZNBztdpNOpeAdyeH zLoIiVONDDwwU++3/TY22pNBASYOVMAFOEzMaB6VPwmW1IP7FewU+eOOk/UXRsdNqoTy yNlCzuVhP2gxNM54RGml0cUHOjVPQG795xZpt7I0Xmj/W/MJbGLljyihk/T5s8r84458 +Gxx2PYeVYyDSa32VpyHPp/grXq/t/iphXCidkiDcpw9l033leNFKoG0TyRIpLs1Mu9P L8Kg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b=ge7D1BbJ; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id s24-20020a63dc18000000b00502e28a848esi1722228pgg.353.2023.03.10.03.17.04; Fri, 10 Mar 2023 03:17:29 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b=ge7D1BbJ; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230000AbjCJLFy (ORCPT <rfc822;carlos.wei.hk@gmail.com> + 99 others); Fri, 10 Mar 2023 06:05:54 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41692 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229598AbjCJLFw (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 10 Mar 2023 06:05:52 -0500 Received: from mail-pf1-x434.google.com (mail-pf1-x434.google.com [IPv6:2607:f8b0:4864:20::434]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B04B0DB6FD; Fri, 10 Mar 2023 03:05:51 -0800 (PST) Received: by mail-pf1-x434.google.com with SMTP id a7so3350465pfx.10; Fri, 10 Mar 2023 03:05:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1678446351; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=mGDXehSv6v2/D0JVf9XtETXZgmT2wS6LdL23tIL4g10=; b=ge7D1BbJ4Alm9bFtz18cnSDZjuLOmJjClVb4OQIEIZvKSELkBpUdP6juxrj5O6os2H nAnjjZGuWn4/iw1eb9hBi6I62RapyKOq45rrwmydepWzKiw83ltKiPXl+VRArXzgcl9I 6HsLXRa+HMrCSFwSI54Uh9O7kuH3nYPC1gJoPrcxZIgQb7f4fkBvNKMbYuBfpOrCeTBi MAeQ3eNif4CceQr71Q9JPapootB0ACJ/LeB2rd2IjeRW2Nuc1fB4JrpvinElR+lu2aeX WWOFMZvEPF836T0YEcwt7W9wycvOMKgRuDulX5GQbsVzhyj+ANTl6PaSI7l2beWfJIUX BnmA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678446351; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=mGDXehSv6v2/D0JVf9XtETXZgmT2wS6LdL23tIL4g10=; b=qDjk7VSEUazz7PbF0/0PI/6Rm4A4C/3SCUT0NFR/AdVHx1vRLKqmdGfibIFVd32qic oKdmSZKatdBykE61CHNjSwf3uldKKsprX6Ixyt4uHCvdVWHS5jh/MOgZU/CCCeeDEKB3 X5oM6XmLsj8IGBnUFhT/x1945a4+xc/hDu2VIlGL/UwRFstf2WT+p3W8ACV+dok7XzBa rE/KLyKmzFhPL/dqKiXFM9t5cCMYMdFtMy6jwm4sdLlLInodKxS+2vEq5IqcPm1NFoOE xuKdhtXcN/niVXSZIxnd0eGuCNvft85ig540Q9fd5CuzVVSuIR5PvAKBPcNJzLZSN8Ox KGWA== X-Gm-Message-State: AO0yUKXgtzUa+BNx+MdIhj30NZ3I0R/FRXdJJuXaeiatjyyZ2ZeVKY04 OWCNYIO5JPsdRYw4fncDuazH5Y0jGnA= X-Received: by 2002:aa7:940b:0:b0:5a8:b705:4dd3 with SMTP id x11-20020aa7940b000000b005a8b7054dd3mr21395673pfo.13.1678446351208; Fri, 10 Mar 2023 03:05:51 -0800 (PST) Received: from localhost.localdomain (n220246252084.netvigator.com. [220.246.252.84]) by smtp.gmail.com with ESMTPSA id h12-20020a62b40c000000b005a8b4dcd213sm1170602pfn.78.2023.03.10.03.05.47 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 10 Mar 2023 03:05:50 -0800 (PST) From: Jianhua Lu <lujianhua000@gmail.com> To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>, David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch> Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, ~postmarketos/upstreaming@lists.sr.ht, phone-devel@vger.kernel.org, Jianhua Lu <lujianhua000@gmail.com> Subject: [PATCH] drm/mipi-dsi: Add a mipi_dual_dsi_dcs_write_seq() macro Date: Fri, 10 Mar 2023 19:05:42 +0800 Message-Id: <20230310110542.6649-1-lujianhua000@gmail.com> X-Mailer: git-send-email 2.39.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-1.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1759979293376240169?= X-GMAIL-MSGID: =?utf-8?q?1759979293376240169?= |
Series |
drm/mipi-dsi: Add a mipi_dual_dsi_dcs_write_seq() macro
|
|
Commit Message
Jianhua Lu
March 10, 2023, 11:05 a.m. UTC
The panels with two dsi connected (sync dual dsi mode) need to transmit
dcs command to the two dsi host simultaneously, let's add
mipi_dual_dsi_dcs_write_seq() macro for this kind of panels.
Signed-off-by: Jianhua Lu <lujianhua000@gmail.com>
---
include/drm/drm_mipi_dsi.h | 15 +++++++++++++++
1 file changed, 15 insertions(+)
Comments
On Fri, 10 Mar 2023, Jianhua Lu <lujianhua000@gmail.com> wrote: > The panels with two dsi connected (sync dual dsi mode) need to transmit > dcs command to the two dsi host simultaneously, let's add > mipi_dual_dsi_dcs_write_seq() macro for this kind of panels. If we were to add a helper for this case, it should be a proper function and not a macro like this. We'd also need to see a user for this upstream. > > Signed-off-by: Jianhua Lu <lujianhua000@gmail.com> > --- > include/drm/drm_mipi_dsi.h | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h > index c9df0407980c..d0f0f75d4d83 100644 > --- a/include/drm/drm_mipi_dsi.h > +++ b/include/drm/drm_mipi_dsi.h > @@ -336,6 +336,21 @@ int mipi_dsi_dcs_get_display_brightness_large(struct mipi_dsi_device *dsi, > } \ > } while (0) > > +/** > + * mipi_dsi_dcs_write_seq - transmit a DCS command with payload > + * @dsi: array of 2 DSI peripheral devices This makes the assumption the devices are stored in an array. What if drivers want to store them differently, for whatever reason? Maybe they have an array of some container structs that have the devices? Maybe they just have two struct mipi_dsi_device pointers? > + * @cmd: Command > + * @seq: buffer containing data to be transmitted > + */ > +#define mipi_dual_dsi_dcs_write_seq(dsi, cmd, seq...) \ > + do { \ > + if (ARRAY_SIZE(dsi) > 2) \ > + return -EINVAL; \ > + int i; \ I believe this should lead to a warning for mixing code and declarations. > + for (i = 0; i < ARRAY_SIZE(dsi); i++) \ > + mipi_dsi_dcs_write_seq(dsi[i], cmd, seq); \ This ignores errors. > + } while (0) > + Without an example user, I'm not yet convinced about the usefulness of the helper, but I'd imagine something like this would be a more generic approach, not enforcing the array, and handling errors properly: ssize_t mipi_dsi_dual_dcs_write(struct mipi_dsi_device *dsi0, struct mipi_dsi_device *dsi1, u8 cmd, const void *data, size_t len) { ssize_t err = 0; if (dsi0) err = mipi_dsi_dcs_write(dsi0, cmd, data, len); if (dsi1 && !err) err = mipi_dsi_dcs_write(dsi1, cmd, data, len); return err; } But even that begs the question where does it end? There are a lot of mipi_dsi_dcs_*() functions as well as mipi_dsi_generic_write(). Dual wrappers for all of them? :o BR, Jani. > /** > * struct mipi_dsi_driver - DSI driver > * @driver: device driver model driver
On Fri, Mar 10, 2023 at 01:54:18PM +0200, Jani Nikula wrote: > On Fri, 10 Mar 2023, Jianhua Lu <lujianhua000@gmail.com> wrote: > > The panels with two dsi connected (sync dual dsi mode) need to transmit > > dcs command to the two dsi host simultaneously, let's add > > mipi_dual_dsi_dcs_write_seq() macro for this kind of panels. > > If we were to add a helper for this case, it should be a proper function > and not a macro like this. > > We'd also need to see a user for this upstream. > > > > > Signed-off-by: Jianhua Lu <lujianhua000@gmail.com> > > --- > > include/drm/drm_mipi_dsi.h | 15 +++++++++++++++ > > 1 file changed, 15 insertions(+) > > > > diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h > > index c9df0407980c..d0f0f75d4d83 100644 > > --- a/include/drm/drm_mipi_dsi.h > > +++ b/include/drm/drm_mipi_dsi.h > > @@ -336,6 +336,21 @@ int mipi_dsi_dcs_get_display_brightness_large(struct mipi_dsi_device *dsi, > > } \ > > } while (0) > > > > +/** > > + * mipi_dsi_dcs_write_seq - transmit a DCS command with payload > > + * @dsi: array of 2 DSI peripheral devices > > This makes the assumption the devices are stored in an array. What if > drivers want to store them differently, for whatever reason? Maybe they > have an array of some container structs that have the devices? Maybe > they just have two struct mipi_dsi_device pointers? This array just store two struct mipi_dsi_device pointers > > > + * @cmd: Command > > + * @seq: buffer containing data to be transmitted > > + */ > > +#define mipi_dual_dsi_dcs_write_seq(dsi, cmd, seq...) \ > > + do { \ > > + if (ARRAY_SIZE(dsi) > 2) \ > > + return -EINVAL; \ > > + int i; \ > > I believe this should lead to a warning for mixing code and > declarations. > > > + for (i = 0; i < ARRAY_SIZE(dsi); i++) \ > > + mipi_dsi_dcs_write_seq(dsi[i], cmd, seq); \ > > This ignores errors. mipi_dsi_dcs_write_seq is also a macro, contains error checks in the body block. > > > + } while (0) > > + > > Without an example user, I'm not yet convinced about the usefulness of > the helper, but I'd imagine something like this would be a more generic > approach, not enforcing the array, and handling errors properly: > > ssize_t mipi_dsi_dual_dcs_write(struct mipi_dsi_device *dsi0, > struct mipi_dsi_device *dsi1, > u8 cmd, const void *data, size_t len) > { > ssize_t err = 0; > > if (dsi0) > err = mipi_dsi_dcs_write(dsi0, cmd, data, len); > > if (dsi1 && !err) > err = mipi_dsi_dcs_write(dsi1, cmd, data, len); > > return err; > } Thanks for your explanation and this looks more reasonable. > > But even that begs the question where does it end? There are a lot of > mipi_dsi_dcs_*() functions as well as mipi_dsi_generic_write(). Dual > wrappers for all of them? :o It's definitly useless to wrap all of them. Please ignore this patch. > > > BR, > Jani. > > > > /** > > * struct mipi_dsi_driver - DSI driver > > * @driver: device driver model driver > > -- > Jani Nikula, Intel Open Source Graphics Center
On Fri, 10 Mar 2023, Jianhua Lu <lujianhua000@gmail.com> wrote: > On Fri, Mar 10, 2023 at 01:54:18PM +0200, Jani Nikula wrote: >> On Fri, 10 Mar 2023, Jianhua Lu <lujianhua000@gmail.com> wrote: >> > The panels with two dsi connected (sync dual dsi mode) need to transmit >> > dcs command to the two dsi host simultaneously, let's add >> > mipi_dual_dsi_dcs_write_seq() macro for this kind of panels. >> >> If we were to add a helper for this case, it should be a proper function >> and not a macro like this. >> >> We'd also need to see a user for this upstream. >> >> > >> > Signed-off-by: Jianhua Lu <lujianhua000@gmail.com> >> > --- >> > include/drm/drm_mipi_dsi.h | 15 +++++++++++++++ >> > 1 file changed, 15 insertions(+) >> > >> > diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h >> > index c9df0407980c..d0f0f75d4d83 100644 >> > --- a/include/drm/drm_mipi_dsi.h >> > +++ b/include/drm/drm_mipi_dsi.h >> > @@ -336,6 +336,21 @@ int mipi_dsi_dcs_get_display_brightness_large(struct mipi_dsi_device *dsi, >> > } \ >> > } while (0) >> > >> > +/** >> > + * mipi_dsi_dcs_write_seq - transmit a DCS command with payload >> > + * @dsi: array of 2 DSI peripheral devices >> >> This makes the assumption the devices are stored in an array. What if >> drivers want to store them differently, for whatever reason? Maybe they >> have an array of some container structs that have the devices? Maybe >> they just have two struct mipi_dsi_device pointers? > This array just store two struct mipi_dsi_device pointers >> >> > + * @cmd: Command >> > + * @seq: buffer containing data to be transmitted >> > + */ >> > +#define mipi_dual_dsi_dcs_write_seq(dsi, cmd, seq...) \ >> > + do { \ >> > + if (ARRAY_SIZE(dsi) > 2) \ >> > + return -EINVAL; \ >> > + int i; \ >> >> I believe this should lead to a warning for mixing code and >> declarations. >> >> > + for (i = 0; i < ARRAY_SIZE(dsi); i++) \ >> > + mipi_dsi_dcs_write_seq(dsi[i], cmd, seq); \ >> >> This ignores errors. > mipi_dsi_dcs_write_seq is also a macro, contains error checks in the body block. Ugh, I think it's pretty scary to hide control flow like return statements in macros like this. IMO the the main user of mipi_dsi_dcs_write_seq() i.e. panel_nv3051d_init_sequence() in drivers/gpu/drm/panel/panel-newvision-nv3051d.c should be written to do the writes from an array in a loop instead. BR, Jani. >> >> > + } while (0) >> > + >> >> Without an example user, I'm not yet convinced about the usefulness of >> the helper, but I'd imagine something like this would be a more generic >> approach, not enforcing the array, and handling errors properly: >> >> ssize_t mipi_dsi_dual_dcs_write(struct mipi_dsi_device *dsi0, >> struct mipi_dsi_device *dsi1, >> u8 cmd, const void *data, size_t len) >> { >> ssize_t err = 0; >> >> if (dsi0) >> err = mipi_dsi_dcs_write(dsi0, cmd, data, len); >> >> if (dsi1 && !err) >> err = mipi_dsi_dcs_write(dsi1, cmd, data, len); >> >> return err; >> } > Thanks for your explanation and this looks more reasonable. >> >> But even that begs the question where does it end? There are a lot of >> mipi_dsi_dcs_*() functions as well as mipi_dsi_generic_write(). Dual >> wrappers for all of them? :o > It's definitly useless to wrap all of them. Please ignore this patch. >> >> >> BR, >> Jani. >> >> >> > /** >> > * struct mipi_dsi_driver - DSI driver >> > * @driver: device driver model driver >> >> -- >> Jani Nikula, Intel Open Source Graphics Center
(CC Javier) On Fri, Mar 10, 2023 at 2:52 PM Jani Nikula <jani.nikula@linux.intel.com> wrote: > >> > + for (i = 0; i < ARRAY_SIZE(dsi); i++) \ > >> > + mipi_dsi_dcs_write_seq(dsi[i], cmd, seq); \ > >> > >> This ignores errors. > > mipi_dsi_dcs_write_seq is also a macro, contains error checks in the body block. > > Ugh, I think it's pretty scary to hide control flow like return > statements in macros like this. The macros are written like this because: #define mipi_dsi_generic_write_seq(dsi, seq...) (...) static const u8 d[] = { seq }; Array of bytes ret = mipi_dsi_generic_write(dsi, d, ARRAY_SIZE(d)); So we can use use ARRAY_SIZE() in the macro and pass in any arbitrary sequence, e.g. mipi_dsi_generic_write_seq(dsi, 0xfb, 0x01); Any function-esque definitions will (as in your example) require a length to be passed in so it would become: mipi_dsi_generic_write_seq(dsi, 0xfb, 0x01, 2); And if you grep mipi_dsi_generic_write_seq | wc -l you find all the 245 opportunities to get that last len wrong and cause an out-of-bounds bug. I think this macro is the lesser evil for this reason, also it saves code that you otherwise have to do manually, and one should never put a person to do a machine's job. Any suggestions to rewrite the macro using varargs welcome. I think it is not very easy to do without the preprocessor. Yours, Linus Walleij
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h index c9df0407980c..d0f0f75d4d83 100644 --- a/include/drm/drm_mipi_dsi.h +++ b/include/drm/drm_mipi_dsi.h @@ -336,6 +336,21 @@ int mipi_dsi_dcs_get_display_brightness_large(struct mipi_dsi_device *dsi, } \ } while (0) +/** + * mipi_dsi_dcs_write_seq - transmit a DCS command with payload + * @dsi: array of 2 DSI peripheral devices + * @cmd: Command + * @seq: buffer containing data to be transmitted + */ +#define mipi_dual_dsi_dcs_write_seq(dsi, cmd, seq...) \ + do { \ + if (ARRAY_SIZE(dsi) > 2) \ + return -EINVAL; \ + int i; \ + for (i = 0; i < ARRAY_SIZE(dsi); i++) \ + mipi_dsi_dcs_write_seq(dsi[i], cmd, seq); \ + } while (0) + /** * struct mipi_dsi_driver - DSI driver * @driver: device driver model driver