lkml.org 
[lkml]   [2024]   [May]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v2 4/8] drm/mipi-dsi: Introduce mipi_dsi_*_write_seq_multi()
Hi,

On Mon, Apr 29, 2024 at 8:39 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
> On Mon, 29 Apr 2024, Doug Anderson <dianders@chromium.org> wrote:
> > Hi,
> >
> > On Mon, Apr 29, 2024 at 2:38 AM Neil Armstrong
> > <neil.armstrong@linaro.org> wrote:
> >>
> >> > +/**
> >> > + * struct mipi_dsi_multi_context - Context to call multiple MIPI DSI funcs in a row
> >> > + * @dsi: Pointer to the MIPI DSI device
> >> > + * @accum_err: Storage for the accumulated error over the multiple calls. Init
> >> > + * to 0. If a function encounters an error then the error code will be
> >> > + * stored here. If you call a function and this points to a non-zero
> >> > + * value then the function will be a noop. This allows calling a function
> >> > + * many times in a row and just checking the error at the end to see if
> >> > + * any of them failed.
> >> > + */
> >> > +
> >> > +struct mipi_dsi_multi_context {
> >> > + struct mipi_dsi_device *dsi;
> >> > + int accum_err;
> >> > +};
> >>
> >> I like the design, but having a context struct seems over-engineered while we could pass
> >> a single int over without encapsulating it with mipi_dsi_multi_context.
> >>
> >> void mipi_dsi_dcs_write_buffer_multi(struct mipi_dsi_multi_context *ctx,
> >> const void *data, size_t len);
> >> vs
> >> void mipi_dsi_dcs_write_buffer_multi(struct mipi_dsi_device *dsi, int *accum_err,
> >> const void *data, size_t len);
> >>
> >> is the same, and it avoids having to declare a mipi_dsi_multi_context and set ctx->dsi,
> >> and I'll find it much easier to migrate, just add a &ret and make sure ret is initialized to 0.
> >
> > Yeah, I had the same reaction when Jani suggested the context style
> > [1] and I actually coded it up exactly as you suggest above. I then
> > changed my mind and went with the context. My motivation was that when
> > I tested it I found that using the context produced smaller code.
> > Specifically, from the description of this patch we see we end up
> > with:
> >
> > Total: Before=10651, After=9663, chg -9.28%
> >
> > ...when I didn't have the context and I had the accum_err then instead
> > of getting ~9% smaller I believe it actually got ~0.5% bigger. This
> > makes some sense as the caller has to pass 4 parameters to each call
> > instead of 3.
> >
> > It's not a giant size difference but it was at least some motivation
> > that helped push me in this direction. I'd also say that when I looked
> > at the code in the end the style grew on me. It's really not too
> > terrible to have one line in your functions that looks like:
> >
> > struct mipi_dsi_multi_context ctx = { .dsi = boe->dsi };
> >
> > ...and if that becomes the canonical way to do it then it's really
> > hard to accidentally forget to initialize the error value. With the
> > other API it's _very_ easy to forget to initialize the error value and
> > the compiler won't yell at you. It also makes it very obvious to the
> > caller that this function is doing something a little different than
> > most Linux APIs with that error return.
> >
> > So I guess I'd say that I ended up being pretty happy with the
> > "context" even if it does feel a little over engineered and I'd argue
> > to keep it that way. That being said, if you feel strongly about it
> > then we can perhaps get others to chime in to see which style they
> > prefer? Let me know what you think.
> >
> >
> > [1] https://lore.kernel.org/r/8734r85tcf.fsf@intel.com
>
> FWIW, I don't feel strongly about this, and I could be persuaded either
> way, but I've got this gut feeling that an extensible context parameter
> might be benefitial future proofing in this case.

I've gone ahead and sent out a v3 where I still leave it as taking
`struct mipi_dsi_multi_context`. Neil: if you feel strongly about it,
I have no problem sending out a v4. I still think that the size
savings of using the context are a good "tiebreaking" factor in
choosing between the two styles... ;-)

-Doug

\
 
 \ /
  Last update: 2024-05-01 17:55    [W:0.518 / U:0.924 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site