linux-kernel - Re: [PATCH 02/30] spi: dw: Use ternary op to init set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200930145759.7djm5xijhg6mjtg3@mobilestation>
Date:   Wed, 30 Sep 2020 17:57:59 +0300
From:   Serge Semin <Sergey.Semin@...kalelectronics.ru>
To:     Mark Brown <broonie@...nel.org>
CC:     Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
        Ramil Zaripov <Ramil.Zaripov@...kalelectronics.ru>,
        Pavel Parkhomenko <Pavel.Parkhomenko@...kalelectronics.ru>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Lars Povlsen <lars.povlsen@...rochip.com>,
        "wuxu . wu" <wuxu.wu@...wei.com>, Feng Tang <feng.tang@...el.com>,
        Rob Herring <robh+dt@...nel.org>, <linux-spi@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 02/30] spi: dw: Use ternary op to init set_cs callback

Mark,
A concrete question is below the main text.)

On Wed, Sep 30, 2020 at 12:55:55AM +0300, Serge Semin wrote:
> On Tue, Sep 29, 2020 at 02:11:53PM +0100, Mark Brown wrote:
> > On Sun, Sep 20, 2020 at 02:28:46PM +0300, Serge Semin wrote:
> > > Simplify the dw_spi_add_host() method a bit by replacing the set_cs
> > > callback overwrite procedure with direct setting the callback if a custom
> > > version of one is specified.
> > 
> > > -	master->set_cs = dw_spi_set_cs;
> > > +	master->set_cs = dws->set_cs ?: dw_spi_set_cs;
> > 
> > > -	if (dws->set_cs)
> > > -		master->set_cs = dws->set_cs;
> > 
> 

> > This doesn't look like a win for legibility or comprehensibility.
> 
> Assigning a default value and redefining it way later doesn't look legible
> either, because in that case you'd need to keep in mind, that some callback has
> already been set. Moreover it does one redundant assignment. That's why I
> decided to implement the setting up by means of the ternary op.
> 
> If you don't like the ternary op, then we could use an explicit if-else
> statement here. But I'd insist on implementing the assignment in a one
> place of the function instead of having it partly perform here and partly there.
> Like this:
> 
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -477,7 +477,10 @@ int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
>  	master->num_chipselect = dws->num_cs;
>  	master->setup = dw_spi_setup;
>  	master->cleanup = dw_spi_cleanup;
> -	master->set_cs = dw_spi_set_cs;
> +	if (dws->set_cs)
> +		master->set_cs = dws->set_cs;
> +	else
> +		master->set_cs = dw_spi_set_cs;
>  	master->transfer_one = dw_spi_transfer_one;
>  	master->handle_err = dw_spi_handle_err;
>  	master->max_speed_hz = dws->max_freq;
> 
> Personally I prefer the ternary op in such situations. The operator provides an
> elegant small well known solution for the default-assignments. I don't see it
> as non-legible or incomprehensible. (I don't really understand why you and
> Andy don't like the operator that much =))
> 
> -Sergey

Judging by having your comment on this patch you obviously didn't like the
ternary operator used to assign a default value to the set_cs callback. So I
suggested a solution, which may suit you. What do you think about it? Agree,
disagree, insist on leaving this part of the code along, etc.

-Sergey