This driver tries to access a block of data on a i2c bus and it tries
to manually make a device command frame and a consecutively read frame,
then uses i2c_transfer() to read data. But this has already been
implemented in i2c_smbus_read_i2c_block_data().
Sorry for not having this device by my hand, which is a little expansive
for me, but I have another i2c device and tested with both i2c_transfer()
and i2c_smbus_read_i2c_block_data() and they all ends the same.
I'm not familiar with the SMBus, don't know if the lidar_smbus_xfer()
function is the same as i2c_smbus_read_block_data()? The original code
is commented with something I'm not sure, but I think if it's a standard
SMBus, it should be able to use in here.
Hoping for someone to explain.
Signed-off-by: Song Qiang <[email protected]>
---
.../iio/proximity/pulsedlight-lidar-lite-v2.c | 18 +-----------------
1 file changed, 1 insertion(+), 17 deletions(-)
diff --git a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
index 47af54f14756..ca880ba8e820 100644
--- a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
+++ b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
@@ -63,23 +63,7 @@ static const struct iio_chan_spec lidar_channels[] = {
static int lidar_i2c_xfer(struct lidar_data *data, u8 reg, u8 *val, int len)
{
- struct i2c_client *client = data->client;
- struct i2c_msg msg[2];
- int ret;
-
- msg[0].addr = client->addr;
- msg[0].flags = client->flags | I2C_M_STOP;
- msg[0].len = 1;
- msg[0].buf = (char *) ®
-
- msg[1].addr = client->addr;
- msg[1].flags = client->flags | I2C_M_RD;
- msg[1].len = len;
- msg[1].buf = (char *) val;
-
- ret = i2c_transfer(client->adapter, msg, 2);
-
- return (ret == 2) ? 0 : -EIO;
+ return i2c_smbus_read_i2c_block_data(data->client, reg, len, val);
}
static int lidar_smbus_xfer(struct lidar_data *data, u8 reg, u8 *val, int len)
--
2.17.1
On Wed, Sep 12, 2018 at 8:51 PM Song Qiang <[email protected]> wrote:
>
> This driver tries to access a block of data on a i2c bus and it tries
> to manually make a device command frame and a consecutively read frame,
> then uses i2c_transfer() to read data. But this has already been
> implemented in i2c_smbus_read_i2c_block_data().
> Sorry for not having this device by my hand, which is a little expansive
> for me, but I have another i2c device and tested with both i2c_transfer()
> and i2c_smbus_read_i2c_block_data() and they all ends the same.
> I'm not familiar with the SMBus, don't know if the lidar_smbus_xfer()
> function is the same as i2c_smbus_read_block_data()? The original code
> is commented with something I'm not sure, but I think if it's a standard
> SMBus, it should be able to use in here.
> Hoping for someone to explain.
>
Yes actually there is a reason for this insanity!
It isn't actually SMBUS (note the lidar_smbus_xfer function below it)
and has a odd way to read registers via I2C.
Basically the I2C_M_STOP flags is the reason you can use the standard
i2c_smbus_read_i2c_block_data().
Page 6 in this datasheet
* http://static.garmin.com/pumac/LIDAR_Lite_v3_Operation_Manual_and_Technical_Specifications.pdf
> Signed-off-by: Song Qiang <[email protected]>
> ---
> .../iio/proximity/pulsedlight-lidar-lite-v2.c | 18 +-----------------
> 1 file changed, 1 insertion(+), 17 deletions(-)
>
> diff --git a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> index 47af54f14756..ca880ba8e820 100644
> --- a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> +++ b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> @@ -63,23 +63,7 @@ static const struct iio_chan_spec lidar_channels[] = {
>
> static int lidar_i2c_xfer(struct lidar_data *data, u8 reg, u8 *val, int len)
> {
> - struct i2c_client *client = data->client;
> - struct i2c_msg msg[2];
> - int ret;
> -
> - msg[0].addr = client->addr;
> - msg[0].flags = client->flags | I2C_M_STOP;
> - msg[0].len = 1;
> - msg[0].buf = (char *) ®
> -
> - msg[1].addr = client->addr;
> - msg[1].flags = client->flags | I2C_M_RD;
> - msg[1].len = len;
> - msg[1].buf = (char *) val;
> -
> - ret = i2c_transfer(client->adapter, msg, 2);
> -
> - return (ret == 2) ? 0 : -EIO;
> + return i2c_smbus_read_i2c_block_data(data->client, reg, len, val);
> }
>
> static int lidar_smbus_xfer(struct lidar_data *data, u8 reg, u8 *val, int len)
> --
> 2.17.1
>
On Thu, Sep 13, 2018 at 02:43:35PM -0700, Matt Ranostay wrote:
> On Wed, Sep 12, 2018 at 8:51 PM Song Qiang <[email protected]> wrote:
> >
> > This driver tries to access a block of data on a i2c bus and it tries
> > to manually make a device command frame and a consecutively read frame,
> > then uses i2c_transfer() to read data. But this has already been
> > implemented in i2c_smbus_read_i2c_block_data().
> > Sorry for not having this device by my hand, which is a little expansive
> > for me, but I have another i2c device and tested with both i2c_transfer()
> > and i2c_smbus_read_i2c_block_data() and they all ends the same.
> > I'm not familiar with the SMBus, don't know if the lidar_smbus_xfer()
> > function is the same as i2c_smbus_read_block_data()? The original code
> > is commented with something I'm not sure, but I think if it's a standard
> > SMBus, it should be able to use in here.
> > Hoping for someone to explain.
> >
>
> Yes actually there is a reason for this insanity!
>
> It isn't actually SMBUS (note the lidar_smbus_xfer function below it)
> and has a odd way to read registers via I2C.
> Basically the I2C_M_STOP flags is the reason you can use the standard
> i2c_smbus_read_i2c_block_data().
>
> Page 6 in this datasheet
> * http://static.garmin.com/pumac/LIDAR_Lite_v3_Operation_Manual_and_Technical_Specifications.pdf
>
> > Signed-off-by: Song Qiang <[email protected]>
> > ---
> > .../iio/proximity/pulsedlight-lidar-lite-v2.c | 18 +-----------------
> > 1 file changed, 1 insertion(+), 17 deletions(-)
> >
> > diff --git a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> > index 47af54f14756..ca880ba8e820 100644
> > --- a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> > +++ b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> > @@ -63,23 +63,7 @@ static const struct iio_chan_spec lidar_channels[] = {
> >
> > static int lidar_i2c_xfer(struct lidar_data *data, u8 reg, u8 *val, int len)
> > {
> > - struct i2c_client *client = data->client;
> > - struct i2c_msg msg[2];
> > - int ret;
> > -
> > - msg[0].addr = client->addr;
> > - msg[0].flags = client->flags | I2C_M_STOP;
> > - msg[0].len = 1;
> > - msg[0].buf = (char *) ®
> > -
> > - msg[1].addr = client->addr;
> > - msg[1].flags = client->flags | I2C_M_RD;
> > - msg[1].len = len;
> > - msg[1].buf = (char *) val;
> > -
> > - ret = i2c_transfer(client->adapter, msg, 2);
> > -
> > - return (ret == 2) ? 0 : -EIO;
> > + return i2c_smbus_read_i2c_block_data(data->client, reg, len, val);
> > }
> >
> > static int lidar_smbus_xfer(struct lidar_data *data, u8 reg, u8 *val, int len)
> > --
> > 2.17.1
> >
Hi Matt,
Thanks for the explanition, now I understand why we have to use
lidar_smbus_xfer() here.
So we can use the standard i2c_smbus_read_i2c_block_data() here?
I'm thinking since you are the author it seems like you have the
hardware, would you mind to test if it's working? I'd appriciate
that.
yours,
Song Qiang
On Thu, Sep 13, 2018 at 11:51:45AM +0800, Song Qiang wrote:
> This driver tries to access a block of data on a i2c bus and it tries
> to manually make a device command frame and a consecutively read frame,
> then uses i2c_transfer() to read data. But this has already been
> implemented in i2c_smbus_read_i2c_block_data().
> Sorry for not having this device by my hand, which is a little expansive
> for me, but I have another i2c device and tested with both i2c_transfer()
> and i2c_smbus_read_i2c_block_data() and they all ends the same.
> I'm not familiar with the SMBus, don't know if the lidar_smbus_xfer()
> function is the same as i2c_smbus_read_block_data()? The original code
> is commented with something I'm not sure, but I think if it's a standard
> SMBus, it should be able to use in here.
> Hoping for someone to explain.
This is not how you write a commit message.
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
> Signed-off-by: Song Qiang <[email protected]>
> ---
Your doubts/suggestions/rants all should be here just below
the `---` line, so that if maintainer applies the patch
then your queries doesn't get included in the commit
message.
And no need to reply an explicit "thanks".
That is implicit ;)
We already have a ~30k email traffic on LKML...
https://marc.info/?l=linux-kernel&r=1&w=2
Thanks
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology