Hi Lars,

Thanks for the review.

On 10/19/21 5:20 PM, Anand Ashok Dumbre wrote:

quoted

[....]
+struct ams {
+	void __iomem *base;
+	void __iomem *ps_base;
+	void __iomem *pl_base;
+	struct clk *clk;
+	struct device *dev;
+	/* check kernel doc above */
+	struct mutex lock;
+	/* check kernel doc above */

Do we need this comment?

Will remove.

quoted

+	spinlock_t intr_lock;
+	unsigned int alarm_mask;
+	unsigned int masked_alarm;
+	u64 intr_mask;
+	int irq;

The irq field is only ever used in the probe function. Could be made a local
variable.

Will make it local.

quoted

+	struct delayed_work ams_unmask_work; };
[...]

+static void ams_enable_channel_sequence(struct iio_dev *indio_dev) {
+	int i;
+	unsigned long long scan_mask;
+	struct ams *ams = iio_priv(indio_dev);
+
+	/*
+	 * Enable channel sequence. First 22 bits of scan_mask represent
+	 * PS channels, and next remaining bits represent PL channels.
+	 */
+
+	/* Run calibration of PS & PL as part of the sequence */
+	scan_mask = 0x1 | BIT(AMS_PS_SEQ_MAX);
+	for (i = 0; i < indio_dev->num_channels; i++)
+		scan_mask |= BIT_ULL(indio_dev->channels[i].scan_index);

There are channels with a scan index > 63. These need to be skipped here to
avoid undefined behavior. UBSAN is reporting an error here.

I will fix it.

quoted

[...]
+	if (ams->pl_base) {
+		/* put sysmon in a soft reset to change the sequence */
+		ams_pl_update_reg(ams, AMS_REG_CONFIG1,

AMS_CONF1_SEQ_MASK,

quoted

+				  AMS_CONF1_SEQ_DEFAULT);
+
+		/* configure basic channels */
+		scan_mask = FIELD_GET(AMS_PL_SEQ_MASK, scan_mask);
+		writel(FIELD_GET(AMS_REG_SEQ0_MASK, scan_mask),
+		       ams->pl_base + AMS_REG_SEQ_CH0);
+		writel(FIELD_GET(AMS_REG_SEQ2_MASK, scan_mask),
+		       ams->pl_base + AMS_REG_SEQ_CH2);
+		writel(FIELD_GET(AMS_REG_SEQ1_MASK, scan_mask),
+		       ams->pl_base + AMS_REG_SEQ_CH1);

Hm, the ordering is a bit strange here: 0, 2, 1.

Will reorder 0,1,2.

quoted

+
+		/* set continuous sequence mode */
+		ams_pl_update_reg(ams, AMS_REG_CONFIG1,

AMS_CONF1_SEQ_MASK,

quoted

+				  AMS_CONF1_SEQ_CONTINUOUS);
+	}
+}
+

