linux-kernel - Re: [PATCH] drm/mipi-dsi: Add a mipi_dual_dsi_dcs_write

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 10 Mar 2023 20:20:07 +0800
From:   Jianhua Lu <lujianhua000@...il.com>
To:     Jani Nikula <jani.nikula@...ux.intel.com>
Cc:     Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>, phone-devel@...r.kernel.org,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        ~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH] drm/mipi-dsi: Add a mipi_dual_dsi_dcs_write_seq() macro

On Fri, Mar 10, 2023 at 01:54:18PM +0200, Jani Nikula wrote:
> On Fri, 10 Mar 2023, Jianhua Lu <lujianhua000@...il.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@...il.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