Re: [PATCH v6] at24: Support SMBus read/write of 16-bit devices
On Fri, 18 Dec 2015 13:47:50 -0600 (CST), Aaron Sierra wrote: > - Original Message - > > From: "Wolfram Sang" <w...@the-dreams.de> > > Sent: Friday, December 11, 2015 7:19:40 AM > > > > I remember I had a lengthy discussion with Guenter Roeck about 16 bit > > support via smbus. I don't have the bandwidth right now to recap the > > discussion and check if the concerns apply to your implementation as > > well. Could you check this thread > > > > https://lkml.org/lkml/2015/2/4/436 > > > > and report back if it applies to your patch as well? This would speed up > > the review significantly. > > The real bus-level multi-master issue is still present. However, it is > documented via comments in the driver and warnings in the Kconfig. > > The other (and apparently bigger) issue of a single master experiencing > corruption due to simultaneous access through i2c-dev and this driver > appears to have been resolved. The i2cdump, i2cget, and i2cset utilities > refuse to access any device that has a driver bound to it. My attempts > to cause corruption using i2cdump were met with the following message > until I unregistered the at24 driver from the device: > ># i2cdump 8 0x54 >No size specified (using byte-data access) >Error: Could not set address to 0x54: Device or resource busy You could always bypass the security check and force the access with option -f. But then there are plenty of ways to corrupt any device that way, regardless of the driver (or even without any driver bound to the device, if you run multiple i2c-dev-based user-space tools in parallel.) So I'd say this is not relevant. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/1] i2c: taos-evm: replace simple_strtoul by kstrtou8
Hi Corentin,
On Wed, 18 Nov 2015 13:55:56 +0100, LABBE Corentin wrote:
> The simple_strtoul function is marked as obsolete.
> This patch replace it by kstrtou8.
>
> Signed-off-by: LABBE Corentin <clabbe.montj...@gmail.com>
Reviewed-by: Jean Delvare <jdelv...@suse.de>
Tested-by: Jean Delvare <jdelv...@suse.de>
Note: when there's a single patch you don't have to send an email with
PATCH 0/1.
> ---
> drivers/i2c/busses/i2c-taos-evm.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-taos-evm.c
> b/drivers/i2c/busses/i2c-taos-evm.c
> index 4c7fc2d..f673f5d 100644
> --- a/drivers/i2c/busses/i2c-taos-evm.c
> +++ b/drivers/i2c/busses/i2c-taos-evm.c
> @@ -130,7 +130,13 @@ static int taos_smbus_xfer(struct i2c_adapter *adapter,
> u16 addr,
> return 0;
> } else {
> if (p[0] == 'x') {
> - data->byte = simple_strtol(p + 1, NULL, 16);
> + /*
> + * voluntarily dropping error code of kstrtou8 since all
> + * error code that it could return are invalid according
> + * to Documentation/i2c/fault-codes
> + */
> + if (kstrtou8(p + 1, 16, >byte))
> + return -EPROTO;
> return 0;
> }
> }
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/1] i2c: taos-evm: replace simple_strtoul by kstrtou8
Hi Corentin,
On Wed, 18 Nov 2015 09:00:53 +0100, LABBE Corentin wrote:
> The simple_strtoul function is marked as obsolete.
> This patch replace it by kstrtou8.
>
> Signed-off-by: LABBE Corentin <clabbe.montj...@gmail.com>
> ---
> drivers/i2c/busses/i2c-taos-evm.c | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-taos-evm.c
> b/drivers/i2c/busses/i2c-taos-evm.c
> index 4c7fc2d..552f127 100644
> --- a/drivers/i2c/busses/i2c-taos-evm.c
> +++ b/drivers/i2c/busses/i2c-taos-evm.c
> @@ -70,6 +70,7 @@ static int taos_smbus_xfer(struct i2c_adapter *adapter, u16
> addr,
> struct serio *serio = adapter->algo_data;
> struct taos_data *taos = serio_get_drvdata(serio);
> char *p;
> + int err;
>
> /* Encode our transaction. "@" is for the device address, "$" for the
> SMBus command and "#" for the data. */
> @@ -130,7 +131,14 @@ static int taos_smbus_xfer(struct i2c_adapter *adapter,
> u16 addr,
> return 0;
> } else {
> if (p[0] == 'x') {
> - data->byte = simple_strtol(p + 1, NULL, 16);
> + /*
> + * voluntary dropping error code of kstrtou8 since all
"Voluntarily" or "Willingly" would be more correct.
> + * error code that it could return are invalid according
> + * to Documentation/i2c/fault-codes
> + */
> + err = kstrtou8(p + 1, 16, >byte);
> + if (err)
> + return -EPROTO;
> return 0;
> }
> }
Thanks for the patch. Note that you don't strictly need the "err"
variable as you never use its value.
Reviewed-by: Jean Delvare <jdelv...@suse.de>
Tested-by: Jean Delvare <jdelv...@suse.de>
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6] at24: Support SMBus read/write of 16-bit devices
On Mon, 16 Nov 2015 18:02:19 -0600 (CST), Aaron Sierra wrote: > Previously, the at24 driver would bail out in the case of a 16-bit > addressable EEPROM attached to an SMBus controller. This is because > SMBus block reads and writes don't map to I2C multi-byte reads and > writes when the offset portion is 2 bytes. > > Instead of bailing out, this patch settles for functioning with single > byte read SMBus cycles. Writes can be block or single-byte, depending > on SMBus controller features. > > Read access is not without some risk. Multiple SMBus cycles are > required to read even one byte. If the SMBus has multiple masters and > one accesses this EEPROM between the dummy address write and the > subsequent current-address-read cycle(s), this driver will receive > data from the wrong address. > > Functionality has been tested with the following devices: > > AT24CM01 attached to Intel ISCH SMBus > AT24C512 attached to Intel I801 SMBus > > Read performance: > 3.6 KB/s with 32-byte* access > > *limited to 32-bytes by I2C_SMBUS_BLOCK_MAX. > > Write performance: > 248 B/s with 1-byte page (default) > 3.9 KB/s with 128-byte* page (via platform data) > > *limited to 31-bytes by I2C_SMBUS_BLOCK_MAX - 1. > > Signed-off-by: Nate Case <nc...@xes-inc.com> > Signed-off-by: Aaron Sierra <asie...@xes-inc.com> > Reviewed-by: Jean Delvare <jdelv...@suse.de> > --- > v2 - Account for changes related to introduction of > i2c_smbus_read_i2c_block_data_or_emulated() > v3 - Consolidate three patches into one > - Expand comments regarding SMBus multi-master read risks. > - Rely on current-address-read for improved read performance (i.e. one > dummy address write followed by multiple individual byte reads). > This improves performance from 1.4 KiB/s to 3.6 KiB/s. > - Use struct at24_data's writebuf instead of kzalloc-ing > - Only limit write_max by 1-byte when accessing a 16-bit device with > block writes instead of attempting to preserve a power-of-two. > - Style fixes (indentation, parentheses, unnecessary masking, etc.) > v4 - Address 16-bit safety in Kconfig > - Set "count" to zero later in at24_smbus_read_block_data() > - Fix over-80-columns issues in at24_eeprom_read() > - Fix write_max off-by-one in at24_probe() > - Check SMBus functionality needed for 16-bit device reads > - Homogenize indentation of SMBus functionality checks for SMBus write > v5 - 16-bit device read needs READ_BYTE not READ_BYTE_DATA > - Clarify write_max limiting with smbus_max > - Add X-ES copyright > v6 - Update comment associated with SMBus functionality testing for 16-bit > device read (READ_BYTE not READ_BYTE_DATA) > > drivers/misc/eeprom/Kconfig | 5 +- > drivers/misc/eeprom/at24.c | 132 > +++- > 2 files changed, 122 insertions(+), 15 deletions(-) > (...) Sweet, thanks for your patience. Now it's up to Wolfram. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] at24: Support SMBus read/write of 16-bit devices
p;& write_max > I2C_SMBUS_BLOCK_MAX)
> - write_max = I2C_SMBUS_BLOCK_MAX;
> + if (use_smbus && write_max >= I2C_SMBUS_BLOCK_MAX)
> + write_max = I2C_SMBUS_BLOCK_MAX -
> + !!(chip.flags & AT24_FLAG_ADDR16);
Beh. OK, it works, I will admit it's even kind of clever, but it also
looks fragile and confusing to some degree. What is wrong with just
spelling out the condition explicitly?
unsigned smbus_limit = (chip.flags & AT24_FLAG_ADDR16) ?
I2C_SMBUS_BLOCK_MAX - 1 :
I2C_SMBUS_BLOCK_MAX;
if (use_smbus && write_max > smbus_limit)
write_max = smbus_limit;
This might not even be slower, and IMHO it is easier to understand.
> at24->write_max = write_max;
>
> /* buffer (data + address at the beginning) */
I have no objection to this patch being merged into the upstream
kernel, but ultimately this is Wolfram's call.
Reviewed-by: Jean Delvare <jdelv...@suse.de>
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3] Intel Lewisburg device IDs for SMBus
On Thu, 5 Nov 2015 11:40:25 -0800, Alexandra Yates wrote:
> Adding Intel codename Lewisburg platform device IDs for SMBus.
>
> Signed-off-by: Alexandra Yates <alexandra.ya...@linux.intel.com>
> Reviewed-by: Jean Delvare <jdelv...@suse.de>
> ---
> Documentation/i2c/busses/i2c-i801 | 1 +
> drivers/i2c/busses/Kconfig| 1 +
> drivers/i2c/busses/i2c-i801.c | 6 ++
> 3 files changed, 8 insertions(+)
>
> diff --git a/Documentation/i2c/busses/i2c-i801
> b/Documentation/i2c/busses/i2c-i801
> index 6a4b1af..1bba38d 100644
> --- a/Documentation/i2c/busses/i2c-i801
> +++ b/Documentation/i2c/busses/i2c-i801
> @@ -32,6 +32,7 @@ Supported adapters:
>* Intel Sunrise Point-LP (PCH)
>* Intel DNV (SOC)
>* Intel Broxton (SOC)
> + * Intel Lewisburg (PCH)
> Datasheets: Publicly available at the Intel website
>
> On Intel Patsburg and later chipsets, both the normal host SMBus controller
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index e24c2b6..7b0aa82 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -126,6 +126,7 @@ config I2C_I801
> Sunrise Point-LP (PCH)
> DNV (SOC)
> Broxton (SOC)
> + Lewisburg (PCH)
>
> This driver can also be built as a module. If so, the module
> will be called i2c-i801.
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index c306751..f62d697 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -62,6 +62,8 @@
> * Sunrise Point-LP (PCH)0x9d23 32 hardyes yes yes
> * DNV (SOC) 0x19df 32 hardyes yes yes
> * Broxton (SOC) 0x5ad4 32 hardyes yes yes
> + * Lewisburg (PCH) 0xa1a3 32 hardyes yes yes
> + * Lewisburg Supersku (PCH) 0xa223 32 hardyes yes yes
> *
> * Features supported by this driver:
> * Software PEC no
> @@ -206,6 +208,8 @@
> #define PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS0x9d23
> #define PCI_DEVICE_ID_INTEL_DNV_SMBUS0x19df
> #define PCI_DEVICE_ID_INTEL_BROXTON_SMBUS0x5ad4
> +#define PCI_DEVICE_ID_INTEL_LEWISBURG_SMBUS 0xa1a3
> +#define PCI_DEVICE_ID_INTEL_LEWISBURG_SSKU_SMBUS 0xa223
>
> struct i801_mux_config {
> char *gpio_chip;
> @@ -869,6 +873,8 @@ static const struct pci_device_id i801_ids[] = {
> { PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS) },
> { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_DNV_SMBUS) },
> { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BROXTON_SMBUS) },
> + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LEWISBURG_SMBUS)
> },
> + { PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_LEWISBURG_SSKU_SMBUS) },
> { 0, }
> };
>
Perfect, thanks.
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] Intel Lewisburg device IDs for SMBus
Hi Alexandra,
On Wed, 4 Nov 2015 11:09:16 -0800, Alexandra Yates wrote:
> Adding Intel codename Lewisburg platform device IDs for SMBus.
>
> Signed-off-by: Alexandra Yates <alexandra.ya...@linux.intel.com>
> ---
> Documentation/i2c/busses/i2c-i801 | 1 +
> drivers/i2c/busses/Kconfig| 1 +
> drivers/i2c/busses/i2c-i801.c | 6 ++
> 3 files changed, 8 insertions(+)
>
> diff --git a/Documentation/i2c/busses/i2c-i801
> b/Documentation/i2c/busses/i2c-i801
> index 6a4b1af..1bba38d 100644
> --- a/Documentation/i2c/busses/i2c-i801
> +++ b/Documentation/i2c/busses/i2c-i801
> @@ -32,6 +32,7 @@ Supported adapters:
>* Intel Sunrise Point-LP (PCH)
>* Intel DNV (SOC)
>* Intel Broxton (SOC)
> + * Intel Lewisburg (PCH)
> Datasheets: Publicly available at the Intel website
>
> On Intel Patsburg and later chipsets, both the normal host SMBus controller
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index e24c2b6..7b0aa82 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -126,6 +126,7 @@ config I2C_I801
> Sunrise Point-LP (PCH)
> DNV (SOC)
> Broxton (SOC)
> + Lewisburg (PCH)
>
> This driver can also be built as a module. If so, the module
> will be called i2c-i801.
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index c306751..76fcef4 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -62,6 +62,8 @@
> * Sunrise Point-LP (PCH)0x9d23 32 hardyes yes yes
> * DNV (SOC) 0x19df 32 hardyes yes yes
> * Broxton (SOC) 0x5ad4 32 hardyes yes yes
> + * Lewisburg (PCH) 0xA1A3 32 hardyes yes yes
> + * Lewisburg Supersku (PCH) 0xA223 32 hardyes yes yes
It's a bit unfortunate that you used upper-case letters for the
hexadecimal numbers when all other entries used lower-case letters.
> *
> * Features supported by this driver:
> * Software PEC no
> @@ -181,6 +183,8 @@
>STATUS_ERROR_FLAGS)
>
> /* Older devices have their ID defined in */
> +#define PCI_DEVICE_ID_INTEL_LEWISBURG_SMBUS 0xA1A3
> +#define PCI_DEVICE_ID_INTEL_LEWISBURG_SSKU_SMBUS 0xA223
Same here. Plus, why are you adding the IDs at the top of the list here
while you added them at the bottom of the first list...
> #define PCI_DEVICE_ID_INTEL_BAYTRAIL_SMBUS 0x0f12
> #define PCI_DEVICE_ID_INTEL_BRASWELL_SMBUS 0x2292
> #define PCI_DEVICE_ID_INTEL_COUGARPOINT_SMBUS0x1c22
> @@ -864,6 +868,8 @@ static const struct pci_device_id i801_ids[] = {
> { PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_WILDCATPOINT_SMBUS) },
> { PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_WILDCATPOINT_LP_SMBUS) },
> { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BAYTRAIL_SMBUS) },
> + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LEWISBURG_SMBUS)
> },
> + { PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_LEWISBURG_SSKU_SMBUS) },
> { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BRASWELL_SMBUS) },
> { PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS) },
> { PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS) },
... and now in the middle of the last list? I would appreciate more
consistency.
These are all details of course, overall the changes look good, so you
can add:
Reviewed-by: Jean Delvare <jdelv...@suse.de>
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] at24: Support SMBus read/write of 16-bit devices
. */
> if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> - if (chip.flags & AT24_FLAG_ADDR16)
> - return -EPFNOSUPPORT;
> -
> - if (i2c_check_functionality(client->adapter,
> + if (chip.flags & AT24_FLAG_ADDR16) {
> + /*
> + * This will be slow, but better than nothing
> + * (e.g. read @ 3.6 KiB/s).
> + */
> + use_smbus = I2C_SMBUS_BYTE_DATA;
> + } else if (i2c_check_functionality(client->adapter,
> I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
> use_smbus = I2C_SMBUS_I2C_BLOCK_DATA;
> } else if (i2c_check_functionality(client->adapter,
> @@ -549,7 +632,17 @@ static int at24_probe(struct i2c_client *client, const
> struct i2c_device_id *id)
> if (i2c_check_functionality(client->adapter,
> I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) {
> use_smbus_write = I2C_SMBUS_I2C_BLOCK_DATA;
> - } else if (i2c_check_functionality(client->adapter,
> + } else if ((chip.flags & AT24_FLAG_ADDR16) &&
> + i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_WRITE_WORD_DATA)) {
> + /*
> + * We need SMBUS_WRITE_WORD_DATA to implement
> + * byte writes for 16-bit address devices.
> + */
> + use_smbus_write = I2C_SMBUS_BYTE_DATA;
> + chip.page_size = 1;
> + } else if (!(chip.flags & AT24_FLAG_ADDR16) &&
> + i2c_check_functionality(client->adapter,
> I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) {
> use_smbus_write = I2C_SMBUS_BYTE_DATA;
> chip.page_size = 1;
> @@ -599,7 +692,8 @@ static int at24_probe(struct i2c_client *client, const
> struct i2c_device_id *id)
> if (write_max > io_limit)
> write_max = io_limit;
> if (use_smbus && write_max > I2C_SMBUS_BLOCK_MAX)
> - write_max = I2C_SMBUS_BLOCK_MAX;
> + write_max = I2C_SMBUS_BLOCK_MAX -
> + !!(chip.flags & AT24_FLAG_ADDR16);
This test still looks wrong to me when AT24_FLAG_ADDR16 is set. If
write_max is 33 you'll properly clip it to 31. However if it's 32
you'll leave it at 32 and the subsequent block write attempts of 33
bytes will fail.
> at24->write_max = write_max;
>
> /* buffer (data + address at the beginning) */
Otherwise looks "good", assuming the user understands the risks and
limitations.
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: taos-evm: replace simple_strtoul by kstrtou8
Hi Corentin,
On Thu, 5 Nov 2015 10:32:48 +0100, LABBE Corentin wrote:
> The simple_strtoul function is marked as obsolete.
> This patch replace it by kstrtou8.
>
> Signed-off-by: LABBE Corentin <clabbe.montj...@gmail.com>
> ---
> drivers/i2c/busses/i2c-taos-evm.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
Thanks for the cleanup. I tested it on the hardware I have, no problem.
One comment below.
>
> diff --git a/drivers/i2c/busses/i2c-taos-evm.c
> b/drivers/i2c/busses/i2c-taos-evm.c
> index 4c7fc2d..9dc6cff 100644
> --- a/drivers/i2c/busses/i2c-taos-evm.c
> +++ b/drivers/i2c/busses/i2c-taos-evm.c
> @@ -70,6 +70,7 @@ static int taos_smbus_xfer(struct i2c_adapter *adapter, u16
> addr,
> struct serio *serio = adapter->algo_data;
> struct taos_data *taos = serio_get_drvdata(serio);
> char *p;
> + int err;
>
> /* Encode our transaction. "@" is for the device address, "$" for the
> SMBus command and "#" for the data. */
> @@ -130,7 +131,9 @@ static int taos_smbus_xfer(struct i2c_adapter *adapter,
> u16 addr,
> return 0;
> } else {
> if (p[0] == 'x') {
> - data->byte = simple_strtol(p + 1, NULL, 16);
> + err = kstrtou8(p + 1, 16, >byte);
> + if (err)
> + return err;
While in general I am in favor of passing error values down the stack,
here I'm not sure. kstrtou8 could return -ERANGE or -EINVAL which makes
no sense as an i2c adapter fault code. According to
Documentation/i2c/fault-codes, -EPROTO or -EIO would be more
appropriate.
> return 0;
> }
> }
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Intel LBG device IDs for SMBus
Hi Alexandra,
On Tue, 3 Nov 2015 10:52:34 -0800, Alexandra Yates wrote:
> Adding Intel codename Lewisburg platform device IDs for SMBus.
>
> Signed-off-by: Alexandra Yates <alexandra.ya...@linux.intel.com>
> ---
> Documentation/i2c/busses/i2c-i801 | 1 +
> drivers/i2c/busses/Kconfig| 1 +
> drivers/i2c/busses/i2c-i801.c | 6 ++
> 3 files changed, 8 insertions(+)
This patch doesn't apply because it ignores recent changes made to the
same files: "i2c: i801: Add support for Intel DNV" by Mika Westerberg,
"i2c: i801: Add support for Intel Broxton" by Jarkko Nikula and "i2c:
i801: Document Intel DNV and Broxton" by Jarkko Nikula.
All of these patches are in Wolfram's i2c tree:
http://git.kernel.org/cgit/linux/kernel/git/wsa/linux.git/log/?h=i2c/for-next
So please rebase your patch on that tree. Also see my comments below.
>
> diff --git a/Documentation/i2c/busses/i2c-i801
> b/Documentation/i2c/busses/i2c-i801
> index 82f48f7..2ccde60 100644
> --- a/Documentation/i2c/busses/i2c-i801
> +++ b/Documentation/i2c/busses/i2c-i801
> @@ -30,6 +30,7 @@ Supported adapters:
>* Intel BayTrail (SOC)
>* Intel Sunrise Point-H (PCH)
>* Intel Sunrise Point-LP (PCH)
> + * Intel Lewisburg (PCH) (LBG)
"(LBG)" doesn't need to be mentioned.
> Datasheets: Publicly available at the Intel website
>
> On Intel Patsburg and later chipsets, both the normal host SMBus controller
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 08b8617..3adc2b7 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -124,6 +124,7 @@ config I2C_I801
> BayTrail (SOC)
> Sunrise Point-H (PCH)
> Sunrise Point-LP (PCH)
> + Lewisburg (PCH)
>
> This driver can also be built as a module. If so, the module
> will be called i2c-i801.
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index eaef9bc..34da07b 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -60,6 +60,8 @@
> * BayTrail (SOC)0x0f12 32 hardyes yes yes
> * Sunrise Point-H (PCH) 0xa123 32 hardyes yes yes
> * Sunrise Point-LP (PCH)0x9d23 32 hardyes yes yes
> + * Lewisburg (PCH) 0xA1A3 32 hardyes yes yes
> + * Lewisburg Supersku (PCH) 0xA223 32 hardyes yes yes
> *
> * Features supported by this driver:
> * Software PEC no
> @@ -179,6 +181,8 @@
>STATUS_ERROR_FLAGS)
>
> /* Older devices have their ID defined in */
> +#define PCI_DEVICE_ID_INTEL_LBG_SMBUS0xA1A3
> +#define PCI_DEVICE_ID_INTEL_LBG_SSKU_SMBUS 0xA223
Please spell out LEWISBURG here. 3-letter shortcuts everywhere hurt
readability considerably over time.
> #define PCI_DEVICE_ID_INTEL_BAYTRAIL_SMBUS 0x0f12
> #define PCI_DEVICE_ID_INTEL_BRASWELL_SMBUS 0x2292
> #define PCI_DEVICE_ID_INTEL_COUGARPOINT_SMBUS0x1c22
> @@ -860,6 +864,8 @@ static const struct pci_device_id i801_ids[] = {
> { PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_WILDCATPOINT_SMBUS) },
> { PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_WILDCATPOINT_LP_SMBUS) },
> { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BAYTRAIL_SMBUS) },
> + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LBG_SMBUS) },
> + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LBG_SSKU_SMBUS) },
> { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BRASWELL_SMBUS) },
> { PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS) },
> { PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS) },
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] at24: Support SMBus block writes to 16-bit devices
Hi Aaron,
On Mon, 2 Nov 2015 10:35:22 -0600 (CST), Aaron Sierra wrote:
> - Original Message -
> > From: "Jean Delvare" <jdelv...@suse.de>
> > Sent: Monday, November 2, 2015 7:42:09 AM
> > On Thu, 3 Sep 2015 14:52:35 -0500 (CDT), Aaron Sierra wrote:
> > > @@ -612,7 +640,8 @@ static int at24_probe(struct i2c_client *client, const
> > > struct i2c_device_id *id)
> > > if (write_max > io_limit)
> > > write_max = io_limit;
> > > if (use_smbus && write_max > I2C_SMBUS_BLOCK_MAX)
> > > - write_max = I2C_SMBUS_BLOCK_MAX;
> > > + write_max = I2C_SMBUS_BLOCK_MAX >>
> > > + !!(chip.flags & AT24_FLAG_ADDR16);
> >
> > Looks bogus when AT24_FLAG_ADDR16 flag is set, in two ways:
> >
> > 1* I2C_SMBUS_BLOCK_MAX is 32. If write_max was 33, you'll set it to
> >33 >> 1 = 16. But if write_max was 31, you'll leave it to 31. Makes
> >no sense to me.
>
> That's true, I hadn't considered that case. I assumed page_size would
> be a power-of-two based on its name, though that isn't enforced anywhere.
It is checked but not enforced:
if (!is_power_of_2(chip.page_size))
dev_warn(>dev,
"page_size looks suspicious (no power of 2)!\n");
The thing is that I don't think a write operation can cross a page
boundary.
But as I understand it, this piece of code in at24_eeprom_write()
takes care of that:
/* Never roll over backwards, to the start of this page */
next_page = roundup(offset + 1, at24->chip.page_size);
if (offset + count > next_page)
count = next_page - offset;
So in the above example, if page size is 32 and write_max is 31, you'll
still need 2 write operations per page. If you write the whole eeprom,
that's no different from having write_max at 16. But if page size is 64
or more, having write_max at 31 instead of 16 will improve the
performance. Also think of partial writes, they can always benefit from
greater write_max.
> > 2* Why I2C_SMBUS_BLOCK_MAX >> 1 in the first place? You only need to
> >steal one byte from the data buffer for the extra address byte, so
> >I'd expect write_max to be capped at I2C_SMBUS_BLOCK_MAX - 1.
>
> I think that I had a vague concern about sending a non-power-of-two amount
> of data per transfer. I don't see why that should be an issue.
Should indeed not be an issue as long as you don't cross a page
boundary.
> > (...)
> > The code you added will never be executed anyway, because of this test
> > in at24_probe():
> >
> > if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> > if (chip.flags & AT24_FLAG_ADDR16)
> > return -EPFNOSUPPORT;
>
> That's true, if this patch is considered by itself. This test is
> updated by the 3rd patch in the series of three that I submitted.
The whole point of splitting the changes into multiple patches is that
they can be applied, tested and accepted (or rejected) independently.
Patches can depend on previous patch in the series but not on following
patches.
In general I am very much in favor of splitting the changes to make
reviews easier, but for this patch series, if there is no easy way to
split the changes, I believe it would make more sense to go with a
single patch.
> P.S. I submitted a v2 of these patches, but only 3/3 is affected.
I don't think I ever received v2 :(
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] at24: Support SMBus byte writes to 16-bit devices
On Thu, 3 Sep 2015 14:53:18 -0500 (CDT), Aaron Sierra wrote:
> Introduce at24_smbus_write_byte_data() to allow very slow (e.g.
> 248 B/s) write access to 16-bit EEPROM devices attached to SMBus
> controllers like the Intel SCH.
>
> Signed-off-by: Nate Case <nc...@xes-inc.com>
> Signed-off-by: Aaron Sierra <asie...@xes-inc.com>
> ---
> drivers/misc/eeprom/at24.c | 35 ---
> 1 file changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 4cf53a0..2ee1301 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -134,6 +134,25 @@ MODULE_DEVICE_TABLE(i2c, at24_ids);
> /*-*/
>
> /*
> + * Write a byte to an AT24 device using SMBus cycles.
> + */
> +static inline s32 at24_smbus_write_byte_data(struct at24_data *at24,
> + struct i2c_client *client, u16 offset, u8 value)
> +{
> + if (!(at24->chip.flags & AT24_FLAG_ADDR16))
> + return i2c_smbus_write_byte_data(client, offset, value);
> +
> + /*
> + * Emulate I2C multi-byte write by using SMBus "write word"
> + * cycle. We split up the 16-bit offset among the "command"
> + * byte and the first data byte.
> + */
> + return i2c_smbus_write_word_data(client,
> + ((offset >> 8) & 0xff),
Useless masking.
> + (value << 8) | (offset & 0xff));
> +}
> +
> +/*
> * Write block data to an AT24 device using SMBus cycles.
> */
> static inline s32 at24_smbus_write_i2c_block_data(struct at24_data *at24,
> @@ -401,8 +420,8 @@ static ssize_t at24_eeprom_write(struct at24_data *at24,
> const char *buf,
> client, offset, count, buf);
> break;
> case I2C_SMBUS_BYTE_DATA:
> - status = i2c_smbus_write_byte_data(client,
> - offset, buf[0]);
> + status = at24_smbus_write_byte_data(at24,
> + client, offset, buf[0]);
> break;
> }
>
> @@ -590,7 +609,17 @@ static int at24_probe(struct i2c_client *client, const
> struct i2c_device_id *id)
> if (i2c_check_functionality(client->adapter,
> I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) {
> use_smbus_write = I2C_SMBUS_I2C_BLOCK_DATA;
> - } else if (i2c_check_functionality(client->adapter,
> + } else if (chip.flags & AT24_FLAG_ADDR16 &&
Parentheses around (x & y) please.
> + i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_WRITE_WORD_DATA)) {
Would be good to align this the same way the other two are aligned.
> + /*
> + * We need SMBUS_WRITE_WORD_DATA to implement
> + * byte writes for 16-bit address devices.
> + */
> + use_smbus_write = I2C_SMBUS_BYTE_DATA;
> + chip.page_size = 1;
> + } else if (!(chip.flags & AT24_FLAG_ADDR16) &&
> + i2c_check_functionality(client->adapter,
> I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) {
> use_smbus_write = I2C_SMBUS_BYTE_DATA;
> chip.page_size = 1;
Like patch 1/3, the code you are adding in this patch can't be tested.
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] at24: Support 16-bit devices on SMBus
break;
> case I2C_SMBUS_BYTE_DATA:
> - status = i2c_smbus_read_byte_data(client, offset);
> + status = at24_smbus_read_byte_data(at24,
> + client, offset);
> if (status >= 0) {
> buf[0] = status;
> status = count;
> @@ -584,10 +611,13 @@ static int at24_probe(struct i2c_client *client, const
> struct i2c_device_id *id)
>
> /* Use I2C operations unless we're stuck with SMBus extensions. */
> if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> - if (chip.flags & AT24_FLAG_ADDR16)
> - return -EPFNOSUPPORT;
> -
> - if (i2c_check_functionality(client->adapter,
> + if (chip.flags & AT24_FLAG_ADDR16) {
> + /*
> + * This will be slow, but better than nothing
> + * (e.g. read @ 1.4 KiB/s).
> + */
> + use_smbus = I2C_SMBUS_BYTE_DATA;
> + } else if (i2c_check_functionality(client->adapter,
> I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
> use_smbus = I2C_SMBUS_I2C_BLOCK_DATA;
> } else if (i2c_check_functionality(client->adapter,
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: i2c-tools available commands
Stephen, Please keep the list on Cc and don't top-post. Le Monday 02 November 2015 à 13:23 +, Stephen Davis a écrit : > Thanks for the response. What I'm trying to do is use the i2c port on the Pi. > I've > seen how it can be done with Python, but we are trying to do it without it > (if possible). I've got a slew of microcontrollers on the i2c bus and am an in > the process of writing our comm protocol so I really just need a read and > write > from the Pi's i2c. Is this possible? I'm running Raspbian Wheezy (Desbian). If your micro-controllers are connected to the I2C port of the Pi, you should indeed be able to talk to them from user-space. Anyway if it worked with python you can definitely do it without it. Depending on the exact protocol required by the micro-controllers, i2cget and i2cset may be enough (if the micro-controllers only use basic SMBus-compliant commands) or you may need Wolfram Sang's upcoming i2ctransfer tool (I reviewed it some times ago, no news since, Wolfram where are we with it?) If even that isn't enough, you may have to write your code in C directly, linking with the upcoming libi2c library. It's a bit difficult at the moment though with the hosting website being down. > Is this the i2cget and i2cset functions? Those seem to want an > internal address of the slave devices (seems more geared towards > external memory). i2cget and i2cset want a slave address function. Your micro-controllers need to have a slave I2C address if you want to be able to talk to them from the Pi. Check the python code and the hardware documentation, the slave address is certainly mentioned there. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] at24: Support SMBus block writes to 16-bit devices
Hi Aaron,
Sorry for the late reply.
On Thu, 3 Sep 2015 14:52:35 -0500 (CDT), Aaron Sierra wrote:
> Introduce at24_smbus_write_i2c_block_data() to allow very slow write
> access to 16-bit EEPROM devices attached to SMBus controllers like
> the Intel I801.
This would be very bad hardware design. Are there actual systems out
there which use large EEPROMs over SMBus? I would only consider adding
this feature to the at24 driver if there is a real-world use case.
Otherwise the same can be done from user-space with eeprog.
>
> With an AT24C512 device:
> 248 B/s with 1-byte page (default)
> 3.9 KB/s with 128-byte* page (via platform data)
>
> *limited to 16-bytes by I2C_SMBUS_BLOCK_MAX / 2.
>
> Signed-off-by: Aaron Sierra <asie...@xes-inc.com>
> ---
> drivers/misc/eeprom/at24.c | 35 ---
> 1 file changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 2d3db81..4cf53a0 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -134,6 +134,34 @@ MODULE_DEVICE_TABLE(i2c, at24_ids);
> /*-*/
>
> /*
> + * Write block data to an AT24 device using SMBus cycles.
> + */
> +static inline s32 at24_smbus_write_i2c_block_data(struct at24_data *at24,
> + const struct i2c_client *client, u16 off, u8 len, const u8 *vals)
> +{
> + u8 *addr16;
> + s32 res;
> +
> + if (!(at24->chip.flags & AT24_FLAG_ADDR16))
> + return i2c_smbus_write_i2c_block_data(client, off, len, vals);
> +
> + addr16 = kzalloc(len + 1, GFP_KERNEL);
> + if (!addr16)
> + return -ENOMEM;
Allocating and freeing memory with every transfer is a bad idea, as
this slows the operation even more and favors memory fragmentation.
Why don't you use at24_data.writebuf?
> +
> + /* Insert extra address byte into data stream */
> + addr16[0] = off & 0xff;
> + memcpy(addr16 + 1, vals, len);
> +
> + res = i2c_smbus_write_i2c_block_data(client,
> + (off >> 8) & 0xff, len + 1, addr16);
Useless masking.
> +
> + kfree(addr16);
> +
> + return res;
> +}
> +
> +/*
> * This routine supports chips which consume multiple I2C addresses. It
> * computes the addressing information to be used for a given r/w request.
> * Assumes that sanity checks for offset happened at sysfs-layer.
> @@ -369,8 +397,8 @@ static ssize_t at24_eeprom_write(struct at24_data *at24,
> const char *buf,
> if (at24->use_smbus_write) {
> switch (at24->use_smbus_write) {
> case I2C_SMBUS_I2C_BLOCK_DATA:
> - status = i2c_smbus_write_i2c_block_data(client,
> - offset, count, buf);
> + status = at24_smbus_write_i2c_block_data(at24,
> + client, offset, count, buf);
> break;
> case I2C_SMBUS_BYTE_DATA:
> status = i2c_smbus_write_byte_data(client,
> @@ -612,7 +640,8 @@ static int at24_probe(struct i2c_client *client, const
> struct i2c_device_id *id)
> if (write_max > io_limit)
> write_max = io_limit;
> if (use_smbus && write_max > I2C_SMBUS_BLOCK_MAX)
> - write_max = I2C_SMBUS_BLOCK_MAX;
> + write_max = I2C_SMBUS_BLOCK_MAX >>
> + !!(chip.flags & AT24_FLAG_ADDR16);
Looks bogus when AT24_FLAG_ADDR16 flag is set, in two ways:
1* I2C_SMBUS_BLOCK_MAX is 32. If write_max was 33, you'll set it to
33 >> 1 = 16. But if write_max was 31, you'll leave it to 31. Makes
no sense to me.
2* Why I2C_SMBUS_BLOCK_MAX >> 1 in the first place? You only need to
steal one byte from the data buffer for the extra address byte, so
I'd expect write_max to be capped at I2C_SMBUS_BLOCK_MAX - 1.
> at24->write_max = write_max;
>
> /* buffer (data + address at the beginning) */
The code you added will never be executed anyway, because of this test
in at24_probe():
if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
if (chip.flags & AT24_FLAG_ADDR16)
return -EPFNOSUPPORT;
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: i2c-tools available commands
Hi Stephen, Le Monday 26 October 2015 à 23:52 +, Stephen Davis a écrit : > I have just started to use i2c-tools via the rasp2c module using > nodejs on my Raspberry Pi. I have not been able to track down the > commands that are available to i2c-tools. Is there a place where I can > find this information? I have no idea what rasp2c and nodejs are. Commands included in i2c-tools are i2cdetect, i2cdump, i2cget and i2cset. They all come with a manual page. Also included are i2c-stub-from-dump, ddcmon, decode-dimms and decode-vaio but probably of lesser interest to you. The exact list of commands included in your specific case depends what the packager decided to do. I suppose that commands that are not relevant on the Raspberry Pi are not included in the package. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: i801: Document Intel DNV and Broxton
Le Monday 26 October 2015 à 13:31 +0200, Mika Westerberg a écrit : > On Mon, Oct 26, 2015 at 01:26:56PM +0200, Jarkko Nikula wrote: > > Add missing entries into i2c-i801 documentation and Kconfig about recently > > added Intel DNV and Broxton. > > > > Suggested-by: Jean Delvare <jdelv...@suse.de> > > Signed-off-by: Jarkko Nikula <jarkko.nik...@linux.intel.com> > > Cc: Mika Westerberg <mika.westerb...@linux.intel.com> > > Thanks Jarkko! > > Reviewed-by: Mika Westerberg <mika.westerb...@linux.intel.com> Reviewed-by: Jean Delvare <jdelv...@suse.de> Thanks, -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: i801: Add support for Intel DNV
Hi Mika,
On Tue, 13 Oct 2015 15:41:39 +0300, Mika Westerberg wrote:
> Intel DNV SoC has the same legacy SMBus host controller than Intel
> Sunrisepoint PCH. It also has same iTCO watchdog on the bus.
>
> Add DNV PCI ID to the list of supported devices.
>
> Signed-off-by: Mika Westerberg <mika.westerb...@linux.intel.com>
> ---
> drivers/i2c/busses/i2c-i801.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index eaef9bc9d88c..47c2ddf76264 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -60,6 +60,7 @@
> * BayTrail (SOC)0x0f12 32 hardyes yes yes
> * Sunrise Point-H (PCH) 0xa123 32 hardyes yes yes
> * Sunrise Point-LP (PCH)0x9d23 32 hardyes yes yes
> + * DNV (SOC) 0x19df 32 hardyes yes yes
> *
> * Features supported by this driver:
> * Software PEC no
> @@ -202,6 +203,7 @@
> #define PCI_DEVICE_ID_INTEL_WILDCATPOINT_LP_SMBUS0x9ca2
> #define PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS 0xa123
> #define PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS0x9d23
> +#define PCI_DEVICE_ID_INTEL_DNV_SMBUS0x19df
Can you please get this added to pci.ids?
http://pci-ids.ucw.cz/read/PC/8086
>
> struct i801_mux_config {
> char *gpio_chip;
> @@ -863,6 +865,7 @@ static const struct pci_device_id i801_ids[] = {
> { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BRASWELL_SMBUS) },
> { PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS) },
> { PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS) },
> + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_DNV_SMBUS) },
> { 0, }
> };
>
> @@ -1256,6 +1259,7 @@ static int i801_probe(struct pci_dev *dev, const struct
> pci_device_id *id)
> switch (dev->device) {
> case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS:
> case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS:
> + case PCI_DEVICE_ID_INTEL_DNV_SMBUS:
> priv->features |= FEATURE_I2C_BLOCK_READ;
> priv->features |= FEATURE_IRQ;
> priv->features |= FEATURE_SMBUS_PEC;
Looks good, but please also update Documentation/i2c/busses/i2c-i801
and drivers/i2c/busses/Kconfig.
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: i801: Add support for Intel Broxton
Hi Jarkko,
On Thu, 22 Oct 2015 17:16:58 +0300, Jarkko Nikula wrote:
> This patch adds the SMBUS PCI ID of Intel Broxton.
>
> Signed-off-by: Jarkko Nikula <jarkko.nik...@linux.intel.com>
> ---
> This goes on top of Mika's "[PATCH] i2c: i801: Add support for Intel DNV":
> http://marc.info/?l=linux-i2c=14447401042=2
> ---
> drivers/i2c/busses/i2c-i801.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 47c2ddf76264..d8219bc2ac4e 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -61,6 +61,7 @@
> * Sunrise Point-H (PCH) 0xa123 32 hardyes yes yes
> * Sunrise Point-LP (PCH)0x9d23 32 hardyes yes yes
> * DNV (SOC) 0x19df 32 hardyes yes yes
> + * Broxton (SOC) 0x5ad4 32 hardyes yes yes
> *
> * Features supported by this driver:
> * Software PEC no
> @@ -204,6 +205,7 @@
> #define PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS 0xa123
> #define PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS0x9d23
> #define PCI_DEVICE_ID_INTEL_DNV_SMBUS0x19df
> +#define PCI_DEVICE_ID_INTEL_BROXTON_SMBUS0x5ad4
Can you please get this added to pci.ids?
http://pci-ids.ucw.cz/read/PC/8086
> struct i801_mux_config {
> char *gpio_chip;
> @@ -866,6 +868,7 @@ static const struct pci_device_id i801_ids[] = {
> { PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS) },
> { PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS) },
> { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_DNV_SMBUS) },
> + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BROXTON_SMBUS) },
> { 0, }
> };
>
Does this one not have FEATURE_TCO as DNV does? If it does, you'll need
to add a line for it in the switch block in i801_probe().
Please also update Documentation/i2c/busses/i2c-i801 and
drivers/i2c/busses/Kconfig.
Thanks,
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: randconfig build error with next-20150812, in drivers/i2c/busses/i2c-i801.c
On Wed, 12 Aug 2015 08:42:18 -0700, Jim Davis wrote: > Building with the attached random configuration file, > > drivers/built-in.o: In function `dmi_check_onboard_devices': > i2c-i801.c:(.text+0x126b36): undefined reference to `i2c_new_device' > drivers/built-in.o: In function `i801_remove': > i2c-i801.c:(.text+0x126b86): undefined reference to `i2c_del_adapter' > drivers/built-in.o: In function `i801_probe': > i2c-i801.c:(.text+0x127d08): undefined reference to `i2c_add_adapter' > i2c-i801.c:(.text+0x127d6e): undefined reference to `i2c_new_device' Sorry for the late reply. The error is caused by the following combination of options: CONFIG_I2C=m CONFIG_I2C_I801=y I can reproduce it even with mainline now. This is caused by CONFIG_ITCO_WDT=y, which selects CONFIG_I2C_I801=y without selecting its dependencies (CONFIG_I2C.) Thanks for reporting, I'll post a fix shortly. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] watchdog: iTCO_wdt: Fix unmet dependency in select
ITCO_WDT selects I2C_I801 but does not select its dependencies (I2C.)
This can result in link-time failures:
drivers/built-in.o: In function `i801_remove':
i2c-i801.c:(.text+0x1a6126): undefined reference to `i2c_del_adapter'
drivers/built-in.o: In function `dmi_check_onboard_devices':
i2c-i801.c:(.text+0x1a7531): undefined reference to `i2c_new_device'
drivers/built-in.o: In function `i801_probe':
i2c-i801.c:(.text+0x1a7aa8): undefined reference to `i2c_add_adapter'
i2c-i801.c:(.text+0x1a7e14): undefined reference to `i2c_new_device'
So we must select I2C as well.
Reported-by: Jim Davis <jim.ep...@gmail.com>
Signed-off-by: Jean Delvare <jdelv...@suse.de>
Fixes: 2a7a0e9bf7 ("watchdog: iTCO_wdt: Add support for TCO on Intel
Sunrisepoint")
Cc: Matt Fleming <matt.flem...@intel.com>
Cc: Guenter Roeck <li...@roeck-us.net>
Cc: Lee Jones <lee.jo...@linaro.org>
Cc: Wim Van Sebroeck <w...@iguana.be>
---
drivers/watchdog/Kconfig |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- linux-4.3-rc2.orig/drivers/watchdog/Kconfig 2015-09-24 11:43:29.959432358
+0200
+++ linux-4.3-rc2/drivers/watchdog/Kconfig 2015-09-25 09:56:20.518840112
+0200
@@ -818,6 +818,7 @@ config ITCO_WDT
depends on (X86 || IA64) && PCI
select WATCHDOG_CORE
select LPC_ICH if !EXPERT
+ select I2C if !EXPERT
select I2C_I801 if !EXPERT
---help---
Hardware driver for the intel TCO timer based watchdog devices.
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] watchdog: iTCO_wdt: Fix unmet dependency in select
Hi Guenter,
Le Friday 25 September 2015 à 06:29 -0700, Guenter Roeck a écrit :
> On 09/25/2015 01:10 AM, Jean Delvare wrote:
> > ITCO_WDT selects I2C_I801 but does not select its dependencies (I2C.)
> > This can result in link-time failures:
> >
> > drivers/built-in.o: In function `i801_remove':
> > i2c-i801.c:(.text+0x1a6126): undefined reference to `i2c_del_adapter'
> > drivers/built-in.o: In function `dmi_check_onboard_devices':
> > i2c-i801.c:(.text+0x1a7531): undefined reference to `i2c_new_device'
> > drivers/built-in.o: In function `i801_probe':
> > i2c-i801.c:(.text+0x1a7aa8): undefined reference to `i2c_add_adapter'
> > i2c-i801.c:(.text+0x1a7e14): undefined reference to `i2c_new_device'
> >
> > So we must select I2C as well.
> >
> > Reported-by: Jim Davis <jim.ep...@gmail.com>
> > Signed-off-by: Jean Delvare <jdelv...@suse.de>
> > Fixes: 2a7a0e9bf7 ("watchdog: iTCO_wdt: Add support for TCO on Intel
> > Sunrisepoint")
> > Cc: Matt Fleming <matt.flem...@intel.com>
> > Cc: Guenter Roeck <li...@roeck-us.net>
> > Cc: Lee Jones <lee.jo...@linaro.org>
> > Cc: Wim Van Sebroeck <w...@iguana.be>
> > ---
> > drivers/watchdog/Kconfig |2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > --- linux-4.3-rc2.orig/drivers/watchdog/Kconfig 2015-09-24
> > 11:43:29.959432358 +0200
> > +++ linux-4.3-rc2/drivers/watchdog/Kconfig 2015-09-25 09:56:20.518840112
> > +0200
> > @@ -818,6 +818,7 @@ config ITCO_WDT
> > depends on (X86 || IA64) && PCI
> > select WATCHDOG_CORE
> > select LPC_ICH if !EXPERT
> > + select I2C if !EXPERT
> > select I2C_I801 if !EXPERT
> > ---help---
> > Hardware driver for the intel TCO timer based watchdog devices.
> >
> >
>
> I had fixed the problem differently, similar to other drivers.
>
> http://patchwork.roeck-us.net/patch/239/
404 Not Found?
> Waiting for Wim to apply it.
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: randconfig build error with next-20150812, in drivers/i2c/busses/i2c-i801.c
Le Friday 25 September 2015 à 14:37 +0100, Matt Fleming a écrit : > On Fri, 25 Sep, at 09:36:10AM, Jean Delvare wrote: > > On Wed, 12 Aug 2015 08:42:18 -0700, Jim Davis wrote: > > > Building with the attached random configuration file, > > > > > > drivers/built-in.o: In function `dmi_check_onboard_devices': > > > i2c-i801.c:(.text+0x126b36): undefined reference to `i2c_new_device' > > > drivers/built-in.o: In function `i801_remove': > > > i2c-i801.c:(.text+0x126b86): undefined reference to `i2c_del_adapter' > > > drivers/built-in.o: In function `i801_probe': > > > i2c-i801.c:(.text+0x127d08): undefined reference to `i2c_add_adapter' > > > i2c-i801.c:(.text+0x127d6e): undefined reference to `i2c_new_device' > > > > Sorry for the late reply. The error is caused by the following > > combination of options: > > > > CONFIG_I2C=m > > CONFIG_I2C_I801=y > > > > I can reproduce it even with mainline now. This is caused by > > CONFIG_ITCO_WDT=y, which selects CONFIG_I2C_I801=y without selecting > > its dependencies (CONFIG_I2C.) > > > > Thanks for reporting, I'll post a fix shortly. > > Sorry that you got caught up in this build error Jean. No big deal :) > Guenter (Cc'd) posted a fix here, > > > https://lkml.kernel.org/r/1441978088-27288-1-git-send-email-li...@roeck-us.net Actually I prefer Guenter's approach. I am never comfortable with drivers selecting subsystems. So you can add: Acked-by: Jean Delvare <jdelv...@suse.de> to Guenter's patch and drop mine. Thanks, -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] i2c: added FUNC flag for unsupported clock stretching
On Fri, 28 Aug 2015 12:43:26 +0200, Nicola Corna wrote:
> Added I2C_FUNC_NO_CLK_STRETCH, to be used when clock stretching is not
> supported.
> Added this flag to drivers/i2c/algos/i2c-algo-bit.c when getscl is not
> available.
>
> Signed-off-by: Nicola Corna <nic...@corna.info>
> ---
> Documentation/i2c/functionality | 1 +
> drivers/i2c/algos/i2c-algo-bit.c | 5 -
> include/uapi/linux/i2c.h | 1 +
> 3 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/i2c/functionality b/Documentation/i2c/functionality
> index 4aae8ed..f53807e 100644
> --- a/Documentation/i2c/functionality
> +++ b/Documentation/i2c/functionality
> @@ -21,6 +21,7 @@ For the most up-to-date list of functionality constants,
> please check
>I2C_M_REV_DIR_ADDR and I2C_M_NO_RD_ACK
>flags (which modify the I2C protocol!)
>I2C_FUNC_NOSTARTCan skip repeated start sequence
> + I2C_FUNC_NO_CLK_STRETCH Does NOT support clock stretching
>I2C_FUNC_SMBUS_QUICKHandles the SMBus write_quick command
>I2C_FUNC_SMBUS_READ_BYTEHandles the SMBus read_byte command
>I2C_FUNC_SMBUS_WRITE_BYTE Handles the SMBus write_byte command
> diff --git a/drivers/i2c/algos/i2c-algo-bit.c
> b/drivers/i2c/algos/i2c-algo-bit.c
> index 899bede..82cad0b 100644
> --- a/drivers/i2c/algos/i2c-algo-bit.c
> +++ b/drivers/i2c/algos/i2c-algo-bit.c
> @@ -602,10 +602,13 @@ bailout:
>
> static u32 bit_func(struct i2c_adapter *adap)
> {
> + struct i2c_algo_bit_data *bit_adap = adap->algo_data;
> +
> return I2C_FUNC_I2C | I2C_FUNC_NOSTART | I2C_FUNC_SMBUS_EMUL |
> I2C_FUNC_SMBUS_READ_BLOCK_DATA |
> I2C_FUNC_SMBUS_BLOCK_PROC_CALL |
> -I2C_FUNC_10BIT_ADDR | I2C_FUNC_PROTOCOL_MANGLING;
> +I2C_FUNC_10BIT_ADDR | I2C_FUNC_PROTOCOL_MANGLING |
> +(bit_adap->getscl ? 0 : I2C_FUNC_NO_CLK_STRETCH);
> }
>
>
> diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
> index b0a7dd6..59e4b43 100644
> --- a/include/uapi/linux/i2c.h
> +++ b/include/uapi/linux/i2c.h
> @@ -88,6 +88,7 @@ struct i2c_msg {
> #define I2C_FUNC_SMBUS_PEC 0x0008
> #define I2C_FUNC_NOSTART 0x0010 /* I2C_M_NOSTART */
> #define I2C_FUNC_SLAVE 0x0020
> +#define I2C_FUNC_NO_CLK_STRETCH 0x0040 /* No check for SCL
> low */
> #define I2C_FUNC_SMBUS_BLOCK_PROC_CALL 0x00008000 /* SMBus 2.0 */
> #define I2C_FUNC_SMBUS_QUICK 0x0001
> #define I2C_FUNC_SMBUS_READ_BYTE 0x0002
Reviewed-by: Jean Delvare <jdelv...@suse.de>
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/3] I2C/SMBus: add support for Host Notify (in i2c_i801)
On Tue, 22 Sep 2015 13:41:27 -0400, Benjamin Tissoires wrote: > Hi All, > > any estimate when anybody will be able to review this series? It's on my to-do list, hopefully this week or early next week. For the core part it's probably better if Wolfram can do the review. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] i2c-tools: add new tool 'i2ctransfer'
Hi Wolfram, On Fri, 19 Jun 2015 12:40:31 +0200, Wolfram Sang wrote: > From: Wolfram Sang <wsa+rene...@sang-engineering.com> > > This tool allows to construct and concat multiple I2C messages into one > single transfer. Its aim is to test I2C master controllers, and so there > is no SMBus fallback. Which is fine. That wouldn't make much sense anyway as not all I2C transactions fit into the SMBus set. For SMBus transactions we already have i2cdump, i2cget and i2cset. > I've been missing such a tool a number of times now, so I finally got > around to writing it myself. As with all I2C tools, it can be dangerous, > but it can also be very useful when developing. I am not sure if distros > should supply it, I'll leave that to Jean's experience. For embedded > build systems, I think this should be selectable. I think it can be included together with the other tools. It's just as dangerous a tool as the other ones, not more. The fact that it can't be used on SMBus-only controllers even kind of makes it less dangerous. > Tested with various Renesas I2C IP cores as well as Tegra and AT91. > > Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com> Not needed for i2c-tools contributions. > --- > tools/Module.mk | 8 +- > tools/i2ctransfer.c | 320 > > > 2 files changed, 327 insertions(+), 1 deletion(-) > create mode 100644 tools/i2ctransfer.c Where is the manual page? We need one, it's mandatory for some distributions. And "make install" currently fails because tools/i2ctransfer.8 is missing. While this is not kernel code, I would recommend that you run the kernel's scripts/checkpatch.pl on tools/i2ctransfer.c. Most of the problems reported are relevant and fixing them would improve readability. Overall it looks really good. I made a lot of comments below but most of them only express my preference and you are free to ignore them if you disagree. > diff --git a/tools/Module.mk b/tools/Module.mk > index 641ac81..7192361 100644 > --- a/tools/Module.mk > +++ b/tools/Module.mk > @@ -18,7 +18,7 @@ else > TOOLS_LDFLAGS+= -Llib -li2c > endif > > -TOOLS_TARGETS:= i2cdetect i2cdump i2cset i2cget > +TOOLS_TARGETS:= i2cdetect i2cdump i2cset i2cget i2ctransfer > > # > # Programs > @@ -36,6 +36,9 @@ $(TOOLS_DIR)/i2cset: $(TOOLS_DIR)/i2cset.o > $(TOOLS_DIR)/i2cbusses.o $(TOOLS_DIR) > $(TOOLS_DIR)/i2cget: $(TOOLS_DIR)/i2cget.o $(TOOLS_DIR)/i2cbusses.o > $(TOOLS_DIR)/util.o > $(CC) $(LDFLAGS) -o $@ $^ $(TOOLS_LDFLAGS) > > +$(TOOLS_DIR)/i2ctransfer: $(TOOLS_DIR)/i2ctransfer.o > $(TOOLS_DIR)/i2cbusses.o $(TOOLS_DIR)/util.o > + $(CC) $(LDFLAGS) -o $@ $^ $(TOOLS_LDFLAGS) > + > # > # Objects > # > @@ -52,6 +55,9 @@ $(TOOLS_DIR)/i2cset.o: $(TOOLS_DIR)/i2cset.c > $(TOOLS_DIR)/i2cbusses.h $(TOOLS_DI > $(TOOLS_DIR)/i2cget.o: $(TOOLS_DIR)/i2cget.c $(TOOLS_DIR)/i2cbusses.h > $(TOOLS_DIR)/util.h version.h $(INCLUDE_DIR)/i2c/smbus.h > $(CC) $(CFLAGS) $(TOOLS_CFLAGS) -c $< -o $@ > > +$(TOOLS_DIR)/i2ctransfer.o: $(TOOLS_DIR)/i2ctransfer.c > $(TOOLS_DIR)/i2cbusses.h $(TOOLS_DIR)/util.h version.h > + $(CC) $(CFLAGS) $(TOOLS_CFLAGS) -c $< -o $@ > + > $(TOOLS_DIR)/i2cbusses.o: $(TOOLS_DIR)/i2cbusses.c $(TOOLS_DIR)/i2cbusses.h > $(CC) $(CFLAGS) $(TOOLS_CFLAGS) -c $< -o $@ > > diff --git a/tools/i2ctransfer.c b/tools/i2ctransfer.c > new file mode 100644 > index 000..27f4d7a > --- /dev/null > +++ b/tools/i2ctransfer.c > @@ -0,0 +1,320 @@ > +/* > +i2ctransfer.c - A user-space program to send concatenated i2c messages > +Copyright (C) 2015 Wolfram Sang <w...@sang-engineering.com> > +Copyright (C) 2015 Renesas Electronics Corporation > + > +Based on i2cget.c: > +Copyright (C) 2005-2012 Jean Delvare <jdelv...@suse.de> > + > +which is based on i2cset.c: > +Copyright (C) 2001-2003 Frodo Looijaard <fro...@dds.nl>, and > + Mark D. Studebaker <mdsxyz...@yahoo.com> > +Copyright (C) 2004-2005 Jean Delvare I think you can skip this. If anyone really cares, it's already mentioned in i2cget.c. > + > +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. > +*/
[PATCH] i2c-dev: Fix I2C_SLAVE ioctl comment
The first part of the comment is wrong since November 2007, delete it.
The second part of the comment is related to I2C_PEC, not I2C_SLAVE, so
move it where it belongs.
Signed-off-by: Jean Delvare <jdelv...@suse.de>
Cc: Wolfram Sang <w...@the-dreams.de>
---
drivers/i2c/i2c-dev.c | 17 +++--
1 file changed, 7 insertions(+), 10 deletions(-)
--- linux-4.2.orig/drivers/i2c/i2c-dev.c2015-09-11 11:22:00.962436523
+0200
+++ linux-4.2/drivers/i2c/i2c-dev.c 2015-09-11 11:22:22.532879192 +0200
@@ -421,16 +421,6 @@ static long i2cdev_ioctl(struct file *fi
switch (cmd) {
case I2C_SLAVE:
case I2C_SLAVE_FORCE:
- /* NOTE: devices set up to work with "new style" drivers
-* can't use I2C_SLAVE, even when the device node is not
-* bound to a driver. Only I2C_SLAVE_FORCE will work.
-*
-* Setting the PEC flag here won't affect kernel drivers,
-* which will be using the i2c_client node registered with
-* the driver model core. Likewise, when that client has
-* the PEC flag already set, the i2c-dev driver won't see
-* (or use) this setting.
-*/
if ((arg > 0x3ff) ||
(((client->flags & I2C_M_TEN) == 0) && arg > 0x7f))
return -EINVAL;
@@ -446,6 +436,13 @@ static long i2cdev_ioctl(struct file *fi
client->flags &= ~I2C_M_TEN;
return 0;
case I2C_PEC:
+ /*
+* Setting the PEC flag here won't affect kernel drivers,
+* which will be using the i2c_client node registered with
+* the driver model core. Likewise, when that client has
+* the PEC flag already set, the i2c-dev driver won't see
+* (or use) this setting.
+*/
if (arg)
client->flags |= I2C_CLIENT_PEC;
else
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c-dev: Fix typo in ioctl name reference
On Thu, 10 Sep 2015 20:00:12 +0200, Wolfram Sang wrote: > On Tue, Sep 08, 2015 at 11:05:49AM +0200, Jean Delvare wrote: > > The ioctl is named I2C_RDWR for "I2C read/write". But references to it > > were misspelled "rdrw". Fix them. > > > > Signed-off-by: Jean Delvare <jdelv...@suse.de> > > Cc: Wolfram Sang <w...@the-dreams.de> > > Wow! Amazing how long this went unnoticed/unfixed. Indeed :/ I don't think this constant was much (or ever) used in user-space before i2ctransfer. That would be why. > > -#define I2C_RDRW_IOCTL_MAX_MSGS 42 > > +#define I2C_RDWR_IOCTL_MAX_MSGS 42 > > +/* Originally defined with a typo, keep if for now for compatibility */ > > I would drop the 'for now' and keep it forever. A define doesn't hurt > and if it increases backwards compatibility, why not? I will also do > s/if/it/. No need to resend. Sorry for the typo and thanks for catching it. "for now" or not is up to you, my idea was that it probably wasn't used in user-space yet so getting rid of it shouldn't hurt, while keeping it would encourage people to use the wrong one instead of the good one. So on second thought a better strategy may be to NOT keep the compatibility in the first place. It's not an ABI change anyway, and it's a minor thing really. Nobody is ever going to hit this limit in practice, and if one does and did not check beforehand, he/she will simply get -EINVAL from ioctl(), which can be reported to the user just the same. IMHO I2C_RDWR_IOCTL_MAX_MSGS should not have been made visible from user-space in the first place. > > +#define I2C_RDRW_IOCTL_MAX_MSGS I2C_RDWR_IOCTL_MAX_MSGS -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: i2ctransfer (Was: [PATCH 0/8] i2c: img-scb: fixes to support i2c on pistachio)
On Mon, 7 Sep 2015 11:22:23 -0300, Ezequiel Garcia wrote: > On 7 September 2015 at 10:50, Wolfram Sang <w...@the-dreams.de> wrote: > > > >> > That sounds like good testing \o/ If you are happy with i2ctransfer, > >> > then please consider adding Tested-by tags to the i2ctransfer patches, > >> > so Jean gets a more cosy feeling to include them to i2ctools. > >> > > >> > >> Definitely! i2ctransfer was a HUGE help while developing this driver, > >> specially when preparing involved transfers to test repeated starts. > > > > Glad to hear that! I also discovered and fixed a corner case in repeated > > start handling recently in the i2c-rcar driver thanks to i2ctransfer. > > > >> Any chance it gets merged in its current state? I think it'll be useful, > >> even > >> if the usage cli API is still a moving target. > > > > I don't think it is anymore TBH. Not more than any other tool. > > > > Waiving to Jean \o_ :) > > OK, let me wave as well \\O// Review in progress, will continue tomorrow. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/3] i2c: i801: add support of Host Notify
On Wed, 29 Jul 2015 10:58:13 -0400, Benjamin Tissoires wrote: On Sat, Jul 25, 2015 at 5:38 PM, Jean Delvare jdelv...@suse.de wrote: On Sat, 25 Jul 2015 12:22:02 -0400, Benjamin Tissoires wrote: On Sat, Jul 25, 2015 at 12:11 PM, Jean Delvare jdelv...@suse.de wrote: Hi Benjamin, On Tue, 21 Jul 2015 17:24:55 -0400, Benjamin Tissoires wrote: So please disregard this series, I will send a v4 hopefully soonish. From v2 directly to v4? Did I miss something? I had to send a v3 to amend 1/3. Given that it was a small fix, I only resent 1/3 and not the whole series. Never received that one, thus my confusion. After a second thought, I wonder if you received also the v4. It looks fine on my side (you are CC-ed to it), but given that you missed the v3, I'd rather be sure you have the v4 on your radar before we forget about it. Can you check that you have actually received http://www.spinics.net/lists/linux-i2c/msg20616.html and the 3 after? (the v3 was http://www.spinics.net/lists/linux-i2c/msg20418.html) I did receive v4, and also v3. The problem is on my side, the email address change at suse confused my mail filters and the mails were not where I was looking for them. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/3] i2c: i801: add support of Host Notify
Hi Benjamin, On Tue, 21 Jul 2015 17:24:55 -0400, Benjamin Tissoires wrote: So please disregard this series, I will send a v4 hopefully soonish. From v2 directly to v4? Did I miss something? -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/3] i2c: i801: add support of Host Notify
On Sat, 25 Jul 2015 12:22:02 -0400, Benjamin Tissoires wrote: On Sat, Jul 25, 2015 at 12:11 PM, Jean Delvare jdelv...@suse.de wrote: Hi Benjamin, On Tue, 21 Jul 2015 17:24:55 -0400, Benjamin Tissoires wrote: So please disregard this series, I will send a v4 hopefully soonish. From v2 directly to v4? Did I miss something? I had to send a v3 to amend 1/3. Given that it was a small fix, I only resent 1/3 and not the whole series. Never received that one, thus my confusion. To stay consistent, I jumped to v4 for the new version of the series :) OK, no problem. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c-parport: Add VCT-jig adapter
On Mon, 13 Jul 2015 19:31:12 +0200, Ondrej Zary wrote:
Add support for VCT-jig parallel port I2C adapter to i2c-parport.
The adapter schematic can be found here (in the RAR file):
http://remont-aud.net/shop/22/desc/vct-jig-komplekt-dlja-samostojatelnoj-sborki
Signed-off-by: Ondrej Zary li...@rainbow-software.org
---
Documentation/i2c/busses/i2c-parport |1 +
drivers/i2c/busses/i2c-parport.h |8
2 files changed, 9 insertions(+)
diff --git a/Documentation/i2c/busses/i2c-parport
b/Documentation/i2c/busses/i2c-parport
index 0e2d17b..c3dbb3b 100644
--- a/Documentation/i2c/busses/i2c-parport
+++ b/Documentation/i2c/busses/i2c-parport
@@ -20,6 +20,7 @@ It currently supports the following devices:
* (type=5) Analog Devices evaluation boards: ADM1025, ADM1030, ADM1031
* (type=6) Barco LPT-DVI (K5800236) adapter
* (type=7) One For All JP1 parallel port adapter
+ * (type=8) VCT-jig
These devices use different pinout configurations, so you have to tell
the driver what you have, using the type module parameter. There is no
diff --git a/drivers/i2c/busses/i2c-parport.h
b/drivers/i2c/busses/i2c-parport.h
index 4e12945..84a6616 100644
--- a/drivers/i2c/busses/i2c-parport.h
+++ b/drivers/i2c/busses/i2c-parport.h
@@ -89,6 +89,13 @@ static const struct adapter_parm adapter_parm[] = {
.getsda = { 0x80, PORT_STAT, 1 },
.init = { 0x04, PORT_DATA, 1 },
},
+ /* type 8: VCT-jig */
+ {
+ .setsda = { 0x04, PORT_DATA, 1 },
+ .setscl = { 0x01, PORT_DATA, 1 },
+ .getsda = { 0x40, PORT_STAT, 0 },
+ .getscl = { 0x80, PORT_STAT, 1 },
+ },
};
static int type = -1;
@@ -103,4 +110,5 @@ MODULE_PARM_DESC(type,
5 = ADM1025, ADM1030 and ADM1031 evaluation boards\n
6 = Barco LPT-DVI (K5800236) adapter\n
7 = One For All JP1 parallel port adapter\n
+ 8 = VCT-jig\n
);
Looks good.
Reviewed-by: Jean Delvare jdelv...@suse.de
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] i2c-tools: Implement Module.mk for eeprog subdir
On Tue, 7 Jul 2015 17:22:33 +0200, Jean Delvare wrote: Hi Matwey, On Mon, 29 Jun 2015 21:14:47 +0300, Matwey V. Kornilov wrote: Move eeprog to separate subdir and implement Module.mk for it Signed-off-by: Matwey V. Kornilov matwey.korni...@gmail.com --- eeprog/24cXX.c | 185 eeprog/24cXX.h | 58 ++ eeprog/Module.mk | 67 eeprog/README.eeprog | 12 +++ eeprog/eeprog.8| 103 ++ eeprog/eeprog.c| 283 + eepromer/24cXX.c | 185 eepromer/24cXX.h | 58 -- eepromer/Makefile | 6 +- eepromer/README.eeprog | 12 --- eepromer/eeprog.8 | 103 -- eepromer/eeprog.c | 283 - 12 files changed, 710 insertions(+), 645 deletions(-) create mode 100644 eeprog/24cXX.c create mode 100644 eeprog/24cXX.h create mode 100644 eeprog/Module.mk create mode 100644 eeprog/README.eeprog create mode 100644 eeprog/eeprog.8 create mode 100644 eeprog/eeprog.c delete mode 100644 eepromer/24cXX.c delete mode 100644 eepromer/24cXX.h delete mode 100644 eepromer/README.eeprog delete mode 100644 eepromer/eeprog.8 delete mode 100644 eepromer/eeprog.c The patch does not apply to SVN, not sure how you generated it. No big deal, I get the idea, I'll redo it myself. Some SVN magic is needed to handle file moves properly anyway. All committed with some adjustments: http://lm-sensors.org/changeset/6305 http://lm-sensors.org/changeset/6306 http://lm-sensors.org/changeset/6307 http://lm-sensors.org/changeset/6308 Does that work for you? -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] i2c: add SMBus Host Notify support
On Tue, 7 Jul 2015 10:23:33 -0400, Benjamin Tissoires wrote:
Hi Jean,
On Jun 29 2015 or thereabouts, Jean Delvare wrote:
Hi Benjamin,
On Tue, 23 Jun 2015 14:58:18 -0400, Benjamin Tissoires wrote:
SMBus Host Notify allows a slave device to act as a master on a bus to
notify the host of an interrupt. The functionality is directly implemented
in the firmware so we just export a toggle function and rely on the
adapter to actually support this.
Why does it need to be toggled? I mean, if the controller supports it,
why don't we just enable the feature all the time?
That is definitively a solution. I however had once a problem with
suspend/resume where i2c_i801 was not able to reset the Host Notify
feature, but that was maybe a side effect of unloading/reloading the
module tons of times during the session.
I will work on that and submit a v2 this week.
I still wonder if you think we should expose a KConfig parameter for
this or not. I would prefer not to (because it is handled in hardware
and people might screw up their input drivers), but I'd prefer have your
opinion on this first.
What would the Kconfig option do exactly? As a general rule, the fewer
Kconfig options the better. We already have I2C_SMBUS for the SMBus
protocol specifics and Host Notify is one of these.
Signed-off-by: Benjamin Tissoires benjamin.tissoi...@redhat.com
---
Documentation/i2c/smbus-protocol | 4
drivers/i2c/i2c-core.c | 26 ++
include/linux/i2c.h | 8
include/uapi/linux/i2c.h | 1 +
4 files changed, 39 insertions(+)
diff --git a/Documentation/i2c/smbus-protocol
b/Documentation/i2c/smbus-protocol
index 6012b12..af4e4b9 100644
--- a/Documentation/i2c/smbus-protocol
+++ b/Documentation/i2c/smbus-protocol
@@ -199,6 +199,10 @@ alerting device's address.
[S] [HostAddr] [Wr] A [DevAddr] A [DataLow] A [DataHigh] A [P]
+I2C drivers for devices which can trigger SMBus Host Notify should call
+i2c_smbus_toggle_host_notify() to enable SMBUS Host Notify on the adapter
+and implement the optional alert() callback.
+
Packet Error Checking (PEC)
===
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 987c124..eaaf5b0 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -2959,6 +2959,32 @@ int i2c_slave_unregister(struct i2c_client *client)
EXPORT_SYMBOL_GPL(i2c_slave_unregister);
#endif
+int i2c_smbus_toggle_host_notify(struct i2c_client *client, bool state)
+{
+ int ret;
+
+ if (!client)
+ return -EINVAL;
+
+ if (!(client-flags I2C_CLIENT_TEN)) {
+ /* Enforce stricter address checking */
+ ret = i2c_check_addr_validity(client-addr);
+ if (ret)
+ return ret;
+ }
+
+ if (!client-adapter-algo-toggle_smbus_host_notify)
+ return -EOPNOTSUPP;
+
+ i2c_lock_adapter(client-adapter);
+ ret = client-adapter-algo-toggle_smbus_host_notify(client-adapter,
+ state);
+ i2c_unlock_adapter(client-adapter);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(i2c_smbus_toggle_host_notify);
+
As this is an SMBus feature, can't it go to drivers/i2c/i2c-smbus.c?
I asked myself this question, and I figured it was not possible because
i2c-smbus consists in the smbus_alert i2c driver.
After re-reading the code, I see now that it could definitively goes
there too. However, if we choose not to expose the toggle to the
drivers, then there will be no need to make deep changes in i2c-smbus
:).
The smbus_alert i2c driver was just the most simple way to implement
the SMBus Alert feature. It's not an actual device driver, nothing to
be afraid of.
(...)
Thanks for the review!
You're welcome. I wish I could do more but -ENOTIME :/
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] i2c: i801: add support of Host Notify
-ENOMEM.
I think priv-host_notify_fifo is leaked if i801_probe() fails later on.
+ return -ENOMEM;
+ }
+
if (priv-features FEATURE_IRQ) {
init_waitqueue_head(priv-waitq);
@@ -1292,6 +1418,9 @@ static void i801_remove(struct pci_dev *dev)
{
struct i801_priv *priv = pci_get_drvdata(dev);
+ cancel_work_sync(priv-host_notify);
+ kfifo_free(priv-host_notify_fifo);
+
i801_del_mux(priv);
i2c_del_adapter(priv-adapter);
pci_write_config_byte(dev, SMBHSTCFG, priv-original_hstcfg);
@@ -1315,8 +1444,12 @@ static int i801_suspend(struct pci_dev *dev,
pm_message_t mesg)
static int i801_resume(struct pci_dev *dev)
{
+ struct i801_priv *priv = pci_get_drvdata(dev);
+
pci_set_power_state(dev, PCI_D0);
pci_restore_state(dev);
+ if (priv-host_notify_requested)
+ i801_toggle_host_ntfy(priv-adapter, true);
return 0;
}
#else
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index eaaf5b0..87904ec 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -2145,6 +2145,40 @@ int i2c_master_send(const struct i2c_client *client,
const char *buf, int count)
EXPORT_SYMBOL(i2c_master_send);
/**
+ * i2c_alert - call an alert for the given i2c_client.
+ * @client: Handle to slave device
+ * @data: Payload data that will be sent to the slave
Actually this is data that was received from the slave device
(temporarily acting as a master), if I understand how Host Notify works.
+ *
+ * Returns negative errno, or 0.
+ */
+int i2c_alert(struct i2c_client *client, unsigned int data)
+{
+ struct i2c_driver *driver;
+
+ if (!client)
+ return -EINVAL;
Do you actually need to check for that? It seems redundant with the
check in smbus_do_alert().
+
+ /*
+ * Drivers should either disable alerts, or provide at least
+ * a minimal handler. Lock so the driver won't change.
+ */
+ device_lock(client-adapter-dev);
+ if (client-dev.driver) {
+ driver = to_i2c_driver(client-dev.driver);
+ if (driver-alert)
+ driver-alert(client, data);
So you use the same driver callback for SMBus Alert and SMBus Host
Notify. This makes some sense, but if a given driver supports both, how
does it know which event happened? The data is completely different and
most probably the action required from the driver as well.
+ else
+ dev_warn(client-dev, no driver alert()!\n);
+ } else
+ dev_dbg(client-dev, alert with no driver\n);
+ device_unlock(client-adapter-dev);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(i2c_alert);
+
+
Please avoid duplicate blank lines.
+/**
* i2c_master_recv - issue a single I2C message in master receive mode
* @client: Handle to slave device
* @buf: Where to store data read from slave
diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
index 9ebf9cb..26439a8 100644
--- a/drivers/i2c/i2c-smbus.c
+++ b/drivers/i2c/i2c-smbus.c
@@ -41,27 +41,14 @@ static int smbus_do_alert(struct device *dev, void *addrp)
{
struct i2c_client *client = i2c_verify_client(dev);
struct alert_data *data = addrp;
- struct i2c_driver *driver;
if (!client || client-addr != data-addr)
return 0;
if (client-flags I2C_CLIENT_TEN)
return 0;
- /*
- * Drivers should either disable alerts, or provide at least
- * a minimal handler. Lock so the driver won't change.
- */
- device_lock(dev);
- if (client-dev.driver) {
- driver = to_i2c_driver(client-dev.driver);
- if (driver-alert)
- driver-alert(client, data-flag);
- else
- dev_warn(client-dev, no driver alert()!\n);
- } else
- dev_dbg(client-dev, alert with no driver\n);
- device_unlock(dev);
+ if (i2c_alert(client, data-flag))
+ return 0;
/* Stop iterating after we find the device */
return -EBUSY;
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 7ffc970..fbca48a 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -72,6 +72,9 @@ extern int i2c_transfer(struct i2c_adapter *adap, struct
i2c_msg *msgs,
extern int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
int num);
+/* call an alert. */
What a vague comment :(
+extern int i2c_alert(struct i2c_client *client, unsigned int data);
+
/* This is the very generalized SMBus access routine. You probably do not
want to use this, though; one of the functions below may be much easier,
and probably just as fast.
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info
Re: [RFC] i2c-tools: allow linker and compile flags from command line
Hi Wolfram, On Fri, 19 Jun 2015 11:24:32 +0200, Wolfram Sang wrote: Mainly for debugging, so one can pass stuff like -O0 or -static. Signed-off-by: Wolfram Sang w...@the-dreams.de --- tools/Module.mk | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/Module.mk b/tools/Module.mk index 8efddbb..641ac81 100644 --- a/tools/Module.mk +++ b/tools/Module.mk @@ -9,13 +9,13 @@ TOOLS_DIR:= tools -TOOLS_CFLAGS := -Wstrict-prototypes -Wshadow -Wpointer-arith -Wcast-qual \ +TOOLS_CFLAGS += -Wstrict-prototypes -Wshadow -Wpointer-arith -Wcast-qual \ -Wcast-align -Wwrite-strings -Wnested-externs -Winline \ -W -Wundef -Wmissing-prototypes -Iinclude ifeq ($(USE_STATIC_LIB),1) -TOOLS_LDFLAGS:= $(LIB_DIR)/$(LIB_STLIBNAME) +TOOLS_LDFLAGS+= $(LIB_DIR)/$(LIB_STLIBNAME) else -TOOLS_LDFLAGS:= -Llib -li2c +TOOLS_LDFLAGS+= -Llib -li2c Note that -Llib has become -L$(LIB_DIR) meanwhile. endif TOOLS_TARGETS:= i2cdetect i2cdump i2cset i2cget You can already overwrite CFLAGS and LDFLAGS on the command line, and this works for the whole package. Do you really need to set different CFLAGS (or LDFLAGS) for different parts of i2c-tools? Seems overkill to me. If you really need that then this should be done consistently for all modules: not just tools but also lib and now eeprog. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] i2c: i801: add support of Host Notify
On Tue, 7 Jul 2015 16:16:38 -0400, Benjamin Tissoires wrote: On Jul 07 2015 or thereabouts, Jean Delvare wrote: So you use the same driver callback for SMBus Alert and SMBus Host Notify. This makes some sense, but if a given driver supports both, how does it know which event happened? The data is completely different and most probably the action required from the driver as well. Yeah, this gets messy. I re-used the .alert() callback because of the documentation: Alert callback, for example for the SMBus alert protocol. It would seem that the alert is generic and could be re-used. But OTOH, it is not prepared to receive anything else than a SMBus Alert. Given that I had a toggle_host_notify() call, I figured that this was not a problem unless you write a driver which implements both (I can not find a sane use case for this though). But now that this call has disappeared, we would need a way to differentiate the too of them. I can see two solutions out of my head right now: - add a protocol parameter (with an enum) to .alert() - add a new callback .host_notify() in struct i2c_driver. I came to the same conclusion. I think I like the second option more given that it will allow to not touch the current code in i2c_smbus. No strong preference so do it the way you prefer. Thanks, -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] i2c-tools: Implement Module.mk for eeprog subdir
Hi Matwey, On Mon, 29 Jun 2015 21:14:47 +0300, Matwey V. Kornilov wrote: Move eeprog to separate subdir and implement Module.mk for it Signed-off-by: Matwey V. Kornilov matwey.korni...@gmail.com --- eeprog/24cXX.c | 185 eeprog/24cXX.h | 58 ++ eeprog/Module.mk | 67 eeprog/README.eeprog | 12 +++ eeprog/eeprog.8| 103 ++ eeprog/eeprog.c| 283 + eepromer/24cXX.c | 185 eepromer/24cXX.h | 58 -- eepromer/Makefile | 6 +- eepromer/README.eeprog | 12 --- eepromer/eeprog.8 | 103 -- eepromer/eeprog.c | 283 - 12 files changed, 710 insertions(+), 645 deletions(-) create mode 100644 eeprog/24cXX.c create mode 100644 eeprog/24cXX.h create mode 100644 eeprog/Module.mk create mode 100644 eeprog/README.eeprog create mode 100644 eeprog/eeprog.8 create mode 100644 eeprog/eeprog.c delete mode 100644 eepromer/24cXX.c delete mode 100644 eepromer/24cXX.h delete mode 100644 eepromer/README.eeprog delete mode 100644 eepromer/eeprog.8 delete mode 100644 eepromer/eeprog.c The patch does not apply to SVN, not sure how you generated it. No big deal, I get the idea, I'll redo it myself. Some SVN magic is needed to handle file moves properly anyway. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] i2c: add SMBus Host Notify support
(I2C_FUNC_SMBUS_READ_BYTE | \ I2C_FUNC_SMBUS_WRITE_BYTE) -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c-tools: Implement Module.mk for eepromer subdir
Hi Matwey, On Sat, 27 Jun 2015 11:49:07 +0300, Matwey V. Kornilov wrote: eepromer/Module.mk has to be present in order to do make EXTRA=eepromer The problem with the eepromer directory is that it's a mess. There are 3 tools there which do basically the same (programming EEPROMs), but each has its own limitations: * eeprom can't deal with 16-bit addressed EEPROMs nor SMBus-only controllers. * eepromer can't deal with 8-bit addressed EEPROMs nor SMBus-only controllers. * eeprog only uses SMBus byte/word reads and writes, it lacks support for raw I2C block writes so it is slow on large EEPROMs. On top of that, eeprom and eepromer build with a lot of warnings. I really don't want to maintain 3 different tools with the same purpose. eeprog seems to be the most promising of all 3, as it is fully functional, but I don't feel comfortable dropping the other 2 until block write support is added to eeprog. Would you be interested in adding block write support to eeprog? It has been on my to-do list for a long time but I could never find the time to actually look into it. I do not use these tools myself so my personal interest is low and this is why it never makes it to the top of my to-do list. Signed-off-by: Matwey V. Kornilov matwey.korni...@gmail.com --- --- /dev/null +++ i2c-tools-3.1.1/eepromer/Module.mk @@ -0,0 +1,79 @@ +# eepromer +# +# 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. + +EEPROMER_DIR := eepromer + +EEPROMER_CFLAGS := -O2 -Iinclude -Wall +ifeq ($(USE_STATIC_LIB),1) +EEPROMER_LDFLAGS := $(LIB_DIR)/$(LIB_STLIBNAME) +else +EEPROMER_LDFLAGS := -L$(LIB_DIR) -li2c +endif This will link eepromer and eeprom with libi2c even though they don't need it, right? + +EEPROMER_TARGETS := eepromer eeprom eeprog + +# +# Programs +# + +$(EEPROMER_DIR)/eepromer: $(EEPROMER_DIR)/eepromer.o + $(CC) $(LDFLAGS) -o $@ $^ $(EEPROMER_LDFLAGS) + +$(EEPROMER_DIR)/eeprom: $(EEPROMER_DIR)/eeprom.o + $(CC) $(LDFLAGS) -o $@ $^ $(EEPROMER_LDFLAGS) + +$(EEPROMER_DIR)/eeprog: $(EEPROMER_DIR)/eeprog.o $(EEPROMER_DIR)/24cXX.o + $(CC) $(LDFLAGS) -o $@ $^ $(EEPROMER_LDFLAGS) + +# +# Objects +# + +$(EEPROMER_DIR)/eepromer.o: $(EEPROMER_DIR)/eepromer.c + $(CC) $(CFLAGS) $(EEPROMER_CFLAGS) -c $ -o $@ + +$(EEPROMER_DIR)/eeprom.o: $(EEPROMER_DIR)/eeprom.c + $(CC) $(CFLAGS) $(EEPROMER_CFLAGS) -c $ -o $@ + +$(EEPROMER_DIR)/eeprog.o: $(EEPROMER_DIR)/eeprog.c $(EEPROMER_DIR)/24cXX.h + $(CC) $(CFLAGS) $(EEPROMER_CFLAGS) -c $ -o $@ + +$(EEPROMER_DIR)/24cXX.o: $(EEPROMER_DIR)/24cXX.c $(EEPROMER_DIR)/24cXX.h Also depends on $(INCLUDE_DIR)/i2c/smbus.h. + $(CC) $(CFLAGS) $(EEPROMER_CFLAGS) -c $ -o $@ + +# +# Commands +# + +all-eepromer: $(addprefix $(EEPROMER_DIR)/,$(EEPROMER_TARGETS)) + +strip-eepromer: $(addprefix $(EEPROMER_DIR)/,$(EEPROMER_TARGETS)) + strip $(addprefix $(EEPROMER_DIR)/,$(EEPROMER_TARGETS)) + +clean-eepromer: + $(RM) $(addprefix $(EEPROMER_DIR)/,*.o $(EEPROMER_TARGETS)) + +install-eepromer: $(addprefix $(EEPROMER_DIR)/,$(EEPROMER_TARGETS)) + $(INSTALL_DIR) $(DESTDIR)$(sbindir) $(DESTDIR)$(man8dir) + for program in $(EEPROMER_TARGETS) ; do \ + $(INSTALL_PROGRAM) $(EEPROMER_DIR)/$$program $(DESTDIR)$(sbindir) ; \ + $(INSTALL_DATA) $(EEPROMER_DIR)/$$program.8 $(DESTDIR)$(man8dir) ; done + +uninstall-eepromer: + for program in $(EEPROMER_TARGETS) ; do \ + $(RM) $(DESTDIR)$(sbindir)/$$program ; \ + $(RM) $(DESTDIR)$(man8dir)/$$program.8 ; done + +all: all-eepromer + +strip: strip-eepromer + +clean: clean-eepromer + +install: install-eepromer + +uninstall: uninstall-eepromer I don't think it makes sense to integrate eeprom and eepromer in the main build system if the plan is to get rid of them. I'd rather only integrate eeprog, so that distributions ship only that and everybody uses only that. Maybe eeprog should be moved to its own directory, and that new directory would be integrated into the build system while the rest of the eepromer directory would be left alone (and ultimately deleted.) -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: lm-sensor.org is down
Hi Angelo, Le Friday 19 June 2015 à 08:58 +0200, Angelo Compagnucci a écrit : lm-sensorg.org is down: http://isup.me/www.lm-sensor.org It's not just you! http://www.lm-sensor.org looks down from here. I've noticed it too. Hopefully the admin (Axel Thimm) has noticed as well and is already looking at the problem. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] i2c-tools: enable static use of libi2c
On Wed, 17 Jun 2015 21:26:45 +0200, Wolfram Sang wrote: From: Wolfram Sang wsa+rene...@sang-engineering.com When debugging embedded systems, it is often nice to simply TFTP the desired i2ctool to the target without the hazzle of dealing with shared libs. Using -static is overkill, too, so let's add a switch which will only link functions from libi2c statically. Signed-off-by: Wolfram Sang wsa+rene...@sang-engineering.com --- Makefile| 4 tools/Module.mk | 4 2 files changed, 8 insertions(+) (...) Applied, thanks! -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: i2c-tools: add Android.mk
On Wed, 17 Jun 2015 12:38:54 +0200, Angelo Compagnucci wrote: Hi Wolfram, 2015-06-17 12:29 GMT+02:00 Wolfram Sang w...@the-dreams.de: Now if anyone has a more educated view on this than I do, please speak up. Android is really not my thing (as a developer, I mean.) While working again on adding i2ctransfer to i2c-tools, it became clear that this android makefile would need an update for it, too. And since nobody of us is using i2c-tools on android, nobody can provide a tested update. Nobody but me! Actually I'm using it and I had the need for it. Probably there are other people reinventing the wheel outside this mailinglist. Try to google for it. We're not saying the immediate need doesn't exist. We're only questioning whether the i2c-tools repository is the right place for the extra file. This questioning is reinforced by the fact that I never saw any Andoid.mk file in any other upstream project I'm working on. It really reminds me of rpm spec files or Debian-specific files hosted by upstream projects. My experience is that these files bitrot over time because the upstream maintainers don't need them and often can't test them. They are better maintained somewhere else. (And then again I don't understand why they can't just use make as everybody else.) So, it would indeed be easier if the android universe would figure a way in their sphere. So, I agree with Jean. I think it's more a matter of a makefile approach. They use a non recursive one (recommended by several papers[1]) and it's up to i2c-tools's Makefile is not recursive. In fact it's derived from lm-sensors' Makefile, which itself was designed based on the paper Recursive Make Considered Harmful - the one you just mentioned. Ah, irony ;-) i2c-tools's Makefile is modular. Not recursive. external projects if embrace it or no. Honestly I think that a minuscule file with really low maintenance should be added. I'm glad to prepare a new patch if maintainership is interested! I'm not. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c-tools: enable static use of libi2c
Hallo Wolfram, On Tue, 16 Jun 2015 04:01:19 +0200, Wolfram Sang wrote: From: Wolfram Sang wsa+rene...@sang-engineering.com When debugging embedded systems, it is often nice to simply TFTP the desired i2ctool to the target without the hazzle of dealing with shared libs. Using -static is overkill, too, so let's add a switch which will only link functions from libi2c statically. Fine with me. Signed-off-by: Wolfram Sang wsa+rene...@sang-engineering.com --- Makefile| 2 ++ tools/Module.mk | 4 2 files changed, 6 insertions(+) diff --git a/Makefile b/Makefile index 252a126..6d36f2f 100644 --- a/Makefile +++ b/Makefile @@ -33,6 +33,8 @@ CFLAGS += -Wall SOCFLAGS := -fpic -D_REENTRANT $(CFLAGS) BUILD_STATIC_LIB ?= 1 +# Uncomment to use static libi2c +#USE_STATIC_LIB := 1 Any reason for not using ?= as above, with the default being unset? That way BUILD_STATIC_LIB and USE_STATIC_LIB can both be controlled using the same mechanism, and can be changed from their default value on the command line (without patching the Makefile.) KERNELVERSION:= $(shell uname -r) diff --git a/tools/Module.mk b/tools/Module.mk index d14bb0c..8efddbb 100644 --- a/tools/Module.mk +++ b/tools/Module.mk @@ -12,7 +12,11 @@ TOOLS_DIR := tools TOOLS_CFLAGS := -Wstrict-prototypes -Wshadow -Wpointer-arith -Wcast-qual \ -Wcast-align -Wwrite-strings -Wnested-externs -Winline \ -W -Wundef -Wmissing-prototypes -Iinclude +ifeq ($(USE_STATIC_LIB),1) +TOOLS_LDFLAGS:= $(LIB_DIR)/$(LIB_STLIBNAME) +else TOOLS_LDFLAGS:= -Llib -li2c +endif Unrelated to your patch, but shouldn't this -Llib rather been written -L$(LIB_DIR)? Also it might make sense to check if USE_STATIC_LIB is set when BUILD_STATIC_LIB isn't and complain about it? TOOLS_TARGETS:= i2cdetect i2cdump i2cset i2cget -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Py-smbus and python3 support: it's time for a new release?
On Wed, 17 Jun 2015 11:23:38 +0200, Angelo Compagnucci wrote: Please, please, find that time! I really would like to package i2c-tools 3.1 with python3 support in buildroot! OK, releasing i2c-tools 3.1.2 was easy after all, it's done now. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Py-smbus and python3 support: it's time for a new release?
Hi Angelo, On Tue, 26 May 2015 09:56:47 +0200, Angelo Compagnucci wrote: Dear Jean Delvare, Now that python3 support was added to python-smbus, could you release a new stable version with it? This way, py-smbus with python3 support could be included in distributions that usually don't pick unstable packages. I understand that distributions don't want to ship unstable packages, however they can (and do) cherry-pick individual commits to fix bugs or add features they want. If you have a specific need, you can always ask your distribution to consider backporting a few commits. In this specific case, there's a dedicated stable branch for i2c-tools 3.1 so it's even easier. Is there any planned roadmap or showstopper that makes this thing difficult? The main blocker is that I'm always so busy and can never find the time for the release. Another problem is that it's the first release which will include libi2c, and with that the first API will be set. After that, it's harder to change it I got it wrong for any reason. I had plans to extend the API a bit too (basically merging i2cbusses into the library.) I guess that's the reason why I'm always delaying the release. But to be honest, I suppose that no problem will be reported until the library is widely used, and that just can't happen until it's part of a stable release. So it's a kind of chicken-and-egg problem. For the legacy branch (3.1), there's no libi2c excuse, and I did consider a release after adding support for python3, but canceled the idea after seeing only 2 lines in the CHANGES file. That being said, there's so little happening on that branch that maybe it's expected that only a couple fixes will make it into every release. I'm adding release i2c-tools to my formal to-do list [1], but I can't promise anything regarding the schedule. [1] A sheet of paper on my desk, nothing fancy, really. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c-tools: enable static use of libi2c
On Wed, 17 Jun 2015 12:53:09 +0200, Wolfram Sang wrote: Hi Jean, When debugging embedded systems, it is often nice to simply TFTP the desired i2ctool to the target without the hazzle of dealing with shared libs. Using -static is overkill, too, so let's add a switch which will only link functions from libi2c statically. Fine with me. Hooray! BUILD_STATIC_LIB ?= 1 +# Uncomment to use static libi2c +#USE_STATIC_LIB := 1 Any reason for not using ?= as above, with the default being unset? Agreed. Unrelated to your patch, but shouldn't this -Llib rather been written -L$(LIB_DIR)? Yes, makes sense. OK, I'll fix it after applying your updated patch. Also it might make sense to check if USE_STATIC_LIB is set when BUILD_STATIC_LIB isn't and complain about it? For easier usage, I'd rather enforce BUILD_STATIC_LIB in that case. You mean that USE_STATIC_LIB = 1 would silently imply BUILD_STATIC_LIB = 1? Yes, that's much better than my proposal. I'm wondering if we should make it even more flexible, but I'm not sure if it's worth the effort... Are you calling make install after that? I guess not. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 2/3] i2c-piix4: Use Macro for AMD CZ SMBus device ID
On Thu, 11 Jun 2015 20:11:46 +0800, Wan ZongShun wrote:
Change AMD CZ SMBUS device ID from 0x790b to
use Macro definition
Signed-off-by: Wan ZongShun vincent@amd.com
---
drivers/i2c/busses/i2c-piix4.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 67cbec6..630bce6 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -245,7 +245,7 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
PIIX4_dev-device == PCI_DEVICE_ID_AMD_HUDSON2_SMBUS
PIIX4_dev-revision = 0x41) ||
(PIIX4_dev-vendor == PCI_VENDOR_ID_AMD
- PIIX4_dev-device == 0x790b
+ PIIX4_dev-device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS
PIIX4_dev-revision = 0x49))
smb_en = 0x00;
else
@@ -545,7 +545,7 @@ static const struct pci_device_id piix4_ids[] = {
{ PCI_DEVICE(PCI_VENDOR_ID_ATI, PCI_DEVICE_ID_ATI_IXP400_SMBUS) },
{ PCI_DEVICE(PCI_VENDOR_ID_ATI, PCI_DEVICE_ID_ATI_SBX00_SMBUS) },
{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_HUDSON2_SMBUS) },
- { PCI_DEVICE(PCI_VENDOR_ID_AMD, 0x790b) },
+ { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) },
{ PCI_DEVICE(PCI_VENDOR_ID_SERVERWORKS,
PCI_DEVICE_ID_SERVERWORKS_OSB4) },
{ PCI_DEVICE(PCI_VENDOR_ID_SERVERWORKS,
Probably too late but anyway:
Acked-by: Jean Delvare jdelv...@suse.de
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH i2c-tools] decode-dimms: correctly check for out-of-bounds vendor id
Hi Lubomir,
On Sat, 30 May 2015 13:03:32 +0200, Lubomir Rintel wrote:
---
eeprom/decode-dimms | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/eeprom/decode-dimms b/eeprom/decode-dimms
index 3a50c07..4a861bd 100755
--- a/eeprom/decode-dimms
+++ b/eeprom/decode-dimms
@@ -345,7 +345,7 @@ sub manufacturer_ddr3($$)
my $manufacturer;
return Invalid if parity($code) != 1;
- return Unknown if ($code 0x7F) - 1 $vendors[$count 0x7F];
+ return Unknown if ($code 0x7F) @{$vendors[$count 0x7F]};
$manufacturer = $vendors[$count 0x7F][($code 0x7F) - 1];
$manufacturer =~ s/ \(former .*\)$// if $opt_side_by_side;
$manufacturer .= ? (Invalid parity) if parity($count) != 1;
Good catch, not sure how we managed to not notice this bug for years
despite the surrounding code being modified several times. Did you find
it by code inspection or did you actually hit the bug on one of your
memory modules?
Note that I would prefer
... if ($code 0x7F) - 1 = @{$vendors[$count 0x7F]};
It is equivalent but more obviously correct as it matches the array
access on the following line.
Also, even after your fix, I think the code is not completely safe, as
we still don't check for ($code 0x7F) being 0, nor do we verify that
$count 0x7F does not exceed the number of elements in @vendors. I'll
fix that.
Thanks,
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH i2c-tools] decode-dimms: Complete check for out-of-bounds vendor ID
We also need to discard vendor ID 0 and to report Unknown if the
vendor is in a page we don't support yet.
---
Lubomir, this goes on top of your (modified) patch. Does it look good
to you? Thanks.
eeprom/decode-dimms |6 --
1 file changed, 4 insertions(+), 2 deletions(-)
--- i2c-tools.orig/eeprom/decode-dimms 2015-06-01 08:19:59.292190666 +0200
+++ i2c-tools/eeprom/decode-dimms 2015-06-01 08:35:31.806092421 +0200
@@ -344,8 +344,10 @@ sub manufacturer_ddr3($$)
my ($count, $code) = @_;
my $manufacturer;
- return Invalid if parity($code) != 1;
- return Unknown if ($code 0x7F) - 1 = @{$vendors[$count 0x7F]};
+ return Invalid if parity($code) != 1
+or ($code 0x7F) == 0;
+ return Unknown if ($count 0x7F) = @vendors
+or ($code 0x7F) - 1 = @{$vendors[$count 0x7F]};
$manufacturer = $vendors[$count 0x7F][($code 0x7F) - 1];
$manufacturer =~ s/ \(former .*\)$// if $opt_side_by_side;
$manufacturer .= ? (Invalid parity) if parity($count) != 1;
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2 i2c-tools] sensors-detect: Manufacturer name changes from JEP106AQ
5 manufacturer names were updated from JEP106AK to JEP106AQ. --- eeprom/decode-dimms | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) --- i2c-tools.orig/eeprom/decode-dimms 2015-06-01 09:19:01.931673815 +0200 +++ i2c-tools/eeprom/decode-dimms 2015-06-01 09:54:06.958469718 +0200 @@ -60,7 +60,7 @@ $revision =~ s/ \([^()]*\)//; RCA, Raytheon, Conexant (Rockwell), Seeq, NXP (former Signetics, Philips Semi.), Synertek, Texas Instruments, Toshiba, Xicor, Zilog, Eurotechnique, Mitsubishi, - Lucent (ATT), Exel, Atmel, SGS/Thomson, + Lucent (ATT), Exel, Atmel, STMicroelectronics (former SGS/Thomson), Lattice Semi., NCR, Wafer Scale Integration, IBM, Tristar, Visic, Intl. CMOS Technology, SSSI, MicrochipTechnology, Ricoh Ltd., VLSI, Micron Technology, @@ -99,7 +99,7 @@ $revision =~ s/ \([^()]*\)//; Century, Hal Computers, Rohm Company Ltd., Juniper Networks, Libit Signal Processing, Mushkin Enhanced Memory, Tundra Semiconductor, Adaptec Inc., LightSpeed Semi., ZSP Corp., AMIC Technology, Adobe Systems, - Dynachip, PNY Electronics, Newport Digital, MMC Networks, + Dynachip, PNY Technologies Inc. (former PNY Electronics), Newport Digital, MMC Networks, T Square, Seiko Epson, Broadcom, Viking Components, V3 Semiconductor, Flextronics (former Orbit), Suwa Electronics, Transmeta, Micron CMS, American Computer Digital Components Inc, Enhance 3000 Inc, Tower Semiconductor, @@ -127,7 +127,7 @@ $revision =~ s/ \([^()]*\)//; HADCO Corporation, Corsair, Actrans System Inc., ALPHA Technologies, Silicon Laboratories, Inc. (Cygnal), Artesyn Technologies, Align Manufacturing, Peregrine Semiconductor, Chameleon Systems, Aplus Flash Technology, MIPS Technologies, Chrysalis ITS, - ADTEC Corporation, Kentron Technologies, Win Technologies, Tachyon Semiconductor (former ASIC Designs Inc.), + ADTEC Corporation, Kentron Technologies, Win Technologies, Tezzaron Semiconductor (former Tachyon Semiconductor), Extreme Packet Devices, RF Micro Devices, Siemens AG, Sarnoff Corporation, Itautec SA (former Itautec Philco SA), Radiata Inc., Benchmark Elect. (AVEX), Legend, SpecTek Incorporated, Hi/fn, Enikia Incorporated, SwitchOn Networks, @@ -177,7 +177,7 @@ $revision =~ s/ \([^()]*\)//; SiCon Video, NanoAmp Solutions, Ericsson Technology, PrairieComm, Mitac International, Layer N Networks, MtekVision, Allegro Networks, Marvell Semiconductors, Netergy Microelectronic, NVIDIA, Internet Machines, - Peak Electronics, Litchfield Communication, Accton Technology, Teradiant Networks, + Memorysolution GmbH (former Peak Electronics), Litchfield Communication, Accton Technology, Teradiant Networks, Scaleo Chip (former Europe Technologies), Cortina Systems, RAM Components, Raqia Networks, ClearSpeed, Matsushita Battery, Xelerated, SimpleTech, Utron Technology, Astec International, AVM gmbH, Redux Communications, @@ -189,7 +189,7 @@ $revision =~ s/ \([^()]*\)//; Potentia Power Systems, C-guys Incorporated, Digital Communications Technology Incorporated, Silicon-Based Technology, Fulcrum Microsystems, Positivo Informatica Ltd, XIOtech Corporation, PortalPlayer, Zhiying Software, Parker Vision, Inc. (former Direct2Data), Phonex Broadband, Skyworks Solutions, - Entropic Communications, Pacific Force Technology, Zensys A/S, Legend Silicon Corp., + Entropic Communications, I'M Intelligent Memory Ltd (former Pacific Force Technology), Zensys A/S, Legend Silicon Corp., sci-worx GmbH, SMSC (former Oasis Silicon Systems), Renesas Electronics (former Renesas Technology), Raza Microelectronics, Phyworks, MediaTek, Non-cents Productions, US Modular, Wintegra Ltd, Mathstar, StarCore, Oplus Technologies, -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/2 i2c-tools] decode-dimms: Complete check for out-of-bounds vendor ID
We also need to discard vendor ID 0 and to report Unknown if the
vendor is in a page we don't support yet.
Additionally, check that data is present at all for DDR3 modules as we
already do for pre-DDR3 modules.
---
Changes since v1:
* Added check for data presence.
eeprom/decode-dimms |8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
--- i2c-tools.orig/eeprom/decode-dimms 2015-06-01 08:19:59.292190666 +0200
+++ i2c-tools/eeprom/decode-dimms 2015-06-01 09:14:30.899898763 +0200
@@ -344,8 +344,12 @@ sub manufacturer_ddr3($$)
my ($count, $code) = @_;
my $manufacturer;
- return Invalid if parity($code) != 1;
- return Unknown if ($code 0x7F) - 1 = @{$vendors[$count 0x7F]};
+ return Undefined unless spd_written($count, $code);
+
+ return Invalid if parity($code) != 1
+or ($code 0x7F) == 0;
+ return Unknown if ($count 0x7F) = @vendors
+or ($code 0x7F) - 1 = @{$vendors[$count 0x7F]};
$manufacturer = $vendors[$count 0x7F][($code 0x7F) - 1];
$manufacturer =~ s/ \(former .*\)$// if $opt_side_by_side;
$manufacturer .= ? (Invalid parity) if parity($count) != 1;
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2 i2c-tools] sensors-detect: Refactor manufacturer decoding
Instead of reencoding pre-DDR3 manufacturer code to DDR3 manufacturer
code, move the common decoding to a separate function.
---
eeprom/decode-dimms | 28 +---
1 file changed, 17 insertions(+), 11 deletions(-)
--- i2c-tools.orig/eeprom/decode-dimms 2015-06-01 09:14:30.899898763 +0200
+++ i2c-tools/eeprom/decode-dimms 2015-06-01 09:19:01.931673815 +0200
@@ -336,6 +336,21 @@ sub parity($)
return ($parity 1);
}
+# The code byte includes parity, the count byte does not.
+sub manufacturer_common($$)
+{
+ my ($count, $code) = @_;
+ my $manufacturer;
+
+ return Invalid if parity($code) != 1
+or ($code = 0x7F) == 0;
+ return Unknown if $count = @vendors
+or $code - 1 = @{$vendors[$count]};
+ $manufacturer = $vendors[$count][$code - 1];
+ $manufacturer =~ s/ \(former .*\)$// if $opt_side_by_side;
+ return $manufacturer;
+}
+
# New encoding format (as of DDR3) for manufacturer just has a count of
# leading 0x7F rather than all the individual bytes. The count bytes includes
# parity!
@@ -346,12 +361,7 @@ sub manufacturer_ddr3($$)
return Undefined unless spd_written($count, $code);
- return Invalid if parity($code) != 1
-or ($code 0x7F) == 0;
- return Unknown if ($count 0x7F) = @vendors
-or ($code 0x7F) - 1 = @{$vendors[$count 0x7F]};
- $manufacturer = $vendors[$count 0x7F][($code 0x7F) - 1];
- $manufacturer =~ s/ \(former .*\)$// if $opt_side_by_side;
+ $manufacturer = manufacturer_common($count 0x7F, $code);
$manufacturer .= ? (Invalid parity) if parity($count) != 1;
return $manufacturer;
}
@@ -369,11 +379,7 @@ sub manufacturer(@)
}
return (Invalid, []) unless defined $first;
- return (Invalid, [$first, @bytes]) if parity($first) != 1;
- if (parity($ai) == 0) {
- $ai |= 0x80;
- }
- return (manufacturer_ddr3($ai, $first), \@bytes);
+ return (manufacturer_common($ai, $first), \@bytes);
}
sub manufacturer_data(@)
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2 i2c-tools] decode-dimms: New manufacturer names from JEP106AQ
--- eeprom/decode-dimms | 37 - 1 file changed, 36 insertions(+), 1 deletion(-) --- i2c-tools.orig/eeprom/decode-dimms 2015-06-01 09:33:11.965773808 +0200 +++ i2c-tools/eeprom/decode-dimms 2015-06-01 09:47:59.001646064 +0200 @@ -302,7 +302,42 @@ $revision =~ s/ \([^()]*\)//; High Bridge Solutions Industria Eletronica, Transcend Technology Co. Ltd., Everspin Technologies, Hon-Hai Precision, Smart Storage Systems, Toumaz Group, Zentel Electronics Corporation, Panram International Corporation, - Silicon Space Technology] + Silicon Space Technology, LITE-ON IT Corporation, Inuitive, HMicro, + BittWare Inc., GLOBALFOUNDRIES, ACPI Digital Co. Ltd, Annapurna Labs, + AcSiP Technology Corporation, Idea! Electronic Systems, Gowe Technology Co. Ltd, + Hermes Testing Solutions Inc., Positivo BGH, Intelligence Silicon Technology], +[3D PLUS, Diehl Aerospace, Fairchild, Mercury Systems, + Sonics Inc., GE Intelligent Platforms GmbH Co., Shenzhen Jinge Information Co. Ltd, + SCWW, Silicon Motion Inc., Anurag, King Kong, + FROM30 Co. Ltd, Gowin Semiconductor Corp, Fremont Micro Devices Ltd, + Ericsson Modems, Exelis, Satixfy Ltd, Galaxy Microsystems Ltd, + Gloway International Co. Ltd, Lab, Smart Energy Instruments, + Approved Memory Corporation, Axell Corporation, ISD Technology Limited, + Phytium, Xi'an SinoChip Semiconductor, Ambiq Micro, eveRAM Technology, Inc., + Infomax, Butterfly Network, Inc., Shenzhen City Gcai Electronics, + Stack Devices Corporation, ADK Media Group, TSP Global Co. Ltd, + HighX, Shenzhen Elicks Technology, ISSI/Chingis, Google Inc., + Dasima International Development, Leahkinn Technology Limited, + HIMA Paul Hildebrandt GmbH Co KG, Keysight Technologies, + Techcomp International (Fastable), Ancore Technology Corporation, + Nuvoton, Korea Uhbele International Group Ltd, Ikegami Tsushinki Co. Ltd, + RelChip Inc., Baikal Electronics, Nemostech Inc., + Memorysolution GmbH, Silicon Integrated Systems Corporation, + Xiede, Multilaser Components, Flash Chi, Jone, + GCT Semiconductor Inc., Hong Kong Zetta Device Technology, + Unimemory Technology(s) Pte Ltd, Cuso, Kuso, + Uniquify Inc., Skymedi Corporation, Core Chance Co. Ltd, + Tekism Co. Ltd, Seagate Technology PLC, Hong Kong Gaia Group Co. Limited, + Gigacom Semiconductor LLC, V2 Technologies, TLi, Neotion, + Lenovo, Shenzhen Zhongteng Electronic Corp. Ltd, Compound Photonics, + Cognimem Technologies, Inc., Shenzhen Pango Microsystems Co. Ltd, + Vasekey, Cal-Comp Industria de Semicondutores, Eyenix Co. Ltd, + Heoriady, Accelerated Memory Production Inc., INVECAS Inc., + AP Memory, Douqi Technology, Etron Technology Inc., + Indie Semiconductor, Socionext Inc., HGST, EVGA, + Audience Inc., EpicGear, Vitesse Enterprise Co., + Foxtronn International Corporation, Bretelon Inc., + Zbit Semiconductor Inc.] ); $use_sysfs = -d '/sys/bus'; -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] i2c-parport: use new parport device model
On Wed, 20 May 2015 22:44:52 +0530, Sudip Mukherjee wrote:
On Wed, May 20, 2015 at 05:49:07PM +0200, Wolfram Sang wrote:
On Wed, May 20, 2015 at 08:57:00PM +0530, Sudip Mukherjee wrote:
static struct parport_driver i2c_parport_driver = {
- .name = i2c-parport,
- .attach = i2c_parport_attach,
- .detach = i2c_parport_detach,
+ .name = i2c-parport,
+ .match_port = i2c_parport_attach,
+ .detach = i2c_parport_detach,
+ .devmodel = true,
Minor nit: I prefer to not use tabs but a single space after the struct
member names. Less hazzle in the future and still readable IMO.
It was having space originally. I changed that into tab as it was
looking good with them as aligned.
As the driver maintainer, I am fine with both unaligned or tab-aligned.
Space-aligned as I did originally was not a good idea, I admit.
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/3] ARM: mediatek: Add driver for Mediatek I2C
Hi Lee, Eddie, Le Tuesday 28 April 2015 à 11:02 +0100, Lee Jones a écrit : On Tue, 28 Apr 2015, Eddie Huang wrote: This series is for Mediatek SoCs I2C controller common bus driver. Earlier MTK SoC ((for example, MT6589, MT8135)) I2C HW has some limitationes. New generation SoC like MT8173 fix following limitations: 1. Only support one i2c_msg number. One exception is WRRD (write then read) mode. WRRD can have two i2c_msg numbers. 2. Mediatek I2C controller support WRRD(write then read) mode, in WRRD mode the Repeat Start will be issued between 2 messages. In this driver if 2 messages is first write then read, the driver will combine 2 messages using Write-Read mode so the RS will be issued between the 2 messages. 3. The max transfer data length is 255 in one message. In WRRD mode, the max data length of second msg is 31. MT8135 and MT6589 can control I2C pins on PMIC(MT6397) by setting the i2c registers in MT8135 side. In this case, driver should set OFFSET_PATH_DIR bit first, the operation on other registers are still the same. For now MT6589/MT8135 support this, MT6577/MT6595/MT8127 do not support. For example, If want to use I2C4/5/6 pins on MT8135 just need to enable the pinmux, else if want to use I2C pins on PMIC(MT6397) need to add mediatek,have-pmic property in the .dts file of each platform. This driver is based on 4.1-rc1. Change in v6: 1. Update binding document not use default clock-frequency as example. 2. Add mtk_i2c_compatible struct and pass hardware capabilities through of_device_id 3. Remove some hardware setting in mtk_i2c_do_transfer to mtk_i2c_init_hw so just init one time. 4. Correct mtk_i2c_parse_dt don't set default clock bug. Change in v5: Apply new i2c_adapter_quirks patch [2]. Change to use dam_map_single to map dma buffer. Add spinlock to fix race condition. Check of_property_read_u32 return value. Remove I2C_FUNC_10BIT_ADDR capability due to driver not implement. Add MT8173 I2C driver. Change in v4: Modify to support i2c_adapter_quirks base on Wolfram's patch [1]. Remove check transfer size and WRRD combine code. Instead, fill quirk property and let i2c_check_for_quirks to do the filter. [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/314804.html [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-February/325744.html Eddie Huang (1): I2C: mediatek: Add driver for MediaTek MT8173 I2C controller Xudong Chen (2): dt-bindings: Add I2C bindings for mt65xx/mt81xx. I2C: mediatek: Add driver for MediaTek I2C controller .../devicetree/bindings/i2c/i2c-mt6577.txt | 41 ++ drivers/i2c/busses/Kconfig | 9 + drivers/i2c/busses/Makefile| 1 + drivers/i2c/busses/i2c-mt65xx.c| 748 + 4 files changed, 799 insertions(+) create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mt6577.txt create mode 100644 drivers/i2c/busses/i2c-mt65xx.c Why have you sent me this? Mind you, I was wondering exactly the same ;-) -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 i2c/for-next] i2c-i801: recover from hardware PEC errors
Hi Ellen, On Mon, 20 Apr 2015 15:52:47 -0700, Ellen Wang wrote: This leads to a related question: If the driver is serialized, then the (status = priv-status) inside wait_event_timeout() isn't strictly necessary, correct? It can just be priv-status. I'm just want to double check that there's no race and I can add a priv-auxsts and not have to stick something like this inside the wait_event_timeout(): (auxsts = priv-auxsts, status = priv-status) To be honest I'm not 100% certain. I think this was only a minor optimization, but I may be wrong. Adding Daniel to Cc, he added that code to the i2c-i801 driver. Daniel, any comment on this? Equally mysterious to me is: priv-status |= status; in i801_isr() where it would seem that a simple = would suffice. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] i2c-tools: i2ctransfer: add new tool
Hi Wolfram, On Mon, 20 Apr 2015 19:36:38 +0200, Wolfram Sang wrote: On Fri, Feb 27, 2015 at 05:16:56PM +0100, Wolfram Sang wrote: This tool allows to construct and concat multiple I2C messages into one single transfer. Its aim is to test I2C master controllers, and so there is no SMBus fallback. Signed-off-by: Wolfram Sang w...@the-dreams.de --- I've been missing such a tool a number of times now, so I finally got around to writing it myself. As with all I2C tools, it can be dangerous, but it can also be very useful when developing. I am not sure if distros should supply it, I'll leave that to Jean's experience. For embedded build systems, I think this should be selectable. It is RFC for now because it needs broader testing and some more beautification. However, I've been using it already to test the i2c_quirk infrastructure and Renesas I2C controllers. Jean, my tests went well and so I want to brush it up for inclusion into i2c-tools upstream. Any show-stoppers you see from a high-level point of view? I think it is a good idea, just I couldn't find the time to review it, sorry :( -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 i2c/for-next] i2c-i801: recover from hardware PEC errors
Hi Ellen, On Wed, 15 Apr 2015 13:46:17 -0700, Ellen Wang wrote: On 4/15/2015 5:08 AM, Jean Delvare wrote: On Mon, 13 Apr 2015 17:11:59 -0700, Ellen Wang wrote: On a CRC error while using hardware-supported PEC, an additional error bit is set in the auxiliary status register. If this bit isn't cleared, all subsequent operations will fail, essentially hanging the controller. The fix is simple: clear the bit at the same time we clear the error bits in the main status register. Signed-off-by: Ellen Wang el...@cumulusnetworks.com --- drivers/i2c/busses/i2c-i801.c | 21 + 1 file changed, 21 insertions(+) Thanks for the patch. I noticed the issue over a year ago and wrote a patch on my own, but then couldn't find the time to test it and finally forgot about it :( so I never posted it. You can read it here for reference: http://jdelvare.nerim.net/devel/misc/i2c-i801-07-check-pec-status.patch I am currently working on other matters but I'll look into this ASAP. A quick comparison between our patches suggests that yours only clears the PEC status bit while mine also reports the error properly to the caller, so mine might be a better working base. Maybe you could review and/or test my patch as a replacement of yours? My version does actually report the error, because the main status register error bit is also set in this case, and that gets detected and reported. It just doesn't doesn't return a specific errno for PEC error. Correct, I did not remember this, sorry. The main difference between our versions is that you check and clear the CRC error bit in i801_check_pre() and i801_checkpost(), while I do it in i801_isr(). i801_isr() is also where it checks and clears the main status register for errors, so it seems natural to do it also for the auxiliary status. The check in i801_check_pre() is in case the CRC error bit is set at the time the driver is loaded. If it is, and we don't clear it, the driver won't work (for PEC transactions or even for all transactions, I'm not sure.) So I think it is needed either way. At the end of the transaction, the main status register is cleared in both i801_isr() and i801_check_post(). The latter is needed in case the driver operates in polling (non-interrupt) mode. That was the only mode available originally and my patch is so old that it is entirely possible that I wrote it before interrupt support was added to the driver. In fact that might even be the reason why I never submitted it, it may not support interrupt mode properly. So it is possible that the aux status register must be cleared in both i801_isr() and i801_check_post() to be on the safe side. Probably what we should do is combine the two versions, especially if we want to return a PEC-specific errno. I think to do that properly in the current structure of the code, we should save the auxiliary status in priv-auxsts in i801_isr(), the same way it saves priv-status. This ways, isr() can clear the error in the hardware and check_post() can still see it and process accordingly. What do you think? (I actually thought of this, but opted for a minimal fix.) This is more Sounds like a good idea, but I still believe that i801_check_post() will also have to clear the CRC error bit, for the non-interrupt case. complicated, and functions like i801_wait_intr() return the status and it can't return two values easily. I don't think we actually need to deal with the aux status register value in i801_wait_intr(). According to my notes, whenever the CRC error bit is set, SMBHSTSTS_DEV_ERR is also set in the main status register. So it's enough to check for that bit in i801_wait_intr(). If you need to pass the aux register status to i801_check_post(), in the non-interrupt case you could simply do: return i801_check_post(priv, status, inb_p(SMBAUXSTS(priv))); If we decide to go with your version, I can certainly test it. We have a machine with the right hardware combination, and it generates PEC errors deterministically. Well as I wrote above, it is possible that my patch doesn't work at all in the interrupt case, or that it works but is racy. So most probably we have to do as you said and combine both patches into one. By the way, I noticed a small bug in i801_transaction(). At line 391, it has status = -ETIMEOUT, but status at that point should be the register value not -errno. It probably should be return -ETIMEOUT, but I'm not sure whether hardware cleanup is necessary at that point. I can't see any bug there. status can be either the (positive) register value or a negative error code. It is passed to i801_check_post() which can deal with both. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 i2c/for-next] i1c: i801: recover from hardware PEC errors
Hi Ellen, On Mon, 13 Apr 2015 17:11:59 -0700, Ellen Wang wrote: On a CRC error while using hardware-supported PEC, an additional error bit is set in the auxiliary status register. If this bit isn't cleared, all subsequent operations will fail, essentially hanging the controller. The fix is simple: clear the bit at the same time we clear the error bits in the main status register. Signed-off-by: Ellen Wang el...@cumulusnetworks.com --- drivers/i2c/busses/i2c-i801.c | 21 + 1 file changed, 21 insertions(+) Thanks for the patch. I noticed the issue over a year ago and wrote a patch on my own, but then couldn't find the time to test it and finally forgot about it :( so I never posted it. You can read it here for reference: http://jdelvare.nerim.net/devel/misc/i2c-i801-07-check-pec-status.patch I am currently working on other matters but I'll look into this ASAP. A quick comparison between our patches suggests that yours only clears the PEC status bit while mine also reports the error properly to the caller, so mine might be a better working base. Maybe you could review and/or test my patch as a replacement of yours? Thanks, -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/86] i2c/i801: linux/pci_ids.h - uapi/linux/pci_ids.h
Hi Wolfram, On Fri, 3 Apr 2015 21:09:17 +0200, Wolfram Sang wrote: On Sun, Mar 29, 2015 at 03:37:30PM +0200, Michael S. Tsirkin wrote: Header moved from linux/pci_ids.h to uapi/linux/pci_ids.h, update i2c documentation accordingly. Signed-off-by: Michael S. Tsirkin m...@redhat.com Acked-by: Wolfram Sang w...@the-dreams.de I don't know if I should take it. If so, let me know. Don't, the series was nacked meanwhile. Thanks, -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] support i2c 10-bit addressing
Hi Peter, Guenter,
First of all: the right list to discuss this is linux-i2c (Cc'd) with
me in Cc.
On Sat, 28 Mar 2015 11:01:30 -0700, Guenter Roeck wrote:
On 03/28/2015 06:17 AM, Peter Chang wrote:
sigh gmail appears to have futzed w/ the attachment type.
2015-03-28 6:12 GMT-07:00 Peter Chang d...@google.com:
the address parsing doesn't have the adapter's support bits yet, so it
looks a little out of place.
There is a reason for discouraging attachments: If one replies (like me here),
the source code is not part of the reply, making a review really difficult.
Not sure what the above comment is supposed to mean. I can not parse it,
sorry.
Neither can I.
Unless I misunderstand the code, it now accepts addresses up to 0x3ff
unconditionally. If the adapter doesn't support 10 bit addresses, the
code then doesn't even try to set 10-bit address mode. 10-bit addresses
should not be accepted if the adapter does not support it.
It's even worse than that. The code also considers all addresses below
0x77 to always be 7-bit addresses, which is not correct. I2C specifies
two address ranges: a 7-bit one (valid addresses from 0x03 to 0x77) and
a 10-bit one (valid addresses 0x00 to 0x3ff.) These two ranges do NOT
overlap, even though the numbers are the same. In other words, 10-bit
address 0x10 exists and is NOT the same as 7-bit address 0x10.
This means that you can't guess whether the user means a 7-bit address
or a 10-bit address. The intent must be expressed explicitly with a
command-line parameter, and then the address validity must be checked
according to this parameter.
I would suggest to rearrange the code a bit to include the 10bit check
in check_funcs. Something like
if (...
|| check_funcs(file, size, pec, address 0x77)
|| ...
might do. This would make the code easier to read and address
the problem where a 10-bit address is provided but not supported by
the adapter.
The condition must be more explicit than address 0x77, I'd rather
expect something like:
check_funcs(file, size, pec, tenbit)
where tenbit is set by passing -t on the command line, for example.
Then
if (address 0x77 ioctl(file, I2C_TENBIT, 1) 0) {
fprintf(stderr, ...);
return -errno;
}
should work in set_slave_addr without the need to pass funcs to it.
Unfortunately not, as addresses = 0x77 are equally valid 10-bit
addresses. You must explicitly tell set_slave_addr whether the address
is a 7-bit or 10-bit one. But please do that by passing a flag (e.g.
tenbit as above) to it, not funcs.
You'll also need to update the Usage: strings for the various tools.
Correct, and the manual pages too.
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/86] i2c/i801: linux/pci_ids.h - uapi/linux/pci_ids.h
On Sun, 29 Mar 2015 15:37:30 +0200, Michael S. Tsirkin wrote: Header moved from linux/pci_ids.h to uapi/linux/pci_ids.h, update i2c documentation accordingly. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- Documentation/i2c/busses/i2c-i801 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/i2c/busses/i2c-i801 b/Documentation/i2c/busses/i2c-i801 index 82f48f7..8036644 100644 --- a/Documentation/i2c/busses/i2c-i801 +++ b/Documentation/i2c/busses/i2c-i801 @@ -141,7 +141,7 @@ host bridge PCI device. Get yours with lspci -n -v -s 00:00.0: Here the host bridge ID is 2570 (82865G/PE/P), the subvendor ID is 1043 (Asus) and the subdevice ID is 80f2 (P4P800-X). You can find the symbolic -names for the bridge ID and the subvendor ID in include/linux/pci_ids.h, +names for the bridge ID and the subvendor ID in include/uapi/linux/pci_ids.h, and then add a case for your subdevice ID at the right place in drivers/pci/quirks.c. Then please give it very good testing, to make sure that the unhidden SMBus doesn't conflict with e.g. ACPI. No objection from me. Reviewed-by: Jean Delvare jdelv...@suse.de -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: i2c-tools: add Android.mk
Hi Angelo, On Fri, 27 Feb 2015 22:05:12 +0100, Angelo Compagnucci wrote: This patch adds an Android.mk to compile i2c-tools under Android. Hope this helps! I have to say I don't know what to do with this. For one thing, I don't really understand why Android needs a separate build mechanism when all other Linux and BSD incarnations seem to be fine with the standard Makefile. GNU make certainly runs on Android, right? And you would typically cross-compile for Android anyway. Also I have not seen an Android.mk file in any other project I'm working on, and I can't see why i2c-tools would be special in this respect. For another, the current build mechanism is a modular one where the main Makefile includes Module.mk files present in the different subdirectories. I like this design and having a single Android.mk file at the root of the project is inconsistent and breaks that design. As a summary I don't see why anything special is needed for Android, but if something like that is really needed then it should respect the existing modular design. Now if anyone has a more educated view on this than I do, please speak up. Android is really not my thing (as a developer, I mean.) Thanks, -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 4/5] i2c: i801: Remove pci_enable_device() call from i801_resume()
On Fri, 13 Feb 2015 15:52:24 +0200, Jarkko Nikula wrote:
Since pci_disable_device() is not called from i801_suspend() and power
state is set already it means that subsequent pci_enable_device() calls do
practically nothing but monotonically increase struct pci_dev enable_cnt.
Signed-off-by: Jarkko Nikula jarkko.nik...@linux.intel.com
---
drivers/i2c/busses/i2c-i801.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index b1d725d758bb..5fb35464f693 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1324,7 +1324,7 @@ static int i801_resume(struct pci_dev *dev)
{
pci_set_power_state(dev, PCI_D0);
pci_restore_state(dev);
- return pci_enable_device(dev);
+ return 0;
}
#else
#define i801_suspend NULL
Reviewed-by: Jean Delvare jdelv...@suse.de
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 5/5] i2c: i801: Use managed pcim_* PCI device initialization and reservation
On Fri, 13 Feb 2015 15:52:25 +0200, Jarkko Nikula wrote: Simplifies the code a bit and makes easier to disable PCI device on driver detach by removing the pcim_pin_device() call in the future if needed. Reason why i2c-i801.c doesn't ever call pci_disable_device() was because it made some systems to hang during power-off. See commit d6fcb3b9cf77 ([PATCH] i2c-i801.c: don't pci_disable_device() after it was just enabled) and http://marc.info/?l=linux-kernelm=115160053309535w=2 Signed-off-by: Jarkko Nikula jarkko.nik...@linux.intel.com --- Changes from v2: - over 80 characters long pcim_iomap_regions line splitted - gotos and error labels removed --- drivers/i2c/busses/i2c-i801.c | 25 + 1 file changed, 9 insertions(+), 16 deletions(-) (...) Reviewed-by: Jean Delvare jdelv...@suse.de Wolfram, please commit this series, I have reviewed all patches and tested the updated driver successfully on 3 different machines. Thanks Jarkko for the good work. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/5] i2c: i801: Remove pci_enable_device() call from i801_resume()
Hi Jarkko,
On Fri, 13 Feb 2015 13:13:18 +0200, Jarkko Nikula wrote:
On 02/13/2015 12:33 PM, Jean Delvare wrote:
This looks reasonable but have you tested this change on a range of
actual laptops to make sure it has no unexpected side effect?
Unfortunately I have only limited amount of test hw which are working
fine even if PCI device is disabled so I cannot hit those issues that
were seen in the past.
I suppose this patch unlikely cause regression since if you look at the
call chain pci_enable_device() - pci_enable_device_flags() it doesn't
go beyond taking the current power state since enable_cnt is always
greater than 1.
drivers/pci/pci.c: pci_enable_device_flags():
if (dev-pm_cap) {
u16 pmcsr;
pci_read_config_word(dev, dev-pm_cap + PCI_PM_CTRL, pmcsr);
dev-current_state = (pmcsr PCI_PM_CTRL_STATE_MASK);
}
if (atomic_inc_return(dev-enable_cnt) 1)
return 0; /* already enabled */
To me it seems current power state taking is practically no-op when
device is enabled since pci_set_power_state() calls in i801_suspend()
and i801_resume() have already cached it.
OK, you convinced me. I'll still test the updated driver on my two
IBM/Lenovo Thinkpad laptops (T60p and X230) to make sure we didn't miss
anything.
Thanks,
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 1/5] i2c: i801: Don't break user-visible strings
On Wed, 11 Feb 2015 14:32:04 +0200, Jarkko Nikula wrote:
It makes more difficult to grep these error prints from sources if they are
split to multiple source lines.
Signed-off-by: Jarkko Nikula jarkko.nik...@linux.intel.com
---
drivers/i2c/busses/i2c-i801.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 8fafb254e42a..7d1f4a478c54 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1192,8 +1192,8 @@ static int i801_probe(struct pci_dev *dev, const struct
pci_device_id *id)
/* Determine the address of the SMBus area */
priv-smba = pci_resource_start(dev, SMBBAR);
if (!priv-smba) {
- dev_err(dev-dev, SMBus base address uninitialized,
- upgrade BIOS\n);
+ dev_err(dev-dev,
+ SMBus base address uninitialized, upgrade BIOS\n);
err = -ENODEV;
goto exit;
}
@@ -1206,8 +1206,9 @@ static int i801_probe(struct pci_dev *dev, const struct
pci_device_id *id)
err = pci_request_region(dev, SMBBAR, i801_driver.name);
if (err) {
- dev_err(dev-dev, Failed to request SMBus region
- 0x%lx-0x%Lx\n, priv-smba,
+ dev_err(dev-dev,
+ Failed to request SMBus region 0x%lx-0x%Lx\n,
+ priv-smba,
(unsigned long long)pci_resource_end(dev, SMBBAR));
goto exit;
}
Looks good.
Reviewed-by: Jean Delvare jdelv...@suse.de
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 3/5] i2c: i801: Use managed devm_* memory and irq allocation
On Wed, 11 Feb 2015 14:32:06 +0200, Jarkko Nikula wrote:
This simplifies the error and remove paths.
Signed-off-by: Jarkko Nikula jarkko.nik...@linux.intel.com
---
drivers/i2c/busses/i2c-i801.c | 17 ++---
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 5f827dfc671a..b1d725d758bb 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1138,7 +1138,7 @@ static int i801_probe(struct pci_dev *dev, const struct
pci_device_id *id)
int err, i;
struct i801_priv *priv;
- priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+ priv = devm_kzalloc(dev-dev, sizeof(*priv), GFP_KERNEL);
if (!priv)
return -ENOMEM;
@@ -1253,8 +1253,9 @@ static int i801_probe(struct pci_dev *dev, const struct
pci_device_id *id)
if (priv-features FEATURE_IRQ) {
init_waitqueue_head(priv-waitq);
- err = request_irq(dev-irq, i801_isr, IRQF_SHARED,
- dev_driver_string(dev-dev), priv);
+ err = devm_request_irq(dev-dev, dev-irq, i801_isr,
+IRQF_SHARED,
+dev_driver_string(dev-dev), priv);
if (err) {
dev_err(dev-dev, Failed to allocate irq %d: %d\n,
dev-irq, err);
@@ -1275,7 +1276,7 @@ static int i801_probe(struct pci_dev *dev, const struct
pci_device_id *id)
err = i2c_add_adapter(priv-adapter);
if (err) {
dev_err(dev-dev, Failed to add SMBus adapter\n);
- goto exit_free_irq;
+ goto exit_release;
}
i801_probe_optional_slaves(priv);
@@ -1286,12 +1287,9 @@ static int i801_probe(struct pci_dev *dev, const
struct pci_device_id *id)
return 0;
-exit_free_irq:
- if (priv-features FEATURE_IRQ)
- free_irq(dev-irq, priv);
+exit_release:
pci_release_region(dev, SMBBAR);
exit:
- kfree(priv);
return err;
}
@@ -1303,11 +1301,8 @@ static void i801_remove(struct pci_dev *dev)
i2c_del_adapter(priv-adapter);
pci_write_config_byte(dev, SMBHSTCFG, priv-original_hstcfg);
- if (priv-features FEATURE_IRQ)
- free_irq(dev-irq, priv);
pci_release_region(dev, SMBBAR);
- kfree(priv);
/*
* do not call pci_disable_device(dev) since it can cause hard hangs on
* some systems during power-off (eg. Fujitsu-Siemens Lifebook E8010)
Reviewed-by: Jean Delvare jdelv...@suse.de
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 5/5] i2c: i801: Use managed pcim_* PCI device initialization and reservation
Hi Jarkko,
On Wed, 11 Feb 2015 14:32:08 +0200, Jarkko Nikula wrote:
Simplifies the code a bit and makes easier to disable PCI device on driver
detach by removing the pcim_pin_device() call in the future if needed.
Reason why i2c-i801.c doesn't ever call pci_disable_device() was because it
made some systems to hang during power-off. See commit d6fcb3b9cf77
([PATCH] i2c-i801.c: don't pci_disable_device() after it was just enabled)
and
http://marc.info/?l=linux-kernelm=115160053309535w=2
Signed-off-by: Jarkko Nikula jarkko.nik...@linux.intel.com
---
drivers/i2c/busses/i2c-i801.c | 11 ---
1 file changed, 4 insertions(+), 7 deletions(-)
The checkpatch script complains:
WARNING: line over 80 characters
#90: FILE: drivers/i2c/busses/i2c-i801.c:1206:
+ err = pcim_iomap_regions(dev, 1 SMBBAR,
dev_driver_string(dev-dev));
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 5fb35464f693..9f7b69743233 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1180,12 +1180,13 @@ static int i801_probe(struct pci_dev *dev, const
struct pci_device_id *id)
}
priv-features = ~disable_features;
- err = pci_enable_device(dev);
+ err = pcim_enable_device(dev);
if (err) {
dev_err(dev-dev, Failed to enable SMBus PCI device (%d)\n,
err);
goto exit;
}
+ pcim_pin_device(dev);
/* Determine the address of the SMBus area */
priv-smba = pci_resource_start(dev, SMBBAR);
What is the benefit of this change, compared to just leaving the call
to pci_enable_device() in place?
@@ -1202,7 +1203,7 @@ static int i801_probe(struct pci_dev *dev, const struct
pci_device_id *id)
goto exit;
}
- err = pci_request_region(dev, SMBBAR, dev_driver_string(dev-dev));
+ err = pcim_iomap_regions(dev, 1 SMBBAR,
dev_driver_string(dev-dev));
if (err) {
dev_err(dev-dev,
Failed to request SMBus region 0x%lx-0x%Lx\n,
@@ -1276,7 +1277,7 @@ static int i801_probe(struct pci_dev *dev, const struct
pci_device_id *id)
err = i2c_add_adapter(priv-adapter);
if (err) {
dev_err(dev-dev, Failed to add SMBus adapter\n);
- goto exit_release;
+ goto exit;
}
i801_probe_optional_slaves(priv);
@@ -1287,8 +1288,6 @@ static int i801_probe(struct pci_dev *dev, const struct
pci_device_id *id)
return 0;
-exit_release:
- pci_release_region(dev, SMBBAR);
exit:
return err;
}
Now that the exit path is empty, wouldn't it make sense to return
directly on error? My understanding is that this is one of the benefits
of managed device resources.
@@ -1301,8 +1300,6 @@ static void i801_remove(struct pci_dev *dev)
i2c_del_adapter(priv-adapter);
pci_write_config_byte(dev, SMBHSTCFG, priv-original_hstcfg);
- pci_release_region(dev, SMBBAR);
-
/*
* do not call pci_disable_device(dev) since it can cause hard hangs on
* some systems during power-off (eg. Fujitsu-Siemens Lifebook E8010)
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 5/5] i2c: i801: Use managed pcim_* PCI device initialization and reservation
On Fri, 13 Feb 2015 13:47:07 +0200, Jarkko Nikula wrote:
On 02/13/2015 12:47 PM, Jean Delvare wrote:
On Wed, 11 Feb 2015 14:32:08 +0200, Jarkko Nikula wrote:
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 5fb35464f693..9f7b69743233 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1180,12 +1180,13 @@ static int i801_probe(struct pci_dev *dev, const
struct pci_device_id *id)
}
priv-features = ~disable_features;
- err = pci_enable_device(dev);
+ err = pcim_enable_device(dev);
if (err) {
dev_err(dev-dev, Failed to enable SMBus PCI device (%d)\n,
err);
goto exit;
}
+ pcim_pin_device(dev);
/* Determine the address of the SMBus area */
priv-smba = pci_resource_start(dev, SMBBAR);
What is the benefit of this change, compared to just leaving the call
to pci_enable_device() in place?
If you mean the patch itself I think easy pcim_pin_device() removal is
the biggest benefit if we ever want to experiment does hang during
power-off problem still exist in some machines. All PCI, ACPI and
power-off code have evolved in 9 years so original problem may not exist
anymore. Who knows.
Without this patch pcm_disable_device() needs to be added to error paths
and i801_remove(). (of course trivial but it's always more nice to get
regression from one-liner).
If you mean plain pcim_enable_device() it is required for other pcim_
functions since it allocates pcim_release which takes care of cleanup
bunch of things including pci_release_region().
E.g. pcim_iomap_regions() without pcim_enable_device() causes an oops
when doing cat /proc/ioports after module removal since
pcim_iomap_release() only unmaps but does not release regions.
I meant the latter. If there's a good reason for calling
pcim_enable_device then alright.
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] i2c: i801: Use managed devm_* memory and irq allocation
Hi Jarkko,
On Fri, 6 Feb 2015 15:56:53 +0200, Jarkko Nikula wrote:
This simplifies the error and remove paths.
Signed-off-by: Jarkko Nikula jarkko.nik...@linux.intel.com
---
drivers/i2c/busses/i2c-i801.c | 17 ++---
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index e31aa987b281..b4369d1835ca 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1138,7 +1138,7 @@ static int i801_probe(struct pci_dev *dev, const struct
pci_device_id *id)
int err, i;
struct i801_priv *priv;
- priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+ priv = devm_kzalloc(dev-dev, sizeof(*priv), GFP_KERNEL);
if (!priv)
return -ENOMEM;
@@ -1252,8 +1252,9 @@ static int i801_probe(struct pci_dev *dev, const struct
pci_device_id *id)
if (priv-features FEATURE_IRQ) {
init_waitqueue_head(priv-waitq);
- err = request_irq(dev-irq, i801_isr, IRQF_SHARED,
- dev_driver_string(dev-dev), priv);
+ err = devm_request_irq(dev-dev, dev-irq, i801_isr,
+IRQF_SHARED,
+dev_driver_string(dev-dev), priv);
if (err) {
dev_err(dev-dev, Failed to allocate irq %d: %d\n,
dev-irq, err);
@@ -1274,7 +1275,7 @@ static int i801_probe(struct pci_dev *dev, const struct
pci_device_id *id)
err = i2c_add_adapter(priv-adapter);
if (err) {
dev_err(dev-dev, Failed to add SMBus adapter\n);
- goto exit_free_irq;
+ goto exit_release;
}
i801_probe_optional_slaves(priv);
@@ -1285,12 +1286,9 @@ static int i801_probe(struct pci_dev *dev, const
struct pci_device_id *id)
return 0;
-exit_free_irq:
- if (priv-features FEATURE_IRQ)
- free_irq(dev-irq, priv);
+exit_release:
pci_release_region(dev, SMBBAR);
exit:
- kfree(priv);
return err;
}
@@ -1302,11 +1300,8 @@ static void i801_remove(struct pci_dev *dev)
i2c_del_adapter(priv-adapter);
pci_write_config_byte(dev, SMBHSTCFG, priv-original_hstcfg);
- if (priv-features FEATURE_IRQ)
- free_irq(dev-irq, priv);
pci_release_region(dev, SMBBAR);
- kfree(priv);
/*
* do not call pci_disable_device(dev) since it can cause hard hangs on
* some systems during power-off (eg. Fujitsu-Siemens Lifebook E8010)
Looks good, but what about also converting pci_request_region to
devm_request_region?
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 21/66] rtl2830: implement own I2C locking
Hi Mauro, Antti, On Mon, 2 Feb 2015 18:07:26 -0200, Mauro Carvalho Chehab wrote: Em Tue, 23 Dec 2014 22:49:14 +0200 Antti Palosaari cr...@iki.fi escreveu: Own I2C locking is needed due to two special reasons: 1) Chips uses multiple register pages/banks on single I2C slave. Page is changed via I2C register access. This is no good reason to implement your own i2c bus locking. Lots of i2c slave device work that way, and the way to handle it is through a dedicated lock at the i2c slave device level. This is in addition to the standard i2c bus locking and not a replacement. 2) Chip offers muxed/gated I2C adapter for tuner. Gate/mux is controlled by I2C register access. This, OTOH, is a valid reason for calling __i2c_transfer, and as a matter of fact a number of dvb frontend drivers already do so. Due to these reasons, I2C locking did not fit very well. I don't like the idea of calling __i2c_transfer() without calling first i2c_lock_adapter(). This can be dangerous, as the I2C core itself uses the lock for its own usage. I think the idea is that the i2c bus lock is already held at the time the muxing code is called. This happens each time the I2C muxing chip is an I2C chip itself. Ok, this may eventually work ok for now, but a further change at the I2C core could easily break it. So, we need to double check about such patch with the I2C maintainer. If it breaks than it'll break a dozen drivers which are already doing that, not just this one. But it's OK, I don't see this happening soon. Jean, Are you ok with such patch? If so, please ack. First of all: I am no longer the maintainer of the I2C subsystem. That being said... The changes look OK to me. I think it's how they are presented which make them look suspect. As I understand it, the extra locking at device level is unrelated with calling unlocked i2c transfer functions. The former change is to address the multi-page/bank register mapping, while the latter is to solve the deadlock due to the i2c bus topology and i2c-based muxing. If I am correct then it would be clearer to make that two separate patches with better descriptions. And if I'm wrong then the patch needs a better description too ;-) -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] i2c: Only include slave support if selected
On Mon, 26 Jan 2015 19:05:09 +0100, Wolfram Sang wrote: On Mon, Jan 26, 2015 at 07:00:47PM +0100, Jean Delvare wrote: Make the slave support depend on CONFIG_I2C_SLAVE. Otherwise it gets included unconditionally, even when it is not needed. Signed-off-by: Jean Delvare jdelv...@suse.de Cc: Wolfram Sang w...@the-dreams.de --- Changes since v1: * Let I2C_RCAR select I2C_SLAVE Wolfram, you mentioned i2c-sh_mobile but that driver doesn't seem to implement slave support in v3.19-rc6 so I did not include it. Feel free to add more selects to drivers/i2c/busses/Kconfig to match the current state of your i2c tree. Sorry, my bad. I meant the RCAR driver which you spotted. What about the function pointers in the algo struct? Doh, forgot about them once again. Stay tuned, patch v3 is coming... -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3] i2c: Only include slave support if selected
Make the slave support depend on CONFIG_I2C_SLAVE. Otherwise it gets
included unconditionally, even when it is not needed.
I2C bus drivers which implement slave support must select
I2C_SLAVE.
Signed-off-by: Jean Delvare jdelv...@suse.de
Cc: Wolfram Sang w...@the-dreams.de
---
Changes since v2:
* Added missing #if around i2c_algorithm callbacks
* Improved patch description
drivers/i2c/busses/Kconfig |1 +
drivers/i2c/i2c-core.c |2 ++
include/linux/i2c.h|6 ++
3 files changed, 9 insertions(+)
--- linux-3.19-rc6.orig/drivers/i2c/i2c-core.c 2015-01-26 12:47:26.467671896
+0100
+++ linux-3.19-rc6/drivers/i2c/i2c-core.c 2015-01-26 12:50:23.541420438
+0100
@@ -2962,6 +2962,7 @@ trace:
}
EXPORT_SYMBOL(i2c_smbus_xfer);
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb)
{
int ret;
@@ -3009,6 +3010,7 @@ int i2c_slave_unregister(struct i2c_clie
return ret;
}
EXPORT_SYMBOL_GPL(i2c_slave_unregister);
+#endif
MODULE_AUTHOR(Simon G. Vogl si...@tk.uni-linz.ac.at);
MODULE_DESCRIPTION(I2C-Bus main module);
--- linux-3.19-rc6.orig/include/linux/i2c.h 2015-01-26 12:47:26.470671959
+0100
+++ linux-3.19-rc6/include/linux/i2c.h 2015-01-26 20:10:38.727417893 +0100
@@ -222,7 +222,9 @@ struct i2c_client {
struct device dev; /* the device structure */
int irq;/* irq issued by device */
struct list_head detected;
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
i2c_slave_cb_t slave_cb;/* callback for slave mode */
+#endif
};
#define to_i2c_client(d) container_of(d, struct i2c_client, dev)
@@ -247,6 +249,7 @@ static inline void i2c_set_clientdata(st
/* I2C slave support */
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
enum i2c_slave_event {
I2C_SLAVE_REQ_READ_START,
I2C_SLAVE_REQ_READ_END,
@@ -263,6 +266,7 @@ static inline int i2c_slave_event(struct
{
return client-slave_cb(client, event, val);
}
+#endif
/**
* struct i2c_board_info - template for device creation
@@ -398,8 +402,10 @@ struct i2c_algorithm {
/* To determine what the adapter supports */
u32 (*functionality) (struct i2c_adapter *);
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
int (*reg_slave)(struct i2c_client *client);
int (*unreg_slave)(struct i2c_client *client);
+#endif
};
/**
--- linux-3.19-rc6.orig/drivers/i2c/busses/Kconfig 2015-01-23
10:30:56.044531347 +0100
+++ linux-3.19-rc6/drivers/i2c/busses/Kconfig 2015-01-26 18:55:07.163736062
+0100
@@ -881,6 +881,7 @@ config I2C_XLR
config I2C_RCAR
tristate Renesas R-Car I2C Controller
depends on ARCH_SHMOBILE || COMPILE_TEST
+ select I2C_SLAVE
help
If you say yes to this option, support will be included for the
R-Car I2C controller.
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] i2c: Only include slave support if selected
Make the slave support depend on CONFIG_I2C_SLAVE. Otherwise it gets
included unconditionally, even when it is not needed.
Signed-off-by: Jean Delvare jdelv...@suse.de
Cc: Wolfram Sang w...@the-dreams.de
---
Changes since v1:
* Let I2C_RCAR select I2C_SLAVE
Wolfram, you mentioned i2c-sh_mobile but that driver doesn't seem to
implement slave support in v3.19-rc6 so I did not include it. Feel free
to add more selects to drivers/i2c/busses/Kconfig to match the current
state of your i2c tree.
drivers/i2c/busses/Kconfig |1 +
drivers/i2c/i2c-core.c |2 ++
include/linux/i2c.h|4
3 files changed, 7 insertions(+)
--- linux-3.19-rc6.orig/drivers/i2c/i2c-core.c 2015-01-26 12:47:26.467671896
+0100
+++ linux-3.19-rc6/drivers/i2c/i2c-core.c 2015-01-26 12:50:23.541420438
+0100
@@ -2962,6 +2962,7 @@ trace:
}
EXPORT_SYMBOL(i2c_smbus_xfer);
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb)
{
int ret;
@@ -3009,6 +3010,7 @@ int i2c_slave_unregister(struct i2c_clie
return ret;
}
EXPORT_SYMBOL_GPL(i2c_slave_unregister);
+#endif
MODULE_AUTHOR(Simon G. Vogl si...@tk.uni-linz.ac.at);
MODULE_DESCRIPTION(I2C-Bus main module);
--- linux-3.19-rc6.orig/include/linux/i2c.h 2015-01-26 12:47:26.470671959
+0100
+++ linux-3.19-rc6/include/linux/i2c.h 2015-01-26 12:52:00.027462551 +0100
@@ -222,7 +222,9 @@ struct i2c_client {
struct device dev; /* the device structure */
int irq;/* irq issued by device */
struct list_head detected;
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
i2c_slave_cb_t slave_cb;/* callback for slave mode */
+#endif
};
#define to_i2c_client(d) container_of(d, struct i2c_client, dev)
@@ -247,6 +249,7 @@ static inline void i2c_set_clientdata(st
/* I2C slave support */
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
enum i2c_slave_event {
I2C_SLAVE_REQ_READ_START,
I2C_SLAVE_REQ_READ_END,
@@ -263,6 +266,7 @@ static inline int i2c_slave_event(struct
{
return client-slave_cb(client, event, val);
}
+#endif
/**
* struct i2c_board_info - template for device creation
--- linux-3.19-rc6.orig/drivers/i2c/busses/Kconfig 2015-01-23
10:30:56.044531347 +0100
+++ linux-3.19-rc6/drivers/i2c/busses/Kconfig 2015-01-26 18:55:07.163736062
+0100
@@ -881,6 +881,7 @@ config I2C_XLR
config I2C_RCAR
tristate Renesas R-Car I2C Controller
depends on ARCH_SHMOBILE || COMPILE_TEST
+ select I2C_SLAVE
help
If you say yes to this option, support will be included for the
R-Car I2C controller.
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: I2C slave support
On Mon, 26 Jan 2015 17:30:13 +0100, Wolfram Sang wrote:
Own module: Again, undecided. On the one hand it makes for a nice
encapsulation, on the other hand there is overhead for having another
module. I am very happy that the core code for slave support is so slim.
I gave a try to the separate module approach and I have to agree that
it seems overkill given the small amount of code.
OK, thanks for trying!
Something like this?
Yes, pretty much what I had in mind. One issue, though:
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
enum i2c_slave_event {
I2C_SLAVE_REQ_READ_START,
I2C_SLAVE_REQ_READ_END,
@@ -263,6 +266,7 @@ static inline int i2c_slave_event(struct
{
return client-slave_cb(client, event, val);
}
+#endif
This should fail because bus drivers need those enums for their slave
backend. Try building i2c-sh_mobile which builds with an x86 toolchain
as well.
Sorry I missed that, because there is currently no i2c bus driver
implementing slave support on x86-64.
* Either we leave this included, so bus drivers don't need any ifdeffery
We can do that. The enum itself has no run-time cost so I don't mind.
or
* we mandate that bus drivers also use the ifedeffery. Then, we could
also mask out the (un)reg_slave callbacks in struct i2c_adapter
What do you think?
Oh, I admit I completely missed the (un)reg_slave callbacks in my first
patch.
While I am happy with a few ifdefs in i2c-core and i2c.h, I agree it
will become messy if these are required in device drivers as well.
Hmm, what about bus drivers with slave mode support must select
CONFIG_I2C_SLAVE? This solves my problem nicely, and makes no change
compared to the current situation for people using slave mode.
Thanks,
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: i2c-tools: add compatibility for python2/3 to py-smbus
On Thu, 22 Jan 2015 14:28:10 +0100, Michael Mercier wrote: I have only tested read_i2c_block_data write_i2c_block_data for python 3.2 and python 2.7. I think you can push these patchs upstream. I committed both patches. I also added python 3 support to the stable branch. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
I2C slave support
Hi Wolfram, I find it confusing that I2C slave support is included even when CONFIG_I2C_SLAVE is not set. I don't know if this was discussed before? Most systems don't need this code so including it unconditionally seems suboptimal. I am considering adding ifdefs around the code to only include it when CONFIG_I2C_SLAVE is set. Alternatively the code could be moved to a separate module altogether. What do you think? Thanks, -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: i2c-tools: add compatibility for python2/3 to py-smbus
On Thu, 22 Jan 2015 14:28:10 +0100, Michael Mercier wrote: Thanks a lot! It was a linking problem. I applied the patch and I it works for both python2 and python3. For those who are interested you can choose to the python version with the PYTHON variable: make install # use python so it depends on your system configuration make install PYTHON=python2 # for python 2 explicitly make install PYTHON=python3 # for python 3 explicitly I have only tested read_i2c_block_data write_i2c_block_data for python 3.2 and python 2.7. I think you can push these patchs upstream. Great, thanks for testing :) -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: i2c-tools: add compatibility for python2/3 to py-smbus
Hi Michael, On Thu, 22 Jan 2015 09:56:58 +0100, Michael Mercier wrote: Hi Jean, I just tried you patch and I got an error when I try to import the smbus python package: Thanks for testing and reporting. import smbus ImportError: /usr/local/lib/python3.2/dist-packages/smbus.cpython-32mu.so: undefined symbol: i2c_smbus_process_call Any idea? Not really, again I know nothing about python. But didn't you have the same problem with Angelo's patch? Maybe Angelo has an idea. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: i2c-tools: add compatibility for python2/3 to py-smbus
Hi again Michael, On Thu, 22 Jan 2015 09:56:58 +0100, Michael Mercier wrote: I just tried you patch and I got an error when I try to import the smbus python package: import smbus ImportError: /usr/local/lib/python3.2/dist-packages/smbus.cpython-32mu.so: undefined symbol: i2c_smbus_process_call Any idea? Hmm, OK, I think I see what's going on here. There are two i2c-tools branches: the stable branch (3.1) and the development branch (SVN trunk.) The patch I sent was for the development branch. One important difference between the two branches is that, in the stable branch, the i2c API is implemented as a set of inline functions, while in the development branch it is a library. I don't think that py-smbus was properly ported so most likely it was already broken before my patch. I suppose you are using SVN trunk and this is why you have this linking problem? If that is the case, then maybe the following patch will do the trick: --- py-smbus/Module.mk |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- i2c-tools.orig/py-smbus/Module.mk 2012-04-26 12:05:55.351086796 +0200 +++ i2c-tools/py-smbus/Module.mk2015-01-22 11:47:06.524806243 +0100 @@ -12,7 +12,7 @@ PY_SMBUS_DIR := py-smbus PYTHON ?= python DISTUTILS := \ cd $(PY_SMBUS_DIR) \ - CPPFLAGS=$(CPPFLAGS) -I../include $(PYTHON) setup.py + CPPFLAGS=$(CPPFLAGS) -I../include LDFLAGS=$(LDFLAGS) -Llib -li2c $(PYTHON) setup.py all-python: $(INCLUDE_DIR)/i2c/smbus.h $(DISTUTILS) build But please keep in mind that my python knowledge is non-existent so it might be plain wrong. Hopefully Angelo can comment on this. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: i2c-tools: add compatibility for python2/3 to py-smbus
Hi Angelo,
On Mon, 19 Jan 2015 16:13:26 +0100, Angelo Compagnucci wrote:
Dear Jean Delvare,
Attached you can find a patch that implements py-smbus for python3
with python2 backward compatibility.
This patch is not heavily tested, but it wors well for me.
Thanks a lot for your work. I was finally about to look into this and
you saved me some time :-)
Unfortunately, I don't know how to use svn for sharing source code, it
is so ugly compared to git and it doesn't provide a way to send
patches via email, so the best way I found was to attach the patch
file.
This is fine. You know, this is how people did before git was invented,
and it worked well ;-)
Hope this helps!
Oh yeah. I started from your version and improved it as follows:
* Added back missing header files as pointed out by Michael. I have a
hard time believing you did not need these.
* Reverted some undesirable white space changes.
* Turned most #ifndefs into #ifdefs for readability.
* Shared the module documentation string.
* Added some preprocessing magic to limit the number of #ifdefs.
* Dropped PY3 definition.
Result is below, my patch is significantly smaller and the resulting
code is IMHO more readable. It builds fine for both python 2.7 and 3.3,
however I can't test it, so I would appreciate if you guys could test
and report. If it works fine I'll commit it tomorrow.
For convenience I have also put the pre-patched file at:
http://jdelvare.nerim.net/devel/i2c-tools/smbusmodule.c
Thanks.
---
py-smbus/smbusmodule.c | 47 +--
1 file changed, 45 insertions(+), 2 deletions(-)
--- i2c-tools.orig/py-smbus/smbusmodule.c 2015-01-19 22:09:39.624016377
+0100
+++ i2c-tools/py-smbus/smbusmodule.c2015-01-19 22:59:03.651572099 +0100
@@ -94,7 +94,11 @@ SMBus_dealloc(SMBus *self)
PyObject *ref = SMBus_close(self);
Py_XDECREF(ref);
+#if PY_MAJOR_VERSION = 3
+ Py_TYPE(self)-tp_free((PyObject *)self);
+#else
self-ob_type-tp_free((PyObject *)self);
+#endif
}
#define MAXPATH 16
@@ -434,11 +438,19 @@ SMBus_list_to_data(PyObject *list, union
for (ii = 0; ii len; ii++) {
PyObject *val = PyList_GET_ITEM(list, ii);
+#if PY_MAJOR_VERSION = 3
+ if (!PyLong_Check(val)) {
+ PyErr_SetString(PyExc_TypeError, msg);
+ return 0; /* fail */
+ }
+ data-block[ii+1] = (__u8)PyLong_AS_LONG(val);
+#else
if (!PyInt_Check(val)) {
PyErr_SetString(PyExc_TypeError, msg);
return 0; /* fail */
}
data-block[ii+1] = (__u8)PyInt_AS_LONG(val);
+#endif
}
return 1; /* success */
@@ -637,9 +649,14 @@ static PyGetSetDef SMBus_getset[] = {
};
static PyTypeObject SMBus_type = {
+#if PY_MAJOR_VERSION = 3
+ PyVarObject_HEAD_INIT(NULL, 0)
+ SMBus,/* tp_name */
+#else
PyObject_HEAD_INIT(NULL)
0, /* ob_size */
smbus.SMBus, /* tp_name */
+#endif
sizeof(SMBus), /* tp_basicsize */
0, /* tp_itemsize */
(destructor)SMBus_dealloc, /* tp_dealloc */
@@ -678,24 +695,50 @@ static PyTypeObject SMBus_type = {
SMBus_new, /* tp_new */
};
+#if PY_MAJOR_VERSION = 3
+static struct PyModuleDef SMBusModule = {
+ PyModuleDef_HEAD_INIT,
+ SMBus,/* m_name */
+ SMBus_module_doc, /* m_doc */
+ -1, /* m_size */
+ NULL, /* m_methods */
+ NULL, /* m_reload */
+ NULL, /* m_traverse */
+ NULL, /* m_clear */
+ NULL, /* m_free */
+};
+#define INIT_RETURN(m) return m
+#define INIT_FNAME PyInit_smbus
+#else
static PyMethodDef SMBus_module_methods[] = {
{NULL}
};
+#define INIT_RETURN(m) return
+#define INIT_FNAME initsmbus
+#endif
#ifndef PyMODINIT_FUNC /* declarations for DLL import/export */
#define PyMODINIT_FUNC void
#endif
PyMODINIT_FUNC
-initsmbus(void)
+INIT_FNAME(void)
{
PyObject* m;
if (PyType_Ready(SMBus_type) 0)
- return;
+ INIT_RETURN(NULL);
+#if PY_MAJOR_VERSION = 3
+ m = PyModule_Create(SMBusModule);
+#else
m = Py_InitModule3(smbus, SMBus_module_methods, SMBus_module_doc);
+#endif
+ if (m == NULL)
+ INIT_RETURN(NULL);
Py_INCREF(SMBus_type);
PyModule_AddObject(m, SMBus, (PyObject *)SMBus_type);
+
+ INIT_RETURN(m);
}
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http
Re: [PATCH] README: add info about current maintainer
Hi Wolfram, On Mon, 19 Jan 2015 10:31:38 +0100, Wolfram Sang wrote: Having the maintainer on CC helps to see i2c-tools related messages in the bulk of messages for the Linux kernel I2C subsystem. Signed-off-by: Wolfram Sang w...@the-dreams.de Cc: Jean Delvare jdelv...@suse.de --- README | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README b/README index 6a32150..64b4e3e 100644 --- a/README +++ b/README @@ -93,3 +93,5 @@ Please post your questions and bug reports to the linux-i2c mailing list: linux-i2c@vger.kernel.org For additional information about this list, see: http://vger.kernel.org/vger-lists.html#linux-i2c +The current maintainer is: + Jean Delvare jdelv...@suse.de Good idea, I have applied a similar change also explicitly requesting that I am Cc'd. http://www.lm-sensors.org/changeset/6261 Thanks, -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Py-smbus for Python 3
Hi all, On Mon, 19 Jan 2015 10:24:20 +0100, Wolfram Sang wrote: On Mon, Jan 19, 2015 at 10:12:15AM +0100, Michael Mercier wrote: Hi, I am working on a project that use the py-smbus module. The problem is I am using python 3.x and the py-smbus library is not compatible with this version. I found a lot of hack to do so, like: http://www.raspberrypi.org/forums/viewtopic.php?f=32t=22348 http://procrastinative.ninja/2014/07/21/smbus-for-python34-on-raspberry/ http://comments.gmane.org/gmane.linux.drivers.i2c/11290 http://www.spinics.net/lists/linux-i2c/msg08427.html I eventually manage to make it work but this code only working with python 3. I like distribute it but I do not know where. Is there someone that can merge this upstream properly (with python2 compatibility)? I can also put it on github to make it available so any interested people could improve it? What do you think? Adding Jean to the list, he is the current maintainer of i2c-tools. Indeed there have been many people doing the same, unfortunately nobody ever posted a patch which would preserve compatibility with python 2. I don't know a thing about python but I can't imagine that this is not possible. Unfortunately, as long as nobody contributes a patch which makes py-smbus work for both python version 2 and version 3, I can't apply it. I don't think it makes sense to break compatibility with one version to make the other one work. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [RFC] i2c: Don't wait for device release in i2c_del_adapter
Hi Pantelis, On Thu, 15 Jan 2015 18:25:22 +0200, Pantelis Antoniou wrote: The remove method of the mux gets called and on it’s remove method i2c_del_mux_adapter is called, which in turn calls i2c_del_adapter() which hangs. Anyway, finally figured out what the problem was, a reference was held, and not released properly in the i2c notifier pathc. I’ll send out an updated patch later today. Having that call wait for the other release call to happen is really old, as Jean points out, from 2003. We have fixed sysfs since then to detach the files from the devices easier, we used to have some nasy reference count issues in that area. Perhaps this isn't needed anymore, It’s awfully easy to get a hang with this blocking wait. Turns a memory leak into a hard hang. Turns an invisible memory leak into a visible bug, giving you the opportunity to fix it. You should be grateful ;-) That being said, if this completion serves no other purpose then it can go away, I don't mind. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [RFC] i2c: Don't wait for device release in i2c_del_adapter
On Wed, 14 Jan 2015 15:54:52 +0200, Pantelis Antoniou wrote: On Jan 14, 2015, at 15:49 , Jean Delvare jdelv...@suse.de wrote: That being said, nobody complained about this in 11 years, so I would be surprised if it was plain wrong. I'd rather question the test code. Where is it? No-one has apparently tested removing an i2c mux device on a running system before. I did that. On my system the i2c-i801 driver instantiates an i2c-mux-gpio device. Unloading the i2c-i801 driver removes that device and it works fine. Note however that I am not able to unload module i2c-i801 immediately, I have to unload module i2c-mux-gpio first, as the latter holds a reference to the former. This is because i2c-mux-gpio calls i2c_get_adapter() on the parent I2C bus segment at probing time. I see that i2c-mux-pinctrl does the same, but i2c-mux-pca954x and i2c-mux-pca9541 do not. This might be an issue. I doubt this is related to your problem though, as these are module references and not device references. Were you able to figure out who is holding a reference to your i2c_adapter? This would help us understand who is doing something wrong here, i2c-core or your test code. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [RFC] i2c: Don't wait for device release in i2c_del_adapter
Hi Guenter, On Wed, 14 Jan 2015 08:24:43 -0800, Guenter Roeck wrote: On Wed, Jan 14, 2015 at 04:15:25PM +0100, Jean Delvare wrote: I did that. On my system the i2c-i801 driver instantiates an i2c-mux-gpio device. Unloading the i2c-i801 driver removes that device and it works fine. Note however that I am not able to unload module i2c-i801 immediately, I have to unload module i2c-mux-gpio first, as the latter holds a reference to the former. This is because i2c-mux-gpio calls i2c_get_adapter() on the parent I2C bus segment at probing time. I see that i2c-mux-pinctrl does the same, but i2c-mux-pca954x and i2c-mux-pca9541 do not. This might be an issue. I doubt this is related to your problem though, as these are module references and not device references. Should we add get/put functions to those drivers ? I'm not sure. If you unload a driver which instantiated a pca9540-like device, without unloading i2c-mux-pca954x first, does something bad happen? -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [RFC] i2c: Don't wait for device release in i2c_del_adapter
Hi Wolfram, Pantelis,
On Tue, 13 Jan 2015 16:29:57 +0100, Wolfram Sang wrote:
On Mon, Jan 12, 2015 at 07:00:50PM +0200, Pantelis Antoniou wrote:
Waiting for the device release method to be called when
going through i2c_del_adapter is wrong and leads to deadlock
when removing an i2c mux device.
Adding Jean to the list, maybe he knows more details from the past.
For instance when using the OF i2c mux unitest removal we get this.
I can't parse the sentence above, sorry. What is unitest?
[c055dfdc] (__schedule) from [c0561b74] (schedule_timeout+0x18/0x198)
[c0561b74] (schedule_timeout) from [c055ee74]
(wait_for_common+0xf8/0x138)
[c055ee74] (wait_for_common) from [c03f724c]
(i2c_del_adapter+0x174/0x1cc)
[c03f724c] (i2c_del_adapter) from [c03f8480]
(i2c_del_mux_adapter+0x48/0x60)
[c03f8480] (i2c_del_mux_adapter) from [c046cf00]
(selftest_i2c_mux_remove+0x28/0x34)
[c046cf00] (selftest_i2c_mux_remove) from [c03f5234]
(i2c_device_remove+0x34/0x70)
[c03f5234] (i2c_device_remove) from [c03322ec]
(__device_release_driver+0x7c/0xc0)
[c03322ec] (__device_release_driver) from [c0332968]
(driver_detach+0x8c/0xb4)
[c0332968] (driver_detach) from [c0332088] (bus_remove_driver+0x64/0x8c)
[c0332088] (bus_remove_driver) from [c07f1858]
(of_selftest+0x1f84/0x20f0)
[c07f1858] (of_selftest) from [c0008b04] (do_one_initcall+0x104/0x1b4)
[c0008b04] (do_one_initcall) from [c07c4d9c]
(kernel_init_freeable+0x110/0x1d8)
[c07c4d9c] (kernel_init_freeable) from [c05591ec] (kernel_init+0x8/0xe4)
[c05591ec] (kernel_init) from [c000deb8] (ret_from_fork+0x14/0x3c)
2 locks held by swapper/0/1:
#0: (dev-mutex){..}, at: [c0332944] driver_detach+0x68/0xb4
#1: (dev-mutex){..}, at: [c0332954] driver_detach+0x78/0xb4
Kernel panic - not syncing: hung_task: blocked tasks
CPU: 0 PID: 16 Comm: khungtaskd Not tainted 3.19.0-rc4-00022-g261647c #443
Hardware name: Generic AM33XX (Flattened Device Tree)
[c00140a8] (unwind_backtrace) from [c00110dc] (show_stack+0x10/0x14)
[c00110dc] (show_stack) from [c055c3ac] (dump_stack+0x70/0x8c)
[c055c3ac] (dump_stack) from [c055ad10] (panic+0x88/0x1f0)
[c055ad10] (panic) from [c00adef8] (watchdog+0x2d4/0x350)
[c00adef8] (watchdog) from [c004e984] (kthread+0xd8/0xec)
[c004e984] (kthread) from [c000deb8] (ret_from_fork+0x14/0x3c)
The device release method is never called and we hang waiting for it.
This patch is marked as an [RFC] since the original code seems to
really want to protect against a race from sysfs accessors, which
I don't see how it could be a problem.
The code is not from me, it's from Greg KH. It was written in August
2003 when Greg converted the i2c subsystem to somewhat fit into the
(then) new device driver model. So that was a long time ago and it is
possible that this is no longer necessary. FWIW I can't see anything
similar in other subsystems.
In fact I'm not sure what purpose the completion serves exactly. I
expected it to delay the i2c adapter removal until no sysfs user of it
was left (as the comment suggests.) However I just tested unloading an
i2c bus driver while its adapter's new_device attribute was opened and
rmmod returned immediately. So it doesn't look like accessing sysfs
attributes actually takes a reference to the underlying i2c_adapter.
That being said, nobody complained about this in 11 years, so I would
be surprised if it was plain wrong. I'd rather question the test code.
Where is it?
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: How should dev_[gs]et_drvdata be used?
Hi Uwe,
On Tue, 25 Nov 2014 22:14:32 +0100, Uwe Kleine-König wrote:
On Wed, Jan 08, 2014 at 02:28:49PM +0100, Jean Delvare wrote:
Having looked at the code in deeper detail, I think I understand what
is going on. The problem is with:
i2c_set_adapdata(priv-adapter, priv);
at the beginning of i801_probe(). It triggers the allocation of dev-p
by the driver core. If we bail out at any point before i2c_add_adapter
(and subsequently device_register) is called, then that memory is never
freed.
Unfortunately it is not possible to move the i2c_set_adapdata() call
after i2c_add_adapter(), because the data pointer is needed by code
which runs as part of i2c_add_adapter().
We could move it right before the call to i2c_add_adapter(), to make
the problem window smaller, but this wouldn't solve the problem
completely, as i2c_add_adapter() itself can fail before
device_register() is called.
The only solution I can think of at this point is to stop using
i2c_set_adapdata() altogether, and use i2c_adapter.algo_data instead:
From: Jean Delvare kh...@linux-fr.org
Subject: i2c-i801: Use i2c_adapter.algo_data
Use i2c_adapter.algo_data instead of i2c_set/get_adapdata(). The
latter makes use of the driver core's private data mechanism, which
allocates memory. That memory is never released if an error happens
between the call to i2c_set_adapdata() and the actual i2c_adapter
registration.
Since commit 1bb6c08abfb6 (which makes the driver core use a pointer in
struct device again for dev_set_drvdata; went into v3.16-rc1) this patch
is obsolete, right?
Correct. It was never applied upstream anyway, which is good as I never
liked it.
(Still there might be the opportunity for a few patches converting all
driver to i2c_set_adapdata and then drop adapter.algo_data.)
That's at least 35 bus drivers that would have to be converted then.
But you should first check if it is possible to get rid of
i2c_adapter.algo_data without breaking i2c-algo-bit and i2c-mux. If not
then converting the bus drivers would really only be a minor cleanup
with no real benefit (not saying it's not worth it though.)
Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Peter Wu lekenst...@gmail.com
---
drivers/i2c/busses/i2c-i801.c |6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -671,7 +671,7 @@ static s32 i801_access(struct i2c_adapte
int hwpec;
int block = 0;
int ret, xact = 0;
- struct i801_priv *priv = i2c_get_adapdata(adap);
+ struct i801_priv *priv = adap-algo_data;
hwpec = (priv-features FEATURE_SMBUS_PEC) (flags I2C_CLIENT_PEC)
size != I2C_SMBUS_QUICK
@@ -774,7 +774,7 @@ static s32 i801_access(struct i2c_adapte
static u32 i801_func(struct i2c_adapter *adapter)
{
- struct i801_priv *priv = i2c_get_adapdata(adapter);
+ struct i801_priv *priv = adapter-algo_data;
return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
@@ -1117,7 +1117,7 @@ static int i801_probe(struct pci_dev *de
if (!priv)
return -ENOMEM;
- i2c_set_adapdata(priv-adapter, priv);
+ priv-adapter.algo_data = priv;
priv-adapter.owner = THIS_MODULE;
priv-adapter.class = i801_get_adapter_class(priv);
priv-adapter.algo = smbus_algorithm;
I can't think of any drawback, other than the feeling that switching
from a generic implementation to a specific one is a move in the wrong
direction.
If the above is the proper fix then we may consider just changing the
implementation of i2c_set/get_adapdata to use adapter.algo_data instead
of calling dev_set/get_drvdata(). This would let us fix all the drivers
at once.
I'll bring the topic upstream for discussion.
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/4 v2] i2c-i801: Make interrupt mode more robust
Hi all, This is a patch series aiming at making the interrupt mode of the i2c-i801 driver more robust. A number of driver lock-ups have been reported on a few systems over the past few years, all related to interrupt mode. So let's make it more tolerant to unreliable hardware or whatever the actual problem is. i2c-i801: Use wait_event_timeout to wait for interrupts i2c-i801: Fallback to polling if request_irq() fails i2c-i801: Check if interrupts are disabled i2c-i801: Drop useless debug message Changes since v1: * Updated according to Wolfram's review. Thanks! -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4 v2] i2c-i801: Use wait_event_timeout to wait for interrupts
Some systems have been reported to have trouble with interrupts. Use
wait_event_timeout() instead of wait_event() so we don't get stuck in
that case, and log the problem.
Signed-off-by: Jean Delvare jdelv...@suse.de
Cc: Wolfram Sang w...@the-dreams.de
---
No change since v1.
drivers/i2c/busses/i2c-i801.c | 25 ++---
1 file changed, 22 insertions(+), 3 deletions(-)
--- linux-3.18-rc4.orig/drivers/i2c/busses/i2c-i801.c 2014-11-10
22:26:47.212135115 +0100
+++ linux-3.18-rc4/drivers/i2c/busses/i2c-i801.c2014-11-10
22:29:18.040417276 +0100
@@ -2,7 +2,7 @@
Copyright (c) 1998 - 2002 Frodo Looijaard fro...@dds.nl,
Philip Edelbrock p...@netroedge.com, and Mark D. Studebaker
mdsxyz...@yahoo.com
-Copyright (C) 2007 - 2012 Jean Delvare jdelv...@suse.de
+Copyright (C) 2007 - 2014 Jean Delvare jdelv...@suse.de
Copyright (C) 2010 Intel Corporation,
David Woodhouse dw...@infradead.org
@@ -373,6 +373,7 @@ static int i801_transaction(struct i801_
{
int status;
int result;
+ const struct i2c_adapter *adap = priv-adapter;
result = i801_check_pre(priv);
if (result 0)
@@ -381,7 +382,14 @@ static int i801_transaction(struct i801_
if (priv-features FEATURE_IRQ) {
outb_p(xact | SMBHSTCNT_INTREN | SMBHSTCNT_START,
SMBHSTCNT(priv));
- wait_event(priv-waitq, (status = priv-status));
+ result = wait_event_timeout(priv-waitq,
+ (status = priv-status),
+ adap-timeout);
+ if (!result) {
+ status = -ETIMEDOUT;
+ dev_warn(priv-pci_dev-dev,
+Timeout waiting for interrupt!\n);
+ }
priv-status = 0;
return i801_check_post(priv, status);
}
@@ -529,6 +537,7 @@ static int i801_block_transaction_byte_b
int smbcmd;
int status;
int result;
+ const struct i2c_adapter *adap = priv-adapter;
result = i801_check_pre(priv);
if (result 0)
@@ -557,7 +566,14 @@ static int i801_block_transaction_byte_b
priv-data = data-block[1];
outb_p(priv-cmd | SMBHSTCNT_START, SMBHSTCNT(priv));
- wait_event(priv-waitq, (status = priv-status));
+ result = wait_event_timeout(priv-waitq,
+ (status = priv-status),
+ adap-timeout);
+ if (!result) {
+ status = -ETIMEDOUT;
+ dev_warn(priv-pci_dev-dev,
+Timeout waiting for interrupt!\n);
+ }
priv-status = 0;
return i801_check_post(priv, status);
}
@@ -1215,6 +1231,9 @@ static int i801_probe(struct pci_dev *de
outb_p(inb_p(SMBAUXCTL(priv))
~(SMBAUXCTL_CRC | SMBAUXCTL_E32B), SMBAUXCTL(priv));
+ /* Default timeout in interrupt mode: 200 ms */
+ priv-adapter.timeout = HZ / 5;
+
if (priv-features FEATURE_IRQ) {
init_waitqueue_head(priv-waitq);
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/4 v2] i2c-i801: Fallback to polling if request_irq() fails
The i2c-i801 driver can work without interrupts, so there is no reason
to make a request_irq failure fatal. Instead we can simply fallback
to polling.
Signed-off-by: Jean Delvare jdelv...@suse.de
Cc: Wolfram Sang w...@the-dreams.de
---
Changes since v1:
* Always log whether the driver is using polling or PCI interrupt.
drivers/i2c/busses/i2c-i801.c |6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
--- linux-3.18-rc4.orig/drivers/i2c/busses/i2c-i801.c 2014-11-10
22:29:42.788955868 +0100
+++ linux-3.18-rc4/drivers/i2c/busses/i2c-i801.c2014-11-11
14:56:35.777941563 +0100
@@ -1242,10 +1242,11 @@ static int i801_probe(struct pci_dev *de
if (err) {
dev_err(dev-dev, Failed to allocate irq %d: %d\n,
dev-irq, err);
- goto exit_release;
+ priv-features = ~FEATURE_IRQ;
}
- dev_info(dev-dev, SMBus using PCI Interrupt\n);
}
+ dev_info(dev-dev, SMBus using %s\n,
+priv-features FEATURE_IRQ ? PCI interrupt : polling);
/* set up the sysfs linkage to our parent device */
priv-adapter.dev.parent = dev-dev;
@@ -1272,7 +1273,6 @@ static int i801_probe(struct pci_dev *de
exit_free_irq:
if (priv-features FEATURE_IRQ)
free_irq(dev-irq, priv);
-exit_release:
pci_release_region(dev, SMBBAR);
exit:
kfree(priv);
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/4 v2] i2c-i801: Check if interrupts are disabled
There is a control bit in the PCI configuration space which disables
interrupts. If this bit is set, the driver should not try to make use
of interrupts, it won't receive any.
Signed-off-by: Jean Delvare jdelv...@suse.de
Cc: Wolfram Sang w...@the-dreams.de
---
Changes since v1:
* Turned warning message into info message and made it shorter.
drivers/i2c/busses/i2c-i801.c | 20
1 file changed, 20 insertions(+)
--- linux-3.18-rc4.orig/drivers/i2c/busses/i2c-i801.c 2014-11-11
14:56:35.777941563 +0100
+++ linux-3.18-rc4/drivers/i2c/busses/i2c-i801.c2014-11-11
15:32:30.128418755 +0100
@@ -110,12 +110,16 @@
/* PCI Address Constants */
#define SMBBAR 4
+#define SMBPCICTL 0x004
#define SMBPCISTS 0x006
#define SMBHSTCFG 0x040
/* Host status bits for SMBPCISTS */
#define SMBPCISTS_INTS 0x08
+/* Control bits for SMBPCICTL */
+#define SMBPCICTL_INTDIS 0x0400
+
/* Host configuration bits for SMBHSTCFG */
#define SMBHSTCFG_HST_EN 1
#define SMBHSTCFG_SMB_SMI_EN 2
@@ -1235,6 +1239,22 @@ static int i801_probe(struct pci_dev *de
priv-adapter.timeout = HZ / 5;
if (priv-features FEATURE_IRQ) {
+ u16 pcictl, pcists;
+
+ /* Complain if an interrupt is already pending */
+ pci_read_config_word(priv-pci_dev, SMBPCISTS, pcists);
+ if (pcists SMBPCISTS_INTS)
+ dev_warn(dev-dev, An interrupt is pending!\n);
+
+ /* Check if interrupts have been disabled */
+ pci_read_config_word(priv-pci_dev, SMBPCICTL, pcictl);
+ if (pcictl SMBPCICTL_INTDIS) {
+ dev_info(dev-dev, Interrupts are disabled\n);
+ priv-features = ~FEATURE_IRQ;
+ }
+ }
+
+ if (priv-features FEATURE_IRQ) {
init_waitqueue_head(priv-waitq);
err = request_irq(dev-irq, i801_isr, IRQF_SHARED,
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/4 v2] i2c-i801: Drop useless debug message
Don't log the host status register value in i801_isr(), it has very little value and fills up the log when debugging is enabled. Signed-off-by: Jean Delvare jdelv...@suse.de Cc: Wolfram Sang w...@the-dreams.de --- No change since v1. drivers/i2c/busses/i2c-i801.c |3 --- 1 file changed, 3 deletions(-) --- linux-3.18-rc4.orig/drivers/i2c/busses/i2c-i801.c 2014-11-10 19:54:05.042326020 +0100 +++ linux-3.18-rc4/drivers/i2c/busses/i2c-i801.c2014-11-10 19:56:51.562907696 +0100 @@ -507,9 +507,6 @@ static irqreturn_t i801_isr(int irq, voi return IRQ_NONE; status = inb_p(SMBHSTSTS(priv)); - if (status != 0x42) - dev_dbg(priv-pci_dev-dev, irq: status = %02x\n, status); - if (status SMBHSTSTS_BYTE_DONE) i801_isr_byte_done(priv); -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] i2c-i801: Fallback to polling if request_irq() fails
Hi Wolfram,
Thanks for the quick review, very appreciated.
On Tue, 11 Nov 2014 11:57:34 +0100, Wolfram Sang wrote:
On Mon, Nov 10, 2014 at 10:31:04PM +0100, Jean Delvare wrote:
The i2c-i801 driver can work without interrupts, so there is no reason
to make a request_irq failure fatal. Instead we can simply fallback
to polling.
Signed-off-by: Jean Delvare jdelv...@suse.de
Cc: Wolfram Sang w...@the-dreams.de
---
drivers/i2c/busses/i2c-i801.c |7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
--- linux-3.18-rc4.orig/drivers/i2c/busses/i2c-i801.c 2014-11-10
22:29:42.788955868 +0100
+++ linux-3.18-rc4/drivers/i2c/busses/i2c-i801.c2014-11-10
22:29:44.416991298 +0100
@@ -1242,9 +1242,11 @@ static int i801_probe(struct pci_dev *de
if (err) {
dev_err(dev-dev, Failed to allocate irq %d: %d\n,
dev-irq, err);
- goto exit_release;
+ priv-features = ~FEATURE_IRQ;
+ dev_info(dev-dev, SMBus using polling\n);
Shouldn't this message also be printed for !FEATURE_IRQ? I'd think it
should be moved to another place.
Good point, I'll rework that part of the code and resubmit.
We can also deduce from the dev_err
above that polling will be used, no?
Having to deduce things doesn't make my supporter's life easy. Given
the problems that have been reported by the i2c-i801 driver users
lately, I'd rather be verbose during initialization to make bug reports
easier to analyze.
+ } else {
+ dev_info(dev-dev, SMBus using PCI interrupt\n);
}
- dev_info(dev-dev, SMBus using PCI Interrupt\n);
}
/* set up the sysfs linkage to our parent device */
@@ -1272,7 +1274,6 @@ static int i801_probe(struct pci_dev *de
exit_free_irq:
if (priv-features FEATURE_IRQ)
free_irq(dev-irq, priv);
-exit_release:
pci_release_region(dev, SMBBAR);
exit:
kfree(priv);
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] i2c-i801: Check if interrupts are disabled
Hi Wolfram,
On Tue, 11 Nov 2014 11:58:54 +0100, Wolfram Sang wrote:
On Mon, Nov 10, 2014 at 10:31:39PM +0100, Jean Delvare wrote:
There is a control bit in the PCI configuration space which disables
interrupts. If this bit is set, the driver should not try to make use
of interrupts, it won't receive any.
Signed-off-by: Jean Delvare jdelv...@suse.de
Cc: Wolfram Sang w...@the-dreams.de
---
drivers/i2c/busses/i2c-i801.c | 21 +
1 file changed, 21 insertions(+)
--- linux-3.18-rc4.orig/drivers/i2c/busses/i2c-i801.c 2014-11-10
22:29:44.416991298 +0100
+++ linux-3.18-rc4/drivers/i2c/busses/i2c-i801.c2014-11-10
22:29:46.915045662 +0100
@@ -110,12 +110,16 @@
/* PCI Address Constants */
#define SMBBAR 4
+#define SMBPCICTL 0x004
#define SMBPCISTS 0x006
#define SMBHSTCFG 0x040
/* Host status bits for SMBPCISTS */
#define SMBPCISTS_INTS 0x08
+/* Control bits for SMBPCICTL */
+#define SMBPCICTL_INTDIS 0x0400
+
/* Host configuration bits for SMBHSTCFG */
#define SMBHSTCFG_HST_EN 1
#define SMBHSTCFG_SMB_SMI_EN 2
@@ -1235,6 +1239,23 @@ static int i801_probe(struct pci_dev *de
priv-adapter.timeout = HZ / 5;
if (priv-features FEATURE_IRQ) {
+ u16 pcictl, pcists;
+
+ /* Complain if an interrupt is already pending */
+ pci_read_config_word(priv-pci_dev, SMBPCISTS, pcists);
+ if (pcists SMBPCISTS_INTS)
+ dev_warn(dev-dev, An interrupt is pending!\n);
You think it is better to not clear it?
I admit I did not think about it. As I am trying to better understand
the few mysterious failure cases that have been reported to me, I just
wanted to log everything out of the ordinary. I have no idea what's
considered the right thing to do in such a situation. Do you believe
that clearing the interrupt is the appropriate action in that case?
+
+ /* Check if interrupts have been disabled */
+ pci_read_config_word(priv-pci_dev, SMBPCICTL, pcictl);
+ if (pcictl SMBPCICTL_INTDIS) {
+ dev_warn(dev-dev,
+Interrupts are disabled, will use polling\n);
I'd say it is more info than warning?
Correct, I'll change it.
+ priv-features = ~FEATURE_IRQ;
+ }
+ }
+
+ if (priv-features FEATURE_IRQ) {
init_waitqueue_head(priv-waitq);
err = request_irq(dev-irq, i801_isr, IRQF_SHARED,
Thanks again,
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] i2c-i801: Check if interrupts are disabled
On Tue, 11 Nov 2014 15:41:07 +0100, Wolfram Sang wrote: + if (pcists SMBPCISTS_INTS) + dev_warn(dev-dev, An interrupt is pending!\n); You think it is better to not clear it? I admit I did not think about it. As I am trying to better understand the few mysterious failure cases that have been reported to me, I just wanted to log everything out of the ordinary. I have no idea what's considered the right thing to do in such a situation. Do you believe that clearing the interrupt is the appropriate action in that case? Depends on the driver, I'd say (which I haven't looked at in detail). If this causes the irq handler to be called as soon as the irq is registered, and if the state machine gets confused then, then it should be cleared beforehand, of course. The driver should survive just fine if the irq handler is called early, but lacking the context, it won't do anything useful. Essentially it will clear the interrupt and wake up an empty queue. My actual concern is that I don't know what that would mean if an interrupt is pending at driver load time. If this is a leftover from a previous driver removal, or from the machine initialization (BIOS) then clearing it would be the right thing to do. But this could also mean that something (e.g. ACPI) is using the hardware and we should not. That being said, hitting the very moment where an interrupt is pending would be pretty random, so we can hardly rely on that anyway. Or this could mean that the interrupt setup is wrong somehow and we can't trust the SMBPCISTS_INTS because it is always set. At this point I just don't know. Another thing I am wondering about, and thinking about it again (I wrote this code several months ago) this is probably the reason why I added the test in the first place: can a new interrupt be triggered as long as the previous one has not been cleared? If not, that could explain why the driver sometimes didn't work at all in interrupt mode on some systems. If this is not the case, then it might be better to be less intrusive, spit the warning, and wait for somebody to show up with this message in the logs. That was my intent, yes. We can always add an action later when we understand the situation better. If we ever get such a report, which might in fact never happen. Thanks, -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: mux: create proper topology in sysfs
Hi Danielle, On Tue, 4 Nov 2014 01:07:08 -0800, Danielle Costantino wrote: This is a tool I have been developing for complex I2C bus structures (lsi2c) the output of this should help with this complex multiplexer testing. I was able to overcome the problem by creating persistent paths describing where the devices reside. I will be testing compatibility with this setup. I will be reporting on my test results within a few days. [root@p2020 ~]# ./lsi2c -h Usage: lsi2c [OPTION]... -c, --config-file Specify a config file -C, --print-configDisplay i2c devices in configuration file -a, --all Print all i2c-devs in bus tree -d, --print-devices Display sysfs i2c devices -t, --treePrint i2c bus and children -p, --pathParse an i2c-dev path -P, --probe Probe an i2c-dev at addr on path -F, --funcPrint I2C bus functionality -T, --timeout Set adapter timeout in milliseconds -S, --retry-count Set adapter max retry count -h, --helpDisplay this help text -V, --version Display the program version -v, --verbose Be verbose -i, --initialize Initialize devices in configuration file -r, --remove Remove devices in configuration file -k, --kmodTry to initialize i2c_dev kernel module Use `-' after `-c' to read the config file from stdin. Is the source code of this tool available somewhere? Thanks, -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
