Hi Lars,
Thanks for the review.
> 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?
Will remove.
> > + 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.
> > + 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.
> > [...]
> > + 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.
Will reorder 0,1,2.
> > +
> > + /* 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.
Will do.
> > + 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.
> > + 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.
> > + *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.
Will add in remove function.
> > [...]
> > +
> > + 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().
Correct. Will return if ret < 0.
> > + 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;
> > +}
Thanks,
Anand
