On Fri, 10 Mar 2023, Jianhua Lu <lujianhua000@xxxxxxxxx> wrote:
> 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.
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
