Re: [PATCH] drm/mipi-dsi: Add a mipi_dual_dsi_dcs_write_seq() macro

[Jianhua Lu]

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