lkml.org 
[lkml]   [2020]   [Sep]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2020-09-30 16:58    [W:0.066 / U:17.524 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site