Re: [PATCH v9 3/5] iio: adc: Add Xilinx AMS driver

Andy Shevchenko
2021-11-17T12:49:08Z

On Tue, Nov 16, 2021 at 08:29:21PM +0000, Anand Ashok Dumbre wrote:
> > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Sent: Tuesday 16 November 2021 5:39 PM
> > On Tue, Nov 16, 2021 at 03:08:40PM +0000, Anand Ashok Dumbre wrote:

...

> > > +#define AMS_ALARM_THR_MIN 0x0000
> > > +#define AMS_ALARM_THR_MAX 0xFFFF
> >
> > If this is limited by hardware register, I would rather use (BIT(16) - 1)
> > notation. It will give immediately amount of bits used for the value.

> So ~(BIT(16) - 1) for AMS_ALARM_THR_MIN

This will give wrong value, so preserving 0 as plain decimal is fine.

> (BIT(16) - 1) for AMS_ALARM_THR_MAX

...

> > > + u32 reg;
> > > + int ret;
> >
> > u32 expect = AMS_PS_CSTS_PS_READY;
> >
> > (Use similar approach for other readX_poll_timeout() cases)
> >
> > > + ret = readl_poll_timeout(ams->base + AMS_PS_CSTS, reg,
> > > + (reg & AMS_PS_CSTS_PS_READY) ==
> > > + AMS_PS_CSTS_PS_READY, 0,
> > > + AMS_INIT_TIMEOUT_US);
> >
> > ret = readl_poll_timeout(ams->base + AMS_PS_CSTS, reg,
> > (reg & expect) == expect,
> > 0, AMS_INIT_TIMEOUT_US);

> > 0?!

Any comments on this?

Besides there are other cases you haven't answered on, so I assume you agreed
to change as suggested.

> > > + if (ret)
> > > + return ret;
> >
> > ...
> >
> > > + ret = readl(ams->base + AMS_PL_CSTS);
> > > + if (ret == 0)
> > > + return ret;
> >
> > Assigning u32 to int seems wrong.
>
> It's a single bit register.
> Even if I use u32 here, the return type is int.

The problem here is that you checked not for error code, readl() doesn't return
an error. So semantic issue.

> So, is it ok if I read using u32 and return it by typecasting to int?

No. You need to have something like this:

value = readl(...);
if (value == 0)
return 0;

this will keep proper meaning of each number and variable, while compiler may
optimize it.

...

> > > + regval = readl(ams->pl_base +
> > > + AMS_REG_CONFIG4);
> >
> > One line?
> >
> > > + regval = readl(ams->pl_base +
> > > + AMS_REG_CONFIG4);
> >
> > Ditto and so on...
> >
> It goes over 80 chars per line.

Is it a problem?

...

> > > + schedule_delayed_work(&ams->ams_unmask_work,
> > > + msecs_to_jiffies(AMS_UNMASK_TIMEOUT_MS));
> >
> > Can be one line.
>
> Over 80 characters.

Is it a problem?

> Oh! I just saw that upto 100 chars is ok.

--
With Best Regards,
Andy Shevchenko