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
