Messages in this thread | | | From | Anand Ashok Dumbre <> | Subject | RE: [PATCH v7 2/4] iio: adc: Add Xilinx AMS driver | Date | Wed, 27 Oct 2021 09:35:07 +0000 |
| |
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
| |