+
[...]
+
+static int ams_read_raw(struct iio_dev *indio_dev,
+			struct iio_chan_spec const *chan,
+			int *val, int *val2, long mask)
+{
+	struct ams *ams = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&ams->lock);
+		if (chan->scan_index >= (AMS_PS_SEQ_MAX * 3)) {

This AMS_PS_SEQ_MAX * 3 really deserves a define. AMS_CTRL_SEQ_BASE
or something like that.

Will do.

quoted

+			ret = ams_read_vcc_reg(ams, chan->address, val);
+			if (ret) {
+				mutex_unlock(&ams->lock);
+				return -EINVAL;
+			}
+			ams_enable_channel_sequence(indio_dev);
+		} else if (chan->scan_index >= AMS_PS_SEQ_MAX)
+			*val = readl(ams->pl_base + chan->address);
+		else
+			*val = readl(ams->ps_base + chan->address);
+		mutex_unlock(&ams->lock);
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			switch (chan->address) {
+			case AMS_SUPPLY1:
+			case AMS_SUPPLY2:
+			case AMS_SUPPLY3:
+			case AMS_SUPPLY4:
+				*val = AMS_SUPPLY_SCALE_3VOLT;
+				break;
+			case AMS_SUPPLY5:
+			case AMS_SUPPLY6:
+				if (chan->scan_index < AMS_PS_SEQ_MAX)
+					*val = AMS_SUPPLY_SCALE_6VOLT;
+				else
+					*val = AMS_SUPPLY_SCALE_3VOLT;
+				break;
+			case AMS_SUPPLY7:
+			case AMS_SUPPLY8:
+				*val = AMS_SUPPLY_SCALE_6VOLT;
+				break;
+			case AMS_SUPPLY9:
+			case AMS_SUPPLY10:
+				if (chan->scan_index < AMS_PS_SEQ_MAX)
+					*val = AMS_SUPPLY_SCALE_3VOLT;
+				else
+					*val = AMS_SUPPLY_SCALE_6VOLT;
+				break;

For the PL the range of supply 7 to 10 depends on the CONFIG_REG4 and can
be either 3V or 6V. Might be worth checking that register, rather than
hardcoding the value.

Yes it seems better to do.

quoted

+			case AMS_VCC_PSPLL0:
+			case AMS_VCC_PSPLL3:
+			case AMS_VCCINT:
+			case AMS_VCCBRAM:
+			case AMS_VCCAUX:
+			case AMS_PSDDRPLL:
+			case AMS_PSINTFPDDR:

This is missing AMS_VCCAMS, AMS_VREFP, AMS_VREFN. They all use the 3V
scale.

The only ones on a 1V scale are the external channels.

Maye it is easier to write this as

     case AMS_VP_VN:
     case AMS_REG_VAUX ... AMS_REG_VAUX(15):
         *val = AMS_SUPPLY_SCALE_1VOLT;
         break;
     default:
         *val = AMS_SUPPLY_SCALE_3VOLT;
         break;

Will fix this as well.

quoted

+				*val = AMS_SUPPLY_SCALE_3VOLT;
+				break;
+			default:
+				*val = AMS_SUPPLY_SCALE_1VOLT;
+				break;
+			}
+			*val2 = AMS_SUPPLY_SCALE_DIV_BIT;
+			return IIO_VAL_FRACTIONAL_LOG2;
+		case IIO_TEMP:
+			*val = AMS_TEMP_SCALE;
+			*val2 = AMS_TEMP_SCALE_DIV_BIT;
+			return IIO_VAL_FRACTIONAL_LOG2;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_OFFSET:
+		/* Only the temperature channel has an offset */
+		*val = AMS_TEMP_OFFSET;
+		return IIO_VAL_INT;
+	}
+
+	return -EINVAL;
+}
+
[...]

+static int ams_init_module(struct iio_dev *indio_dev, struct device_node

*np,

quoted

+			   struct iio_chan_spec *channels) {
+	struct ams *ams = iio_priv(indio_dev);
+	int num_channels = 0;
+
+	if (of_device_is_compatible(np, "xlnx,zynqmp-ams-ps")) {
+		ams->ps_base = of_iomap(np, 0);

No unmap anywhere in the driver. Same for the other base addresses.

Will add in remove function.

quoted

[...]
+
+	return num_channels;
+}
[...]
+static int ams_probe(struct platform_device *pdev) {
[...]
+	ams->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(ams->clk))
+		return PTR_ERR(ams->clk);
+	clk_prepare_enable(ams->clk);
+	devm_add_action_or_reset(&pdev->dev,

ams_clk_disable_unprepare,

quoted

+				 ams->clk);

Needs to check for errors. Same for other users of
devm_add_action_or_reset().

quoted

+
+	INIT_DELAYED_WORK(&ams->ams_unmask_work,

ams_unmask_worker);

quoted

+	devm_add_action_or_reset(&pdev->dev,

ams_cancel_delayed_work,

quoted

+				 &ams->ams_unmask_work);
+
+	ret = ams_init_device(ams);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to initialize AMS\n");
+		return ret;
+	}
+
+	ret = ams_parse_dt(indio_dev, pdev);
+	if (ret) {
+		dev_err(&pdev->dev, "failure in parsing DT\n");
+		return ret;
+	}
+
+	ams_enable_channel_sequence(indio_dev);
+
+	ams->irq = platform_get_irq(pdev, 0);
+	if (ams->irq == -EPROBE_DEFER) {

What about other errors? E.g. if the interrupt is not specified int he
devicetree we'll pass the invalid irq number to request_irq().

Correct. Will return if ret < 0.

quoted

+		ret = -EPROBE_DEFER;
+		return ret;
+	}
+
+	ret = devm_request_irq(&pdev->dev, ams->irq, &ams_irq, 0, "ams-

irq",

quoted

+			       indio_dev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to register interrupt\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, indio_dev);
+
+	ret = devm_iio_device_register(&pdev->dev, indio_dev);
+
+	return ret;
+}

Thanks,
Anand