2019-05-22 12:16:12

by Geordan Neukum

[permalink] [raw]
Subject: [PATCH 0/6] Minor updates to kpc_i2c driver and kpc2000 core

Attached are an assortment of minor updates to the kpc_i2c driver as
well as a build fix for all of those who will need the KPC2000 core.

Thanks,
Geordan

Geordan Neukum (6):
staging: kpc2000: make kconfig symbol 'KPC2000' select dependencies
staging: kpc2000: kpc_i2c: remove unused module param disable_features
staging: kpc2000: kpc_i2c: newline fixups to meet linux style guide
staging: kpc2000: kpc_i2c: use <linux/io.h> instead of <asm/io.h>
staging: kpc2000: kpc_i2c: Remove unnecessary function tracing prints
staging: kpc2000: kpc_i2c: add static qual to local symbols in
kpc_i2c.c

drivers/staging/kpc2000/Kconfig | 2 +
drivers/staging/kpc2000/kpc2000_i2c.c | 118 +++++++-------------------
2 files changed, 34 insertions(+), 86 deletions(-)

--
2.21.0


2019-05-22 12:16:26

by Geordan Neukum

[permalink] [raw]
Subject: [PATCH 4/6] staging: kpc2000: kpc_i2c: use <linux/io.h> instead of <asm/io.h>

Rather than include asm/io.h, include linux/io.h. Issue reported
by the script checkpatch.pl.

Signed-off-by: Geordan Neukum <[email protected]>
---
drivers/staging/kpc2000/kpc2000_i2c.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/kpc2000/kpc2000_i2c.c b/drivers/staging/kpc2000/kpc2000_i2c.c
index a1ebc2386d70..5d98ed54c05c 100644
--- a/drivers/staging/kpc2000/kpc2000_i2c.c
+++ b/drivers/staging/kpc2000/kpc2000_i2c.c
@@ -19,7 +19,7 @@
#include <linux/init.h>
#include <linux/module.h>
#include <linux/types.h>
-#include <asm/io.h>
+#include <linux/io.h>
#include <linux/io-64-nonatomic-lo-hi.h>
#include <linux/export.h>
#include <linux/slab.h>
--
2.21.0

2019-05-22 12:16:34

by Geordan Neukum

[permalink] [raw]
Subject: [PATCH 5/6] staging: kpc2000: kpc_i2c: Remove unnecessary function tracing prints

Many of the functions in kpc_i2c log debug-level messages to the
kernel log message buffer upon invocation. This is unnecessary, as
debugging tools like kgdb, kdb, etc. or the tracing tool ftrace
should be able to provide this same information. Therefore, remove
these print statements.

