| Message ID | 1437744042-24471-1-git-send-email-cmo@melexis.com |
|---|---|
| State | Rejected |
| Headers | show |
I hope you have received the below patch? I have received undelivered notice from Simon mailserver, so I hope some of you can accept this patch. On 24 July 2015 at 15:20, Crt Mori <cmo@melexis.com> wrote: > If you want to send more than 1 byte as command to slave you need to go away > from the smbus (which has a 8 bit command defined in specification). i2c has > no such limitation in specification and since we want to have a longer than > 1 byte command, we need additional function to handle it. With outside > buffers we have also avoided endianness problems (if we would just define a > u16 command instead of u8). Also there are now no limitations (as per i2c > specification) of command length. > Addressed read function is when we have a slave device which accepts commands > longer than byte or some subcommands in same write string before issuing a > repeat start before read cycle to read more than byte (or byte) from slave > device. > > Signed-off-by: Crt Mori <cmo@melexis.com> > --- > drivers/i2c/i2c-core.c | 40 ++++++++++++++++++++++++++++++++++++++++ > include/linux/i2c.h | 4 ++++ > 2 files changed, 44 insertions(+) > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c > index 069a41f..bccf83f 100644 > --- a/drivers/i2c/i2c-core.c > +++ b/drivers/i2c/i2c-core.c > @@ -2980,6 +2980,46 @@ int i2c_slave_unregister(struct i2c_client *client) > EXPORT_SYMBOL_GPL(i2c_slave_unregister); > #endif > > +/** > + * i2c_addressed_read - writes read commands with desired length and read > + * desired length of data. > + * @client: Handle to slave device > + * @command: Pointer to command location > + * @cmd_len: Number of command bytes on command location > + * @data: Pointer to read data location > + * @data_len: Expected number of read bytes > + * > + * Addressed read on slave device when command is longer than byte. Returns > + * negative errno on error and 0 on success. > + */ > +s32 i2c_addressed_read(const struct i2c_client *client, u8 *command, > + u16 cmd_len, u8 *data, u16 data_len) > +{ > + s32 status; > + struct i2c_msg msg[2] = { > + { > + .addr = client->addr, > + .flags = client->flags, > + .len = cmd_len, > + .buf = command, > + }, > + { > + .addr = client->addr, > + .flags = client->flags | I2C_M_RD, > + .len = data_len, > + .buf = data, > + }, > + }; > + > + status = i2c_transfer(client->adapter, msg, 2); > + > + if (status < 0) > + return status; > + > + return 0; > +} > +EXPORT_SYMBOL(i2c_addressed_read); > + > MODULE_AUTHOR("Simon G. Vogl <simon@tk.uni-linz.ac.at>"); > MODULE_DESCRIPTION("I2C-Bus main module"); > MODULE_LICENSE("GPL"); > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index e83a738..d3cc1af 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -121,6 +121,10 @@ extern s32 i2c_smbus_read_i2c_block_data(const struct i2c_client *client, > extern s32 i2c_smbus_write_i2c_block_data(const struct i2c_client *client, > u8 command, u8 length, > const u8 *values); > + > +s32 i2c_addressed_read(const struct i2c_client *client, u8 *command, > + u16 cmd_len, u8 *data, u16 data_len); > + > #endif /* I2C */ > > /** > -- > 2.1.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 24, 2015 at 03:20:42PM +0200, Crt Mori wrote: > If you want to send more than 1 byte as command to slave you need to go away > from the smbus (which has a 8 bit command defined in specification). i2c has > no such limitation in specification and since we want to have a longer than > 1 byte command, we need additional function to handle it. With outside > buffers we have also avoided endianness problems (if we would just define a > u16 command instead of u8). Also there are now no limitations (as per i2c > specification) of command length. > Addressed read function is when we have a slave device which accepts commands > longer than byte or some subcommands in same write string before issuing a > repeat start before read cycle to read more than byte (or byte) from slave > device. > > Signed-off-by: Crt Mori <cmo@melexis.com> I don't see much gain over using i2c_transfer directly to be honest. Do you see much use of that in the kernel? Any other gain I missed? And if at all, the function should be probably named i2c_write_then_read or something. > --- > drivers/i2c/i2c-core.c | 40 ++++++++++++++++++++++++++++++++++++++++ > include/linux/i2c.h | 4 ++++ > 2 files changed, 44 insertions(+) > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c > index 069a41f..bccf83f 100644 > --- a/drivers/i2c/i2c-core.c > +++ b/drivers/i2c/i2c-core.c > @@ -2980,6 +2980,46 @@ int i2c_slave_unregister(struct i2c_client *client) > EXPORT_SYMBOL_GPL(i2c_slave_unregister); > #endif > > +/** > + * i2c_addressed_read - writes read commands with desired length and read > + * desired length of data. > + * @client: Handle to slave device > + * @command: Pointer to command location > + * @cmd_len: Number of command bytes on command location > + * @data: Pointer to read data location > + * @data_len: Expected number of read bytes > + * > + * Addressed read on slave device when command is longer than byte. Returns > + * negative errno on error and 0 on success. > + */ > +s32 i2c_addressed_read(const struct i2c_client *client, u8 *command, > + u16 cmd_len, u8 *data, u16 data_len) > +{ > + s32 status; > + struct i2c_msg msg[2] = { > + { > + .addr = client->addr, > + .flags = client->flags, > + .len = cmd_len, > + .buf = command, > + }, > + { > + .addr = client->addr, > + .flags = client->flags | I2C_M_RD, > + .len = data_len, > + .buf = data, > + }, > + }; > + > + status = i2c_transfer(client->adapter, msg, 2); > + > + if (status < 0) > + return status; > + > + return 0; > +} > +EXPORT_SYMBOL(i2c_addressed_read); > + > MODULE_AUTHOR("Simon G. Vogl <simon@tk.uni-linz.ac.at>"); > MODULE_DESCRIPTION("I2C-Bus main module"); > MODULE_LICENSE("GPL"); > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index e83a738..d3cc1af 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -121,6 +121,10 @@ extern s32 i2c_smbus_read_i2c_block_data(const struct i2c_client *client, > extern s32 i2c_smbus_write_i2c_block_data(const struct i2c_client *client, > u8 command, u8 length, > const u8 *values); > + > +s32 i2c_addressed_read(const struct i2c_client *client, u8 *command, > + u16 cmd_len, u8 *data, u16 data_len); > + > #endif /* I2C */ > > /** > -- > 2.1.4 >
On 9 August 2015 at 09:40, Wolfram Sang <wsa@the-dreams.de> wrote: > On Fri, Jul 24, 2015 at 03:20:42PM +0200, Crt Mori wrote: >> If you want to send more than 1 byte as command to slave you need to go away >> from the smbus (which has a 8 bit command defined in specification). i2c has >> no such limitation in specification and since we want to have a longer than >> 1 byte command, we need additional function to handle it. With outside >> buffers we have also avoided endianness problems (if we would just define a >> u16 command instead of u8). Also there are now no limitations (as per i2c >> specification) of command length. >> Addressed read function is when we have a slave device which accepts commands >> longer than byte or some subcommands in same write string before issuing a >> repeat start before read cycle to read more than byte (or byte) from slave >> device. >> >> Signed-off-by: Crt Mori <cmo@melexis.com> > > I don't see much gain over using i2c_transfer directly to be honest. Do > you see much use of that in the kernel? Any other gain I missed? And if > at all, the function should be probably named i2c_write_then_read or > something. > This debate is what I wanted to have. The new Melexis sensor will be using it and since I think some others might also be using it in future I would rather put it to i2c. It is more i2c command than sensor specific so I think it fits into i2c. Another gain is that when number of functions each device does increase, as well as ram/rom addressing read commands with longer than 8-bit register read commands will be needed. Especially since most sensors have digital design of i2c interface (not software) which means native access to registers is required, therefor 8bit read addressing (smbus) might not be suitable. I can fix the function name, but in core this is a master read command. >> --- >> drivers/i2c/i2c-core.c | 40 ++++++++++++++++++++++++++++++++++++++++ >> include/linux/i2c.h | 4 ++++ >> 2 files changed, 44 insertions(+) >> >> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c >> index 069a41f..bccf83f 100644 >> --- a/drivers/i2c/i2c-core.c >> +++ b/drivers/i2c/i2c-core.c >> @@ -2980,6 +2980,46 @@ int i2c_slave_unregister(struct i2c_client *client) >> EXPORT_SYMBOL_GPL(i2c_slave_unregister); >> #endif >> >> +/** >> + * i2c_addressed_read - writes read commands with desired length and read >> + * desired length of data. >> + * @client: Handle to slave device >> + * @command: Pointer to command location >> + * @cmd_len: Number of command bytes on command location >> + * @data: Pointer to read data location >> + * @data_len: Expected number of read bytes >> + * >> + * Addressed read on slave device when command is longer than byte. Returns >> + * negative errno on error and 0 on success. >> + */ >> +s32 i2c_addressed_read(const struct i2c_client *client, u8 *command, >> + u16 cmd_len, u8 *data, u16 data_len) >> +{ >> + s32 status; >> + struct i2c_msg msg[2] = { >> + { >> + .addr = client->addr, >> + .flags = client->flags, >> + .len = cmd_len, >> + .buf = command, >> + }, >> + { >> + .addr = client->addr, >> + .flags = client->flags | I2C_M_RD, >> + .len = data_len, >> + .buf = data, >> + }, >> + }; >> + >> + status = i2c_transfer(client->adapter, msg, 2); >> + >> + if (status < 0) >> + return status; >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(i2c_addressed_read); >> + >> MODULE_AUTHOR("Simon G. Vogl <simon@tk.uni-linz.ac.at>"); >> MODULE_DESCRIPTION("I2C-Bus main module"); >> MODULE_LICENSE("GPL"); >> diff --git a/include/linux/i2c.h b/include/linux/i2c.h >> index e83a738..d3cc1af 100644 >> --- a/include/linux/i2c.h >> +++ b/include/linux/i2c.h >> @@ -121,6 +121,10 @@ extern s32 i2c_smbus_read_i2c_block_data(const struct i2c_client *client, >> extern s32 i2c_smbus_write_i2c_block_data(const struct i2c_client *client, >> u8 command, u8 length, >> const u8 *values); >> + >> +s32 i2c_addressed_read(const struct i2c_client *client, u8 *command, >> + u16 cmd_len, u8 *data, u16 data_len); >> + >> #endif /* I2C */ >> >> /** >> -- >> 2.1.4 >> -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10 August 2015 at 09:26, Crt Mori <cmo@melexis.com> wrote: > On 9 August 2015 at 09:40, Wolfram Sang <wsa@the-dreams.de> wrote: >> On Fri, Jul 24, 2015 at 03:20:42PM +0200, Crt Mori wrote: >>> If you want to send more than 1 byte as command to slave you need to go away >>> from the smbus (which has a 8 bit command defined in specification). i2c has >>> no such limitation in specification and since we want to have a longer than >>> 1 byte command, we need additional function to handle it. With outside >>> buffers we have also avoided endianness problems (if we would just define a >>> u16 command instead of u8). Also there are now no limitations (as per i2c >>> specification) of command length. >>> Addressed read function is when we have a slave device which accepts commands >>> longer than byte or some subcommands in same write string before issuing a >>> repeat start before read cycle to read more than byte (or byte) from slave >>> device. >>> >>> Signed-off-by: Crt Mori <cmo@melexis.com> >> >> I don't see much gain over using i2c_transfer directly to be honest. Do >> you see much use of that in the kernel? Any other gain I missed? And if >> at all, the function should be probably named i2c_write_then_read or >> something. >> > This debate is what I wanted to have. The new Melexis sensor will be > using it and > since I think some others might also be using it in future I would > rather put it to > i2c. It is more i2c command than sensor specific so I think it fits into i2c. > > Another gain is that when number of functions each device does increase, as > well as ram/rom addressing read commands with longer than 8-bit register read > commands will be needed. Especially since most sensors have digital design of > i2c interface (not software) which means native access to registers is required, > therefor 8bit read addressing (smbus) might not be suitable. > > I can fix the function name, but in core this is a master read command. > For example IIO list just got the driver for ms_sensors [1] which uses the same function. I am sure we can find more. [1] http://www.spinics.net/lists/linux-iio/msg19925.html >>> --- >>> drivers/i2c/i2c-core.c | 40 ++++++++++++++++++++++++++++++++++++++++ >>> include/linux/i2c.h | 4 ++++ >>> 2 files changed, 44 insertions(+) >>> >>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c >>> index 069a41f..bccf83f 100644 >>> --- a/drivers/i2c/i2c-core.c >>> +++ b/drivers/i2c/i2c-core.c >>> @@ -2980,6 +2980,46 @@ int i2c_slave_unregister(struct i2c_client *client) >>> EXPORT_SYMBOL_GPL(i2c_slave_unregister); >>> #endif >>> >>> +/** >>> + * i2c_addressed_read - writes read commands with desired length and read >>> + * desired length of data. >>> + * @client: Handle to slave device >>> + * @command: Pointer to command location >>> + * @cmd_len: Number of command bytes on command location >>> + * @data: Pointer to read data location >>> + * @data_len: Expected number of read bytes >>> + * >>> + * Addressed read on slave device when command is longer than byte. Returns >>> + * negative errno on error and 0 on success. >>> + */ >>> +s32 i2c_addressed_read(const struct i2c_client *client, u8 *command, >>> + u16 cmd_len, u8 *data, u16 data_len) >>> +{ >>> + s32 status; >>> + struct i2c_msg msg[2] = { >>> + { >>> + .addr = client->addr, >>> + .flags = client->flags, >>> + .len = cmd_len, >>> + .buf = command, >>> + }, >>> + { >>> + .addr = client->addr, >>> + .flags = client->flags | I2C_M_RD, >>> + .len = data_len, >>> + .buf = data, >>> + }, >>> + }; >>> + >>> + status = i2c_transfer(client->adapter, msg, 2); >>> + >>> + if (status < 0) >>> + return status; >>> + >>> + return 0; >>> +} >>> +EXPORT_SYMBOL(i2c_addressed_read); >>> + >>> MODULE_AUTHOR("Simon G. Vogl <simon@tk.uni-linz.ac.at>"); >>> MODULE_DESCRIPTION("I2C-Bus main module"); >>> MODULE_LICENSE("GPL"); >>> diff --git a/include/linux/i2c.h b/include/linux/i2c.h >>> index e83a738..d3cc1af 100644 >>> --- a/include/linux/i2c.h >>> +++ b/include/linux/i2c.h >>> @@ -121,6 +121,10 @@ extern s32 i2c_smbus_read_i2c_block_data(const struct i2c_client *client, >>> extern s32 i2c_smbus_write_i2c_block_data(const struct i2c_client *client, >>> u8 command, u8 length, >>> const u8 *values); >>> + >>> +s32 i2c_addressed_read(const struct i2c_client *client, u8 *command, >>> + u16 cmd_len, u8 *data, u16 data_len); >>> + >>> #endif /* I2C */ >>> >>> /** >>> -- >>> 2.1.4 >>> -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> This debate is what I wanted to have. The new Melexis sensor will be > using it and since I think some others might also be using it in > future I would rather put it to i2c. It is more i2c command than > sensor specific so I think it fits into i2c. There is no thing as 'I2C command'. There are just I2C messages combined into a transfer. I think you mix SMBus and I2C terminology here. > Another gain is that when number of functions each device does increase, as > well as ram/rom addressing read commands with longer than 8-bit register read > commands will be needed. Especially since most sensors have digital design of > i2c interface (not software) which means native access to registers is required, > therefor 8bit read addressing (smbus) might not be suitable. Adding a function to i2c-core will grow the kernel for everyone. So, it really has to be justified. So, here is the deal: If you can send a series which fixes existing drivers to use your new function and it shows that it really saves code, we can add it. Otherwise "thinking it might be useful in the future" is too vague for me in this case. > I can fix the function name, but in core this is a master read command. What "core" are you referring to? You set up a write and a read message, no?
On 10 August 2015 at 11:13, Wolfram Sang <wsa@the-dreams.de> wrote: > > > > This debate is what I wanted to have. The new Melexis sensor will be > > using it and since I think some others might also be using it in > > future I would rather put it to i2c. It is more i2c command than > > sensor specific so I think it fits into i2c. > > There is no thing as 'I2C command'. There are just I2C messages combined > into a transfer. I think you mix SMBus and I2C terminology here. > This is true, but write/read sequence can also be a request/reply which is basically a command for reading. > > Another gain is that when number of functions each device does increase, as > > well as ram/rom addressing read commands with longer than 8-bit register read > > commands will be needed. Especially since most sensors have digital design of > > i2c interface (not software) which means native access to registers is required, > > therefor 8bit read addressing (smbus) might not be suitable. > > Adding a function to i2c-core will grow the kernel for everyone. So, it > really has to be justified. So, here is the deal: If you can send a > series which fixes existing drivers to use your new function and it > shows that it really saves code, we can add it. Otherwise "thinking it > might be useful in the future" is too vague for me in this case. > Same function as my exists in: ./drivers/base/regmap/regmap-i2c.c , maybe it is better just to use that? So far in IIO I have found: -./drivers/iio/accel/bmc150-accel.c:913 -./drivers/iio/gyro/itg3200_buffer.c:26 itg3200_read_all_channels Grep showed me quite a bit more in drivers for example (won't list all): -./drivers/mfd/menelaus.c:899 menelaus_read_time -./drivers/mfd/tps65912-i2c.c:25 tps65912_i2c_read -./drivers/rtc/rtc-pcf8563.c:81 pcf8563_read_block_data -./drivers/rtc/rtc-ds1672.c:33 ds1672_get_datetime -./drivers/power/ltc2941-battery-gauge.c:90 ltc294x_read_regs Also in other parts: -./sound/soc/codecs/rl6347a.c:79 rl6347a_hw_read -./sound/soc/codecs/tas5086.c:202 tas5086_reg_read -./sound/soc/codecs/sigmadsp-i2c.c: 37 sigmadsp_read_i2c as well as good resemblance: -./arch/arm/mach-davinci/board-dm644x-evm.c:517 dm6444evm_msp430_get_pins Do you want me to prepare a patch to fix all this? > > I can fix the function name, but in core this is a master read command. > > What "core" are you referring to? > > You set up a write and a read message, no? > True, but focus is on read -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 10, 2015 at 12:06:07PM +0200, Crt Mori wrote: > On 10 August 2015 at 11:13, Wolfram Sang <wsa@the-dreams.de> wrote: > > > > > > > This debate is what I wanted to have. The new Melexis sensor will be > > > using it and since I think some others might also be using it in > > > future I would rather put it to i2c. It is more i2c command than > > > sensor specific so I think it fits into i2c. > > > > There is no thing as 'I2C command'. There are just I2C messages combined > > into a transfer. I think you mix SMBus and I2C terminology here. > > > This is true, but write/read sequence can also be a request/reply > which is basically a command for reading. This interpretation is already a layer above. In I2C, you send a message and then you receive one. > Same function as my exists in: ./drivers/base/regmap/regmap-i2c.c , > maybe it is better just to use that? Yes! Should have thought of that. Plus, you get more benefits like caching if you want. Case closed, thanks!
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index 069a41f..bccf83f 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -2980,6 +2980,46 @@ int i2c_slave_unregister(struct i2c_client *client) EXPORT_SYMBOL_GPL(i2c_slave_unregister); #endif +/** + * i2c_addressed_read - writes read commands with desired length and read + * desired length of data. + * @client: Handle to slave device + * @command: Pointer to command location + * @cmd_len: Number of command bytes on command location + * @data: Pointer to read data location + * @data_len: Expected number of read bytes + * + * Addressed read on slave device when command is longer than byte. Returns + * negative errno on error and 0 on success. + */ +s32 i2c_addressed_read(const struct i2c_client *client, u8 *command, + u16 cmd_len, u8 *data, u16 data_len) +{ + s32 status; + struct i2c_msg msg[2] = { + { + .addr = client->addr, + .flags = client->flags, + .len = cmd_len, + .buf = command, + }, + { + .addr = client->addr, + .flags = client->flags | I2C_M_RD, + .len = data_len, + .buf = data, + }, + }; + + status = i2c_transfer(client->adapter, msg, 2); + + if (status < 0) + return status; + + return 0; +} +EXPORT_SYMBOL(i2c_addressed_read); + MODULE_AUTHOR("Simon G. Vogl <simon@tk.uni-linz.ac.at>"); MODULE_DESCRIPTION("I2C-Bus main module"); MODULE_LICENSE("GPL"); diff --git a/include/linux/i2c.h b/include/linux/i2c.h index e83a738..d3cc1af 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -121,6 +121,10 @@ extern s32 i2c_smbus_read_i2c_block_data(const struct i2c_client *client, extern s32 i2c_smbus_write_i2c_block_data(const struct i2c_client *client, u8 command, u8 length, const u8 *values); + +s32 i2c_addressed_read(const struct i2c_client *client, u8 *command, + u16 cmd_len, u8 *data, u16 data_len); + #endif /* I2C */ /**
If you want to send more than 1 byte as command to slave you need to go away from the smbus (which has a 8 bit command defined in specification). i2c has no such limitation in specification and since we want to have a longer than 1 byte command, we need additional function to handle it. With outside buffers we have also avoided endianness problems (if we would just define a u16 command instead of u8). Also there are now no limitations (as per i2c specification) of command length. Addressed read function is when we have a slave device which accepts commands longer than byte or some subcommands in same write string before issuing a repeat start before read cycle to read more than byte (or byte) from slave device. Signed-off-by: Crt Mori <cmo@melexis.com> --- drivers/i2c/i2c-core.c | 40 ++++++++++++++++++++++++++++++++++++++++ include/linux/i2c.h | 4 ++++ 2 files changed, 44 insertions(+)