| Message ID | 20200920112914.26501-3-Sergey.Semin@baikalelectronics.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spi: dw: Add full Baikal-T1 SPI Controllers support | expand |
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.
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
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
On Wed, Sep 30, 2020 at 05:57:59PM +0300, Serge Semin wrote: > On Wed, Sep 30, 2020 at 12:55:55AM +0300, Serge Semin wrote: > > + if (dws->set_cs) > > + master->set_cs = dws->set_cs; > > + else > > + master->set_cs = dw_spi_set_cs; > 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. That looks fine.
On Wed, Sep 30, 2020 at 04:01:17PM +0100, Mark Brown wrote: > On Wed, Sep 30, 2020 at 05:57:59PM +0300, Serge Semin wrote: > > On Wed, Sep 30, 2020 at 12:55:55AM +0300, Serge Semin wrote: > > > > + if (dws->set_cs) > > > + master->set_cs = dws->set_cs; > > > + else > > > + master->set_cs = dw_spi_set_cs; > > > 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. > > That looks fine. Ok. I'll implement it in the next patchset version. -Sergey
diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c index 55afdcee7d2b..8b3ce5a0378a 100644 --- a/drivers/spi/spi-dw-core.c +++ b/drivers/spi/spi-dw-core.c @@ -482,7 +482,7 @@ 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; + master->set_cs = dws->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; @@ -491,9 +491,6 @@ int dw_spi_add_host(struct device *dev, struct dw_spi *dws) master->flags = SPI_MASTER_GPIO_SS; master->auto_runtime_pm = true; - if (dws->set_cs) - master->set_cs = dws->set_cs; - /* Get default rx sample delay */ device_property_read_u32(dev, "rx-sample-delay-ns", &dws->def_rx_sample_dly_ns);
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. Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> --- drivers/spi/spi-dw-core.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)