RE: [PATCH] i2c/tegra: I2C driver uses the suspend_noirq/resume_noirq

[Stephen Warren]

Stephen Warren wrote at Tuesday, August 30, 2011 10:25 AM:
> Ben, Arnd,
> 
> Could you please ack/nack the patch at the start of this thread for Colin;
> see below.

Ben, can you please comment on the acceptability of this patch?

Or Arnd, did Mark's most recent explanation of the situation provide enough
context for you to ack/nak it?

Thanks.

> Thanks.
> 
> Colin Cross wrote at Wednesday, August 24, 2011 3:34 PM:
> > On Wed, Aug 24, 2011 at 2:28 PM, Stephen Warren  wrote:
> > > Mark Brown wrote at Thursday, August 11, 2011 9:15 PM:
> > >> On Thu, Aug 11, 2011 at 07:59:27PM -0700, Colin Cross wrote:
> > >> > On Thu, Aug 11, 2011 at 5:45 PM, Mark Brown
> > >>
> > >> > > For example with ASoC we'd sort all the components before the ASoC 
> > >> > > card
> > >> > > without regard for their bus dependencies or any other dependencies 
> > >> > > they
> > >> > > have (eg, their regulators). Since the ASoC card is a platform device
> > >> > > it's likely to have registered early with no regard for where the 
> > >> > > buses
> > >> > > the card needs are registered. I'd expect there's a reasonable chance
> > >> > > it'll actually make things worse in the short term.
> > >>
> > >> > You can't just move everything after the card, you have to move
> > >> > everything after the last device that was probed, and it only works if
> > >> > nothing depends on any of the devices that are moved.
> > >>
> > >> Sorry, I said that the wrong way round due to trying to reply quickly -
> > >> the card would be the thing that moves since that's the thing that
> > >> actually does the suspend but we've *no* idea which device we need to
> > >> move it after.  Since all the function does is a direct move after or
> > >> before a single device all we can do is pick one and pray that it's the
> > >> right device.
> > >
> > > Colin,
> > >
> > > This thread seems to have died down; how should we make progress?
> > >
> > > It sounds like the suspend_irq solution is the current de-facto standard;
> > > not optimal, but all we really have right now and already in use. I could
> > > certainly see avoiding this solution if it was the first time it was
> > > employed, but re-using it seems reasonable to me?
> > >
> > > Alternatively, are you attending either Linux Plumbers Conference or the
> > > Kernel Summit? Mark implied this topic might well come up for discussion
> > > there. Unfortunately, I won't be able to make LPC due to a conflict.
> > I don't think I'll be able to make it.
> >
> > > (and you'd mentioned having the subsystem maintainers weigh in on this;
> > > which sub-system; IRQ, power, I2C, ...?)
> >
> > If Ben says its OK, its fine with me.  Or maybe Arnd wants to weigh in?

--
nvpublic

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 1/5] i2c: Add irq_gpio field to struct i2c_client, i2c_board_info.

[Jonathan Cameron]

On 09/20/11 05:16, Grant Likely wrote:
> On Fri, Sep 02, 2011 at 08:56:20AM +0200, Jean Delvare wrote:
>> Stephen,
>>
>> Can you please fix your e-mail client / system / whatever so that your
>> patch series are no longer sent duplicated?
>>
>> On Thu,  1 Sep 2011 16:04:27 -0600, Stephen Warren wrote:
>>> Some devices use a single pin as both an IRQ and a GPIO. In that case,
>>> irq_gpio is the GPIO ID for that pin. Not all drivers use this feature.
>>> Where they do, and the use of this feature is optional, and the system
>>> wishes to disable this feature, this field must be explicitly set to a
>>> defined invalid GPIO ID, such as -1.
>>>
>>> Signed-off-by: Stephen Warren 
>>> ---
>>> v3: Also add the field to i2c_board_info, and copy the field from
>>> i2c_board_info to i2c_client upon instantiation
>>
>> I don't get the idea. The i2c core doesn't make any use of the field,
>> and that field will only be used by a few drivers amongst the 420+
>> i2c drivers in the tree. This looks like a waste of memory. What's wrong
>> with including the new field in the private platform or arch data
>> structure for drivers which need it?
> 
> I have to second the concern; but for a different reason.  This
> shouldn't even remotely be necessary.  If the pin is used as an
> interrupt, then interrupt controller driver (which I would assume is
> also the gpio controller driver) should be responsible for setting up
> the pin so that it can be used correctly as a irq line.  Why does the
> gpio number need to be explicitly passed?
The particular driver covered here is somewhat of a false positive.
It really ought to be rewritten to do everything 'properly' with
interrupts.  Right now no one with the inclination has the hardware
to fix it up.

The nasty case we are trying to cover is peripherals that use level
interrupts talking to gpio chips that only do edge triggered ones.
We use the pin as an irq but have to query it as a gpio to discover
if the sneaky chip raised it again without it actually going low.

There are sometimes work arounds involving polling registers on
the device to check if the interrupt is active, but give the bus is
slow, pinging the gpio to find out it's state is often much faster.

So it's a dirty hack for dirty hardware.  Having said that I agree
that it's a niche case and certainly shouldn't be part of any core
code, unless it is done right at the core in the generic interrupt
code and previous discussions suggest that is tricky to say the least!


--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html