Re: Reading /sys with side effects (was Re: [PATCH 1/2] Documentation: leds: Add description of LED Flash class extension)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hi Pavel,

On 02/02/2015 02:51 PM, Pavel Machek wrote:
Hi!

[Actually, you could _always_ do two reads on those devices, discard
first result, and return the second. But I'm not sure how hardware
will like that.]

This would be the most sensible option.


However, let's analyze the typical use cases for flash strobing:


-------------------------------------------------------------


Version without faults caching:

============
Driver side:
============

read_faults()
	faults = read_i2c(); //read faults
	if faults
		write_i2c(); //clear faults, only for some devices
		faults = read_i2c(); //read faults
	return faults

================
User space side:
================

1. faults = `cat flash_faults` //read_faults()
2. if faults then
	print "Unable to strobe the flash LED due to faults"
    else
	echo 1 > flash_strobe


Version with faults caching:

============
Driver side:
============

read_faults()
	faults |= read_i2c(); //read faults

clear_faults()
	write_i2c(); //clear faults
	faults = 0;


================
User space side:
================

1. faults = `cat flash_faults` //read_faults()
2. if faults then
	echo 0 > flash_faults  //clear_faults()
	faults = `cat flash_faults` //read_faults()
3, if !faults
	echo 1 > flash_strobe
    else
	print "Unable to strobe the flash LED due to faults"


-------------------------------------------------------------

 From the above it seems that version with clearing faults on read
results in the simpler flash strobing procedure on userspace side,
by the cost of additional bus access on the driver side.

I like caching version more (as it will allow by-hand debugging of
"why did not flash fire? Aha, lets see in the file, there was fault),
but both should be acceptable.

we don't need additional attribute, just writing the flash_faults
attribute can do the clearing.

Yes, writing flash_faults to clear is acceptable.

I've been just inspired with another approach:
Faults register is read in the strobe_set callback, right after sending
flash strobe command to the device. The userspace can read the cached
faults through flash_faults attribute.

This way, we avoid reading sysfs attribute with side effect and
gain the possibility of giving immediate feedback to the user.

Exemplary use case:

1. echo 1 > flash_strobe
	write_i2c();		//strobe flash
	faults = read_i2c();	//read faults
	if faults
		return -EINVAL;
	return 0;

2. cat flash_faults
	return faults;	

--
Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux