Re: [PATCH v2 3/9] iio: backend adi-axi-dac: extend features

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Le 05/09/2024 à 17:17, Angelo Dureghello a écrit :
From: Angelo Dureghello <adureghello-rdvid1DuHRBWk0Htik3J/w@xxxxxxxxxxxxxxxx>

Extend DAC backend with new features required for the AXI driver
version for the ad3552r DAC. Mainly, a new compatible string has
been added to support a DAC IP very similar to the generic DAC IP
but with some customizations to work with the ad3552r.

Then, a serie of generic functions has been added to match with
ad3552r needs. Function names has been kept generic as much as
possible, to allow re-utilization from other frontend drivers.

Hi,

...

+static int axi_dac_read_raw(struct iio_backend *back,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	struct axi_dac_state *st = iio_backend_get_priv(back);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_FREQUENCY:
+		*val = clk_get_rate(devm_clk_get(st->dev, 0));

Having a devm_clk_get() in such a place is really unusual.
Is it correct?

This look like a memory leak to me.

+
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}

...

+		/*
+		 * Both REG_CNTRL_2 and AXI_DAC_CNTRL_DATA_WR need to know
+		 * the data size. So keeping data size control here only,
+		 * since data size is mandatory for to the current transfer.

"... for to ..." sounds strange to my *non*-native English ears.

+		 * DDR state handled separately by specific backend calls,
+		 * generally all raw register writes are SDR.
+		 */
+		if (data_size == 1)
+			ret = regmap_set_bits(st->regmap, AXI_DAC_REG_CNTRL_2,
+					      AXI_DAC_SYMB_8B);
+		else
+			ret = regmap_clear_bits(st->regmap, AXI_DAC_REG_CNTRL_2,
+						AXI_DAC_SYMB_8B);
+		if (ret)
+			return ret;

...

@@ -556,10 +792,12 @@ static int axi_dac_probe(struct platform_device *pdev)
  	if (!st)
  		return -ENOMEM;
- expected_ver = device_get_match_data(&pdev->dev);
-	if (!expected_ver)
+	info = device_get_match_data(&pdev->dev);
+	if (!info)

writing:
	st->info = device_get_match_data(&pdev->dev);
	if (!st->info)

would save the 'info' variable and a few lines of code without loosing (IMHO) readability.

CJ

  		return -ENODEV;
+ st->info = info;
+
  	clk = devm_clk_get_enabled(&pdev->dev, NULL);
  	if (IS_ERR(clk))
  		return dev_err_probe(&pdev->dev, PTR_ERR(clk),
@@ -588,12 +826,13 @@ static int axi_dac_probe(struct platform_device *pdev)
  	if (ret)
  		return ret;
- if (ADI_AXI_PCORE_VER_MAJOR(ver) != ADI_AXI_PCORE_VER_MAJOR(*expected_ver)) {
+	if (ADI_AXI_PCORE_VER_MAJOR(ver) !=
+		ADI_AXI_PCORE_VER_MAJOR(st->info->version)) {
  		dev_err(&pdev->dev,
  			"Major version mismatch. Expected %d.%.2d.%c, Reported %d.%.2d.%c\n",
-			ADI_AXI_PCORE_VER_MAJOR(*expected_ver),
-			ADI_AXI_PCORE_VER_MINOR(*expected_ver),
-			ADI_AXI_PCORE_VER_PATCH(*expected_ver),
+			ADI_AXI_PCORE_VER_MAJOR(st->info->version),
+			ADI_AXI_PCORE_VER_MINOR(st->info->version),
+			ADI_AXI_PCORE_VER_PATCH(st->info->version),
  			ADI_AXI_PCORE_VER_MAJOR(ver),
  			ADI_AXI_PCORE_VER_MINOR(ver),
  			ADI_AXI_PCORE_VER_PATCH(ver));
@@ -631,10 +870,18 @@ static int axi_dac_probe(struct platform_device *pdev)
  	return 0;
  }

...





[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux