[PATCH 0/2] Move the SMBus API documentation to libi2c

[PATCH 0/2] Move the SMBus API documentation to libi2c

[Jean Delvare]

This is a cross-project patch set moving the SMBus-level API
documentation from the kernel to libi2c.

[1/2] libi2c: Add a manual page to document the API
[2/2] docs: i2c: dev-interface: document the actual implementation

It applies on top of Luca's recent patch set which cleans up the whole
i2c documentation. I'll rebase it if needed when Luca sends v2 of his
patch set.

-- 
Jean Delvare
SUSE L3 Support

[PATCH 1/2] libi2c: Add a manual page to document the API

[Jean Delvare]

It is good practice for a library to come with a complete API description.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
---
 lib/libi2c.3 |  137 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 137 insertions(+)

--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ i2c-tools/lib/libi2c.3	2020-01-23 10:45:45.648471093 +0100
@@ -0,0 +1,137 @@
+.\" Copyright (C) 2019  Jean Delvare <jdelvare@suse.de>
+.\" libi2c is distributed under the LGPL
+.TH libi2c 3  "December 2019" "i2c-tools" "Linux Programmer's Manual"
+
+.SH NAME
+libi2c \- publicly accessible functions provided by the i2c library
+
+.SH SYNOPSIS
+.nf
+.B #include <linux/i2c.h>
+.B #include <i2c/smbus.h>
+
+/* Universal SMBus transaction */
+.BI "__s32 i2c_smbus_access(int " file ", char " read_write ", __u8 " command ","
+.BI "                       int " size ", union i2c_smbus_data *" data ");"
+
+/* Simple SMBus transactions */
+.BI "__s32 i2c_smbus_write_quick(int " file ", __u8 " value ");"
+.BI "__s32 i2c_smbus_read_byte(int " file ");"
+.BI "__s32 i2c_smbus_write_byte(int " file ", __u8 " value ");"
+.BI "__s32 i2c_smbus_read_byte_data(int " file ", __u8 " command ");"
+.BI "__s32 i2c_smbus_write_byte_data(int " file ", __u8 " command ", __u8 " value ");"
+.BI "__s32 i2c_smbus_read_word_data(int " file ", __u8 " command ");"
+.BI "__s32 i2c_smbus_write_word_data(int " file ", __u8 " command ", __u16 " value ");"
+.BI "__s32 i2c_smbus_process_call(int " file ", __u8 " command ", __u16 " value ");"
+
+/* Block SMBus (and non-SMBus) transactions */
+.BI "__s32 i2c_smbus_read_block_data(int " file ", __u8 " command ", __u8 *" values ");"
+.BI "__s32 i2c_smbus_write_block_data(int " file ", __u8 " command ", __u8 " length ","
+.BI "                                 const __u8 *" values ");"
+.BI "__s32 i2c_smbus_block_process_call(int " file ", __u8 " command ", __u8 " length ","
+.BI "                                   __u8 *" values ");"
+.BI "__s32 i2c_smbus_read_i2c_block_data(int " file ", __u8 " command ", __u8 " length ","
+.BI "                                    __u8 *" values ");"
+.BI "__s32 i2c_smbus_write_i2c_block_data(int " file ", __u8 " command ", __u8 " length ","
+.BI "                                     const __u8 *" values ");"
+
+.SH DESCRIPTION
+This library offers to user-space an SMBus-level API similar to the in-kernel
+one.
+Each function is a wrapper around the appropriate ioctl call for i2c-dev to
+process.
+The i2c-dev kernel driver will convert the ioctl to its in-kernel
+equivalent function call, and pass the result back to the user-space caller.
+
+.B i2c_smbus_access()
+is the universal function to run any SMBus transaction.
+You have to fill out and link the data structures yourself.
+It returns 0 on success, or a negative \fBerrno\fR value on error.
+In practice you should never need to call this function directly, instead use
+one of the specific functions below, which will prepare the data and then
+call it for you.
+
+.B i2c_smbus_write_quick()
+runs an SMBus "Quick command" transaction.
+
+.B i2c_smbus_write_byte()
+runs an SMBus "Send byte" transaction.
+
+.B i2c_smbus_write_byte_data()
+runs an SMBus "Write byte" transaction.
+
+.B i2c_smbus_write_word_data()
+runs an SMBus "Write word" transaction.
+
+These write transaction functions return 0 on success.
+On error, a negative \fBerrno\fR value is returned.
+
+.B i2c_smbus_read_byte()
+runs an SMBus "Receive byte" transaction.
+
+.B i2c_smbus_read_byte_data()
+runs an SMBus "Read byte" transaction.
+
+.B i2c_smbus_read_word_data()
+runs an SMBus "Read word" transaction.
+
+.B i2c_smbus_process_call()
+runs an SMBus "Process call" transaction.
+
+These read transaction functions return the read byte or word value on success.
+On error, a negative \fBerrno\fR value is returned.
+
+.B i2c_smbus_write_block_data()
+runs an SMBus "Block write" transaction.
+
+.B i2c_smbus_read_block_data()
+runs an SMBus "Block read" transaction.
+
+.B i2c_smbus_block_process_call()
+runs an SMBus "Block write-block read process call" transaction.
+
+These block transaction functions return 0 on success.
+On error, a negative \fBerrno\fR value is returned.
+The block length is limited to 32 bytes.
+
+.B i2c_smbus_write_i2c_block_data()
+runs an "I2C block write" transaction. This is typically used to write to
+an EEPROM up to 4 kb in size.
+
+.B i2c_smbus_read_i2c_block_data()
+runs an "I2C block read" transaction. This is typically used to read from
+an EEPROM up to 4 kb in size.
+
+While technically not part of the SMBus specification, these I2C block
+transactions are supported by many SMBus host controllers.
+These block transaction functions return 0 on success.
+On error, a negative \fBerrno\fR value is returned.
+Like their SMBus counterparts, the block length is limited to 32 bytes.
+
+.SH DATA STRUCTURES
+
+Structure \fBi2c_smbus_ioctl_data\fR is used to send data to and retrieve
+data from the kernel through the i2c-dev driver.
+It will be filled out for you by \fBi2c_smbus_access()\fR so you do not need
+to care about the details.
+
+Union \fBi2c_smbus_data\fR is used to store all possible SMBus data.
+
+union \fBi2c_smbus_data\fR {
+.br
+	\fB__u8\fR byte;
+.br
+	\fB__u16\fR word;
+.br
+	\fB__u8\fR block[I2C_SMBUS_BLOCK_MAX + 2];
+.br
+};
+
+\fBblock[0]\fR is used for length and the last byte is reserved.
+If you use the higher-level functions, this structure will be filled out for
+you so you do not have to care about the details.
+Only if you call \fBi2c_smbus_access()\fR directly, you need to fill it out
+yourself.
+
+.SH AUTHOR
+Simon G. Vogl, Frodo Looijaard, Jean Delvare and others

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/2] libi2c: Add a manual page to document the API

[Jean Delvare]

On Thu, 23 Jan 2020 11:03:55 +0100, Jean Delvare wrote:
> It is good practice for a library to come with a complete API description.
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> ---
>  lib/libi2c.3 |  137 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 137 insertions(+)
> (...)

I just realized I forgot the integration with the Makefile so that the
manual page will get installed. Sorry about that, I'll fix that in v2.

-- 
Jean Delvare
SUSE L3 Support

[PATCH 2/2] docs: i2c: dev-interface: document the actual implementation

[Jean Delvare]

The old i2c-dev API based on inline functions is long gone, we have
libi2c now which implements the same as real functions and comes with
complete API documentation. Update the dev-interface documentation
file accordingly to only mention what can be done without the
library, and redirect the reader to the libi2c manual page for the
rest.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Reported-by: Lei YU <mine260309@gmail.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: Luca Ceresoli <luca@lucaceresoli.net>
---
 Documentation/i2c/dev-interface.rst |   67 ++++++++++++++++--------------------
 1 file changed, 30 insertions(+), 37 deletions(-)

--- linux-5.4.orig/Documentation/i2c/dev-interface.rst	2020-01-16 10:32:18.436175325 +0100
+++ linux-5.4/Documentation/i2c/dev-interface.rst	2020-01-16 10:32:25.205247017 +0100
@@ -22,10 +22,11 @@ C example
 =========
 
 So let's say you want to access an I2C adapter from a C program.
-First, you need to include these two headers::
+First, you need to include these three header files::
 
+  #include <linux/i2c.h>
   #include <linux/i2c-dev.h>
-  #include <i2c/smbus.h>
+  #include <sys/ioctl.h>
 
 Now, you have to decide which adapter you want to access. You should
 inspect /sys/class/i2c-dev/ or run "i2cdetect -l" to decide this.
@@ -62,19 +63,23 @@ I2C to communicate with your device. SMB
   __u8 reg = 0x10; /* Device register to access */
   __s32 res;
   char buf[10];
+  struct i2c_smbus_ioctl_data args;
+  union i2c_smbus_data data;
 
   /* Using SMBus commands */
