Re: [PATCH 5/6] media: platform: Use str_enable_disable-like helpers

[Tomasz Figa]

On Wed, Jan 15, 2025 at 4:39 PM Sakari Ailus
<sakari.ailus@xxxxxxxxxxxxxxx> wrote:
>
> Hi Laurent,
>
> On Tue, Jan 14, 2025 at 10:42:40PM +0200, Laurent Pinchart wrote:
> > Hi Krzysztof,
> >
> > Thank you for the patch.
> >
> > On Tue, Jan 14, 2025 at 08:46:21PM +0100, Krzysztof Kozlowski wrote:
> > > Replace ternary (condition ? "enable" : "disable") syntax with helpers
> > > from string_choices.h because:
> > > 1. Simple function call with one argument is easier to read.  Ternary
> > >    operator has three arguments and with wrapping might lead to quite
> > >    long code.
> >
> > It's more difficult to read for me.
>
> I don't have any issue in using the ternary operator either. Using these
> helpers makes the lines generally 3 characters shorter.
>
> >
> > > 2. Is slightly shorter thus also easier to read.
> > > 3. It brings uniformity in the text - same string.
> > > 4. Allows deduping by the linker, which results in a smaller binary
> > >    file.
> >
> > I don't see why the linker can't de-dup string in the current code.
>
> In fact the functions are static inline so from that point of view I don't
> think there's any difference.
>
> >
> > I'm sorry, I just don't see the point in doing this. I'd like to avoid
> > those changes in the Linux media subsystem, or at the very least in
> > drivers I maintain.
>
> I don't have much of an opinion, perhaps I slightly prefer using these as
> the rest of the kernel does, too. Yet if we choose not to use these
> helpers, we continue to be occasional targets of largish patchsets "fixing"
> this.

To put one more aspect on the scales:

These kinds of patches actually make it more difficult to backport
changes (e.g. fixes) to stable kernels, so my preference would be to
only use the new helpers in new drivers.

Best regards,
Tomasz