| Message ID | 20240204162106.1179621-1-andy.shevchenko@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/1] spi: fsl-dspi: Unify error messaging in dspi_request_dma() | expand |
On Sun, Feb 04, 2024 at 06:21:06PM +0200, andy.shevchenko@gmail.com wrote: > Use `ret = dev_err_probe(...);` pattern for all messages in dspi_request_dma() > for the sake of uniforming them. While at it, fix indentation issue reported > by Vladimir Oltean. When did I do that? This is v1. > > Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> > --- > drivers/spi/spi-fsl-dspi.c | 15 +++++---------- > 1 file changed, 5 insertions(+), 10 deletions(-) > > diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c > index c9eae046f66c..e0832f1f10bd 100644 > --- a/drivers/spi/spi-fsl-dspi.c > +++ b/drivers/spi/spi-fsl-dspi.c > @@ -502,15 +502,12 @@ static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr) > return -ENOMEM; > > dma->chan_rx = dma_request_chan(dev, "rx"); > - if (IS_ERR(dma->chan_rx)) { > - return dev_err_probe(dev, PTR_ERR(dma->chan_rx), > - "rx dma channel not available\n"); > - } > + if (IS_ERR(dma->chan_rx)) > + return dev_err_probe(dev, PTR_ERR(dma->chan_rx), "rx dma channel not available\n"); > > dma->chan_tx = dma_request_chan(dev, "tx"); > if (IS_ERR(dma->chan_tx)) { > - ret = PTR_ERR(dma->chan_tx); > - dev_err_probe(dev, ret, "tx dma channel not available\n"); > + ret = dev_err_probe(dev, PTR_ERR(dma->chan_tx), "tx dma channel not available\n"); > goto err_tx_channel; > } > > @@ -541,16 +538,14 @@ static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr) > cfg.direction = DMA_DEV_TO_MEM; > ret = dmaengine_slave_config(dma->chan_rx, &cfg); > if (ret) { > - dev_err(dev, "can't configure rx dma channel\n"); > - ret = -EINVAL; > + ret = dev_err_probe(dev, -EINVAL, "can't configure rx dma channel\n"); Passing -EINVAL to dev_err_probe() here doesn't work. It overwrites the "ret" from dmaengine_slave_config(). int dev_err_probe(const struct device *dev, int err, const char *fmt, ...) { struct va_format vaf; va_list args; va_start(args, fmt); vaf.fmt = fmt; vaf.va = &args; if (err != -EPROBE_DEFER) { // <-------- always true dev_err(dev, "error %pe: %pV", ERR_PTR(err), &vaf); } else { device_set_deferred_probe_reason(dev, &vaf); dev_dbg(dev, "error %pe: %pV", ERR_PTR(err), &vaf); } va_end(args); return err; } > goto err_slave_config; > } > > cfg.direction = DMA_MEM_TO_DEV; > ret = dmaengine_slave_config(dma->chan_tx, &cfg); > if (ret) { > - dev_err(dev, "can't configure tx dma channel\n"); > - ret = -EINVAL; > + ret = dev_err_probe(dev, -EINVAL, "can't configure tx dma channel\n"); Same here. > goto err_slave_config; > } > > -- > 2.43.0 > >
On Sun, Feb 04, 2024 at 06:21:06PM +0200, andy.shevchenko@gmail.com wrote: > Use `ret = dev_err_probe(...);` pattern for all messages in dspi_request_dma() > for the sake of uniforming them. While at it, fix indentation issue reported > by Vladimir Oltean. "making them uniform" sounds better than "uniforming them".
On Sun, Feb 04, 2024 at 08:24:17PM +0200, Vladimir Oltean wrote: > > @@ -541,16 +538,14 @@ static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr) > > cfg.direction = DMA_DEV_TO_MEM; > > ret = dmaengine_slave_config(dma->chan_rx, &cfg); > > if (ret) { > > - dev_err(dev, "can't configure rx dma channel\n"); > > - ret = -EINVAL; > > + ret = dev_err_probe(dev, -EINVAL, "can't configure rx dma channel\n"); > > Passing -EINVAL to dev_err_probe() here doesn't work. It overwrites the "ret" > from dmaengine_slave_config(). > > int dev_err_probe(const struct device *dev, int err, const char *fmt, ...) > { > struct va_format vaf; > va_list args; > > va_start(args, fmt); > vaf.fmt = fmt; > vaf.va = &args; > > if (err != -EPROBE_DEFER) { // <-------- always true > dev_err(dev, "error %pe: %pV", ERR_PTR(err), &vaf); > } else { > device_set_deferred_probe_reason(dev, &vaf); > dev_dbg(dev, "error %pe: %pV", ERR_PTR(err), &vaf); > } > > va_end(args); > > return err; > } Ah, the original code also ignores the dmaengine_slave_config() return code and replaces it with -EINVAL? I wonder why that is... It doesn't appear to be a widespread pattern. Pretty arbitrary. Could you please make 2 patches, one which preserves the original return code and another which uses dev_err_probe()?
On Sun, Feb 4, 2024 at 8:24 PM Vladimir Oltean <olteanv@gmail.com> wrote: > On Sun, Feb 04, 2024 at 06:21:06PM +0200, andy.shevchenko@gmail.com wrote: > > Use `ret = dev_err_probe(...);` pattern for all messages in dspi_request_dma() > > for the sake of uniforming them. While at it, fix indentation issue reported > > by Vladimir Oltean. > > When did I do that? This is v1. In the original submission (v2 of it to be precise) by Minjie. ... > > ret = dmaengine_slave_config(dma->chan_rx, &cfg); > > if (ret) { > > - dev_err(dev, "can't configure rx dma channel\n"); > > - ret = -EINVAL; > > + ret = dev_err_probe(dev, -EINVAL, "can't configure rx dma channel\n"); > > Passing -EINVAL to dev_err_probe() here doesn't work. It overwrites the "ret" > from dmaengine_slave_config(). True, but this patch doesn't change the behaviour. ... > > if (ret) { > > - dev_err(dev, "can't configure tx dma channel\n"); > > - ret = -EINVAL; > > + ret = dev_err_probe(dev, -EINVAL, "can't configure tx dma channel\n"); > > Same here. Same answer here. > > goto err_slave_config; > > }
On Sun, Feb 4, 2024 at 8:26 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > On Sun, Feb 04, 2024 at 06:21:06PM +0200, andy.shevchenko@gmail.com wrote: > > Use `ret = dev_err_probe(...);` pattern for all messages in dspi_request_dma() > > for the sake of uniforming them. While at it, fix indentation issue reported > > by Vladimir Oltean. > > "making them uniform" sounds better than "uniforming them". Sure, thanks for the suggestion.
On Sun, Feb 4, 2024 at 8:31 PM Vladimir Oltean <olteanv@gmail.com> wrote: > On Sun, Feb 04, 2024 at 08:24:17PM +0200, Vladimir Oltean wrote: ... > > Passing -EINVAL to dev_err_probe() here doesn't work. It overwrites the "ret" > > from dmaengine_slave_config(). > Ah, the original code also ignores the dmaengine_slave_config() return > code and replaces it with -EINVAL? I wonder why that is... It doesn't > appear to be a widespread pattern. Pretty arbitrary. Could you please > make 2 patches, one which preserves the original return code and another > which uses dev_err_probe()? Sure.
diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c index c9eae046f66c..e0832f1f10bd 100644 --- a/drivers/spi/spi-fsl-dspi.c +++ b/drivers/spi/spi-fsl-dspi.c @@ -502,15 +502,12 @@ static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr) return -ENOMEM; dma->chan_rx = dma_request_chan(dev, "rx"); - if (IS_ERR(dma->chan_rx)) { - return dev_err_probe(dev, PTR_ERR(dma->chan_rx), - "rx dma channel not available\n"); - } + if (IS_ERR(dma->chan_rx)) + return dev_err_probe(dev, PTR_ERR(dma->chan_rx), "rx dma channel not available\n"); dma->chan_tx = dma_request_chan(dev, "tx"); if (IS_ERR(dma->chan_tx)) { - ret = PTR_ERR(dma->chan_tx); - dev_err_probe(dev, ret, "tx dma channel not available\n"); + ret = dev_err_probe(dev, PTR_ERR(dma->chan_tx), "tx dma channel not available\n"); goto err_tx_channel; } @@ -541,16 +538,14 @@ static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr) cfg.direction = DMA_DEV_TO_MEM; ret = dmaengine_slave_config(dma->chan_rx, &cfg); if (ret) { - dev_err(dev, "can't configure rx dma channel\n"); - ret = -EINVAL; + ret = dev_err_probe(dev, -EINVAL, "can't configure rx dma channel\n"); goto err_slave_config; } cfg.direction = DMA_MEM_TO_DEV; ret = dmaengine_slave_config(dma->chan_tx, &cfg); if (ret) { - dev_err(dev, "can't configure tx dma channel\n"); - ret = -EINVAL; + ret = dev_err_probe(dev, -EINVAL, "can't configure tx dma channel\n"); goto err_slave_config; }
Use `ret = dev_err_probe(...);` pattern for all messages in dspi_request_dma() for the sake of uniforming them. While at it, fix indentation issue reported by Vladimir Oltean. Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> --- drivers/spi/spi-fsl-dspi.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)