RE: [PATCH v7 2/4] iio: adc: Add Xilinx AMS driver

[Anand Ashok Dumbre]

Hi Lars,

Thanks for the review.
I did a bit of digging and found the rest of the review.

> On 10/19/21 5:20 PM, Anand Ashok Dumbre wrote:
> > +static int ams_init_device(struct ams *ams) {
> > +	u32 reg;
> > +	int ret;
> > +
> > [...]
> > +	if (ams->pl_base) {
> > +		writel(AMS_PL_RESET_VALUE, ams->pl_base +
> AMS_VP_VN);
> > +
> > +		ret = readl_poll_timeout(ams->base + AMS_PL_CSTS, reg,
> > +					 (reg &
> AMS_PL_CSTS_ACCESS_MASK) ==
> > +					 AMS_PL_CSTS_ACCESS_MASK, 0,
> > +					 AMS_INIT_TIMEOUT_US);
> 
> The PL_CSTS register indicates whether the PL monitor can be accessed
> through the AMS.
> 
> But here we access the reset register even before the check. In addition
> there is really no point in polling the register as the state will not change. If
> the PL can not be accessed this should just return. And only after verifying
> that the PL can be accessed should the reset be done.

I missed this. I will fix this and change the order of execution.

> 
> > +		if (ret)
> > +			return ret;
> > +
> > +		/* put sysmon in a default state */
> > +		ams_pl_update_reg(ams, AMS_REG_CONFIG1,
> AMS_CONF1_SEQ_MASK,
> > +				  AMS_CONF1_SEQ_DEFAULT);
> > +	}
> > +
> > +	[...]
> > +
> > +	return 0;
> > +}
> > +
> >
> > +static int ams_probe(struct platform_device *pdev) {
> > [..]
> > +
> > +	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;
> > +	}
> > +
> These two need to be called the other way around. ams_init_device() wants
> to access the IO registers, but they are only mapped in ams_parse_dt().

That is correct. I will fix this as well.

Thanks,
Anand