All of lore.kernel.org
 help / color / mirror / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
Cc: Lee Jones <lee.jones@linaro.org>,
	linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] staging: most: dim2: force fcnt=3 on Renesas GEN3
Date: Mon, 27 Sep 2021 17:50:55 +0200	[thread overview]
Message-ID: <YVHoXyULqXsWktMN@kroah.com> (raw)
In-Reply-To: <17e51208-18b9-56d8-e8e3-2e40d6e94438@cogentembedded.com>

On Mon, Sep 27, 2021 at 06:40:13PM +0300, Nikita Yushchenko wrote:
> > > +	dev_fcnt = pdata && pdata->fcnt ? pdata->fcnt : fcnt;
> > 
> > Please use a real if () statement here and do not bury real logic in a
> > crazy line like this one, as that is all but impossible to maintain over
> > time.
> 
> The same source file already uses the same form of conditional expressions several lines above:
> 
> > ret = pdata && pdata->enable ? pdata->enable(pdev) : 0;

It's not good there either.

There is a reason this code is still in drivers/staging/ and that is one
of them.  Let's not duplicate bad code for no real reason please.

> > dev->disable_platform = pdata ? pdata->disable : NULL;
> 
> Shall I use real if statement for my expression while keeping those as-is? This looks ... strange.
> Or shall I convert all conditional expressions to if statements?

Do not add additional ? : statements in this patch.  I would be glad to
take an add-on patch after this that fixes up the other uses in the
driver, but that should be separate as that is a separate issue here.

thanks,

greg k-h

  reply	other threads:[~2021-09-27 15:50 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-21 16:51 [PATCH] staging: most: dim2: force fcnt=3 on Renesas GEN3 Nikita Yushchenko
2021-09-27 15:29 ` Greg Kroah-Hartman
2021-09-27 15:40   ` Nikita Yushchenko
2021-09-27 15:50     ` Greg Kroah-Hartman [this message]
2021-09-27 15:58       ` [PATCH v2] " Nikita Yushchenko
2021-09-28  7:11         ` Greg Kroah-Hartman
2021-09-27 16:06       ` [PATCH] staging: most: dim2: use if statements instead of ?: expressions Nikita Yushchenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YVHoXyULqXsWktMN@kroah.com \
    --to=gregkh@linuxfoundation.org \
    --cc=lee.jones@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-staging@lists.linux.dev \
    --cc=nikita.yoush@cogentembedded.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.