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
