On 10/19/21 5:20 PM, Anand Ashok Dumbre wrote:
[....]
+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?
+ 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.
+ 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.
[...]
+ 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,
+ 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.
+
+ /* set continuous sequence mode */
+ ams_pl_update_reg(ams, AMS_REG_CONFIG1, AMS_CONF1_SEQ_MASK,
+ 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.
+ 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.
+ 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;
+ *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,
+ 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.
[...]
+
+ 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,
+ ams->clk);
Needs to check for errors. Same for other users of
devm_add_action_or_reset().
+
+ INIT_DELAYED_WORK(&ams->ams_unmask_work, ams_unmask_worker);
+ devm_add_action_or_reset(&pdev->dev, ams_cancel_delayed_work,
+ &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().
+ ret = -EPROBE_DEFER;
+ return ret;
+ }
+
+ ret = devm_request_irq(&pdev->dev, ams->irq, &ams_irq, 0, "ams-irq",
+ 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;
+}