-  res = i2c_smbus_read_word_data(file, reg);
+  args.read_write = I2C_SMBUS_READ;
+  args.command = reg;
+  args.size = I2C_SMBUS_WORD_DATA;
+  args.data = &data;
+
+  res = ioctl(file, I2C_SMBUS, &args);
   if (res < 0) {
     /* ERROR HANDLING: I2C transaction failed */
   } else {
-    /* res contains the read word */
+    /* data contains the read word */
   }
 
-  /*
-   * Using I2C Write, equivalent of
-   * i2c_smbus_write_word_data(file, reg, 0x6543)
-   */
+  /* Using I2C Write */
   buf[0] = reg;
   buf[1] = 0x43;
   buf[2] = 0x65;
@@ -82,7 +87,7 @@ I2C to communicate with your device. SMB
     /* ERROR HANDLING: I2C transaction failed */
   }
 
-  /* Using I2C Read, equivalent of i2c_smbus_read_byte(file) */
+  /* Using I2C Read */
   if (read(file, buf, 1) != 1) {
     /* ERROR HANDLING: I2C transaction failed */
   } else {
@@ -93,10 +98,8 @@ Note that only a subset of the I2C and S
 the means of read() and write() calls. In particular, so-called combined
 transactions (mixing read and write messages in the same transaction)
 aren't supported. For this reason, this interface is almost never used by
-user-space programs.
-
-IMPORTANT: because of the use of inline functions, you *have* to use
-'-O' or some variation when you compile your program!
+user-space programs. See the I2C_RDWR ioctl below for combined transaction
+support.
 
 
 Full interface description
@@ -107,7 +110,11 @@ Full interface description
 ``ioctl(file, I2C_SLAVE, long addr)``
   Change slave address. The address is passed in the 7 lower bits of the
   argument (except for 10 bit addresses, passed in the 10 lower bits in this
-  case).
+  case). Fails if the address is already busy.
+
+``ioctl(file, I2C_SLAVE_FORCE, long addr)``
+  Same as I2C_SLAVE but succeeds even if the address was already busy.
+  Dangerous, don't use.
 
 ``ioctl(file, I2C_TENBIT, long select)``
   Selects ten bit addresses if select not equals 0, selects normal 7 bit
@@ -148,30 +155,16 @@ You can do plain I2C transactions by usi
 You do not need to pass the address byte; instead, set it through
 ioctl I2C_SLAVE before you try to access the device.
 
-You can do SMBus level transactions (see documentation file smbus-protocol
-for details) through the following functions::
 
-  __s32 i2c_smbus_write_quick(int file, __u8 value);
-  __s32 i2c_smbus_read_byte(int file);
-  __s32 i2c_smbus_write_byte(int file, __u8 value);
-  __s32 i2c_smbus_read_byte_data(int file, __u8 command);
-  __s32 i2c_smbus_write_byte_data(int file, __u8 command, __u8 value);
-  __s32 i2c_smbus_read_word_data(int file, __u8 command);
-  __s32 i2c_smbus_write_word_data(int file, __u8 command, __u16 value);
-  __s32 i2c_smbus_process_call(int file, __u8 command, __u16 value);
-  __s32 i2c_smbus_read_block_data(int file, __u8 command, __u8 *values);
-  __s32 i2c_smbus_write_block_data(int file, __u8 command, __u8 length,
-                                   __u8 *values);
-
-All these transactions return -1 on failure; you can read errno to see
-what happened. The 'write' transactions return 0 on success; the
-'read' transactions return the read value, except for read_block, which
-returns the number of values read. The block buffers need not be longer
-than 32 bytes.
-
-The above functions are made available by linking against the libi2c library,
-which is provided by the i2c-tools project.  See:
-https://git.kernel.org/pub/scm/utils/i2c-tools/i2c-tools.git/.
+C Library
+=========
+
+For SMBus level transactions you will want to use libi2c which offers a
+much nicer API similar to the in-kernel API. See the libi2c(3) manual page
+for details. The library is part of i2c-tools:
+https://git.kernel.org/pub/scm/utils/i2c-tools/i2c-tools.git/
+
+Python bindings are also available for libi2c, in the same project.
 
 
 Implementation details


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/2] docs: i2c: dev-interface: document the actual implementation

[Wolfram Sang]

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

On Thu, Jan 23, 2020 at 11:11:37AM +0100, Jean Delvare wrote:
> The old i2c-dev API based on inline functions is long gone, we have
> libi2c now which implements the same as real functions and comes with
> complete API documentation. Update the dev-interface documentation
> file accordingly to only mention what can be done without the
> library, and redirect the reader to the libi2c manual page for the
> rest.
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Reported-by: Lei YU <mine260309@gmail.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: Luca Ceresoli <luca@lucaceresoli.net>

