Page MenuHomeFreeBSD

New driver for intersil I2C ISL29018 Digital Ambient Light Sensor
ClosedPublic

Authored by grembo on Jun 14 2015, 11:40 AM.
Tags
None
Referenced Files
F82188537: D2811.id7308.diff
Fri, Apr 26, 7:11 AM
F82165751: D2811.id7323.diff
Fri, Apr 26, 3:05 AM
F82152414: D2811.id6711.diff
Thu, Apr 25, 11:59 PM
F82148857: D2811.id7326.diff
Thu, Apr 25, 11:25 PM
F82140664: D2811.id7314.diff
Thu, Apr 25, 9:27 PM
Unknown Object (File)
Fri, Apr 19, 5:52 AM
Unknown Object (File)
Mar 3 2024, 8:18 AM
Unknown Object (File)
Mar 2 2024, 4:56 PM

Details

Summary

This simple driver makes use of SMBus/ig4 to read values from
the ISL29018 light sensor. It's not using any of the advanced
features like interrupts etc. and it doesn't allow changing
the controller configuration. Measurements are read directly
using sysctl. There are many ways this driver could be improved
(run a separate thread that feeds data in a certain interval -
also a function the device supports directly, devd support...).

Despite those shortcomings, the driver is still useful in its current state,
e.g. to adjust screen brightness on a laptop, simplified real life example:

#!/bin/sh

update_brightness()
(
      BRIGHTNESS=$(( `sysctl -n dev.isl.0.als` ))
      if [ $BRIGHTNESS -lt 15 ]; then
              BRIGHTNESS=15
      fi
      if [ $BRIGHTNESS -gt 100 ]; then
              BRIGHTNESS=100  
      fi
      return $BRIGHTNESS
)
...
update_brightness
B_LAST=$?
intel_backlight $B_LAST
...

It creates a device /dev/islX, which is not used for anything right now.
Once the driver is in HEAD, additional features might be added by
me or other developers quite easily. The documentation is fairly
good and available at

http://www.intersil.com/content/dam/Intersil/documents/isl2/isl29018.pdf

The device shows up properly in devinfo:

ig4iic1
 smbus1
   isl0

Example sysctl output:

# sysctl dev.isl.0
dev.isl.0.range: 1000
dev.isl.0.resolution: 65536
dev.isl.0.prox: 13298
dev.isl.0.ir: 13312
dev.isl.0.als: 43
dev.isl.0.%parent: smbus1
dev.isl.0.%pnpinfo: 
dev.isl.0.%location: addr=0x44
dev.isl.0.%driver: isl
dev.isl.0.%desc: ISL Digital Ambient Light Sensor
Test Plan

Tested the driver itself for almost half a year (before I did
some cleanup to submit it to review) reading the current asl
a few million calls.
Man pages have been checked using igor and mandoc -Tlint.

kldload ig4
kldload isl
igor isl.4
mandoc -Tlint isl.4

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

grembo retitled this revision from to New driver for intersil I2C ISL29018 Digital Ambient Light Sensor.
grembo updated this object.
grembo edited the test plan for this revision. (Show Details)
grembo added reviewers: jhb, adrian.
grembo edited edge metadata.

Feedback I received from @wblock by email:

This looks very good. I just wanted to suggest two things:

  1. Use "through" instead of "over" when talking about the sysctl

interface (access is provided *through* ...).

  1. Include an example section with at least a single example of setting

the screen brightness on a specific laptop. Many people would benefit
from that.

grembo updated this object.

Fix use of bzero and remove unnecessary cast.

Manpage changes as requested by @wblock.

Please note that the port referenced hasn't been created yet. Code review
request D2999 has been created to cover this.

wblock added a reviewer: wblock.

Man page looks good. One note on "modern GPU" specifics. Please check for problems with 'mandoc -Tlint' and 'igor -R' before commit. Thanks!

