Hi, On Fri, Apr 26, 2024 at 11:33 PM Sam Ravnborg <sam@xxxxxxxxxxxx> wrote: > > > --- > > Right now this patch introduces two new functions in > > drm_mipi_dsi.c. Alternatively we could have changed the prototype of > > the "chatty" functions and made the deprecated macros adapt to the new > > prototype. While this sounds nice, it bloated callers of the > > deprecated functioin a bit because it caused the compiler to emit less > > optimal code. It doesn't seem terrible to add two more functions, so I > > went that way. > The concern here is when it will be cleaned up. As a minimum please > consider adding an item to todo.rst that details what should be done > to get rid of the now obsolete chatty functions so someone can pick it > up. Sure, I can add something to do TODO. Do folks agree that's the preferred way to do things? A few thoughts I had: 1. In theory it could be useful to keep _both_ the "chatty" and "multi" variants of the functions. In at least a few places the "multi" variant was awkward. The resulting auo_kd101n80_45na_init() [1] looked awkward. If I was writing just this function I would have chosen an API more like the "chatty" one, but since I was trying to make all the init functions similar I kept them all using the "multi" API. Does it make sense to keep both long term? [1] https://lore.kernel.org/all/20240426165839.v2.7.Ib5030ab5cd41b4e08b1958bd7e51571725723008@changeid/ 2. Going completely the opposite direction, we could also not bother saving as much space today and _force_ everyone to move to the new "multi" API to get the full space savings. So I guess three options: a) leave my patches the way they are and add a TODO, b) Keep the "chatty" variants long term and remove my "after-the-cut", or c) Don't get as much space savings today but don't introduce a new exported function. Opinions? > > @@ -792,6 +792,34 @@ int mipi_dsi_generic_write_chatty(struct mipi_dsi_device *dsi, > > } > > EXPORT_SYMBOL(mipi_dsi_generic_write_chatty); > > > > +/** > > + * mipi_dsi_generic_write_multi() - mipi_dsi_generic_write_chatty() w/ accum_err > > + * @ctx: Context for multiple DSI transactions > > + * @payload: buffer containing the payload > > + * @size: size of payload buffer > > + * > > + * Like mipi_dsi_generic_write_chatty() but deals with errors in a way that > > + * makes it convenient to make several calls in a row. > > + */ > > +void mipi_dsi_generic_write_multi(struct mipi_dsi_multi_context *ctx, > > + const void *payload, size_t size) > > +{ > > + struct mipi_dsi_device *dsi = ctx->dsi; > > + struct device *dev = &dsi->dev; > > + ssize_t ret; > > + > > + if (ctx->accum_err) > > + return; > > + > > + ret = mipi_dsi_generic_write(dsi, payload, size); > > + if (ret < 0) { > > + ctx->accum_err = ret; > > + dev_err_ratelimited(dev, "sending generic data %*ph failed: %d\n", > > + (int)size, payload, ctx->accum_err); > > + } > I see no point in using ratelimited and then change it in the next > patch. Sure, I'll re-order patches. > > @@ -197,6 +197,22 @@ struct mipi_dsi_device { > > struct drm_dsc_config *dsc; > > }; > > > > +/** > > + * 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; > > +}; > Inline comments is - I think - preferred. Sure, I'll update in the next version.
