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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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