| Message ID | 20240911154820.2846187-5-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: isch: Put the driver into shape | expand |
Hi Andy, kernel test robot noticed the following build warnings: [auto build test WARNING on andi-shyti/i2c/i2c-host] [also build test WARNING on linus/master v6.11-rc7] [cannot apply to next-20240913] [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#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/i2c-isch-Add-missed-else/20240912-002224 base: https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host patch link: https://lore.kernel.org/r/20240911154820.2846187-5-andriy.shevchenko%40linux.intel.com patch subject: [PATCH v1 04/12] i2c: isch: Switch to memory mapped IO accessors config: i386-buildonly-randconfig-001-20240913 (https://download.01.org/0day-ci/archive/20240914/202409140743.kKVc8T3C-lkp@intel.com/config) compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240914/202409140743.kKVc8T3C-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202409140743.kKVc8T3C-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/i2c/busses/i2c-isch.c:296:32: warning: format specifies type 'unsigned int' but the argument has type 'resource_size_t' (aka 'unsigned long long') [-Wformat] 296 | "SMBus SCH adapter at %04x", res->start); | ~~~~ ^~~~~~~~~~ | %04llx 1 warning generated. vim +296 drivers/i2c/busses/i2c-isch.c 276 277 static int smbus_sch_probe(struct platform_device *dev) 278 { 279 struct resource *res; 280 int retval; 281 282 res = platform_get_resource(dev, IORESOURCE_IO, 0); 283 if (!res) 284 return -EBUSY; 285 286 sch_smba = devm_ioport_map(&dev->dev, res->start, resource_size(res)); 287 if (!sch_smba) { 288 dev_err(&dev->dev, "SMBus region %pR already in use!\n", res); 289 return -EBUSY; 290 } 291 292 /* set up the sysfs linkage to our parent device */ 293 sch_adapter.dev.parent = &dev->dev; 294 295 snprintf(sch_adapter.name, sizeof(sch_adapter.name), > 296 "SMBus SCH adapter at %04x", res->start); 297 298 retval = i2c_add_adapter(&sch_adapter); 299 if (retval) 300 sch_smba = NULL; 301 302 return retval; 303 } 304
Hi Andy, kernel test robot noticed the following build warnings: [auto build test WARNING on andi-shyti/i2c/i2c-host] [also build test WARNING on linus/master v6.11-rc7] [cannot apply to next-20240913] [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#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/i2c-isch-Add-missed-else/20240912-002224 base: https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host patch link: https://lore.kernel.org/r/20240911154820.2846187-5-andriy.shevchenko%40linux.intel.com patch subject: [PATCH v1 04/12] i2c: isch: Switch to memory mapped IO accessors config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20240914/202409141436.QFCDQrRF-lkp@intel.com/config) compiler: alpha-linux-gcc (GCC) 13.3.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240914/202409141436.QFCDQrRF-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202409141436.QFCDQrRF-lkp@intel.com/ All warnings (new ones prefixed by >>): drivers/i2c/busses/i2c-isch.c: In function 'smbus_sch_probe': >> drivers/i2c/busses/i2c-isch.c:296:42: warning: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'resource_size_t' {aka 'long long unsigned int'} [-Wformat=] 296 | "SMBus SCH adapter at %04x", res->start); | ~~~^ ~~~~~~~~~~ | | | | | resource_size_t {aka long long unsigned int} | unsigned int | %04llx vim +296 drivers/i2c/busses/i2c-isch.c 276 277 static int smbus_sch_probe(struct platform_device *dev) 278 { 279 struct resource *res; 280 int retval; 281 282 res = platform_get_resource(dev, IORESOURCE_IO, 0); 283 if (!res) 284 return -EBUSY; 285 286 sch_smba = devm_ioport_map(&dev->dev, res->start, resource_size(res)); 287 if (!sch_smba) { 288 dev_err(&dev->dev, "SMBus region %pR already in use!\n", res); 289 return -EBUSY; 290 } 291 292 /* set up the sysfs linkage to our parent device */ 293 sch_adapter.dev.parent = &dev->dev; 294 295 snprintf(sch_adapter.name, sizeof(sch_adapter.name), > 296 "SMBus SCH adapter at %04x", res->start); 297 298 retval = i2c_add_adapter(&sch_adapter); 299 if (retval) 300 sch_smba = NULL; 301 302 return retval; 303 } 304
Hi Andy, On Mon, Sep 16, 2024 at 12:07:28PM GMT, Andy Shevchenko wrote: > On Sat, Sep 14, 2024 at 02:56:19PM +0800, kernel test robot wrote: > > Hi Andy, > > > > kernel test robot noticed the following build warnings: > > > > [auto build test WARNING on andi-shyti/i2c/i2c-host] > > [also build test WARNING on linus/master v6.11-rc7] > > [cannot apply to next-20240913] > > [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#_base_tree_information] > > > > url: https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/i2c-isch-Add-missed-else/20240912-002224 > > base: https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host > > patch link: https://lore.kernel.org/r/20240911154820.2846187-5-andriy.shevchenko%40linux.intel.com > > patch subject: [PATCH v1 04/12] i2c: isch: Switch to memory mapped IO accessors > > config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20240914/202409141436.QFCDQrRF-lkp@intel.com/config) > > compiler: alpha-linux-gcc (GCC) 13.3.0 > > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240914/202409141436.QFCDQrRF-lkp@intel.com/reproduce) > > > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > > the same patch/commit), kindly add following tags > > | Reported-by: kernel test robot <lkp@intel.com> > > | Closes: https://lore.kernel.org/oe-kbuild-all/202409141436.QFCDQrRF-lkp@intel.com/ > > > > All warnings (new ones prefixed by >>): > > > > drivers/i2c/busses/i2c-isch.c: In function 'smbus_sch_probe': > > >> drivers/i2c/busses/i2c-isch.c:296:42: warning: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'resource_size_t' {aka 'long long unsigned int'} [-Wformat=] > > 296 | "SMBus SCH adapter at %04x", res->start); > > | ~~~^ ~~~~~~~~~~ > > | | | > > | | resource_size_t {aka long long unsigned int} > > | unsigned int > > | %04llx > > Yeah, this should be something like %pa, but the problem with that that it > always uses the same, fixed-width format with a prefix. We don't want this. But > to make sure we have proper specifier we need to introduce a temporary variable > and assign the resource start address to it and then use that variable in here. > I'll update this in v2 and send it after we have v6.12-rc1 is out. Feel free to send it, I will apply it in i2c/i2c-host-for-6.12, that's where I'm collecting the next patches. Andi
On Mon, Sep 16, 2024 at 12:10:32PM +0200, Andi Shyti wrote: > On Mon, Sep 16, 2024 at 12:07:28PM GMT, Andy Shevchenko wrote: > > On Sat, Sep 14, 2024 at 02:56:19PM +0800, kernel test robot wrote: ... > > > drivers/i2c/busses/i2c-isch.c: In function 'smbus_sch_probe': > > > >> drivers/i2c/busses/i2c-isch.c:296:42: warning: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'resource_size_t' {aka 'long long unsigned int'} [-Wformat=] > > > 296 | "SMBus SCH adapter at %04x", res->start); > > > | ~~~^ ~~~~~~~~~~ > > > | | | > > > | | resource_size_t {aka long long unsigned int} > > > | unsigned int > > > | %04llx > > > > Yeah, this should be something like %pa, but the problem with that that it > > always uses the same, fixed-width format with a prefix. We don't want this. But > > to make sure we have proper specifier we need to introduce a temporary variable > > and assign the resource start address to it and then use that variable in here. > > I'll update this in v2 and send it after we have v6.12-rc1 is out. > > Feel free to send it, I will apply it in i2c/i2c-host-for-6.12, > that's where I'm collecting the next patches. But I believe it's a material for v6.13, no?
On Mon, Sep 16, 2024 at 01:58:06PM +0200, Andi Shyti wrote: > On Mon, Sep 16, 2024 at 01:30:48PM GMT, Andy Shevchenko wrote: > > On Mon, Sep 16, 2024 at 12:10:32PM +0200, Andi Shyti wrote: > > > On Mon, Sep 16, 2024 at 12:07:28PM GMT, Andy Shevchenko wrote: ... > > > Feel free to send it, I will apply it in i2c/i2c-host-for-6.12, > > > that's where I'm collecting the next patches. > > > > But I believe it's a material for v6.13, no? > > yes, I gave it the wrong name :-) > I renamed it now to i2c/i2c-host-for-6.13(*). > > But it doesn't matter. It will become the next i2c/i2c-host after > Linus has taken the actual for-6.12 patches into mainline. v2 has just been sent out.
diff --git a/drivers/i2c/busses/i2c-isch.c b/drivers/i2c/busses/i2c-isch.c index f44c5fa276dc..8d34d4398f9b 100644 --- a/drivers/i2c/busses/i2c-isch.c +++ b/drivers/i2c/busses/i2c-isch.c @@ -24,16 +24,17 @@ #include <linux/io.h> #include <linux/stddef.h> #include <linux/string_choices.h> +#include <linux/types.h> /* SCH SMBus address offsets */ -#define SMBHSTCNT (0 + sch_smba) -#define SMBHSTSTS (1 + sch_smba) -#define SMBHSTCLK (2 + sch_smba) -#define SMBHSTADD (4 + sch_smba) /* TSA */ -#define SMBHSTCMD (5 + sch_smba) -#define SMBHSTDAT0 (6 + sch_smba) -#define SMBHSTDAT1 (7 + sch_smba) -#define SMBBLKDAT (0x20 + sch_smba) +#define SMBHSTCNT 0x00 +#define SMBHSTSTS 0x01 +#define SMBHSTCLK 0x02 +#define SMBHSTADD 0x04 /* TSA */ +#define SMBHSTCMD 0x05 +#define SMBHSTDAT0 0x06 +#define SMBHSTDAT1 0x07 +#define SMBBLKDAT 0x20 /* Other settings */ #define MAX_RETRIES 5000 @@ -45,12 +46,33 @@ #define SCH_WORD_DATA 0x03 #define SCH_BLOCK_DATA 0x05 -static unsigned short sch_smba; static struct i2c_adapter sch_adapter; +static void __iomem *sch_smba; + static int backbone_speed = 33000; /* backbone speed in kHz */ module_param(backbone_speed, int, S_IRUSR | S_IWUSR); MODULE_PARM_DESC(backbone_speed, "Backbone speed in kHz, (default = 33000)"); +static inline u8 sch_io_rd8(void __iomem *smba, unsigned int offset) +{ + return ioread8(smba + offset); +} + +static inline void sch_io_wr8(void __iomem *smba, unsigned int offset, u8 value) +{ + iowrite8(value, smba + offset); +} + +static inline u16 sch_io_rd16(void __iomem *smba, unsigned int offset) +{ + return ioread16(smba + offset); +} + +static inline void sch_io_wr16(void __iomem *smba, unsigned int offset, u16 value) +{ + iowrite16(value, smba + offset); +} + /* * Start the i2c transaction -- the i2c_access will prepare the transaction * and this function will execute it. @@ -64,20 +86,20 @@ static int sch_transaction(struct i2c_adapter *adap) dev_dbg(&adap->dev, "Transaction (pre): CNT=%02x, CMD=%02x, ADD=%02x, DAT0=%02x, DAT1=%02x\n", - inb(SMBHSTCNT), - inb(SMBHSTCMD), inb(SMBHSTADD), inb(SMBHSTDAT0), - inb(SMBHSTDAT1)); + sch_io_rd8(sch_smba, SMBHSTCNT), sch_io_rd8(sch_smba, SMBHSTCMD), + sch_io_rd8(sch_smba, SMBHSTADD), + sch_io_rd8(sch_smba, SMBHSTDAT0), sch_io_rd8(sch_smba, SMBHSTDAT1)); /* Make sure the SMBus host is ready to start transmitting */ - temp = inb(SMBHSTSTS) & 0x0f; + temp = sch_io_rd8(sch_smba, SMBHSTSTS) & 0x0f; if (temp) { /* Can not be busy since we checked it in sch_access */ if (temp & 0x01) dev_dbg(&adap->dev, "Completion (%02x). Clear...\n", temp); if (temp & 0x06) dev_dbg(&adap->dev, "SMBus error (%02x). Resetting...\n", temp); - outb(temp, SMBHSTSTS); - temp = inb(SMBHSTSTS) & 0x0f; + sch_io_wr8(sch_smba, SMBHSTSTS, temp); + temp = sch_io_rd8(sch_smba, SMBHSTSTS) & 0x0f; if (temp) { dev_err(&adap->dev, "SMBus is not ready: (%02x)\n", temp); return -EAGAIN; @@ -85,11 +107,13 @@ static int sch_transaction(struct i2c_adapter *adap) } /* start the transaction by setting bit 4 */ - outb(inb(SMBHSTCNT) | 0x10, SMBHSTCNT); + temp = sch_io_rd8(sch_smba, SMBHSTCNT); + temp |= 0x10; + sch_io_wr8(sch_smba, SMBHSTCNT, temp); do { usleep_range(100, 200); - temp = inb(SMBHSTSTS) & 0x0f; + temp = sch_io_rd8(sch_smba, SMBHSTSTS) & 0x0f; } while ((temp & 0x08) && (retries++ < MAX_RETRIES)); /* If the SMBus is still busy, we give up */ @@ -105,8 +129,8 @@ static int sch_transaction(struct i2c_adapter *adap) dev_err(&adap->dev, "Error: no response!\n"); } else if (temp & 0x01) { dev_dbg(&adap->dev, "Post complete!\n"); - outb(temp, SMBHSTSTS); - temp = inb(SMBHSTSTS) & 0x07; + sch_io_wr8(sch_smba, SMBHSTSTS, temp); + temp = sch_io_rd8(sch_smba, SMBHSTSTS) & 0x07; if (temp & 0x06) { /* Completion clear failed */ dev_dbg(&adap->dev, @@ -117,9 +141,9 @@ static int sch_transaction(struct i2c_adapter *adap) dev_dbg(&adap->dev, "No such address.\n"); } dev_dbg(&adap->dev, "Transaction (post): CNT=%02x, CMD=%02x, ADD=%02x, DAT0=%02x, DAT1=%02x\n", - inb(SMBHSTCNT), - inb(SMBHSTCMD), inb(SMBHSTADD), inb(SMBHSTDAT0), - inb(SMBHSTDAT1)); + sch_io_rd8(sch_smba, SMBHSTCNT), sch_io_rd8(sch_smba, SMBHSTCMD), + sch_io_rd8(sch_smba, SMBHSTADD), + sch_io_rd8(sch_smba, SMBHSTDAT0), sch_io_rd8(sch_smba, SMBHSTDAT1)); return result; } @@ -137,12 +161,12 @@ static s32 sch_access(struct i2c_adapter *adap, u16 addr, int i, len, temp, rc; /* Make sure the SMBus host is not busy */ - temp = inb(SMBHSTSTS) & 0x0f; + temp = sch_io_rd8(sch_smba, SMBHSTSTS) & 0x0f; if (temp & 0x08) { dev_dbg(&adap->dev, "SMBus busy (%02x)\n", temp); return -EAGAIN; } - temp = inw(SMBHSTCLK); + temp = sch_io_rd16(sch_smba, SMBHSTCLK); if (!temp) { /* * We can't determine if we have 33 or 25 MHz clock for @@ -151,47 +175,47 @@ static s32 sch_access(struct i2c_adapter *adap, u16 addr, * run ~75 kHz instead which should do no harm. */ dev_notice(&adap->dev, "Clock divider uninitialized. Setting defaults\n"); - outw(backbone_speed / (4 * 100), SMBHSTCLK); + sch_io_wr16(sch_smba, SMBHSTCLK, backbone_speed / (4 * 100)); } dev_dbg(&adap->dev, "access size: %d %s\n", size, str_read_write(read_write)); switch (size) { case I2C_SMBUS_QUICK: - outb((addr << 1) | read_write, SMBHSTADD); + sch_io_wr8(sch_smba, SMBHSTADD, (addr << 1) | read_write); size = SCH_QUICK; break; case I2C_SMBUS_BYTE: - outb((addr << 1) | read_write, SMBHSTADD); + sch_io_wr8(sch_smba, SMBHSTADD, (addr << 1) | read_write); if (read_write == I2C_SMBUS_WRITE) - outb(command, SMBHSTCMD); + sch_io_wr8(sch_smba, SMBHSTCMD, command); size = SCH_BYTE; break; case I2C_SMBUS_BYTE_DATA: - outb((addr << 1) | read_write, SMBHSTADD); - outb(command, SMBHSTCMD); + sch_io_wr8(sch_smba, SMBHSTADD, (addr << 1) | read_write); + sch_io_wr8(sch_smba, SMBHSTCMD, command); if (read_write == I2C_SMBUS_WRITE) - outb(data->byte, SMBHSTDAT0); + sch_io_wr8(sch_smba, SMBHSTDAT0, data->byte); size = SCH_BYTE_DATA; break; case I2C_SMBUS_WORD_DATA: - outb((addr << 1) | read_write, SMBHSTADD); - outb(command, SMBHSTCMD); + sch_io_wr8(sch_smba, SMBHSTADD, (addr << 1) | read_write); + sch_io_wr8(sch_smba, SMBHSTCMD, command); if (read_write == I2C_SMBUS_WRITE) { - outb(data->word & 0xff, SMBHSTDAT0); - outb((data->word & 0xff00) >> 8, SMBHSTDAT1); + sch_io_wr8(sch_smba, SMBHSTDAT0, data->word >> 0); + sch_io_wr8(sch_smba, SMBHSTDAT1, data->word >> 8); } size = SCH_WORD_DATA; break; case I2C_SMBUS_BLOCK_DATA: - outb((addr << 1) | read_write, SMBHSTADD); - outb(command, SMBHSTCMD); + sch_io_wr8(sch_smba, SMBHSTADD, (addr << 1) | read_write); + sch_io_wr8(sch_smba, SMBHSTCMD, command); if (read_write == I2C_SMBUS_WRITE) { len = data->block[0]; if (len == 0 || len > I2C_SMBUS_BLOCK_MAX) return -EINVAL; - outb(len, SMBHSTDAT0); + sch_io_wr8(sch_smba, SMBHSTDAT0, len); for (i = 1; i <= len; i++) - outb(data->block[i], SMBBLKDAT+i-1); + sch_io_wr8(sch_smba, SMBBLKDAT + i - 1, data->block[i]); } size = SCH_BLOCK_DATA; break; @@ -200,7 +224,10 @@ static s32 sch_access(struct i2c_adapter *adap, u16 addr, return -EOPNOTSUPP; } dev_dbg(&adap->dev, "write size %d to 0x%04x\n", size, SMBHSTCNT); - outb((inb(SMBHSTCNT) & 0xb0) | (size & 0x7), SMBHSTCNT); + + temp = sch_io_rd8(sch_smba, SMBHSTCNT); + temp = (temp & 0xb0) | (size & 0x7); + sch_io_wr8(sch_smba, SMBHSTCNT, temp); rc = sch_transaction(adap); if (rc) /* Error in transaction */ @@ -212,17 +239,18 @@ static s32 sch_access(struct i2c_adapter *adap, u16 addr, switch (size) { case SCH_BYTE: case SCH_BYTE_DATA: - data->byte = inb(SMBHSTDAT0); + data->byte = sch_io_rd8(sch_smba, SMBHSTDAT0); break; case SCH_WORD_DATA: - data->word = inb(SMBHSTDAT0) + (inb(SMBHSTDAT1) << 8); + data->word = (sch_io_rd8(sch_smba, SMBHSTDAT0) << 0) + + (sch_io_rd8(sch_smba, SMBHSTDAT1) << 8); break; case SCH_BLOCK_DATA: - data->block[0] = inb(SMBHSTDAT0); + data->block[0] = sch_io_rd8(sch_smba, SMBHSTDAT0); if (data->block[0] == 0 || data->block[0] > I2C_SMBUS_BLOCK_MAX) return -EPROTO; for (i = 1; i <= data->block[0]; i++) - data->block[i] = inb(SMBBLKDAT+i-1); + data->block[i] = sch_io_rd8(sch_smba, SMBBLKDAT + i - 1); break; } return 0; @@ -255,26 +283,21 @@ static int smbus_sch_probe(struct platform_device *dev) if (!res) return -EBUSY; - if (!devm_request_region(&dev->dev, res->start, resource_size(res), - dev->name)) { - dev_err(&dev->dev, "SMBus region 0x%x already in use!\n", - sch_smba); + sch_smba = devm_ioport_map(&dev->dev, res->start, resource_size(res)); + if (!sch_smba) { + dev_err(&dev->dev, "SMBus region %pR already in use!\n", res); return -EBUSY; } - sch_smba = res->start; - - dev_dbg(&dev->dev, "SMBA = 0x%X\n", sch_smba); - /* set up the sysfs linkage to our parent device */ sch_adapter.dev.parent = &dev->dev; snprintf(sch_adapter.name, sizeof(sch_adapter.name), - "SMBus SCH adapter at %04x", sch_smba); + "SMBus SCH adapter at %04x", res->start); retval = i2c_add_adapter(&sch_adapter); if (retval) - sch_smba = 0; + sch_smba = NULL; return retval; } @@ -283,7 +306,7 @@ static void smbus_sch_remove(struct platform_device *pdev) { if (sch_smba) { i2c_del_adapter(&sch_adapter); - sch_smba = 0; + sch_smba = NULL; } }
Convert driver to use memory mapped IO accessors. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/i2c/busses/i2c-isch.c | 133 ++++++++++++++++++++-------------- 1 file changed, 78 insertions(+), 55 deletions(-)