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
