Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines

[Benjamin Tissoires]

On Jan 04 2017 or thereabouts, Pali RohÃr wrote:
> On Wednesday 04 January 2017 14:02:52 Benjamin Tissoires wrote:
> > > > > No. DT platforms won't have an issue: they don't change anything, and
> > > > > there will be a new /dev/freefall misc device for the platforms that
> > > > 
> > > > And this is wrong! There should not be any /dev/freefall device
> > > > connected with signal host notify. /dev/freefall is for hardware which
> > > > supports free fall hdd detection.
> > 
> > On the principle, I agree. But let's be realistic: this device will be
> > created but no events will be generated from it.
> 
> And I think this is a problem. This provides false information to kernel
> and also to userspace applications that there is working free fall
> sensor (even there is not).
> 
> > It's a capability for the chip to provide freefall,
> 
> That it is not truth. Not every chip has it. Platform data are there to
> provides this information for driver, if HW has freefall capability or
> not.
> 
> > so I don't see why it's such an issue to
> > have one created which says nothing. (talking about non Dell platforms
> > here too)
> > 
> > > > 
> > > > > don't have the irq set *and* if the device is on a Host Notify capable
> > > > > adapter, currently only i2c-i801.
> > > > 
> > > > I understood, but in future more bus drivers could support host notify.
> > > 
> > > This is really fragile. lis3lv02d will empty IRQ (0) will work on some
> > > i2c buses and on some not. I would really expect that i2c driver
> > > (lis3lv02d) will work in same way with any i2c bus driver. And not that
> > > on i801 will acts differently as on e.g. omap3 i2c bus.
> > 
> > I really don't follow you here. On a bus with no Host Notify, the IRQ
> > will be 0 and it will work in the same way.
> 
> That it OK.
> 
> > On a bus with Host Notify,
> > the IRQ should be set to something negative in the i2c_board_info
> 
> Not only in i2c_board_info but also in DTS. Which means every one DTS
> needs to be updates -- also those outside of linux kernel. Which is for
> me wrong.
> 
> > to be
> > sure to not have an Host Notify irq assigned. In both case the
> > lis3lv02d_i2c driver will just do:
> > 	if (client->irq > 0)
> > 		lis3_dev.irq = client->irq;
> > 
> > So there is no bus adapter specifics in the driver.
> > 
> > > 
> > > > This is basically incorrect if we use one property for two different
> > > > things (host notify and hdd free fall).
> > 
> > Host Notify is a way for a I2C device to report an interrupt without
> > having a dedicated line assigned (and so an IRQ).
> 
> Yes. But lis3lv02d i2c devices which I have uses dedicated IRQ. They
> does not work in way like you describe.
> 
> > So the chip could use
> > Host Notify to report hdd free fall if it supports the feature.
> 
> Old existing HW could not. As those were not designed for such purpose.
> 
> Yes, new HW can be designed in this way, but linux kernel should not
> drop (or modify) correct support for old HW devices just because there
> is new HW design.

I never said we should drop support of old hardware. I was just
explaining what is Host Notify and saying that it is not incompatible
with lis3lv02d. 

> 
> > > > 
> > > > For me it looks like that we should separate these two things into two
> > > > IRQ properties. It is really wrong design if one property is used for
> > > > two different things.
> > 
> > I won't say the design is wrong because Host Notify really is just an
> > IRQ.
> 
> Yes it is IRQ, but different. And lis3lv02d does not issue host notify
> IRQ, so it should not be assigned to lis3lv02d.

Sigh.

> 
> > Where the issue lies now is that lis3lv02d makes the assumption
> > that an irq attached to an i2c_client means that it will report
> > freefall.
> 
> Yes.
> 
> > It was correct before, but now it's not true anymore.
> 
> Agree.
> 
> > So maybe this series should also add a way to provide the source of
> > the IRQ (Host Notify or ACPI or DT). Then, lis3lv02d could ignore or
> > not the incoming IRQ.
> 
> Yes, this is an option how to fix this problem.

How about:
---