I wonder if we shouldn't move the 'C library'  paragraph before the 'C
example'? To make sure people are aware of it (and use it) early before
digging into the low-level C code?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] docs: i2c: dev-interface: document the actual implementation

[Luca Ceresoli]

Hi Jean, Wolfram,

On 23/01/20 12:09, Wolfram Sang wrote:
> On Thu, Jan 23, 2020 at 11:11:37AM +0100, Jean Delvare wrote:
>> The old i2c-dev API based on inline functions is long gone, we have
>> libi2c now which implements the same as real functions and comes with
>> complete API documentation. Update the dev-interface documentation
>> file accordingly to only mention what can be done without the
>> library, and redirect the reader to the libi2c manual page for the
>> rest.
>>
>> Signed-off-by: Jean Delvare <jdelvare@suse.de>
>> Reported-by: Lei YU <mine260309@gmail.com>
>> Cc: Wolfram Sang <wsa@the-dreams.de>
>> Cc: Luca Ceresoli <luca@lucaceresoli.net>
> 
> I wonder if we shouldn't move the 'C library'  paragraph before the 'C
> example'? To make sure people are aware of it (and use it) early before
> digging into the low-level C code?

I agree, it would be better. For the rest it look good.

-- 
Luca

Re: [PATCH 2/2] docs: i2c: dev-interface: document the actual implementation

[Jean Delvare]

Hi Luca, Wolfram,

On Thu, 23 Jan 2020 14:42:33 +0100, Luca Ceresoli wrote:
> On 23/01/20 12:09, Wolfram Sang wrote:
> > On Thu, Jan 23, 2020 at 11:11:37AM +0100, Jean Delvare wrote:  
> >> The old i2c-dev API based on inline functions is long gone, we have
> >> libi2c now which implements the same as real functions and comes with
> >> complete API documentation. Update the dev-interface documentation
> >> file accordingly to only mention what can be done without the
> >> library, and redirect the reader to the libi2c manual page for the
> >> rest.
> >>
> >> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> >> Reported-by: Lei YU <mine260309@gmail.com>
> >> Cc: Wolfram Sang <wsa@the-dreams.de>
> >> Cc: Luca Ceresoli <luca@lucaceresoli.net>  
> > 
> > I wonder if we shouldn't move the 'C library'  paragraph before the 'C
> > example'? To make sure people are aware of it (and use it) early before
> > digging into the low-level C code?  
> 
> I agree, it would be better. For the rest it look good.

Hmmm. It's not like you can do everything with libi2c so you should
always use it. There are several things that can't be done with libi2c
so you will have to do them "manually". Anything that doesn't fit in the
SMBus specification basically. As a matter of fact, i2ctransfer does
not use libi2c.

Also, even when using libi2c, you still need to explicitly open the
device node, set the slave address, and close the device when you are
done (just seeing that's missing from the C example but it should be
added). So the C example is still relevant even if you use libi2c.

So I'm not sure swapping the sections makes that much sense. What would
help on the other hand is to add a pointer to the C library section at
the point of the C example where using the library would simplify the
code. Would that work for you?

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/2] docs: i2c: dev-interface: document the actual implementation

[Luca Ceresoli]

Hi,

On 03/02/20 14:27, Jean Delvare wrote:
> Hi Luca, Wolfram,
> 
> On Thu, 23 Jan 2020 14:42:33 +0100, Luca Ceresoli wrote:
>> On 23/01/20 12:09, Wolfram Sang wrote:
>>> On Thu, Jan 23, 2020 at 11:11:37AM +0100, Jean Delvare wrote:  
>>>> The old i2c-dev API based on inline functions is long gone, we have
>>>> libi2c now which implements the same as real functions and comes with
>>>> complete API documentation. Update the dev-interface documentation
>>>> file accordingly to only mention what can be done without the
>>>> library, and redirect the reader to the libi2c manual page for the
>>>> rest.
>>>>
>>>> Signed-off-by: Jean Delvare <jdelvare@suse.de>
>>>> Reported-by: Lei YU <mine260309@gmail.com>
>>>> Cc: Wolfram Sang <wsa@the-dreams.de>
>>>> Cc: Luca Ceresoli <luca@lucaceresoli.net>  
>>>
>>> I wonder if we shouldn't move the 'C library'  paragraph before the 'C
>>> example'? To make sure people are aware of it (and use it) early before
>>> digging into the low-level C code?  
>>
>> I agree, it would be better. For the rest it look good.
> 
> Hmmm. It's not like you can do everything with libi2c so you should
> always use it. There are several things that can't be done with libi2c
> so you will have to do them "manually". Anything that doesn't fit in the
> SMBus specification basically. As a matter of fact, i2ctransfer does
> not use libi2c.
> 
> Also, even when using libi2c, you still need to explicitly open the
> device node, set the slave address, and close the device when you are
> done (just seeing that's missing from the C example but it should be
> added). So the C example is still relevant even if you use libi2c.
> 
> So I'm not sure swapping the sections makes that much sense. What would
> help on the other hand is to add a pointer to the C library section at
> the point of the C example where using the library would simplify the
> code. Would that work for you?

