Hi,
On Fri, Jul 18, 2025 at 10:17 AM Brigham Campbell
<m...@brighamcampbell.com> wrote:
>
> On Fri Jul 18, 2025 at 10:10 AM MDT, Doug Anderson wrote:
> >> +#define mipi_dsi_dual(_func, _dsi1, _dsi2, _ctx, ...) \
> >> + _mipi_dsi_dual(_func, _dsi1, _dsi2, _ctx, ##__VA_ARGS__)
> >> +
> >> +#define _mipi_dsi_dual(_func, _dsi1, _dsi2, _ctx, ...) \
> >> + do { \
> >> + (_ctx)->dsi = (_dsi1); \
> >> + _func((_ctx), ##__VA_ARGS__); \
> >
> > nit: shouldn't func be in parenthesis for safety? It's unlikely to
> > really matter, but just in case it's somehow a calculated value that
> > would make it safe from an order-of-operations point of view.
>
> My assumption is that wrapping _func in parenthesis would cause a
> compilation error in the case of _func being a macro (more on that
> later...). I'll test that later today.
Huh, OK. If that's the case then no need to do it.
> >> + (_ctx)->dsi = (_dsi2); \
> >> + _func((_ctx), ##__VA_ARGS__); \
> >> + } while (0)
> >
> > Can you explain why you need the extra level of indirection here (in
> > other words, why do you need to define _mipi_dsi_dual() and then use
> > it in mipi_dsi_dual())? I don't see it buying anything, but maybe it's
> > performing some magic trick I'm not aware of?
>
> I mentioned this in v3 after the changelog and prompty forgot to include
> that information in v4: The extra indirection between mipi_dsi_dual()
> and _mipi_dsi_dual() is to allow for the expansion of _func in the case
> that _func is also a macro (as is the case with
> mipi_dsi_generic_write_seq_multi, i believe). Compilation fails after
> removing the indirection.
>
> There may very well be a better solution to this problem. I'd appreciate
> your thoughts.
Wow, crazy. I think the C preprocessor is one step away from magic.
While I know there are rules for it, I often find the way that it
behaves to be counter-intuitive. I can't say I've followed exactly how
your solution is working, but if it works and is needed then it's OK
w/ me. It might be worth promoting the note to be in the commit
message itself (or even a code comment?) so future people trying to
understand the code will have some chance of stumbling across it...
You might hate this, but one possible other solution would be to make
a custom `mipi_dsi_dual_dcs_write_seq_multi` (lifting it out of the
novatek driver) and then say that the "_func" parameter can't be a
macro. If you did it correctly, it would be a pretty big space savings
too. Unlike how we did it in the novatek driver, I think a proper way
to do it that would save the most space would be:
#define mipi_dsi_dual_dcs_write_seq_multi(ctx, dsi0, dsi1, cmd, seq...) \
do { \
static const u8 d[] = { cmd, seq }; \
mipi_dsi_dual_dcs_write_buffer_multi(ctx, dsi0, dsi1, \
d, ARRAY_SIZE(d)); \
} while (0)
...and then mipi_dsi_dual_dcs_write_buffer_multi() would be
implemented in drm_mipi_dsi.c.
With the above implementation, you only have one "static const" buffer
(maybe the compiler is smart enough to combine w/ the novatek code,
but it might not be) and also only have a single function call taking
up space in the panel driver. You'd only have the "custom" dual
implementation for the "write_seq" stuff since that appears to be the
most common. All the other DSI calls could use the normal
mipi_dsi_dual() macro...
I was thinking of suggesting that as an optional followup to your
series anyway (for the space savings), but it could also solve some of
the preprocessor woes. :-P
I'm certainly not dead-set on this, so if you want to just keep
something like your current solution that's OK w/ me too.
> > Reading this with a fresh set of eyes, I also realize that this macro
> > is probably vulnerable to issues where arguments have side effects
> > since we have to repeat them. I don't think there's a way around this
> > and I think the macro is still worthwhile, but something to go into
> > with open eyes. Possibly worth noting in the macro description? We
> > could probably at least eliminate the need to reference "_ctx" more
> > than once by assigning it to a local variable. I think referencing
> > "_func" and "__VA_ARGS__" more than once is unavoidable...
>
> I'm using _func, _ctx, and __VA_ARGS__ more than once in this macro and
> I don't expect the indirection to fix the potential issue of unintended
> side effects. I believe we can use GNU extensions to eliminate side
> effects to _ctx,
I wasn't thinking of any GNU extensions. Just using a scope-defined
local variable...
#define _mipi_dsi_dual(_func, _dsi1, _dsi2, _ctx, ...) \
do { \
struct mipi_dsi_multi_context *__ctx = (_ctx); \
__ctx->dsi = (_dsi1); \
...
> but especially since _func can be a macro, I don't
> think there's much to be done about it. Not sure about __VA_ARGS__.
>
> I fear my inexperience is made sorely manifest here.
I think it's a rare person who fully understands the dirty corners of
the C preprocessor and I wouldn't count myself as one of them. I can
sometimes make it do what I want, but I think we're up against my
limits too...
-Doug
