[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: [PATCH v3] i2c_tools.c: add i2ctransfer utility
From:       Nikolaus Voss <nv () vosn ! de>
Date:       2019-02-11 10:16:00
Message-ID: alpine.DEB.2.20.1902110944050.48155 () fox ! voss ! local
[Download RAW message or body]

Hi Denys,

many thanks for reviewing and merging this. For my use case it works as 
expected with your modifications.

On Sun, 10 Feb 2019, Denys Vlasenko wrote:
> On Sun, Feb 10, 2019 at 7:24 PM Denys Vlasenko <vda.linux@googlemail.com> wrote:
> > On Mon, Jan 7, 2019 at 2:29 PM Nikolaus Voss <nv@vosn.de> wrote:
> > > i2ctransfer sends and receives user defined i2c messages
> > > v2: apply Xabier's comments: add -a option, don't decrement argc,
> > > use bb_show_usage() and xzalloc()
> > > v3: fix possible out of bound access to msgs[nmsgs]
> > > 
> > > Reviewed-by: Xabier Oneca -- xOneca <xoneca@gmail.com>
> > > Signed-off-by: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>
> > > ---
> > > miscutils/i2c_tools.c | 206 +++++++++++++++++++++++++++++++++++++++++-
> > > 1 file changed, 205 insertions(+), 1 deletion(-)
> > > 
> > > +                       switch (*arg_ptr++) {
> > > +                       case 'r': flags |= I2C_M_RD; break;
> > > +                       case 'w': break;
> > > +                       default:
> > > +                               bb_show_usage();
> > > +                       }
> > > +
> > > +                       len = strtoul(arg_ptr, &end, 0);
> > > +                       if (len > 0xffff || arg_ptr == end)
> > > +                               bb_error_msg_and_die("Error: Length invalid: \
> > > %s\n", *argv); +
> > > +                       arg_ptr = end;
> > > +                       if (*arg_ptr) {
> > > +                               if (*arg_ptr++ != '@')
> > > +                                       bb_error_msg_and_die("Error: Unknown \
> > > separator after length: %s\n", +                                                \
> > > *argv); +                               bus_addr = xstrtou_range(arg_ptr, 0, \
> > > first, last); +
> > > +                               if (!(opts & opt_f))
> > > +                                       i2c_set_slave_addr(fd, bus_addr, opts & \
> > > opt_f);
> > 
> > This last if() looks fishy. What is it trying to accomplish?

It skips kernel range and busy checking if -f is set:
/tmp # i2cdetect -y 6
i2cdetect: warning: can't use SMBus quick write command, will skip some 
addresses
      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:
10:
20:
30: -- -- -- -- UU -- -- --
40:
50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
60:
70:
/tmp # i2ctransfer -y 6 w1@0x34 0 r1
i2ctransfer: can't set address to 0x34: Device or resource busy
/tmp # i2ctransfer -f -y 6 w1@0x34 0 r1
0x04

As range checking is already done in user mode, only busy checking is 
effective, which is skipped if the force flag of i2c_set_slave_addr() is 
set. So i2c_set_slave_addr() could/should be be called unconditionally, 
the above example would create exactly the same output.

Nikolaus
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox


[prev in list] [next in list] [prev in thread] [next in thread]