[i2c] [patch 4/5] i2c board_info and i2c_new_device()

[David Brownell]

This response relates primarily to "type" in i2c_board_info, not
the issues of how I2C busses get stable identifiers.


> > > I am asking because in my experience most I2C devices don't use an IRQ,
> > 
> > It's fairly common on the systems I've seen.  Lots of RTCs will provide
> > an alarm IRQ; I2C GPIO expanders tend to do so (pin change irq); USB-OTG
> > tranceivers do; power management chips do; various other multi-function
> > chips do ...
> > 
> > Note that you seem to be arguing that chip type information (which not all
> > I2C drivers need) should be part of the client struct, but not the IRQ
> > (which likewise not all drivers need).  That's inconsistent.
> 
> I was not arguing that much. I was raising questions precisely because
> myself am not sure what belongs where. I guess it's rather natural that
> you see the attributes you use frequently as core attributes, and that
> I see the attributes I use frequently as core attributes ;)

It's also true that this stuff is a bit new, and not many drivers
have been converted yet.  So it's normal to find rough edges to be
smoothed.  I'm not at all averse to upgrading the significance of
a "chip type"; I've seen enough need for that too.

I was just pointing out the inconsistency since you were arguing
both sides of the same question.  In my experience that's usually
a sign of someone thinking through an issue ... or else a sign of
someone repeating pseudo-religious dogma (which didn't seem to
be your situation here).

 
> Is it OK to only store a pointer to the actual string in struct
> i2c_board_info, or do we have to allocate space as you did for the
> driver name? I sure don't want to waste 50 bytes to store it, but OTOH
> there must be a reason why you used a real array for the driver name.
> I guess you expect the original string to be __initdata and gone by the
> time we need it?

That was my thought, yes.  I2C needs to support kernels that work on
several different boards/versions, only expending runtime space for
the board which is actually used.


> If we have to store the string itself, then we definitely want to
> execute your "make i2c_client.name shorter" plan first.

That was only a notion.  It would indeed make sense to upgrade it
to a plan and then execute on it.  Another cleanup patch ...


> > That's a pretty standard idiom ... it seems that using the C99 "flexible
> > array member" syntax (omit length of that array at the end) should be
> > an always-portable solution, so I'll switch to that.  The "length 0"
> > syntax is the traditional GCC solution. 
> 
> But is the C99-style accepted? Are there other use cases of either
> syntax in the kernel at the moment, so we can just do the same?

Well, the policy is to use C99 initializers ("{ .field = value, ... }")
so yes, it's very accepted.  Accordingly I don't much want to search
for other examples.


> I'm fine with moving the exports right after the functions, but you
> know the rule: I want a separate patch doing only that. In theory such a
> patch should come first, but I am fine with a patch going after your
> current patchset if it makes things easier for you. I'll be waiting for
> it before I push the whole patchset to -mm, though. So the path is up
> to you, but the end result has to be consistent.

Looks like yet another cleanup patch the series, then ... :)

- Dave