* Re: [PATCH] i2c: New driver for Nuvoton SMBus adapters
2022-03-13 22:40 [PATCH] i2c: New driver for Nuvoton SMBus adapters Adam Honse
@ 2022-03-14 1:17 ` kernel test robot
2022-03-14 11:03 ` Jean Delvare
2022-03-14 20:28 ` kernel test robot
2 siblings, 0 replies; 4+ messages in thread
From: kernel test robot @ 2022-03-14 1:17 UTC (permalink / raw)
To: Adam Honse, jdelvare, linux-i2c, jdelvare; +Cc: kbuild-all
Hi Adam,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on wsa/i2c/for-next]
[also build test WARNING on v5.17-rc8 next-20220310]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Adam-Honse/i2c-New-driver-for-Nuvoton-SMBus-adapters/20220314-064247
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: powerpc64-randconfig-r035-20220314 (https://download.01.org/0day-ci/archive/20220314/202203140908.OiQgTMB7-lkp@intel.com/config)
compiler: powerpc64-linux-gcc (GCC) 11.2.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
# https://github.com/0day-ci/linux/commit/040a1be0d30ce2c611e30eb923d6f1b0afc44e7d
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Adam-Honse/i2c-New-driver-for-Nuvoton-SMBus-adapters/20220314-064247
git checkout 040a1be0d30ce2c611e30eb923d6f1b0afc44e7d
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=powerpc SHELL=/bin/bash drivers/i2c/busses/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/i2c/busses/i2c-nct6775.c: In function 'nct6775_access':
>> drivers/i2c/busses/i2c-nct6775.c:218:31: warning: this statement may fall through [-Wimplicit-fallthrough=]
218 | tmp_data.byte = data->byte;
| ~~~~~~~~~~~~~~^~~~~~~~~~~~
drivers/i2c/busses/i2c-nct6775.c:219:9: note: here
219 | case I2C_SMBUS_BYTE:
| ^~~~
At top level:
drivers/i2c/busses/i2c-nct6775.c:90:27: warning: 'nct6775_device_names' defined but not used [-Wunused-const-variable=]
90 | static const char * const nct6775_device_names[] = {
| ^~~~~~~~~~~~~~~~~~~~
vim +218 drivers/i2c/busses/i2c-nct6775.c
194
195 /* Return negative errno on error. */
196 static s32 nct6775_access(struct i2c_adapter *adap, u16 addr,
197 unsigned short flags, char read_write,
198 u8 command, int size, union i2c_smbus_data *data)
199 {
200 struct i2c_nct6775_adapdata *adapdata = i2c_get_adapdata(adap);
201 unsigned short nuvoton_nct6793d_smba = adapdata->smba;
202 int i, len, cnt;
203 union i2c_smbus_data tmp_data;
204 int timeout = 0;
205
206 tmp_data.word = 0;
207 cnt = 0;
208 len = 0;
209
210 outb_p(NCT6793D_SOFT_RESET, SMBHSTCTL);
211
212 switch (size) {
213 case I2C_SMBUS_QUICK:
214 outb_p((addr << 1) | read_write,
215 SMBHSTADD);
216 break;
217 case I2C_SMBUS_BYTE_DATA:
> 218 tmp_data.byte = data->byte;
219 case I2C_SMBUS_BYTE:
220 outb_p((addr << 1) | read_write,
221 SMBHSTADD);
222 outb_p(command, SMBHSTIDX);
223 if (read_write == I2C_SMBUS_WRITE) {
224 outb_p(tmp_data.byte, SMBHSTDAT);
225 outb_p(NCT6793D_WRITE_BYTE, SMBHSTCMD);
226 } else {
227 outb_p(NCT6793D_READ_BYTE, SMBHSTCMD);
228 }
229 break;
230 case I2C_SMBUS_WORD_DATA:
231 outb_p((addr << 1) | read_write,
232 SMBHSTADD);
233 outb_p(command, SMBHSTIDX);
234 if (read_write == I2C_SMBUS_WRITE) {
235 outb_p(data->word & 0xff, SMBHSTDAT);
236 outb_p((data->word & 0xff00) >> 8, SMBHSTDAT);
237 outb_p(NCT6793D_WRITE_WORD, SMBHSTCMD);
238 } else {
239 outb_p(NCT6793D_READ_WORD, SMBHSTCMD);
240 }
241 break;
242 case I2C_SMBUS_BLOCK_DATA:
243 outb_p((addr << 1) | read_write,
244 SMBHSTADD);
245 outb_p(command, SMBHSTIDX);
246 if (read_write == I2C_SMBUS_WRITE) {
247 len = data->block[0];
248 if (len == 0 || len > I2C_SMBUS_BLOCK_MAX)
249 return -EINVAL;
250 outb_p(len, SMBBLKSZ);
251
252 cnt = 1;
253 if (len >= 4) {
254 for (i = cnt; i <= 4; i++)
255 outb_p(data->block[i], SMBHSTDAT);
256
257 len -= 4;
258 cnt += 4;
259 } else {
260 for (i = cnt; i <= len; i++)
261 outb_p(data->block[i], SMBHSTDAT);
262
263 len = 0;
264 }
265
266 outb_p(NCT6793D_WRITE_BLOCK, SMBHSTCMD);
267 } else {
268 return -EOPNOTSUPP;
269 }
270 break;
271 default:
272 dev_warn(&adap->dev, "Unsupported transaction %d\n", size);
273 return -EOPNOTSUPP;
274 }
275
276 outb_p(NCT6793D_MANUAL_START, SMBHSTCTL);
277
278 while ((size == I2C_SMBUS_BLOCK_DATA) && (len > 0)) {
279 if (read_write == I2C_SMBUS_WRITE) {
280 timeout = 0;
281 while ((inb_p(SMBHSTSTS) & NCT6793D_FIFO_EMPTY) == 0) {
282 if (timeout > MAX_RETRIES)
283 return -ETIMEDOUT;
284
285 usleep_range(250, 500);
286 timeout++;
287 }
288
289 //Load more bytes into FIFO
290 if (len >= 4) {
291 for (i = cnt; i <= (cnt + 4); i++)
292 outb_p(data->block[i], SMBHSTDAT);
293
294 len -= 4;
295 cnt += 4;
296 } else {
297 for (i = cnt; i <= (cnt + len); i++)
298 outb_p(data->block[i], SMBHSTDAT);
299
300 len = 0;
301 }
302 } else {
303 return -EOPNOTSUPP;
304 }
305
306 }
307
308 //wait for manual mode to complete
309 timeout = 0;
310 while ((inb_p(SMBHSTSTS) & NCT6793D_MANUAL_ACTIVE) != 0) {
311 if (timeout > MAX_RETRIES)
312 return -ETIMEDOUT;
313
314 usleep_range(250, 500);
315 timeout++;
316 }
317
318 if ((inb_p(SMBHSTERR) & NCT6793D_NO_ACK) != 0)
319 return -ENXIO;
320
321 else if ((read_write == I2C_SMBUS_WRITE) || (size == I2C_SMBUS_QUICK))
322 return 0;
323
324 switch (size) {
325 case I2C_SMBUS_QUICK:
326 case I2C_SMBUS_BYTE_DATA:
327 data->byte = inb_p(SMBHSTDAT);
328 break;
329 case I2C_SMBUS_WORD_DATA:
330 data->word = inb_p(SMBHSTDAT) + (inb_p(SMBHSTDAT) << 8);
331 break;
332 }
333 return 0;
334 }
335
---
0-DAY CI Kernel Test Service
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] i2c: New driver for Nuvoton SMBus adapters
2022-03-13 22:40 [PATCH] i2c: New driver for Nuvoton SMBus adapters Adam Honse
2022-03-14 1:17 ` kernel test robot
@ 2022-03-14 11:03 ` Jean Delvare
2022-03-14 20:28 ` kernel test robot
2 siblings, 0 replies; 4+ messages in thread
From: Jean Delvare @ 2022-03-14 11:03 UTC (permalink / raw)
To: Adam Honse; +Cc: linux-i2c
Hi Adam,
On Sun, 13 Mar 2022 17:40:20 -0500, Adam Honse wrote:
> This patch introduces a new driver for the SMBus adapter that is built in
> to most Nuvoton Super-IO chips. This SMBus adapter is used for RGB
> lighting control on some ASUS motherboards with Intel chipsets.
>
> The interface's register description is available in the supported
> devices' datasheets. Operation of this interface has been verified
> with OpenRGB on an ASUS PRIME Z270-A motherboard.
>
> Signed-off-by: Adam Honse <calcprogrammer1@gmail.com>
See my review inline below.
> ---
> drivers/i2c/busses/Kconfig | 9 +
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-nct6775.c | 608 +++++++++++++++++++++++++++++++
> 3 files changed, 618 insertions(+)
> create mode 100644 drivers/i2c/busses/i2c-nct6775.c
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 8a6c6ee28556..cf4c5e92738b 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -219,6 +219,15 @@ config I2C_CHT_WC
> combined with a FUSB302 Type-C port-controller as such it is advised
> to also select CONFIG_TYPEC_FUSB302=m.
>
> +config I2C_NCT6775
> + tristate "Nuvoton NCT6775 and compatible SMBus controller"
> + help
> + If you say yes to this option, support will be included for the
> + Nuvoton NCT6775 and compatible SMBus controllers.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-nct6775.
> +
> config I2C_NFORCE2
> tristate "Nvidia nForce2, nForce3 and nForce4"
> depends on PCI
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 1d00dce77098..450cde39c7d5 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_I2C_CHT_WC) += i2c-cht-wc.o
> obj-$(CONFIG_I2C_I801) += i2c-i801.o
> obj-$(CONFIG_I2C_ISCH) += i2c-isch.o
> obj-$(CONFIG_I2C_ISMT) += i2c-ismt.o
> +obj-$(CONFIG_I2C_NCT6775) += i2c-nct6775.o
> obj-$(CONFIG_I2C_NFORCE2) += i2c-nforce2.o
> obj-$(CONFIG_I2C_NFORCE2_S4985) += i2c-nforce2-s4985.o
> obj-$(CONFIG_I2C_NVIDIA_GPU) += i2c-nvidia-gpu.o
> diff --git a/drivers/i2c/busses/i2c-nct6775.c b/drivers/i2c/busses/i2c-nct6775.c
> new file mode 100644
> index 000000000000..fdffe30f595b
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-nct6775.c
> @@ -0,0 +1,608 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * i2c-nct6775 - Driver for the SMBus master functionality of
> + * Nuvoton NCT677x Super-I/O chips
> + *
> + * Copyright (C) 2019 Adam Honse <calcprogrammer1@gmail.com>
> + *
> + * Derived from nct6775 hwmon driver
> + * Copyright (C) 2012 Guenter Roeck <linux@roeck-us.net>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/jiffies.h>
Not needed?
> +#include <linux/platform_device.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/hwmon-vid.h>
You don't need these 3 hwmon header files.
> +#include <linux/err.h>
> +#include <linux/mutex.h>
Also not needed.
> +#include <linux/delay.h>
> +#include <linux/ioport.h>
> +#include <linux/i2c.h>
> +#include <linux/acpi.h>
> +#include <linux/bitops.h>
Not needed.
> +#include <linux/dmi.h>
Not needed.
> +#include <linux/io.h>
> +#include <linux/nospec.h>
Why?
> +
> +#define DRVNAME "i2c-nct6775"
> +
> +/* Nuvoton SMBus address offsets */
> +#define SMBHSTDAT (0 + nuvoton_nct6793d_smba)
> +#define SMBBLKSZ (1 + nuvoton_nct6793d_smba)
> +#define SMBHSTCMD (2 + nuvoton_nct6793d_smba)
> +#define SMBHSTIDX (3 + nuvoton_nct6793d_smba)
> +#define SMBHSTCTL (4 + nuvoton_nct6793d_smba)
> +#define SMBHSTADD (5 + nuvoton_nct6793d_smba)
> +#define SMBHSTERR (9 + nuvoton_nct6793d_smba)
> +#define SMBHSTSTS (0xE + nuvoton_nct6793d_smba)
For consistency, you should be using hexadecimal for all the defines. I
also believe that (nuvoton_nct6793d_smba + 0) would make more sense
than (0 + nuvoton_nct6793d_smba), even though the result is the same.
> +
> +/* Command register */
> +#define NCT6793D_READ_BYTE 0
It is pretty confusing to prefix these constants with NCT6793D when the
driver is named i2c-nct6775.
> +#define NCT6793D_READ_WORD 1
> +#define NCT6793D_READ_BLOCK 2
> +#define NCT6793D_BLOCK_WRITE_READ_PROC_CALL 3
NCT6793D_BLOCK_PROC_CALL would be enough.
> +#define NCT6793D_PROC_CALL 4
> +#define NCT6793D_WRITE_BYTE 8
> +#define NCT6793D_WRITE_WORD 9
> +#define NCT6793D_WRITE_BLOCK 10
> +
> +/* Control register */
> +#define NCT6793D_MANUAL_START 128
> +#define NCT6793D_SOFT_RESET 64
> +
> +/* Error register */
> +#define NCT6793D_NO_ACK 32
> +
> +/* Status register */
> +#define NCT6793D_FIFO_EMPTY 1
> +#define NCT6793D_FIFO_FULL 2
> +#define NCT6793D_MANUAL_ACTIVE 4
The 6 values above are clearly bit masks, so you should use BIT()
instead of decimal value for clarity.
> +
> +#define NCT6775_LD_SMBUS 0x0B
> +
> +/* Other settings */
> +#define MAX_RETRIES 400
> +
> +enum kinds { nct6106, nct6775, nct6776, nct6779, nct6791, nct6792, nct6793,
> + nct6795, nct6796, nct6798 };
> +
> +struct nct6775_sio_data {
> + int sioreg;
> + enum kinds kind;
> +};
> +
> +/* used to set data->name = nct6775_device_names[data->sio_kind] */
> +static const char * const nct6775_device_names[] = {
> + "nct6106",
> + "nct6775",
> + "nct6776",
> + "nct6779",
> + "nct6791",
> + "nct6792",
> + "nct6793",
> + "nct6795",
> + "nct6796",
> + "nct6798",
> +};
> +
> +static const char * const nct6775_sio_names[] __initconst = {
> + "NCT6106D",
> + "NCT6775F",
> + "NCT6776D/F",
> + "NCT6779D",
> + "NCT6791D",
> + "NCT6792D",
> + "NCT6793D",
> + "NCT6795D",
> + "NCT6796D",
> + "NCT6798D",
> +};
> +
> +#define SIO_REG_LDSEL 0x07 /* Logical device select */
> +#define SIO_REG_DEVID 0x20 /* Device ID (2 bytes) */
> +#define SIO_REG_SMBA 0x62 /* SMBus base address register */
> +
> +#define SIO_NCT6106_ID 0xc450
> +#define SIO_NCT6775_ID 0xb470
> +#define SIO_NCT6776_ID 0xc330
> +#define SIO_NCT6779_ID 0xc560
> +#define SIO_NCT6791_ID 0xc800
> +#define SIO_NCT6792_ID 0xc910
> +#define SIO_NCT6793_ID 0xd120
> +#define SIO_NCT6795_ID 0xd350
> +#define SIO_NCT6796_ID 0xd420
> +#define SIO_NCT6798_ID 0xd428
> +#define SIO_ID_MASK 0xFFF0
> +
> +static inline void
> +superio_outb(int ioreg, int reg, int val)
> +{
> + outb(reg, ioreg);
> + outb(val, ioreg + 1);
> +}
> +
> +static inline int
> +superio_inb(int ioreg, int reg)
> +{
> + outb(reg, ioreg);
> + return inb(ioreg + 1);
> +}
> +
> +static inline void
> +superio_select(int ioreg, int ld)
> +{
> + outb(SIO_REG_LDSEL, ioreg);
> + outb(ld, ioreg + 1);
> +}
> +
> +static inline int
> +superio_enter(int ioreg)
> +{
> + /*
> + * Try to reserve <ioreg> and <ioreg + 1> for exclusive access.
> + */
> + if (!request_muxed_region(ioreg, 2, DRVNAME))
> + return -EBUSY;
> +
> + outb(0x87, ioreg);
> + outb(0x87, ioreg);
> +
> + return 0;
> +}
> +
> +static inline void
> +superio_exit(int ioreg)
> +{
> + outb(0xaa, ioreg);
> + outb(0x02, ioreg);
> + outb(0x02, ioreg + 1);
> + release_region(ioreg, 2);
> +}
> +
> +/*
> + * ISA constants
> + */
> +
> +#define IOREGION_ALIGNMENT (~7)
> +#define IOREGION_LENGTH 2
> +#define ADDR_REG_OFFSET 0
> +#define DATA_REG_OFFSET 1
The offset defines are never used.
> +
> +#define NCT6775_REG_BANK 0x4E
> +#define NCT6775_REG_CONFIG 0x40
You don't use these anywhere either.
> +
> +static struct i2c_adapter *nct6775_adapter;
You can't possibly have a driver which claims to support 2 platform
devices but have a single static pointer to store the SMBus adapter's
data. The adapter's data should be stored as per-platform device data
instead.
> +
> +struct i2c_nct6775_adapdata {
> + unsigned short smba;
> +};
> +
> +/* Return negative errno on error. */
> +static s32 nct6775_access(struct i2c_adapter *adap, u16 addr,
> + unsigned short flags, char read_write,
> + u8 command, int size, union i2c_smbus_data *data)
> +{
> + struct i2c_nct6775_adapdata *adapdata = i2c_get_adapdata(adap);
> + unsigned short nuvoton_nct6793d_smba = adapdata->smba;
> + int i, len, cnt;
> + union i2c_smbus_data tmp_data;
"tmp" is never a good variable name. All local variables are temporary
by nature. I don't understand why you need this variable anyway, you
already have *data.
> + int timeout = 0;
> +
> + tmp_data.word = 0;
> + cnt = 0;
> + len = 0;
Do you actually need to initialize these variables? Initializing to 0
"just in case" is bad practice as it prevents the compiler from warning
you if you end up using an uninitialized variable.
> +
> + outb_p(NCT6793D_SOFT_RESET, SMBHSTCTL);
> +
> + switch (size) {
> + case I2C_SMBUS_QUICK:
Are you sure this is actually supported? There's no define for this
command.
> + outb_p((addr << 1) | read_write,
> + SMBHSTADD);
Fits on one line. Same comment several times below.
> + break;
> + case I2C_SMBUS_BYTE_DATA:
> + tmp_data.byte = data->byte;
Please use the fallthrough pseudo keyword to make it clear that the
lack of break is intentional.
> + case I2C_SMBUS_BYTE:
> + outb_p((addr << 1) | read_write,
> + SMBHSTADD);
> + outb_p(command, SMBHSTIDX);
> + if (read_write == I2C_SMBUS_WRITE) {
> + outb_p(tmp_data.byte, SMBHSTDAT);
> + outb_p(NCT6793D_WRITE_BYTE, SMBHSTCMD);
> + } else {
> + outb_p(NCT6793D_READ_BYTE, SMBHSTCMD);
> + }
> + break;
This look highly suspicious. You can't possibly support two different
SMBus transaction types (Receive Byte and Read Byte) with the same
command value (NCT6793D_READ_BYTE). Same for Send Byte vs Write Byte.
> + case I2C_SMBUS_WORD_DATA:
> + outb_p((addr << 1) | read_write,
> + SMBHSTADD);
> + outb_p(command, SMBHSTIDX);
> + if (read_write == I2C_SMBUS_WRITE) {
> + outb_p(data->word & 0xff, SMBHSTDAT);
> + outb_p((data->word & 0xff00) >> 8, SMBHSTDAT);
> + outb_p(NCT6793D_WRITE_WORD, SMBHSTCMD);
> + } else {
> + outb_p(NCT6793D_READ_WORD, SMBHSTCMD);
> + }
> + break;
> + case I2C_SMBUS_BLOCK_DATA:
> + outb_p((addr << 1) | read_write,
> + SMBHSTADD);
> + outb_p(command, SMBHSTIDX);
> + if (read_write == I2C_SMBUS_WRITE) {
> + len = data->block[0];
> + if (len == 0 || len > I2C_SMBUS_BLOCK_MAX)
> + return -EINVAL;
> + outb_p(len, SMBBLKSZ);
> +
> + cnt = 1;
> + if (len >= 4) {
> + for (i = cnt; i <= 4; i++)
> + outb_p(data->block[i], SMBHSTDAT);
> +
> + len -= 4;
> + cnt += 4;
> + } else {
> + for (i = cnt; i <= len; i++)
> + outb_p(data->block[i], SMBHSTDAT);
> +
> + len = 0;
> + }
> +
> + outb_p(NCT6793D_WRITE_BLOCK, SMBHSTCMD);
> + } else {
> + return -EOPNOTSUPP;
> + }
> + break;
> + default:
> + dev_warn(&adap->dev, "Unsupported transaction %d\n", size);
> + return -EOPNOTSUPP;
> + }
> +
> + outb_p(NCT6793D_MANUAL_START, SMBHSTCTL);
> +
> + while ((size == I2C_SMBUS_BLOCK_DATA) && (len > 0)) {
More parentheses than needed.
> + if (read_write == I2C_SMBUS_WRITE) {
> + timeout = 0;
> + while ((inb_p(SMBHSTSTS) & NCT6793D_FIFO_EMPTY) == 0) {
> + if (timeout > MAX_RETRIES)
> + return -ETIMEDOUT;
> +
> + usleep_range(250, 500);
> + timeout++;
> + }
> +
> + //Load more bytes into FIFO
Please use /* */ for comments.
> + if (len >= 4) {
> + for (i = cnt; i <= (cnt + 4); i++)
> + outb_p(data->block[i], SMBHSTDAT);
> +
> + len -= 4;
> + cnt += 4;
> + } else {
> + for (i = cnt; i <= (cnt + len); i++)
> + outb_p(data->block[i], SMBHSTDAT);
> +
> + len = 0;
> + }
> + } else {
This can't happen.
> + return -EOPNOTSUPP;
> + }
> +
> + }
> +
> + //wait for manual mode to complete
> + timeout = 0;
> + while ((inb_p(SMBHSTSTS) & NCT6793D_MANUAL_ACTIVE) != 0) {
> + if (timeout > MAX_RETRIES)
> + return -ETIMEDOUT;
> +
> + usleep_range(250, 500);
> + timeout++;
> + }
> +
> + if ((inb_p(SMBHSTERR) & NCT6793D_NO_ACK) != 0)
> + return -ENXIO;
> +
> + else if ((read_write == I2C_SMBUS_WRITE) || (size == I2C_SMBUS_QUICK))
No blank line before "else". Or just remove the "else" which isn't
needed. Too many parentheses.
> + return 0;
> +
> + switch (size) {
> + case I2C_SMBUS_QUICK:
Never true.
> + case I2C_SMBUS_BYTE_DATA:
> + data->byte = inb_p(SMBHSTDAT);
> + break;
> + case I2C_SMBUS_WORD_DATA:
> + data->word = inb_p(SMBHSTDAT) + (inb_p(SMBHSTDAT) << 8);
| is preferred here.
> + break;
> + }
> + return 0;
> +}
> +
> +static u32 nct6775_func(struct i2c_adapter *adapter)
> +{
> + return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
> + I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
> + I2C_FUNC_SMBUS_BLOCK_DATA;
> +}
> +
> +static const struct i2c_algorithm smbus_algorithm = {
> + .smbus_xfer = nct6775_access,
> + .functionality = nct6775_func,
> +};
> +
> +static int nct6775_add_adapter(unsigned short smba, const char *name, struct i2c_adapter **padap)
IMHO it would make more sense to have this function return the struct
i2c_adapter * (and use ERR_PTR to return errors).
> +{
> + struct i2c_adapter *adap;
> + struct i2c_nct6775_adapdata *adapdata;
> + int retval;
> +
> + adap = kzalloc(sizeof(*adap), GFP_KERNEL);
> + if (adap == NULL)
> + return -ENOMEM;
> +
> + adap->owner = THIS_MODULE;
> + adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
Doesn't seem appropriate if the only known usage of this SMBus
controller is RGB light control.
> + adap->algo = &smbus_algorithm;
> +
> + adapdata = kzalloc(sizeof(*adapdata), GFP_KERNEL);
> + if (adapdata == NULL) {
> + kfree(adap);
> + return -ENOMEM;
> + }
> +
> + adapdata->smba = smba;
> +
> + snprintf(adap->name, sizeof(adap->name),
> + "SMBus NCT67xx adapter%s at %04x", name, smba);
Name is already an empty string, so this could be simplified.
Alternatively, set name to the actual device name (available in
nct6775_sio_names) and use that instead of NCT67xx.
> +
> + i2c_set_adapdata(adap, adapdata);
> +
> + retval = i2c_add_adapter(adap);
> + if (retval) {
> + kfree(adapdata);
> + kfree(adap);
> + return retval;
> + }
> +
> + *padap = adap;
> + return 0;
> +}
> +
> +static void nct6775_remove_adapter(struct i2c_adapter *adap)
> +{
> + struct i2c_nct6775_adapdata *adapdata = i2c_get_adapdata(adap);
> +
> + if (adapdata->smba) {
Always true, as far as I can see.
> + i2c_del_adapter(adap);
> + kfree(adapdata);
> + kfree(adap);
> + }
> +}
> +
> +/*
> + * when Super-I/O functions move to a separate file, the Super-I/O
> + * bus will manage the lifetime of the device and this module will only keep
> + * track of the nct6775 driver. But since we use platform_device_alloc(), we
> + * must keep track of the device
Leading capital and trailing dot would be appreciated.
> + */
> +static struct platform_device *pdev[2];
> +
> +static int nct6775_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct nct6775_sio_data *sio_data = dev_get_platdata(dev);
> + struct resource *res;
> +
> + res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> + if (!devm_request_region(&pdev->dev, res->start, IOREGION_LENGTH,
> + DRVNAME))
> + return -EBUSY;
> +
> + switch (sio_data->kind) {
> + case nct6791:
> + case nct6792:
> + case nct6793:
> + case nct6795:
> + case nct6796:
> + case nct6798:
> + nct6775_add_adapter(res->start, "", &nct6775_adapter);
> + break;
I'm confused. The driver is named i2c-nct6775 but the NCT6775 isn't
supported? I understand the idea of matching the name of the hwmon
driver you used as a based, but it only makes sense if the same devices
are supported.
All references to unsupported devices should be removed from this
driver, there's no reason to make the driver larger.
> + default:
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
> +
> +static struct platform_driver i2c_nct6775_driver = {
> + .driver = {
> + .name = DRVNAME,
> + },
> + .probe = nct6775_probe,
> +};
> +
> +static void __exit i2c_nct6775_exit(void)
> +{
> + int i;
> +
> + if (nct6775_adapter)
> + nct6775_remove_adapter(nct6775_adapter);
That's a weird construct. Unloading the module should remove the
platform devices, and the remove callback of the platform device driver
should remove the adapter.
> +
> + for (i = 0; i < ARRAY_SIZE(pdev); i++) {
> + if (pdev[i])
> + platform_device_unregister(pdev[i]);
> + }
> + platform_driver_unregister(&i2c_nct6775_driver);
> +}
> +
> +/* nct6775_find() looks for a '627 in the Super-I/O config space */
This reference to the Winbond W83627HF chipset is way outdated.
> +static int __init nct6775_find(int sioaddr, struct nct6775_sio_data *sio_data)
> +{
> + u16 val;
> + int err;
> + int addr;
> +
> + err = superio_enter(sioaddr);
> + if (err)
> + return err;
> +
> + val = (superio_inb(sioaddr, SIO_REG_DEVID) << 8) |
> + superio_inb(sioaddr, SIO_REG_DEVID + 1);
> +
> + switch (val & SIO_ID_MASK) {
> + case SIO_NCT6106_ID:
> + sio_data->kind = nct6106;
> + break;
> + case SIO_NCT6775_ID:
> + sio_data->kind = nct6775;
> + break;
> + case SIO_NCT6776_ID:
> + sio_data->kind = nct6776;
> + break;
> + case SIO_NCT6779_ID:
> + sio_data->kind = nct6779;
> + break;
> + case SIO_NCT6791_ID:
> + sio_data->kind = nct6791;
> + break;
> + case SIO_NCT6792_ID:
> + sio_data->kind = nct6792;
> + break;
> + case SIO_NCT6793_ID:
> + sio_data->kind = nct6793;
> + break;
> + case SIO_NCT6795_ID:
> + sio_data->kind = nct6795;
> + break;
> + case SIO_NCT6796_ID:
> + sio_data->kind = nct6796;
> + break;
> + case SIO_NCT6798_ID:
> + sio_data->kind = nct6798;
> + break;
> + default:
> + if (val != 0xffff)
> + pr_debug("unsupported chip ID: 0x%04x\n", val);
> + superio_exit(sioaddr);
> + return -ENODEV;
> + }
> +
> + /* We have a known chip, find the SMBus I/O address */
> + superio_select(sioaddr, NCT6775_LD_SMBUS);
> + val = (superio_inb(sioaddr, SIO_REG_SMBA) << 8)
> + | superio_inb(sioaddr, SIO_REG_SMBA + 1);
> + addr = val & IOREGION_ALIGNMENT;
> + if (addr == 0) {
> + pr_err("Refusing to enable a Super-I/O device with a base I/O port 0\n");
> + superio_exit(sioaddr);
> + return -ENODEV;
> + }
> +
> + superio_exit(sioaddr);
> + pr_info("Found %s or compatible chip at %#x:%#x\n",
> + nct6775_sio_names[sio_data->kind], sioaddr, addr);
> + sio_data->sioreg = sioaddr;
> +
> + return addr;
> +}
> +
> +static int __init i2c_nct6775_init(void)
> +{
> + int i, err;
> + bool found = false;
> + int address;
> + struct resource res;
> + struct nct6775_sio_data sio_data;
> + int sioaddr[2] = { 0x2e, 0x4e };
> +
> + err = platform_driver_register(&i2c_nct6775_driver);
> + if (err)
> + return err;
> +
> + /*
> + * initialize sio_data->kind and sio_data->sioreg.
> + *
> + * when Super-I/O functions move to a separate file, the Super-I/O
> + * driver will probe 0x2e and 0x4e and auto-detect the presence of a
> + * nct6775 hardware monitor, and call probe()
> + */
> + for (i = 0; i < ARRAY_SIZE(pdev); i++) {
> + address = nct6775_find(sioaddr[i], &sio_data);
> + if (address <= 0)
> + continue;
> +
> + found = true;
> +
> + pdev[i] = platform_device_alloc(DRVNAME, address);
> + if (!pdev[i]) {
> + err = -ENOMEM;
> + goto exit_device_unregister;
> + }
> +
> + err = platform_device_add_data(pdev[i], &sio_data,
> + sizeof(struct nct6775_sio_data));
> + if (err)
> + goto exit_device_put;
> +
> + memset(&res, 0, sizeof(res));
> + res.name = DRVNAME;
> + res.start = address;
> + res.end = address + IOREGION_LENGTH - 1;
> + res.flags = IORESOURCE_IO;
> +
> + err = acpi_check_resource_conflict(&res);
> + if (err) {
> + platform_device_put(pdev[i]);
> + pdev[i] = NULL;
> + continue;
> + }
> +
> + err = platform_device_add_resources(pdev[i], &res, 1);
> + if (err)
> + goto exit_device_put;
> +
> + /* platform_device_add calls probe() */
> + err = platform_device_add(pdev[i]);
> + if (err)
> + goto exit_device_put;
> + }
> + if (!found) {
> + err = -ENODEV;
> + goto exit_unregister;
> + }
> +
> + return 0;
> +
> +exit_device_put:
> + platform_device_put(pdev[i]);
> +exit_device_unregister:
> + while (--i >= 0) {
> + if (pdev[i])
> + platform_device_unregister(pdev[i]);
> + }
> +exit_unregister:
> + platform_driver_unregister(&i2c_nct6775_driver);
> + return err;
> +}
> +
> +MODULE_AUTHOR("Adam Honse <calcprogrammer1@gmail.com>");
> +MODULE_DESCRIPTION("SMBus driver for NCT6775F and compatible chips");
> +MODULE_LICENSE("GPL");
> +
> +module_init(i2c_nct6775_init);
> +module_exit(i2c_nct6775_exit);
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] i2c: New driver for Nuvoton SMBus adapters
2022-03-13 22:40 [PATCH] i2c: New driver for Nuvoton SMBus adapters Adam Honse
2022-03-14 1:17 ` kernel test robot
2022-03-14 11:03 ` Jean Delvare
@ 2022-03-14 20:28 ` kernel test robot
2 siblings, 0 replies; 4+ messages in thread
From: kernel test robot @ 2022-03-14 20:28 UTC (permalink / raw)
To: Adam Honse, jdelvare, linux-i2c, jdelvare; +Cc: llvm, kbuild-all
Hi Adam,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on wsa/i2c/for-next]
[also build test WARNING on v5.17-rc8 next-20220310]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Adam-Honse/i2c-New-driver-for-Nuvoton-SMBus-adapters/20220314-064247
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: arm64-allmodconfig (https://download.01.org/0day-ci/archive/20220315/202203150442.qhRCXPnf-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 3e4950d7fa78ac83f33bbf1658e2f49a73719236)
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
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# https://github.com/0day-ci/linux/commit/040a1be0d30ce2c611e30eb923d6f1b0afc44e7d
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Adam-Honse/i2c-New-driver-for-Nuvoton-SMBus-adapters/20220314-064247
git checkout 040a1be0d30ce2c611e30eb923d6f1b0afc44e7d
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/i2c/busses/ drivers/mfd/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/i2c/busses/i2c-nct6775.c:219:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
case I2C_SMBUS_BYTE:
^
drivers/i2c/busses/i2c-nct6775.c:219:2: note: insert '__attribute__((fallthrough));' to silence this warning
case I2C_SMBUS_BYTE:
^
__attribute__((fallthrough));
drivers/i2c/busses/i2c-nct6775.c:219:2: note: insert 'break;' to avoid fall-through
case I2C_SMBUS_BYTE:
^
break;
>> drivers/i2c/busses/i2c-nct6775.c:90:27: warning: unused variable 'nct6775_device_names' [-Wunused-const-variable]
static const char * const nct6775_device_names[] = {
^
drivers/i2c/busses/i2c-nct6775.c:133:1: warning: unused function 'superio_outb' [-Wunused-function]
superio_outb(int ioreg, int reg, int val)
^
3 warnings generated.
vim +219 drivers/i2c/busses/i2c-nct6775.c
88
89 /* used to set data->name = nct6775_device_names[data->sio_kind] */
> 90 static const char * const nct6775_device_names[] = {
91 "nct6106",
92 "nct6775",
93 "nct6776",
94 "nct6779",
95 "nct6791",
96 "nct6792",
97 "nct6793",
98 "nct6795",
99 "nct6796",
100 "nct6798",
101 };
102
103 static const char * const nct6775_sio_names[] __initconst = {
104 "NCT6106D",
105 "NCT6775F",
106 "NCT6776D/F",
107 "NCT6779D",
108 "NCT6791D",
109 "NCT6792D",
110 "NCT6793D",
111 "NCT6795D",
112 "NCT6796D",
113 "NCT6798D",
114 };
115
116 #define SIO_REG_LDSEL 0x07 /* Logical device select */
117 #define SIO_REG_DEVID 0x20 /* Device ID (2 bytes) */
118 #define SIO_REG_SMBA 0x62 /* SMBus base address register */
119
120 #define SIO_NCT6106_ID 0xc450
121 #define SIO_NCT6775_ID 0xb470
122 #define SIO_NCT6776_ID 0xc330
123 #define SIO_NCT6779_ID 0xc560
124 #define SIO_NCT6791_ID 0xc800
125 #define SIO_NCT6792_ID 0xc910
126 #define SIO_NCT6793_ID 0xd120
127 #define SIO_NCT6795_ID 0xd350
128 #define SIO_NCT6796_ID 0xd420
129 #define SIO_NCT6798_ID 0xd428
130 #define SIO_ID_MASK 0xFFF0
131
132 static inline void
133 superio_outb(int ioreg, int reg, int val)
134 {
135 outb(reg, ioreg);
136 outb(val, ioreg + 1);
137 }
138
139 static inline int
140 superio_inb(int ioreg, int reg)
141 {
142 outb(reg, ioreg);
143 return inb(ioreg + 1);
144 }
145
146 static inline void
147 superio_select(int ioreg, int ld)
148 {
149 outb(SIO_REG_LDSEL, ioreg);
150 outb(ld, ioreg + 1);
151 }
152
153 static inline int
154 superio_enter(int ioreg)
155 {
156 /*
157 * Try to reserve <ioreg> and <ioreg + 1> for exclusive access.
158 */
159 if (!request_muxed_region(ioreg, 2, DRVNAME))
160 return -EBUSY;
161
162 outb(0x87, ioreg);
163 outb(0x87, ioreg);
164
165 return 0;
166 }
167
168 static inline void
169 superio_exit(int ioreg)
170 {
171 outb(0xaa, ioreg);
172 outb(0x02, ioreg);
173 outb(0x02, ioreg + 1);
174 release_region(ioreg, 2);
175 }
176
177 /*
178 * ISA constants
179 */
180
181 #define IOREGION_ALIGNMENT (~7)
182 #define IOREGION_LENGTH 2
183 #define ADDR_REG_OFFSET 0
184 #define DATA_REG_OFFSET 1
185
186 #define NCT6775_REG_BANK 0x4E
187 #define NCT6775_REG_CONFIG 0x40
188
189 static struct i2c_adapter *nct6775_adapter;
190
191 struct i2c_nct6775_adapdata {
192 unsigned short smba;
193 };
194
195 /* Return negative errno on error. */
196 static s32 nct6775_access(struct i2c_adapter *adap, u16 addr,
197 unsigned short flags, char read_write,
198 u8 command, int size, union i2c_smbus_data *data)
199 {
200 struct i2c_nct6775_adapdata *adapdata = i2c_get_adapdata(adap);
201 unsigned short nuvoton_nct6793d_smba = adapdata->smba;
202 int i, len, cnt;
203 union i2c_smbus_data tmp_data;
204 int timeout = 0;
205
206 tmp_data.word = 0;
207 cnt = 0;
208 len = 0;
209
210 outb_p(NCT6793D_SOFT_RESET, SMBHSTCTL);
211
212 switch (size) {
213 case I2C_SMBUS_QUICK:
214 outb_p((addr << 1) | read_write,
215 SMBHSTADD);
216 break;
217 case I2C_SMBUS_BYTE_DATA:
218 tmp_data.byte = data->byte;
> 219 case I2C_SMBUS_BYTE:
220 outb_p((addr << 1) | read_write,
221 SMBHSTADD);
222 outb_p(command, SMBHSTIDX);
223 if (read_write == I2C_SMBUS_WRITE) {
224 outb_p(tmp_data.byte, SMBHSTDAT);
225 outb_p(NCT6793D_WRITE_BYTE, SMBHSTCMD);
226 } else {
227 outb_p(NCT6793D_READ_BYTE, SMBHSTCMD);
228 }
229 break;
230 case I2C_SMBUS_WORD_DATA:
231 outb_p((addr << 1) | read_write,
232 SMBHSTADD);
233 outb_p(command, SMBHSTIDX);
234 if (read_write == I2C_SMBUS_WRITE) {
235 outb_p(data->word & 0xff, SMBHSTDAT);
236 outb_p((data->word & 0xff00) >> 8, SMBHSTDAT);
237 outb_p(NCT6793D_WRITE_WORD, SMBHSTCMD);
238 } else {
239 outb_p(NCT6793D_READ_WORD, SMBHSTCMD);
240 }
241 break;
242 case I2C_SMBUS_BLOCK_DATA:
243 outb_p((addr << 1) | read_write,
244 SMBHSTADD);
245 outb_p(command, SMBHSTIDX);
246 if (read_write == I2C_SMBUS_WRITE) {
247 len = data->block[0];
248 if (len == 0 || len > I2C_SMBUS_BLOCK_MAX)
249 return -EINVAL;
250 outb_p(len, SMBBLKSZ);
251
252 cnt = 1;
253 if (len >= 4) {
254 for (i = cnt; i <= 4; i++)
255 outb_p(data->block[i], SMBHSTDAT);
256
257 len -= 4;
258 cnt += 4;
259 } else {
260 for (i = cnt; i <= len; i++)
261 outb_p(data->block[i], SMBHSTDAT);
262
263 len = 0;
264 }
265
266 outb_p(NCT6793D_WRITE_BLOCK, SMBHSTCMD);
267 } else {
268 return -EOPNOTSUPP;
269 }
270 break;
271 default:
272 dev_warn(&adap->dev, "Unsupported transaction %d\n", size);
273 return -EOPNOTSUPP;
274 }
275
276 outb_p(NCT6793D_MANUAL_START, SMBHSTCTL);
277
278 while ((size == I2C_SMBUS_BLOCK_DATA) && (len > 0)) {
279 if (read_write == I2C_SMBUS_WRITE) {
280 timeout = 0;
281 while ((inb_p(SMBHSTSTS) & NCT6793D_FIFO_EMPTY) == 0) {
282 if (timeout > MAX_RETRIES)
283 return -ETIMEDOUT;
284
285 usleep_range(250, 500);
286 timeout++;
287 }
288
289 //Load more bytes into FIFO
290 if (len >= 4) {
291 for (i = cnt; i <= (cnt + 4); i++)
292 outb_p(data->block[i], SMBHSTDAT);
293
294 len -= 4;
295 cnt += 4;
296 } else {
297 for (i = cnt; i <= (cnt + len); i++)
298 outb_p(data->block[i], SMBHSTDAT);
299
300 len = 0;
301 }
302 } else {
303 return -EOPNOTSUPP;
304 }
305
306 }
307
308 //wait for manual mode to complete
309 timeout = 0;
310 while ((inb_p(SMBHSTSTS) & NCT6793D_MANUAL_ACTIVE) != 0) {
311 if (timeout > MAX_RETRIES)
312 return -ETIMEDOUT;
313
314 usleep_range(250, 500);
315 timeout++;
316 }
317
318 if ((inb_p(SMBHSTERR) & NCT6793D_NO_ACK) != 0)
319 return -ENXIO;
320
321 else if ((read_write == I2C_SMBUS_WRITE) || (size == I2C_SMBUS_QUICK))
322 return 0;
323
324 switch (size) {
325 case I2C_SMBUS_QUICK:
326 case I2C_SMBUS_BYTE_DATA:
327 data->byte = inb_p(SMBHSTDAT);
328 break;
329 case I2C_SMBUS_WORD_DATA:
330 data->word = inb_p(SMBHSTDAT) + (inb_p(SMBHSTDAT) << 8);
331 break;
332 }
333 return 0;
334 }
335
---
0-DAY CI Kernel Test Service
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply [flat|nested] 4+ messages in thread