Signed-off-by: Geordan Neukum <[email protected]>
---
drivers/staging/kpc2000/kpc2000_i2c.c | 26 --------------------------
1 file changed, 26 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc2000_i2c.c b/drivers/staging/kpc2000/kpc2000_i2c.c
index 5d98ed54c05c..f9259c06b605 100644
--- a/drivers/staging/kpc2000/kpc2000_i2c.c
+++ b/drivers/staging/kpc2000/kpc2000_i2c.c
@@ -139,8 +139,6 @@ static int i801_check_pre(struct i2c_device *priv)
{
int status;

- dev_dbg(&priv->adapter.dev, "%s\n", __func__);
-
status = inb_p(SMBHSTSTS(priv));
if (status & SMBHSTSTS_HOST_BUSY) {
dev_err(&priv->adapter.dev, "SMBus is busy, can't use it! (status=%x)\n", status);
@@ -165,8 +163,6 @@ static int i801_check_post(struct i2c_device *priv, int status, int timeout)
{
int result = 0;

- dev_dbg(&priv->adapter.dev, "%s\n", __func__);
-
/* If the SMBus is still busy, we give up */
if (timeout) {
dev_err(&priv->adapter.dev, "Transaction timeout\n");
@@ -214,8 +210,6 @@ static int i801_transaction(struct i2c_device *priv, int xact)
int result;
int timeout = 0;

- dev_dbg(&priv->adapter.dev, "%s\n", __func__);
-
result = i801_check_pre(priv);
if (result < 0)
return result;
@@ -244,8 +238,6 @@ static void i801_wait_hwpec(struct i2c_device *priv)
int timeout = 0;
int status;

- dev_dbg(&priv->adapter.dev, "%s\n", __func__);
-
do {
usleep_range(250, 500);
status = inb_p(SMBHSTSTS(priv));
@@ -262,8 +254,6 @@ static int i801_block_transaction_by_block(struct i2c_device *priv, union i2c_sm
int i, len;
int status;

- dev_dbg(&priv->adapter.dev, "%s\n", __func__);
-
inb_p(SMBHSTCNT(priv)); /* reset the data buffer index */

/* Use 32-byte buffer to process this transaction */
@@ -298,8 +288,6 @@ static int i801_block_transaction_byte_by_byte(struct i2c_device *priv, union i2
int result;
int timeout;

- dev_dbg(&priv->adapter.dev, "%s\n", __func__);
-
result = i801_check_pre(priv);
if (result < 0)
return result;
@@ -364,8 +352,6 @@ static int i801_block_transaction_byte_by_byte(struct i2c_device *priv, union i2

static int i801_set_block_buffer_mode(struct i2c_device *priv)
{
- dev_dbg(&priv->adapter.dev, "%s\n", __func__);
-
outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_E32B, SMBAUXCTL(priv));
if ((inb_p(SMBAUXCTL(priv)) & SMBAUXCTL_E32B) == 0)
return -EIO;
@@ -378,8 +364,6 @@ static int i801_block_transaction(struct i2c_device *priv, union i2c_smbus_data
int result = 0;
//unsigned char hostc;

- dev_dbg(&priv->adapter.dev, "%s\n", __func__);
-
if (command == I2C_SMBUS_I2C_BLOCK_DATA) {
if (read_write == I2C_SMBUS_WRITE) {
/* set I2C_EN bit in configuration register */
@@ -427,10 +411,6 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr, unsigned short flags,
int ret, xact = 0;
struct i2c_device *priv = i2c_get_adapdata(adap);

- dev_dbg(&priv->adapter.dev,
- "%s (addr=%0d) flags=%x read_write=%x command=%x size=%x",
- __func__, addr, flags, read_write, command, size);
-
hwpec = (priv->features & FEATURE_SMBUS_PEC) && (flags & I2C_CLIENT_PEC) && size != I2C_SMBUS_QUICK && size != I2C_SMBUS_I2C_BLOCK_DATA;

switch (size) {
@@ -605,9 +585,6 @@ int pi2c_probe(struct platform_device *pldev)
struct i2c_device *priv;
struct resource *res;

- dev_dbg(&pldev->dev, "%s(pldev = %p '%s')\n", __func__, pldev,
- pldev->name);
-
priv = devm_kzalloc(&pldev->dev, sizeof(*priv), GFP_KERNEL);
if (!priv)
return -ENOMEM;
@@ -653,9 +630,6 @@ int pi2c_remove(struct platform_device *pldev)
{
struct i2c_device *lddev;

- dev_dbg(&pldev->dev, "%s(pldev = %p '%s')\n", __func__, pldev,
- pldev->name);
-
lddev = (struct i2c_device *)pldev->dev.platform_data;

i2c_del_adapter(&lddev->adapter);
--
2.21.0

2019-05-22 12:16:47

by Geordan Neukum

[permalink] [raw]
Subject: [PATCH 6/6] staging: kpc2000: kpc_i2c: add static qual to local symbols in kpc_i2c.c

kpc_i2c.c declares:
- two functions
- pi2c_probe()
- pi2c_remove()
- one struct
- i2c_plat_driver_i
which are local to the file, yet missing the static qualifier. Add the
static qualifier to these symbols.

Signed-off-by: Geordan Neukum <[email protected]>
---
drivers/staging/kpc2000/kpc2000_i2c.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc2000_i2c.c b/drivers/staging/kpc2000/kpc2000_i2c.c
index f9259c06b605..97e738349ba2 100644
--- a/drivers/staging/kpc2000/kpc2000_i2c.c
+++ b/drivers/staging/kpc2000/kpc2000_i2c.c
@@ -579,7 +579,7 @@ static const struct i2c_algorithm smbus_algorithm = {
/********************************
*** Part 2 - Driver Handlers ***
********************************/
-int pi2c_probe(struct platform_device *pldev)
+static int pi2c_probe(struct platform_device *pldev)
{
int err;
struct i2c_device *priv;
@@ -626,7 +626,7 @@ int pi2c_probe(struct platform_device *pldev)
return 0;
}

-int pi2c_remove(struct platform_device *pldev)
+static int pi2c_remove(struct platform_device *pldev)
{
struct i2c_device *lddev;

@@ -644,7 +644,7 @@ int pi2c_remove(struct platform_device *pldev)
return 0;
}

-struct platform_driver i2c_plat_driver_i = {
+static struct platform_driver i2c_plat_driver_i = {
.probe = pi2c_probe,
.remove = pi2c_remove,
.driver = {
--
2.21.0

2019-05-22 12:17:18

by Geordan Neukum

[permalink] [raw]
Subject: [PATCH 3/6] staging: kpc2000: kpc_i2c: newline fixups to meet linux style guide

The linux coding style document states:

1) That braces should not be used where a single single statement
will do. Therefore all instances of single block statements
wrapped in braces that do not meet the qualifications of any
of the exceptions to the rule should be fixed up.

2) That the declaration of variables local to a given function
should be immediately followed by a blank newline. Therefore,
the single instance of this in kpc2000_i2c.c should be fixed
up.

Signed-off-by: Geordan Neukum <[email protected]>
---
drivers/staging/kpc2000/kpc2000_i2c.c | 82 ++++++++++-----------------
1 file changed, 29 insertions(+), 53 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc2000_i2c.c b/drivers/staging/kpc2000/kpc2000_i2c.c
index 40a89998726e..a1ebc2386d70 100644
--- a/drivers/staging/kpc2000/kpc2000_i2c.c
+++ b/drivers/staging/kpc2000/kpc2000_i2c.c
@@ -178,9 +178,8 @@ static int i801_check_post(struct i2c_device *priv, int status, int timeout)

/* Check if it worked */
status = inb_p(SMBHSTSTS(priv));
- if ((status & SMBHSTSTS_HOST_BUSY) || !(status & SMBHSTSTS_FAILED)) {
+ if ((status & SMBHSTSTS_HOST_BUSY) || !(status & SMBHSTSTS_FAILED))
dev_err(&priv->adapter.dev, "Failed terminating the transaction\n");
- }
outb_p(STATUS_FLAGS, SMBHSTSTS(priv));
return -ETIMEDOUT;
}
@@ -202,9 +201,8 @@ static int i801_check_post(struct i2c_device *priv, int status, int timeout)
/* Clear error flags */
outb_p(status & STATUS_FLAGS, SMBHSTSTS(priv));
status = inb_p(SMBHSTSTS(priv)) & STATUS_FLAGS;
- if (status) {
+ if (status)
dev_warn(&priv->adapter.dev, "Failed clearing status flags at end of transaction (%02x)\n", status);
- }
}

return result;
@@ -219,9 +217,8 @@ static int i801_transaction(struct i2c_device *priv, int xact)
dev_dbg(&priv->adapter.dev, "%s\n", __func__);

result = i801_check_pre(priv);
- if (result < 0) {
+ if (result < 0)
return result;
- }
/* the current contents of SMBHSTCNT can be overwritten, since PEC,
* INTREN, SMBSCMD are passed in xact
*/
@@ -234,9 +231,8 @@ static int i801_transaction(struct i2c_device *priv, int xact)
} while ((status & SMBHSTSTS_HOST_BUSY) && (timeout++ < MAX_RETRIES));

result = i801_check_post(priv, status, timeout > MAX_RETRIES);
- if (result < 0) {
+ if (result < 0)
return result;
- }

outb_p(SMBHSTSTS_INTR, SMBHSTSTS(priv));
return 0;
@@ -255,9 +251,8 @@ static void i801_wait_hwpec(struct i2c_device *priv)
status = inb_p(SMBHSTSTS(priv));
} while ((!(status & SMBHSTSTS_INTR)) && (timeout++ < MAX_RETRIES));

- if (timeout > MAX_RETRIES) {
+ if (timeout > MAX_RETRIES)
dev_dbg(&priv->adapter.dev, "PEC Timeout!\n");
- }

outb_p(status, SMBHSTSTS(priv));
}
@@ -275,26 +270,22 @@ static int i801_block_transaction_by_block(struct i2c_device *priv, union i2c_sm
if (read_write == I2C_SMBUS_WRITE) {
len = data->block[0];
outb_p(len, SMBHSTDAT0(priv));
- for (i = 0; i < len; i++) {
+ for (i = 0; i < len; i++)
outb_p(data->block[i+1], SMBBLKDAT(priv));
- }
}

status = i801_transaction(priv, I801_BLOCK_DATA | ENABLE_INT9 | I801_PEC_EN * hwpec);
- if (status) {
+ if (status)
return status;
- }

if (read_write == I2C_SMBUS_READ) {
len = inb_p(SMBHSTDAT0(priv));
- if (len < 1 || len > I2C_SMBUS_BLOCK_MAX) {
+ if (len < 1 || len > I2C_SMBUS_BLOCK_MAX)
return -EPROTO;
- }

data->block[0] = len;
- for (i = 0; i < len; i++) {
+ for (i = 0; i < len; i++)
data->block[i + 1] = inb_p(SMBBLKDAT(priv));
- }
}
return 0;
}
@@ -310,9 +301,8 @@ static int i801_block_transaction_byte_by_byte(struct i2c_device *priv, union i2
dev_dbg(&priv->adapter.dev, "%s\n", __func__);

result = i801_check_pre(priv);
- if (result < 0) {
+ if (result < 0)
return result;
- }

len = data->block[0];

@@ -323,23 +313,20 @@ static int i801_block_transaction_byte_by_byte(struct i2c_device *priv, union i2

for (i = 1; i <= len; i++) {
if (i == len && read_write == I2C_SMBUS_READ) {
- if (command == I2C_SMBUS_I2C_BLOCK_DATA) {
+ if (command == I2C_SMBUS_I2C_BLOCK_DATA)
smbcmd = I801_I2C_BLOCK_LAST;
- } else {
+ else
smbcmd = I801_BLOCK_LAST;
- }
} else {
- if (command == I2C_SMBUS_I2C_BLOCK_DATA && read_write == I2C_SMBUS_READ) {
+ if (command == I2C_SMBUS_I2C_BLOCK_DATA && read_write == I2C_SMBUS_READ)
smbcmd = I801_I2C_BLOCK_DATA;
- } else {
+ else
smbcmd = I801_BLOCK_DATA;
- }
}
outb_p(smbcmd | ENABLE_INT9, SMBHSTCNT(priv));

- if (i == 1) {
+ if (i == 1)
outb_p(inb(SMBHSTCNT(priv)) | I801_START, SMBHSTCNT(priv));
- }
/* We will always wait for a fraction of a second! */
timeout = 0;
do {
@@ -348,17 +335,15 @@ static int i801_block_transaction_byte_by_byte(struct i2c_device *priv, union i2
} while ((!(status & SMBHSTSTS_BYTE_DONE)) && (timeout++ < MAX_RETRIES));

result = i801_check_post(priv, status, timeout > MAX_RETRIES);
- if (result < 0) {
+ if (result < 0)
return result;
- }
if (i == 1 && read_write == I2C_SMBUS_READ && command != I2C_SMBUS_I2C_BLOCK_DATA) {
len = inb_p(SMBHSTDAT0(priv));
if (len < 1 || len > I2C_SMBUS_BLOCK_MAX) {
dev_err(&priv->adapter.dev, "Illegal SMBus block read size %d\n", len);
/* Recover */
- while (inb_p(SMBHSTSTS(priv)) & SMBHSTSTS_HOST_BUSY) {
+ while (inb_p(SMBHSTSTS(priv)) & SMBHSTSTS_HOST_BUSY)
outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
- }
outb_p(SMBHSTSTS_INTR, SMBHSTSTS(priv));
return -EPROTO;
}
@@ -366,12 +351,10 @@ static int i801_block_transaction_byte_by_byte(struct i2c_device *priv, union i2
}

/* Retrieve/store value in SMBBLKDAT */
- if (read_write == I2C_SMBUS_READ) {
+ if (read_write == I2C_SMBUS_READ)
data->block[i] = inb_p(SMBBLKDAT(priv));
- }
- if (read_write == I2C_SMBUS_WRITE && i+1 <= len) {
+ if (read_write == I2C_SMBUS_WRITE && i+1 <= len)
outb_p(data->block[i+1], SMBBLKDAT(priv));
- }
/* signals SMBBLKDAT ready */
outb_p(SMBHSTSTS_BYTE_DONE | SMBHSTSTS_INTR, SMBHSTSTS(priv));
}
@@ -384,9 +367,8 @@ static int i801_set_block_buffer_mode(struct i2c_device *priv)
dev_dbg(&priv->adapter.dev, "%s\n", __func__);

outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_E32B, SMBAUXCTL(priv));
- if ((inb_p(SMBAUXCTL(priv)) & SMBAUXCTL_E32B) == 0) {
+ if ((inb_p(SMBAUXCTL(priv)) & SMBAUXCTL_E32B) == 0)
return -EIO;
- }
return 0;
}

@@ -411,12 +393,10 @@ static int i801_block_transaction(struct i2c_device *priv, union i2c_smbus_data
}

if (read_write == I2C_SMBUS_WRITE || command == I2C_SMBUS_I2C_BLOCK_DATA) {
- if (data->block[0] < 1) {
+ if (data->block[0] < 1)
data->block[0] = 1;
- }
- if (data->block[0] > I2C_SMBUS_BLOCK_MAX) {
+ if (data->block[0] > I2C_SMBUS_BLOCK_MAX)
data->block[0] = I2C_SMBUS_BLOCK_MAX;
- }
} else {
data->block[0] = 32; /* max for SMBus block reads */
}
@@ -425,14 +405,12 @@ static int i801_block_transaction(struct i2c_device *priv, union i2c_smbus_data
* SMBus (not I2C) block transactions, even though the datasheet
* doesn't mention this limitation.
*/
- if ((priv->features & FEATURE_BLOCK_BUFFER) && command != I2C_SMBUS_I2C_BLOCK_DATA && i801_set_block_buffer_mode(priv) == 0) {
+ if ((priv->features & FEATURE_BLOCK_BUFFER) && command != I2C_SMBUS_I2C_BLOCK_DATA && i801_set_block_buffer_mode(priv) == 0)
result = i801_block_transaction_by_block(priv, data, read_write, hwpec);
- } else {
+ else
result = i801_block_transaction_byte_by_byte(priv, data, read_write, command, hwpec);
- }
- if (result == 0 && hwpec) {
+ if (result == 0 && hwpec)
i801_wait_hwpec(priv);
- }
if (command == I2C_SMBUS_I2C_BLOCK_DATA && read_write == I2C_SMBUS_WRITE) {
/* restore saved configuration register value */
//TODO: Figure out the right thing to do here...
@@ -465,18 +443,16 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr, unsigned short flags,
dev_dbg(&priv->adapter.dev, " [acc] SMBUS_BYTE\n");

outb_p(((addr & 0x7f) << 1) | (read_write & 0x01), SMBHSTADD(priv));
- if (read_write == I2C_SMBUS_WRITE) {
+ if (read_write == I2C_SMBUS_WRITE)
outb_p(command, SMBHSTCMD(priv));
- }
xact = I801_BYTE;
break;
case I2C_SMBUS_BYTE_DATA:
dev_dbg(&priv->adapter.dev, " [acc] SMBUS_BYTE_DATA\n");
outb_p(((addr & 0x7f) << 1) | (read_write & 0x01), SMBHSTADD(priv));
outb_p(command, SMBHSTCMD(priv));
- if (read_write == I2C_SMBUS_WRITE) {
+ if (read_write == I2C_SMBUS_WRITE)
outb_p(data->byte, SMBHSTDAT0(priv));
- }
xact = I801_BYTE_DATA;
break;
case I2C_SMBUS_WORD_DATA:
@@ -633,9 +609,8 @@ int pi2c_probe(struct platform_device *pldev)
pldev->name);

priv = devm_kzalloc(&pldev->dev, sizeof(*priv), GFP_KERNEL);
- if (!priv) {
+ if (!priv)
return -ENOMEM;
- }

i2c_set_adapdata(&priv->adapter, priv);
priv->adapter.owner = THIS_MODULE;
@@ -677,6 +652,7 @@ int pi2c_probe(struct platform_device *pldev)
int pi2c_remove(struct platform_device *pldev)
{
struct i2c_device *lddev;
+
dev_dbg(&pldev->dev, "%s(pldev = %p '%s')\n", __func__, pldev,
pldev->name);

--
2.21.0

2019-05-22 12:17:29

by Geordan Neukum

[permalink] [raw]
Subject: [PATCH 1/6] staging: kpc2000: make kconfig symbol 'KPC2000' select dependencies

The kpc2000 core makes calls against functions which are conditionally
exported upon the kconfig symbols 'MFD_CORE' and 'UIO' being selected
Therefore, in order to guarantee correct compilation, the 'KPC2000'
kconfig symbol (which brings in that code) must explicitly select its
hard dependencies.

Signed-off-by: Geordan Neukum <[email protected]>
---
drivers/staging/kpc2000/Kconfig | 2 ++
1 file changed, 2 insertions(+)

diff --git a/drivers/staging/kpc2000/Kconfig b/drivers/staging/kpc2000/Kconfig
index fb5922928f47..8992dc67ff37 100644
--- a/drivers/staging/kpc2000/Kconfig
+++ b/drivers/staging/kpc2000/Kconfig
@@ -3,6 +3,8 @@
config KPC2000
bool "Daktronics KPC Device support"
depends on PCI
+ select MFD_CORE
+ select UIO
help
Select this if you wish to use the Daktronics KPC PCI devices

--
2.21.0

2019-05-22 12:17:45

by Geordan Neukum

[permalink] [raw]
Subject: [PATCH 2/6] staging: kpc2000: kpc_i2c: remove unused module param disable_features

The module parameter 'disable_features' is currently unused. Therefore,
it should be removed.

Signed-off-by: Geordan Neukum <[email protected]>
---
drivers/staging/kpc2000/kpc2000_i2c.c | 4 ----
1 file changed, 4 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc2000_i2c.c b/drivers/staging/kpc2000/kpc2000_i2c.c
index 42061318d2d4..40a89998726e 100644
--- a/drivers/staging/kpc2000/kpc2000_i2c.c
+++ b/drivers/staging/kpc2000/kpc2000_i2c.c
@@ -126,10 +126,6 @@ struct i2c_device {
/* Not really a feature, but it's convenient to handle it as such */
#define FEATURE_IDF (1 << 15)

-static unsigned int disable_features;
-module_param(disable_features, uint, S_IRUGO | S_IWUSR);
-MODULE_PARM_DESC(disable_features, "Disable selected driver features");
-
// FIXME!
#undef inb_p
#define inb_p(a) readq((void*)a)
--
2.21.0

2019-05-22 12:29:01

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH 1/6] staging: kpc2000: make kconfig symbol 'KPC2000' select dependencies

On Wed, May 22, 2019 at 12:13:57PM +0000, Geordan Neukum wrote:
> The kpc2000 core makes calls against functions which are conditionally
> exported upon the kconfig symbols 'MFD_CORE' and 'UIO' being selected
> Therefore, in order to guarantee correct compilation, the 'KPC2000'
> kconfig symbol (which brings in that code) must explicitly select its
> hard dependencies.
>
> Signed-off-by: Geordan Neukum <[email protected]>
> ---
> drivers/staging/kpc2000/Kconfig | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/staging/kpc2000/Kconfig b/drivers/staging/kpc2000/Kconfig
> index fb5922928f47..8992dc67ff37 100644
> --- a/drivers/staging/kpc2000/Kconfig
> +++ b/drivers/staging/kpc2000/Kconfig
> @@ -3,6 +3,8 @@
> config KPC2000
> bool "Daktronics KPC Device support"
> depends on PCI
> + select MFD_CORE
> + select UIO

depends on is better than select. There's a change to depend on UIO for
this code already in my -linus branch which will show up in Linus's tree
in a week or so.

Are you sure we need MFD_CORE as well for this code? Why hasn't that
been seen on any build errors?

thanks,

greg k-h

2019-05-22 12:30:04

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH 0/6] Minor updates to kpc_i2c driver and kpc2000 core

On Wed, May 22, 2019 at 12:13:56PM +0000, Geordan Neukum wrote:
> Attached are an assortment of minor updates to the kpc_i2c driver as
> well as a build fix for all of those who will need the KPC2000 core.

Nit, please put "staging" in your 0/6 patch to make it easier for
scripts to pick this up properly. For next time please.

thanks,

greg k-h

2019-05-22 12:48:20

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH 3/6] staging: kpc2000: kpc_i2c: newline fixups to meet linux style guide

On Wed, May 22, 2019 at 12:13:59PM +0000, Geordan Neukum wrote:
> The linux coding style document states:
>
> 1) That braces should not be used where a single single statement
> will do. Therefore all instances of single block statements
> wrapped in braces that do not meet the qualifications of any
> of the exceptions to the rule should be fixed up.
>
> 2) That the declaration of variables local to a given function
> should be immediately followed by a blank newline. Therefore,
> the single instance of this in kpc2000_i2c.c should be fixed
> up.

This really should be 2 different patches, but given that this file is
so messy, I'll take it for now :)

thanks,

greg k-h

2019-05-23 01:37:30

by Geordan Neukum

[permalink] [raw]
Subject: Re: [PATCH 1/6] staging: kpc2000: make kconfig symbol 'KPC2000' select dependencies

On Wed, May 22, 2019 at 12:27 PM Greg Kroah-Hartman
<[email protected]> wrote:
> depends on is better than select. There's a change to depend on UIO for
> this code already in my -linus branch which will show up in Linus's tree
> in a week or so.

Noted on both accounts. Thanks for the feedback and sorry for the
inconvenience on the latter.

> Are you sure we need MFD_CORE as well for this code?

I noticed the build issue when working locally. I was doing
something along the lines of: 'make distclean && make x86_64_defconfig',
selecting 'CONFIG_KPC2000' and 'CONFIG_UIO' via menuconfig, then
running a good old 'make'. From make, I received an error along the
lines of:

ERROR: "mfd_remove_devices"
[drivers/staging/kpc2000/kpc2000/kpc2000.ko] undefined!
ERROR: "mfd_add_devices" [drivers/staging/kpc2000/kpc2000/kpc2000.ko] undefined!
make[1]: *** [scripts/Makefile.modpost:91: __modpost] Error 1
make: *** [Makefile:1290: modules] Error 2

which appears to indicate that those two symbols are undefined. When
I looked, it appeared that those symbols were exported from the
mfd-core which is why I also threw in a select for that Kconfig
symbol. Assuming that I didn't do something silly above, I'd be happy
to submit a new patch (with only a depends on for MFD_CORE) as I
continue trying to fix up the i2c driver.

>Why hasn't that been seen on any build errors?

To be honest, I can't say that I'm familiar with all of the different
build bots out there so I can't even begin to speculate on that one.
If someone could point me in the right direction there, I'd be happy
to investigate further.

Thanks again for your feedback all,
Geordan Neukum

2019-05-23 05:39:24

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH 1/6] staging: kpc2000: make kconfig symbol 'KPC2000' select dependencies

On Thu, May 23, 2019 at 01:35:02AM +0000, Geordan Neukum wrote:
> On Wed, May 22, 2019 at 12:27 PM Greg Kroah-Hartman
> <[email protected]> wrote:
> > depends on is better than select. There's a change to depend on UIO for
> > this code already in my -linus branch which will show up in Linus's tree
> > in a week or so.
>
> Noted on both accounts. Thanks for the feedback and sorry for the
> inconvenience on the latter.
>
> > Are you sure we need MFD_CORE as well for this code?
>
> I noticed the build issue when working locally. I was doing
> something along the lines of: 'make distclean && make x86_64_defconfig',
> selecting 'CONFIG_KPC2000' and 'CONFIG_UIO' via menuconfig, then
> running a good old 'make'. From make, I received an error along the
> lines of:
>
> ERROR: "mfd_remove_devices"
> [drivers/staging/kpc2000/kpc2000/kpc2000.ko] undefined!
> ERROR: "mfd_add_devices" [drivers/staging/kpc2000/kpc2000/kpc2000.ko] undefined!
> make[1]: *** [scripts/Makefile.modpost:91: __modpost] Error 1
> make: *** [Makefile:1290: modules] Error 2
>
> which appears to indicate that those two symbols are undefined. When
> I looked, it appeared that those symbols were exported from the
> mfd-core which is why I also threw in a select for that Kconfig
> symbol. Assuming that I didn't do something silly above, I'd be happy
> to submit a new patch (with only a depends on for MFD_CORE) as I
> continue trying to fix up the i2c driver.

Yes, a depends for MFD_CORE would be good, can you base it against my
staging-linus branch so that fix can go to Linus for this release?

thanks,

greg k-h

2019-05-24 02:42:14

by Geordan Neukum

[permalink] [raw]
Subject: [PATCH v2] staging: kpc2000: Add dependency on MFD_CORE to kconfig symbol 'KPC2000'

The kpc2000 core makes calls against functions conditionally exported
upon selection of the kconfig symbol MFD_CORE. Therefore, the kpc2000
core depends upon the mfd_core, and that dependency must be tracked in
Kconfig to avoid potential build issues.

Signed-off-by: Geordan Neukum <[email protected]>
---
v2 changes
- base patch on staging-linus
- only add MFD_CORE dependency as the UIO dependency has already been
handled by YueHaibing

drivers/staging/kpc2000/Kconfig | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/staging/kpc2000/Kconfig b/drivers/staging/kpc2000/Kconfig
index febe4f8b30e5..ef0f4abe894a 100644
--- a/drivers/staging/kpc2000/Kconfig
+++ b/drivers/staging/kpc2000/Kconfig
@@ -2,6 +2,7 @@

config KPC2000
bool "Daktronics KPC Device support"
+ depends on MFD_CORE
depends on PCI
depends on UIO
help
--
2.21.0

2019-05-24 03:05:48

by Geordan Neukum

[permalink] [raw]
Subject: Re: [PATCH v2] staging: kpc2000: Add dependency on MFD_CORE to kconfig symbol 'KPC2000'

On Fri, May 24, 2019 at 2:38 AM Geordan Neukum <[email protected]> wrote:
> + depends on MFD_CORE

In order for this to work in menuconfig, this either needs to be a
select or I need to
add a prompt to MFD_CORE. I don't have strong feelings either way, but all other
Kconfig options which are related to the MFD_CORE appear to do a straight
selection. Let me know what you think and I'll go that route.

Thanks,
Geordan

2019-05-24 06:52:22

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH v2] staging: kpc2000: Add dependency on MFD_CORE to kconfig symbol 'KPC2000'

On Fri, May 24, 2019 at 03:02:48AM +0000, Geordan Neukum wrote:
> On Fri, May 24, 2019 at 2:38 AM Geordan Neukum <[email protected]> wrote:
> > + depends on MFD_CORE
>
> In order for this to work in menuconfig, this either needs to be a
> select or I need to
> add a prompt to MFD_CORE. I don't have strong feelings either way, but all other
> Kconfig options which are related to the MFD_CORE appear to do a straight
> selection. Let me know what you think and I'll go that route.

Yeah, you are right, sorry about that. Let me just go hand edit-this
patch and queue it up as this was my fault...

thanks,

greg k-h