In my opinion we should first document the recommended way. Assuming
libi2c is the recommended way for all uses it is capable of, that means
documenting libi2c first.

Additionally, before documenting any of them I'd add a preamble similar
to: "The I2C device can be accessed from user space either using the
libi2c library or using low-level C functions directly. libi2c is more
high-level but has limited functionality.". This is so it's clear to the
reader from the beginning that there are two alternative approaches,
whose explanation will follow.

My 2c,
-- 
Luca

Re: [PATCH 2/2] docs: i2c: dev-interface: document the actual implementation

[Wolfram Sang]

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


> In my opinion we should first document the recommended way. Assuming
> libi2c is the recommended way for all uses it is capable of, that means
> documenting libi2c first.
> 
> Additionally, before documenting any of them I'd add a preamble similar
> to: "The I2C device can be accessed from user space either using the
> libi2c library or using low-level C functions directly. libi2c is more
> high-level but has limited functionality.". This is so it's clear to the
> reader from the beginning that there are two alternative approaches,
> whose explanation will follow.

This! If we have such a paragraph, then while I still think swapping
makes a bit more sense, the order is not so important to me anymore.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] docs: i2c: dev-interface: document the actual implementation

[Jean Delvare]

Hi Luca,

On Mon, 3 Feb 2020 17:35:15 +0100, Luca Ceresoli wrote:
> On 03/02/20 14:27, Jean Delvare wrote:
> > Hmmm. It's not like you can do everything with libi2c so you should
> > always use it. There are several things that can't be done with libi2c
> > so you will have to do them "manually". Anything that doesn't fit in the
> > SMBus specification basically. As a matter of fact, i2ctransfer does
> > not use libi2c.
> > 
> > Also, even when using libi2c, you still need to explicitly open the
> > device node, set the slave address, and close the device when you are
> > done (just seeing that's missing from the C example but it should be
> > added). So the C example is still relevant even if you use libi2c.
> > 
> > So I'm not sure swapping the sections makes that much sense. What would
> > help on the other hand is to add a pointer to the C library section at
> > the point of the C example where using the library would simplify the
> > code. Would that work for you?  
> 
> In my opinion we should first document the recommended way. Assuming
> libi2c is the recommended way for all uses it is capable of, that means
> documenting libi2c first.
> 
> Additionally, before documenting any of them I'd add a preamble similar
> to: "The I2C device can be accessed from user space either using the
> libi2c library or using low-level C functions directly. libi2c is more
> high-level but has limited functionality.". This is so it's clear to the
> reader from the beginning that there are two alternative approaches,
> whose explanation will follow.

Thank you for the suggested improvements, I agree they will make the
documentation easier to read. I'll integrate these changes in v2.

It might also make sense to add an example of the C library usage in
libi2c(3).

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/2] docs: i2c: dev-interface: document the actual implementation

[Wolfram Sang]

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


>  So let's say you want to access an I2C adapter from a C program.
> -First, you need to include these two headers::
> +First, you need to include these three header files::

Just stumbled over it: maybe just drop the 'three'? No number makes
maintenance easier and it is not important for the user, I'd think.

>  
> +  #include <linux/i2c.h>
>    #include <linux/i2c-dev.h>
> -  #include <i2c/smbus.h>
> +  #include <sys/ioctl.h>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] docs: i2c: dev-interface: document the actual implementation

[Jean Delvare]

On Wed, 29 Jan 2020 22:13:37 +0100, Wolfram Sang wrote:
> >  So let's say you want to access an I2C adapter from a C program.
> > -First, you need to include these two headers::
> > +First, you need to include these three header files::  
> 
> Just stumbled over it: maybe just drop the 'three'? No number makes
> maintenance easier and it is not important for the user, I'd think.
> 
> >  
> > +  #include <linux/i2c.h>
> >    #include <linux/i2c-dev.h>
> > -  #include <i2c/smbus.h>
> > +  #include <sys/ioctl.h>  

Agreed, fixed.

Thanks,
-- 
Jean Delvare
SUSE L3 Support