linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Wolfram Sang <wsa@the-dreams.de>
To: Jean Delvare <jdelvare@suse.de>
Cc: linux-i2c@vger.kernel.org, linux-sh@vger.kernel.org,
	Magnus Damm <magnus.damm@gmail.com>,
	Simon Horman <horms@verge.net.au>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Geert Uytterhoeven <geert@linux-m68k.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [RFC] i2c-tools: i2ctransfer: add new tool
Date: Sat, 9 May 2015 09:09:14 +0200	[thread overview]
Message-ID: <20150509070914.GB1511@katana> (raw)
In-Reply-To: <20150507220812.3776bb83@endymion.delvare>

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


> BTW I'm curious if you actually needed the + and - suffixes in practice?
> I can easily imagine how the = suffix will be useful in the real
> world, but + and -...  Or maybe just for bus driver testing purpose?

Exactly for that. While you can send complex messages with my tool, it
should be clear this is mainly for testing/developing. Once you know
what you need to do to access a slave, a specfic driver is the better
option, be it in userspace or kernel space.

Those patterns are easily recognizable when fed into an EEPROM. They can
already reveal quite some problems with bus drivers, e.g. off-by-one
errors when fetching data, sending STOP etc.

> > +}
> > +	/* let Linux free malloced memory on termination */
> 
> I don't like this. The memory allocated in i2cbusses.c is freed
> explicitly, so it is inconsistent to not free yours. Freeing the memory

Well, it is documented :)

> explicitly makes the code easier to read and debug as it documents the

I disagree about this one. Currently, we have a LOT of error paths when
parsing input fails. Getting all the error paths right with freeing
memory is usually error-prone (especially with later additions) and will
IMO make the code way less readable. Since the lifetime of those buffers
ends right before the exit(0), it seems unnecessarily complex to me.

However, I need to redo the parsing anyhow. Let's see how it looks like
in the next version. If it is easy/readable to free(), I'll do it.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

      parent reply	other threads:[~2015-05-09  7:09 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-02-27 16:16 [RFC] i2c-tools: i2ctransfer: add new tool Wolfram Sang
2015-04-20 17:36 ` Wolfram Sang
2015-04-21  5:25   ` Jean Delvare
2015-04-21  7:06     ` Wolfram Sang
2015-05-07 20:08 ` Jean Delvare
2015-05-08  8:54   ` Jean Delvare
2015-05-08 14:38     ` Wolfram Sang
2015-05-08 21:40       ` Jean Delvare
2015-05-09  6:50         ` Wolfram Sang
2015-05-08 15:28     ` Randy Grunwell
2015-05-08 18:28       ` Uwe Kleine-König
2015-05-08 20:58       ` Jean Delvare
2015-05-09  7:09   ` Wolfram Sang [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150509070914.GB1511@katana \
    --to=wsa@the-dreams.de \
    --cc=geert@linux-m68k.org \
    --cc=horms@verge.net.au \
    --cc=jdelvare@suse.de \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-i2c@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-sh@vger.kernel.org \
    --cc=magnus.damm@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).