[v1,3/8] ASoC: loongson: Simplify with dev_err_probe()

Commit Message

Binbin Zhou Sept. 9, 2024, 7:19 a.m. UTC

Error handling in probe() can be a bit simpler with dev_err_probe().

Signed-off-by: Binbin Zhou <[e-mail]>
---
 sound/soc/loongson/loongson_card.c    | 23 ++++++++-----------
 sound/soc/loongson/loongson_i2s_pci.c | 33 ++++++++++-----------------
 2 files changed, 21 insertions(+), 35 deletions(-)

Comments

Krzysztof Kozlowski Sept. 9, 2024, 7:45 a.m. UTC | #1

On 09/09/2024 09:19, Binbin Zhou wrote:
> Error handling in probe() can be a bit simpler with dev_err_probe().
> 
> Signed-off-by: Binbin Zhou <[e-mail]>

...

>  
> -	if (has_acpi_companion(dev))
> -		ret = loongson_card_parse_acpi(ls_priv);
> -	else
> -		ret = loongson_card_parse_of(ls_priv);
> -	if (ret < 0)
> -		return ret;
> +	ret = has_acpi_companion(dev) ? loongson_card_parse_acpi(ls_priv)
> +				      : loongson_card_parse_of(ls_priv);
> +	if (ret)
> +		dev_err_probe(dev, ret, "Error parsing acpi/of properties\n");

This does not make anything simpler - you don't use dev_err_probe()
features here. OTOH, you duplicate error messages.

Best regards,
Krzysztof

Mark Brown Sept. 9, 2024, 3:15 p.m. UTC | #2

On Mon, Sep 09, 2024 at 03:19:07PM +0800, Binbin Zhou wrote:

>  	ret = device_property_read_string(dev, "model", &card->name);
> -	if (ret) {
> -		dev_err(dev, "Error parsing card name: %d\n", ret);
> -		return ret;
> -	}
> +	if (ret)
> +		dev_err_probe(dev, ret, "Error parsing card name\n");
> +

This means that we change from failing if there is an error to only
logging about it (and then only during probe).  Perhaps that makes sense
but the changelog should explain what's going on.  Most if not all of
the updates do this, some of them seem like clear bugs.

> -	if (has_acpi_companion(dev))
> -		ret = loongson_card_parse_acpi(ls_priv);
> -	else
> -		ret = loongson_card_parse_of(ls_priv);
> -	if (ret < 0)
> -		return ret;
> +	ret = has_acpi_companion(dev) ? loongson_card_parse_acpi(ls_priv)
> +				      : loongson_card_parse_of(ls_priv);
> +	if (ret)
> +		dev_err_probe(dev, ret, "Error parsing acpi/of properties\n");

Please just write normal conditional statements, they're far clearer -
there's applications for the ternery operator when embedded in other
things but normally it just makes things harder to read.

Patch

diff --git a/sound/soc/loongson/loongson_card.c b/sound/soc/loongson/loongson_card.c
index d3cd23ddd027..f0ed4508f38a 100644
--- a/sound/soc/loongson/loongson_card.c
+++ b/sound/soc/loongson/loongson_card.c
@@ -176,22 +176,17 @@  static int loongson_asoc_card_probe(struct platform_device *pdev)
 	snd_soc_card_set_drvdata(card, ls_priv);
 
 	ret = device_property_read_string(dev, "model", &card->name);
-	if (ret) {
-		dev_err(dev, "Error parsing card name: %d\n", ret);
-		return ret;
-	}
+	if (ret)
+		dev_err_probe(dev, ret, "Error parsing card name\n");
+
 	ret = device_property_read_u32(dev, "mclk-fs", &ls_priv->mclk_fs);
-	if (ret) {
-		dev_err(dev, "Error parsing mclk-fs: %d\n", ret);
-		return ret;
-	}
+	if (ret)
+		dev_err_probe(dev, ret, "Error parsing mclk-fs\n");
 
-	if (has_acpi_companion(dev))
-		ret = loongson_card_parse_acpi(ls_priv);
-	else
-		ret = loongson_card_parse_of(ls_priv);
-	if (ret < 0)
-		return ret;
+	ret = has_acpi_companion(dev) ? loongson_card_parse_acpi(ls_priv)
+				      : loongson_card_parse_of(ls_priv);
+	if (ret)
+		dev_err_probe(dev, ret, "Error parsing acpi/of properties\n");
 
 	return devm_snd_soc_register_card(dev, card);
 }
diff --git a/sound/soc/loongson/loongson_i2s_pci.c b/sound/soc/loongson/loongson_i2s_pci.c
index e8ea28bc5a5f..3872b1d8fce0 100644
--- a/sound/soc/loongson/loongson_i2s_pci.c
+++ b/sound/soc/loongson/loongson_i2s_pci.c
@@ -97,13 +97,12 @@  static int loongson_i2s_pci_probe(struct pci_dev *pdev,
 		dev_err(dev, "iomap_regions failed\n");
 		return ret;
 	}
+
 	i2s->reg_base = pcim_iomap_table(pdev)[0];
 	i2s->regmap = devm_regmap_init_mmio(dev, i2s->reg_base,
 					    &loongson_i2s_regmap_config);
-	if (IS_ERR(i2s->regmap)) {
-		dev_err(dev, "regmap_init_mmio failed\n");
-		return PTR_ERR(i2s->regmap);
-	}
+	if (IS_ERR(i2s->regmap))
+		dev_err_probe(dev, PTR_ERR(i2s->regmap), "regmap_init_mmio failed\n");
 
 	tx_data = &i2s->tx_dma_data;
 	rx_data = &i2s->rx_dma_data;
@@ -115,22 +114,16 @@  static int loongson_i2s_pci_probe(struct pci_dev *pdev,
 	rx_data->order_addr = i2s->reg_base + LS_I2S_RX_ORDER;
 
 	tx_data->irq = fwnode_irq_get_byname(fwnode, "tx");
-	if (tx_data->irq < 0) {
-		dev_err(dev, "dma tx irq invalid\n");
-		return tx_data->irq;
-	}
+	if (tx_data->irq < 0)
+		dev_err_probe(dev, tx_data->irq, "dma tx irq invalid\n");
 
 	rx_data->irq = fwnode_irq_get_byname(fwnode, "rx");
-	if (rx_data->irq < 0) {
-		dev_err(dev, "dma rx irq invalid\n");
-		return rx_data->irq;
-	}
+	if (rx_data->irq < 0)
+		dev_err_probe(dev, rx_data->irq, "dma rx irq invalid\n");
 
-	device_property_read_u32(dev, "clock-frequency", &i2s->clk_rate);
-	if (!i2s->clk_rate) {
-		dev_err(dev, "clock-frequency property invalid\n");
-		return -EINVAL;
-	}
+	ret = device_property_read_u32(dev, "clock-frequency", &i2s->clk_rate);
+	if (ret)
+		dev_err_probe(dev, ret, "clock-frequency property invalid\n");
 
 	dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
 
@@ -141,10 +134,8 @@  static int loongson_i2s_pci_probe(struct pci_dev *pdev,
 
 	ret = devm_snd_soc_register_component(dev, &loongson_i2s_component,
 					      &loongson_i2s_dai, 1);
-	if (ret) {
-		dev_err(dev, "register DAI failed %d\n", ret);
-		return ret;
-	}
+	if (ret)
+		dev_err_probe(dev, ret, "register DAI failed\n");
 
 	return 0;
 }