Re: [PATCH v3 3/3] hwmon: Add Baikal-T1 PVT sensor driver

[kbuild test robot <lkp@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 10848 bytes --]

Hi Serge,

I love your patch! Yet something to improve:

[auto build test ERROR on v5.7-rc7]
[cannot apply to hwmon/hwmon-next next-20200526]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Serge-Semin/hwmon-Add-Baikal-T1-SoC-Process-Voltage-and-Temp-sensor-support/20200526-214218
base:    9cb1fd0efd195590b828b9b865421ad345a4a145
config: s390-allyesconfig (attached as .config)
compiler: s390-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=s390 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

drivers/hwmon/bt1-pvt.c:65:1: warning: 'static' is not at beginning of declaration [-Wold-style-declaration]
65 | const static struct pvt_poly poly_temp_to_N = {
| ^~~~~
drivers/hwmon/bt1-pvt.c:76:1: warning: 'static' is not at beginning of declaration [-Wold-style-declaration]
76 | const static struct pvt_poly poly_N_to_temp = {
| ^~~~~
drivers/hwmon/bt1-pvt.c:97:1: warning: 'static' is not at beginning of declaration [-Wold-style-declaration]
97 | const static struct pvt_poly poly_volt_to_N = {
| ^~~~~
drivers/hwmon/bt1-pvt.c:105:1: warning: 'static' is not at beginning of declaration [-Wold-style-declaration]
105 | const static struct pvt_poly poly_N_to_volt = {
| ^~~~~
drivers/hwmon/bt1-pvt.c: In function 'pvt_update':
>> drivers/hwmon/bt1-pvt.c:142:8: error: implicit declaration of function 'readl_relaxed' [-Werror=implicit-function-declaration]
142 |  old = readl_relaxed(reg);
|        ^~~~~~~~~~~~~
>> drivers/hwmon/bt1-pvt.c:143:2: error: implicit declaration of function 'writel' [-Werror=implicit-function-declaration]
143 |  writel((old & ~mask) | (data & mask), reg);
|  ^~~~~~
drivers/hwmon/bt1-pvt.c: In function 'pvt_soft_isr':
>> drivers/hwmon/bt1-pvt.c:237:14: error: implicit declaration of function 'readl' [-Werror=implicit-function-declaration]
237 |  thres_sts = readl(pvt->regs + PVT_RAW_INTR_STAT);
|              ^~~~~
drivers/hwmon/bt1-pvt.c: At top level:
drivers/hwmon/bt1-pvt.c:795:5: warning: no previous prototype for 'pvt_hwmon_write' [-Wmissing-prototypes]
795 | int pvt_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
|     ^~~~~~~~~~~~~~~
drivers/hwmon/bt1-pvt.c: In function 'pvt_init_iface':
drivers/hwmon/bt1-pvt.c:1013:7: error: implicit declaration of function 'of_property_read_u32' [-Werror=implicit-function-declaration]
1013 |  if (!of_property_read_u32(pvt->dev->of_node,
|       ^~~~~~~~~~~~~~~~~~~~
drivers/hwmon/bt1-pvt.c: At top level:
drivers/hwmon/bt1-pvt.c:1138:34: error: array type has incomplete element type 'struct of_device_id'
1138 | static const struct of_device_id pvt_of_match[] = {
|                                  ^~~~~~~~~~~~
drivers/hwmon/bt1-pvt.c:1139:4: error: field name not in record or union initializer
1139 |  { .compatible = "baikal,bt1-pvt" },
|    ^
drivers/hwmon/bt1-pvt.c:1139:4: note: (near initialization for 'pvt_of_match')
drivers/hwmon/bt1-pvt.c:1138:34: warning: 'pvt_of_match' defined but not used [-Wunused-variable]
1138 | static const struct of_device_id pvt_of_match[] = {
|                                  ^~~~~~~~~~~~
cc1: some warnings being treated as errors

vim +/readl_relaxed +142 drivers/hwmon/bt1-pvt.c

   137	
   138	static inline u32 pvt_update(void __iomem *reg, u32 mask, u32 data)
   139	{
   140		u32 old;
   141	
 > 142		old = readl_relaxed(reg);
 > 143		writel((old & ~mask) | (data & mask), reg);
   144	
   145		return old & mask;
   146	}
   147	
   148	/*
   149	 * Baikal-T1 PVT mode can be updated only when the controller is disabled.
   150	 * So first we disable it, then set the new mode together with the controller
   151	 * getting back enabled. The same concerns the temperature trim and
   152	 * measurements timeout. If it is necessary the interface mutex is supposed
   153	 * to be locked at the time the operations are performed.
   154	 */
   155	static inline void pvt_set_mode(struct pvt_hwmon *pvt, u32 mode)
   156	{
   157		u32 old;
   158	
   159		mode = FIELD_PREP(PVT_CTRL_MODE_MASK, mode);
   160	
   161		old = pvt_update(pvt->regs + PVT_CTRL, PVT_CTRL_EN, 0);
   162		pvt_update(pvt->regs + PVT_CTRL, PVT_CTRL_MODE_MASK | PVT_CTRL_EN,
   163			   mode | old);
   164	}
   165	
   166	static inline u32 pvt_calc_trim(unsigned int temp)
   167	{
   168		temp = clamp_val(temp, 0, PVT_TRIM_TEMP);
   169	
   170		return DIV_ROUND_UP(temp, PVT_TRIM_STEP);
   171	}
   172	
   173	static inline void pvt_set_trim(struct pvt_hwmon *pvt, u32 trim)
   174	{
   175		u32 old;
   176	
   177		trim = FIELD_PREP(PVT_CTRL_TRIM_MASK, trim);
   178	
   179		old = pvt_update(pvt->regs + PVT_CTRL, PVT_CTRL_EN, 0);
   180		pvt_update(pvt->regs + PVT_CTRL, PVT_CTRL_TRIM_MASK | PVT_CTRL_EN,
   181			   trim | old);
   182	}
   183	
   184	static inline void pvt_set_tout(struct pvt_hwmon *pvt, u32 tout)
   185	{
   186		u32 old;
   187	
   188		old = pvt_update(pvt->regs + PVT_CTRL, PVT_CTRL_EN, 0);
   189		writel(tout, pvt->regs + PVT_TTIMEOUT);
   190		pvt_update(pvt->regs + PVT_CTRL, PVT_CTRL_EN, old);
   191	}
   192	
   193	/*
   194	 * This driver can optionally provide the hwmon alarms for each sensor the PVT
   195	 * controller supports. The alarms functionality is made compile-time
   196	 * configurable due to the hardware interface implementation peculiarity
   197	 * described further in this comment. So in case if alarms are unnecessary in
   198	 * your system design it's recommended to have them disabled to prevent the PVT
   199	 * IRQs being periodically raised to get the data cache/alarms status up to
   200	 * date.
   201	 *
   202	 * Baikal-T1 PVT embedded controller is based on the Analog Bits PVT sensor,
   203	 * but is equipped with a dedicated control wrapper. It exposes the PVT
   204	 * sub-block registers space via the APB3 bus. In addition the wrapper provides
   205	 * a common interrupt vector of the sensors conversion completion events and
   206	 * threshold value alarms. Alas the wrapper interface hasn't been fully thought
   207	 * through. There is only one sensor can be activated at a time, for which the
   208	 * thresholds comparator is enabled right after the data conversion is
   209	 * completed. Due to this if alarms need to be implemented for all available
   210	 * sensors we can't just set the thresholds and enable the interrupts. We need
   211	 * to enable the sensors one after another and let the controller to detect
   212	 * the alarms by itself at each conversion. This also makes pointless to handle
   213	 * the alarms interrupts, since in occasion they happen synchronously with
   214	 * data conversion completion. The best driver design would be to have the
   215	 * completion interrupts enabled only and keep the converted value in the
   216	 * driver data cache. This solution is implemented if hwmon alarms are enabled
   217	 * in this driver. In case if the alarms are disabled, the conversion is
   218	 * performed on demand at the time a sensors input file is read.
   219	 */
   220	
   221	#if defined(CONFIG_SENSORS_BT1_PVT_ALARMS)
   222	
   223	#define pvt_hard_isr NULL
   224	
   225	static irqreturn_t pvt_soft_isr(int irq, void *data)
   226	{
   227		const struct pvt_sensor_info *info;
   228		struct pvt_hwmon *pvt = data;
   229		struct pvt_cache *cache;
   230		u32 val, thres_sts, old;
   231	
   232		/*
   233		 * DVALID bit will be cleared by reading the data. We need to save the
   234		 * status before the next conversion happens. Threshold events will be
   235		 * handled a bit later.
   236		 */
 > 237		thres_sts = readl(pvt->regs + PVT_RAW_INTR_STAT);
   238	
   239		/*
   240		 * Then lets recharge the PVT interface with the next sampling mode.
   241		 * Lock the interface mutex to serialize trim, timeouts and alarm
   242		 * thresholds settings.
   243		 */
   244		cache = &pvt->cache[pvt->sensor];
   245		info = &pvt_info[pvt->sensor];
   246		pvt->sensor = (pvt->sensor == PVT_SENSOR_LAST) ?
   247			      PVT_SENSOR_FIRST : (pvt->sensor + 1);
   248	
   249		/*
   250		 * For some reason we have to mask the interrupt before changing the
   251		 * mode, otherwise sometimes the temperature mode doesn't get
   252		 * activated even though the actual mode in the ctrl register
   253		 * corresponds to one. Then we read the data. By doing so we also
   254		 * recharge the data conversion. After this the mode corresponding
   255		 * to the next sensor in the row is set. Finally we enable the
   256		 * interrupts back.
   257		 */
   258		mutex_lock(&pvt->iface_mtx);
   259	
   260		old = pvt_update(pvt->regs + PVT_INTR_MASK, PVT_INTR_DVALID,
   261				 PVT_INTR_DVALID);
   262	
   263		val = readl(pvt->regs + PVT_DATA);
   264	
   265		pvt_set_mode(pvt, pvt_info[pvt->sensor].mode);
   266	
   267		pvt_update(pvt->regs + PVT_INTR_MASK, PVT_INTR_DVALID, old);
   268	
   269		mutex_unlock(&pvt->iface_mtx);
   270	
   271		/*
   272		 * We can now update the data cache with data just retrieved from the
   273		 * sensor. Lock write-seqlock to make sure the reader has a coherent
   274		 * data.
   275		 */
   276		write_seqlock(&cache->data_seqlock);
   277	
   278		cache->data = FIELD_GET(PVT_DATA_DATA_MASK, val);
   279	
   280		write_sequnlock(&cache->data_seqlock);
   281	
   282		/*
   283		 * While PVT core is doing the next mode data conversion, we'll check
   284		 * whether the alarms were triggered for the current sensor. Note that
   285		 * according to the documentation only one threshold IRQ status can be
   286		 * set at a time, that's why if-else statement is utilized.
   287		 */
   288		if ((thres_sts & info->thres_sts_lo) ^ cache->thres_sts_lo) {
   289			WRITE_ONCE(cache->thres_sts_lo, thres_sts & info->thres_sts_lo);
   290			hwmon_notify_event(pvt->hwmon, info->type, info->attr_min_alarm,
   291					   info->channel);
   292		} else if ((thres_sts & info->thres_sts_hi) ^ cache->thres_sts_hi) {
   293			WRITE_ONCE(cache->thres_sts_hi, thres_sts & info->thres_sts_hi);
   294			hwmon_notify_event(pvt->hwmon, info->type, info->attr_max_alarm,
   295					   info->channel);
   296		}
   297	
   298		return IRQ_HANDLED;
   299	}
   300	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 59242 bytes --]