share/man/man4/isl.4
82 ↗(On Diff #6715)

In a couple of years, which GPUs are "modern" will be less clear. Being more specific might be better here, like "Intel GPUs produced since 2005." or "modern Intel GPUs including the HD4000 and later." (These are not literal, just trying to suggest wording.)

This revision is now accepted and ready to land.Jul 6 2015, 2:38 PM
grembo edited edge metadata.
grembo added a subscriber: kib.

Apply lessons learned from @kib's review of cyapa.

This revision now requires review to proceed.Jul 22 2015, 5:07 PM
grembo edited edge metadata.

Man page update: As @wblock pointed out that "modern" is a pretty relative
term that won't age well, I replaced it with "supported". This is more
accurate anyway and anyone dealing with this driver most likely has
a laptop that's modern by today's standards anyway.

More lessons learned from cyapa review

Merge latest changes, add BUGS section to man page, minor clean-ups

Merge latest changes, add BUGS section to man page, minor clean-ups

Merge latest changes, add BUGS section to man page, minor clean-ups

Merge latest changes, add BUGS section to man page, minor clean-ups

Merge latest changes, add BUGS section to man page, minor clean-ups

Merge latest changes, add BUGS section to man page, minor clean-ups

Merge latest changes, add BUGS section to man page, minor clean-ups

Merge latest changes, add BUGS section to man page, minor clean-ups

Merge latest changes, add BUGS section to man page, minor clean-ups

Merge latest changes, add BUGS section to man page, minor clean-ups

In case anyone wonders why I committed the same patch 10 times: Arcanist is showing this on arc diff --update:

/tmp//edit.5nq6p1rhpvs44s4s/differential-update-comments: 10 lines, 359
characters. Linting...
 LINT OKAY  No lint problems.
Running unit tests...
No unit test engine is configured for this project.
 SKIP STAGING  Phabricator does not support staging areas for this
repository. Exception
ERR-CONDUIT-CORE: Include of
'/usr/local/www/phabricator/phabricator/src/extensions/herald/customaction/SetTaskViewPolicyHeraldCustomAction.php'
failed! (Run with `--trace` for a full exception trace.)
In D2811#64067, @grembo wrote:

In case anyone wonders why I committed the same patch 10 times: Arcanist is showing this on arc diff --update:

/tmp//edit.5nq6p1rhpvs44s4s/differential-update-comments: 10 lines, 359
characters. Linting...
 LINT OKAY  No lint problems.
Running unit tests...
No unit test engine is configured for this project.
 SKIP STAGING  Phabricator does not support staging areas for this
repository. Exception
ERR-CONDUIT-CORE: Include of
'/usr/local/www/phabricator/phabricator/src/extensions/herald/customaction/SetTaskViewPolicyHeraldCustomAction.php'
failed! (Run with `--trace` for a full exception trace.)

this was fixed and shouldn't happen again. We do get reports when this happens, but someone needs to be awake to deal with them :-)

In D2811#64102, @eadler wrote:
In D2811#64067, @grembo wrote:

In case anyone wonders why I committed the same patch 10 times: Arcanist is showing this on arc diff --update:

/tmp//edit.5nq6p1rhpvs44s4s/differential-update-comments: 10 lines, 359
characters. Linting...
 LINT OKAY  No lint problems.
Running unit tests...
No unit test engine is configured for this project.
 SKIP STAGING  Phabricator does not support staging areas for this
repository. Exception
ERR-CONDUIT-CORE: Include of
'/usr/local/www/phabricator/phabricator/src/extensions/herald/customaction/SetTaskViewPolicyHeraldCustomAction.php'
failed! (Run with `--trace` for a full exception trace.)

this was fixed and shouldn't happen again. We do get reports when this happens, but someone needs to be awake to deal with them :-)

This one was particularly weird, as there was no indication that it sent the patch successfully.

adrian edited edge metadata.
This revision is now accepted and ready to land.Jul 25 2015, 7:34 PM
grembo edited edge metadata.

Merge latest changes, add BUGS section to man page, minor clean-ups

This revision now requires review to proceed.Jul 25 2015, 7:47 PM
grembo edited edge metadata.

Reduce pause in init (spec sheet asks for 1ms or more)

This revision was automatically updated to reflect the committed changes.
head/sys/dev/isl/isl.c
74

This is unused.

161

This seems unused.

176

This is unused except for setting the sc->unit value (which is then never used).

188

As with the other driver, you can use smbus_get_addr() here.

199

These two variables (sysctl_ctx and sysctl_tree) can just be local vars in the in attach routine. They aren't used anywhere else, so I don't think you need to store them in the softc.

310

A style fix would be to do:

static int
isl_read_sensor(...)

(i.e. make the function name start on a new line)