From:   Jani Nikula <jani.nikula@linux.intel.com>
To:     Jianhua Lu <lujianhua000@gmail.com>
Cc:     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>, phone-devel@vger.kernel.org,
        linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
        ~postmarketos/upstreaming@lists.sr.ht
Subject: Re: [PATCH] drm/mipi-dsi: Add a mipi_dual_dsi_dcs_write_seq() macro
In-Reply-To: <ZAsgd4zsgbvWT0U0@Gentoo>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
References: <20230310110542.6649-1-lujianhua000@gmail.com>
 <87mt4k95zp.fsf@intel.com> <ZAsgd4zsgbvWT0U0@Gentoo>
Date:   Fri, 10 Mar 2023 15:52:45 +0200
Message-ID: <87bkl090ia.fsf@intel.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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

-- 
Jani Nikula, Intel Open Source Graphics Center