Re: [PATCH/RFC v8 02/14] Documentation: leds: Add description of LED Flash class extension

[Pavel Machek <pavel@ucw.cz>]

Hi!

> > > This approach would require implementing additional mechanisms on
> > > both sides: LED Flash class core and a LED Flash class driver.
> > > In the former the sysfs attribute write permissions would have
> > > to be decided in the runtime and in the latter caching mechanism
> > 
> > Write attributes at runtime? Why? We can emulate sane and consistent
> > behaviour for all the controllers: read gives you list of faults,
> > write clears it. We can do it for all the controllers.
> > 
> > Only cost is few lines of code in the drivers where hardware clears
> > faults at read.
> 
> Please take the time to read this, and consider it.
> 
> I'd say the cost is I2C register access, not so much a few lines added to
> the drivers. The functionality and behaviour between the flash controllers
> varies. They have different faults, presence of (some) faults may prevent
> strobing, some support reading the flash status and some don't.
> 
> Some of the flash faults are mostly relevant in production testing, some can
> be used to find hardware issues during use (rare) and some are produced in
> common use (timeout, for instance).
> 
> The V4L2 flash API defines that reading the faults clears them, but does not
> state whether presence of faults would prevent further use of the flash.
> This is flash controller chip specific.

Yeah, but we are discussing sysfs reads. V4L2 API can just behave differently.

> I think you *could* force a policy on the level of kernel API, for instance
> require that the user clears the faults before strobing again rather than
> relying on the chip requiring this instead.

Yes, we could do that.

> Most of the time there are no faults. When there are, they may appear at
> some point of time after the strobing, but how long? Probably roughly after
> the timeout period the flash should have faults available if there were any
> --- except if the strobe is external such as a sensor timed strobe. In that
> case the software running on the CPU has no knowledge when the flash is
> strobed nor when the faults should be read. So the requirement of checking
> the faults would probably have to be limited to software strobe only. The
> user would still have to be able to check the faults for externally strobed
> pulses. Would it be acceptable that the interface was different
> there?

Should the user just read the faults before scheduling next strobe?

> So, after the user has strobed, when the user should check the flash faults?
> After the timeout period has passed? Right before strobing again? If this
> was a requirement, it adds an additional I2C access to potentially the place
> which should absolutely have no extra delay --- the flash strobe time. This
> would be highly unwanted.

I'd do it before strobing again. Not neccessarily "just" before
strobing again (you claim it is slow ... is it really so slow it matters)?

> Finally, should the LED flash class enforce such a policy, would the V4L2
> flash API which is provided to the same devices be changed as well? I'm not
> against that if we have
> 
> 	1) can come up with a good policy that is understood to be
> 	   meaningful for all thinkable flash controller implementations and
> 
> 	2) agreement the behaviour can be changed.

I am saying that reading from /sys should not have side effects. For
V4L2, existing behaviour might be ok.

Each driver should have two operations: read_faults() and
clear_faults().

On devices where i2c read clears faults, operations will be:

int my_faults

read_faults()
	my_faults |= read_i2c_faults()
	return my_faults

clear_faults()
	my_faults = 0

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html