On Tue, Nov 16, 2021 at 08:29:21PM +0000, Anand Ashok Dumbre wrote: > > From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> > > 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
