[PATCH v2 0/5] iio: light: stk3310: per-chip match data and STK36C61 support

[PATCH v2 0/5] iio: light: stk3310: per-chip match data and STK36C61 support

[Jorijn van der Graaf]

The Sensortek STK36C61 is a 3-in-1 ambient light / proximity / RGB
colour sensor whose register interface is compatible with the stk3310
across the ALS and proximity data, threshold, gain and
integration-time registers this driver uses. The part name comes from
the Fairphone 6 manual, which lists the component as "3IN1 ALPS/RGB
sensor/STK36C61". There is no public datasheet; the layout was probed
on the device: chip ID 0x95, the stk3310 STATE/FLAG bit layout, data
and threshold registers, gain steps that scale the ALS reading
accordingly, and an RGBC data block directly after the ALS data.

Since v1 the colour block's controls were located, confirming
Jonathan's guess on v1's patch 3 that they sit in a register we had
not found: the STK37660 datasheet, a documented sibling with the same
data register layout, places a clear-channel gain field in a GAINCTRL
register at 0x4E, split from the RGB gain in ALSCTRL, and stepping it
on the STK36C61 multiplies the clear count by ~4 per step while the
other channels hold still. The STK36C61 patch therefore now exposes
scale and integration time on the colour channels.

Tested on a Fairphone 6 running a 7.2-based tree: this series'
stk3310.c, byte-identical, built as a module against that tree (the
i2c core files the binding behaviour depends on are identical between
the trees). Verified there: DT boot instantiation, the colour
scale/integration-time interface, and the sysfs name behaviours this
series changes: a lower-case name selects the full profile through
the id table, capitals no longer bind, and a full-compatible-string
client now fails probe with the missing-driver-data error. A board
DTS node using the new compatible follows separately via
linux-arm-msm.

Changes in v2:
- new first patch lower-casing the i2c device ID names (Andy), and
  probe now fails on missing match data instead of falling back to
  the stk3310 profile (Andy)
- new precursor patch moving the data registers into the channel
  .address field (Jonathan), split out of the match-data patch; the
  bulk read gains a regmap local variable (Andy)
- the ACPI table entries use named initializers (Jonathan)
- the STK36C61 patch exposes scale and integration time on the colour
  channels (Jonathan's suggestion on v1 patch 3; the clear-gain
  register located via the STK37660 datasheet and verified on the
  device). With those branches of read_raw now handling the intensity
  channels, the ret = -EINVAL single-exit form promised on the v1
  thread (Andy) has no unreachable branch left to apply to
- picked up Krzysztof's Reviewed-by on the binding patch

v1: https://lore.kernel.org/all/20260810110423.41697-1-jorijnvdgraaf@catcrafts.net/

Assisted-by: Claude:claude-fable-5
Signed-off-by: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>

Jorijn van der Graaf (5):
  iio: light: stk3310: lower-case the i2c device ID names
  dt-bindings: iio: light: stk33xx: document the Sensortek STK36C61
  iio: light: stk3310: move the data registers into the channel address
  iio: light: stk3310: add per-chip match data
  iio: light: stk3310: support the Sensortek STK36C61

 Documentation/ABI/testing/sysfs-bus-iio       |   1 +
 .../bindings/iio/light/stk33xx.yaml           |   9 +-
 drivers/iio/light/stk3310.c                   | 203 +++++++++++++-----
 3 files changed, 156 insertions(+), 57 deletions(-)


base-commit: 350d1fb9204b13c5f95e511e98b8bcb47574d425
-- 
2.55.0

[PATCH v2 1/5] iio: light: stk3310: lower-case the i2c device ID names

[Jorijn van der Graaf]

The i2c device IDs were introduced in capitals, mirroring the ACPI
_HID entries added by the same commit be9e6229d676 ("iio: light: Add
support for Sensortek STK3310"); at that point the driver enumerated
through ACPI only, with no OF table and no i2c module alias export.
ACPI _HIDs have their own naming rules; i2c device names
conventionally use the lower-case part name, matching the devicetree
compatible suffix.

The spelling is visible: a client instantiated through the i2c sysfs
interface under the lower-case name taken from a compatible string
binds through the OF table's name fallback, but has no firmware node,
so i2c_match_id() is the only way for it to reach driver match data,
and its string comparison is case-sensitive, so the capitals can never
match. Lower-case the names so such clients match the id table, and
receive the per-chip match data a subsequent change attaches to it.

The module aliases follow the rename (i2c:STK3310 becomes
i2c:stk3310), and a sysfs client instantiated under a capitals name no
longer binds.

Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Assisted-by: Claude:claude-fable-5
Signed-off-by: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>
---
 drivers/iio/light/stk3310.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index 7c8a1d2b2ed0..5b5b6812edc7 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -766,10 +766,10 @@ static DEFINE_SIMPLE_DEV_PM_OPS(stk3310_pm_ops, stk3310_suspend,
 				stk3310_resume);
 
 static const struct i2c_device_id stk3310_i2c_id[] = {
-	{ .name = "STK3013" },
-	{ .name = "STK3310" },
-	{ .name = "STK3311" },
-	{ .name = "STK3335" },
+	{ .name = "stk3013" },
+	{ .name = "stk3310" },
+	{ .name = "stk3311" },
+	{ .name = "stk3335" },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, stk3310_i2c_id);
-- 
2.55.0

Re: [PATCH v2 1/5] iio: light: stk3310: lower-case the i2c device ID names

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Removing the upper-case I2C device IDs breaks backwards compatibility for devices instantiated from userspace via sysfs.
--

commit 7889e08011ff4fb9c503926c95b3280c1405b42a
Author: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>

iio: light: stk3310: lower-case the i2c device ID names

This commit replaces the upper-case I2C device IDs with lower-case versions
to match conventional devicetree compatible suffixes. This allows clients
without a firmware node to correctly match the driver, but it prevents sysfs
clients instantiated under the previous capital names from binding.

> diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
> --- a/drivers/iio/light/stk3310.c
> +++ b/drivers/iio/light/stk3310.c
> @@ -766,10 +766,10 @@ static DEFINE_SIMPLE_DEV_PM_OPS(stk3310_pm_ops, stk3310_suspend,
>  				stk3310_resume);
>  
>  static const struct i2c_device_id stk3310_i2c_id[] = {
> -	{ .name = "STK3013" },
> -	{ .name = "STK3310" },
> -	{ .name = "STK3311" },
> -	{ .name = "STK3335" },

[Severity: Medium]
Does removing the upper-case I2C device IDs break backwards compatibility for
devices instantiated from userspace via sysfs?

Userspace scripts that previously instantiated the device using a command
like "echo STK3310 0x48 > new_device" will fail to bind to the driver, as
the device name will no longer match the I2C device ID table.

Should the upper-case IDs be retained alongside the new lower-case IDs to
preserve ABI backwards compatibility?

> +	{ .name = "stk3013" },
> +	{ .name = "stk3310" },
> +	{ .name = "stk3311" },
> +	{ .name = "stk3335" },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, stk3310_i2c_id);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826175409.326131-1-jorijnvdgraaf@catcrafts.net?part=1

Re: [PATCH v2 1/5] iio: light: stk3310: lower-case the i2c device ID names

[Andy Shevchenko]

On Wed, Aug 26, 2026 at 07:54:05PM +0200, Jorijn van der Graaf wrote:
> The i2c device IDs were introduced in capitals, mirroring the ACPI
> _HID entries added by the same commit be9e6229d676 ("iio: light: Add
> support for Sensortek STK3310"); at that point the driver enumerated
> through ACPI only, with no OF table and no i2c module alias export.
> ACPI _HIDs have their own naming rules; i2c device names
> conventionally use the lower-case part name, matching the devicetree
> compatible suffix.
> 
> The spelling is visible: a client instantiated through the i2c sysfs
> interface under the lower-case name taken from a compatible string
> binds through the OF table's name fallback, but has no firmware node,
> so i2c_match_id() is the only way for it to reach driver match data,
> and its string comparison is case-sensitive, so the capitals can never
> match. Lower-case the names so such clients match the id table, and
> receive the per-chip match data a subsequent change attaches to it.
> 
> The module aliases follow the rename (i2c:STK3310 becomes
> i2c:stk3310), and a sysfs client instantiated under a capitals name no
> longer binds.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
with all responsibility for the theoretically possible breakages
of the weird setups. But I strongly believe that no one is using
capital letters for I²C instantiation of this driver as it was
submitted for ACPI use and followed by DT support.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 2/5] dt-bindings: iio: light: stk33xx: document the Sensortek STK36C61

[Jorijn van der Graaf]

The STK36C61 is a 3-in-1 ambient light / proximity / RGB colour sensor
found in the Fairphone 6. Its ambient light and proximity register
interface is compatible with the stk3310's, while the dedicated
compatible identifies the part, whose colour channels the fallback
does not describe.

Add sensortek,stk36c61 with a sensortek,stk3310 fallback, mirroring
the stk3013 entry, and widen the title and description to cover the
part.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Assisted-by: Claude:claude-fable-5
Signed-off-by: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>
---
 Documentation/devicetree/bindings/iio/light/stk33xx.yaml | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
index e4341fdced98..0165a348918b 100644
--- a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
+++ b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
@@ -4,14 +4,14 @@
 $id: http://devicetree.org/schemas/iio/light/stk33xx.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: |
-  Sensortek STK33xx I2C Ambient Light and Proximity sensor
+title: Sensortek STK33xx/STK36C61 I2C Ambient Light and Proximity sensor
 
 maintainers:
   - Jonathan Cameron <jic23@kernel.org>
 
-description: |
-  Ambient light and proximity sensor over an i2c interface.
+description:
+  Ambient light and proximity sensor over an i2c interface. The STK36C61
+  additionally provides RGBC colour channels.
 
 allOf:
   - $ref: ../common.yaml#
@@ -26,6 +26,7 @@ properties:
       - items:
           - enum:
               - sensortek,stk3013
+              - sensortek,stk36c61
           - const: sensortek,stk3310
 
   reg:
-- 
2.55.0

[PATCH v2 3/5] iio: light: stk3310: move the data registers into the channel address

[Jorijn van der Graaf]

The RAW read selects its data register with a per-channel-type branch.
Record each channel's data register in its .address field and read
from there, so adding a channel does not grow the branch.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Assisted-by: Claude:claude-fable-5
Signed-off-by: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>
---
 drivers/iio/light/stk3310.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index 5b5b6812edc7..156888969366 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -184,6 +184,7 @@ static const struct iio_chan_spec_ext_info stk3310_ext_info[] = {
 static const struct iio_chan_spec stk3310_channels[] = {
 	{
 		.type = IIO_LIGHT,
+		.address = STK3310_REG_ALS_DATA_MSB,
 		.info_mask_separate =
 			BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_SCALE) |
@@ -191,6 +192,7 @@ static const struct iio_chan_spec stk3310_channels[] = {
 	},
 	{
 		.type = IIO_PROXIMITY,
+		.address = STK3310_REG_PS_DATA_MSB,
 		.info_mask_separate =
 			BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_SCALE) |
@@ -370,25 +372,20 @@ static int stk3310_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int *val, int *val2, long mask)
 {
-	u8 reg;
 	__be16 buf;
 	int ret;
 	unsigned int index;
 	struct stk3310_data *data = iio_priv(indio_dev);
 	struct i2c_client *client = data->client;
+	struct regmap *map = data->regmap;
 
 	if (chan->type != IIO_LIGHT && chan->type != IIO_PROXIMITY)
 		return -EINVAL;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		if (chan->type == IIO_LIGHT)
-			reg = STK3310_REG_ALS_DATA_MSB;
-		else
-			reg = STK3310_REG_PS_DATA_MSB;
-
 		mutex_lock(&data->lock);
-		ret = regmap_bulk_read(data->regmap, reg, &buf, sizeof(buf));
+		ret = regmap_bulk_read(map, chan->address, &buf, sizeof(buf));
 		if (ret < 0) {
 			dev_err(&client->dev, "register read failed\n");
 			mutex_unlock(&data->lock);
-- 
2.55.0

Re: [PATCH v2 3/5] iio: light: stk3310: move the data registers into the channel address

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Possible Use-After-Free of `event_interface` in `iio_push_event` on the `iio_device_register` error path.
- [High] Missing locking in `stk3310_write_event` leading to state mismatch between hardware and software.
--

commit 0d9aa45c0bb9cd3c7c737938875870bc51a8c0be
Author: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>

iio: light: stk3310: move the data registers into the channel address

The RAW read selects its data register with a per-channel-type branch.
Record each channel's data register in its .address field and read
from there, so adding a channel does not grow the branch.

> diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
> index 5b5b6812edc75..156888969366c 100644
> --- a/drivers/iio/light/stk3310.c
> +++ b/drivers/iio/light/stk3310.c

[ ... ]

> @@ -370,25 +372,20 @@ static int stk3310_read_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan,
>  			    int *val, int *val2, long mask)
>  {
> -	u8 reg;
>  	__be16 buf;
>  	int ret;
>  	unsigned int index;
>  	struct stk3310_data *data = iio_priv(indio_dev);
>  	struct i2c_client *client = data->client;
> +	struct regmap *map = data->regmap;
>  
>  	if (chan->type != IIO_LIGHT && chan->type != IIO_PROXIMITY)
>  		return -EINVAL;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> -		if (chan->type == IIO_LIGHT)
> -			reg = STK3310_REG_ALS_DATA_MSB;
> -		else
> -			reg = STK3310_REG_PS_DATA_MSB;
> -
>  		mutex_lock(&data->lock);
> -		ret = regmap_bulk_read(data->regmap, reg, &buf, sizeof(buf));
> +		ret = regmap_bulk_read(map, chan->address, &buf, sizeof(buf));
>  		if (ret < 0) {
>  			dev_err(&client->dev, "register read failed\n");
>  			mutex_unlock(&data->lock);

[Severity: High]
This is a pre-existing issue, but is there missing locking in
stk3310_write_event()?

drivers/iio/light/stk3310.c:stk3310_write_event() {
	...
	buf = cpu_to_be16(val);
	ret = regmap_bulk_write(data->regmap, reg, &buf, sizeof(buf));
	if (ret < 0) {
		dev_err(&client->dev, "failed to set PS threshold!\n");
		return ret;
	}

	if (reg == STK3310_REG_THDH_PS)
		data->ps_thdh = val;
	else
		data->ps_thdl = val;
	...
}

Can concurrent sysfs writes to the event threshold file cause the hardware
configuration and software state to diverge?

If two threads call stk3310_write_event() concurrently without data->lock
being held, could they interleave their regmap_bulk_write() and software
state updates? 

For example, if Thread A writes 10 and Thread B writes 20, can the hardware
be set to 20 while the software state incorrectly records 10, leading to
stk3310_resume() restoring the wrong state later?

[Severity: High]
This isn't a bug introduced by this patch, but could there be a
use-after-free in the probe error path?

drivers/iio/light/stk3310.c:stk3310_probe() {
	...
	if (client->irq > 0) {
		ret = devm_request_threaded_irq(&client->dev, client->irq,
						stk3310_irq_handler,
						stk3310_irq_event_handler,
						IRQF_TRIGGER_FALLING |
						IRQF_ONESHOT,
						"stk3310_event", indio_dev);
		if (ret)
			goto err_standby;
	}

	ret = iio_device_register(indio_dev);
	if (ret < 0) {
		dev_err(&client->dev, "device_register failed\n");
		goto err_standby;
	}
	...
}

If iio_device_register() fails, does this code leave the IRQ active?

Since devm_request_threaded_irq() is called before iio_device_register(), a
failure in iio_device_register() will clean up the event interface and
free it. 

Because the IRQ is still active and not disabled on this error path, an
interrupt could fire concurrently.

If that happens, would stk3310_irq_event_handler() call iio_push_event()
and dereference the freed event interface pointer, resulting in memory
corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826175409.326131-1-jorijnvdgraaf@catcrafts.net?part=3

Re: [PATCH v2 3/5] iio: light: stk3310: move the data registers into the channel address

[Andy Shevchenko]

On Wed, Aug 26, 2026 at 07:54:07PM +0200, Jorijn van der Graaf wrote:
> The RAW read selects its data register with a per-channel-type branch.
> Record each channel's data register in its .address field and read
> from there, so adding a channel does not grow the branch.

Fine by me
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 4/5] iio: light: stk3310: add per-chip match data

[Jorijn van der Graaf]

Introduce a chip_info structure carrying the device name and channel
specification, attach it to every i2c, OF and ACPI table entry, and
let probe take it from the match data, failing when there is none.
Every firmware bind path carries match data, and with the id names in
lower case a client instantiated through the sysfs new_device
interface under a compatible-derived name receives it through the id
table. The shared channel definitions move into macros.

The ACPI table entries change to named initializers, matching the
other id tables.

This is a preparatory change for a variant that provides more channels
than the existing parts. No functional change for firmware-described
devices; a sysfs client under a name that binds without matching any
id entry (the full compatible string) now fails probe with an error
instead of probing as an stk3310.

Assisted-by: Claude:claude-fable-5
Signed-off-by: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>
---
 drivers/iio/light/stk3310.c | 95 ++++++++++++++++++++++++-------------
 1 file changed, 61 insertions(+), 34 deletions(-)

diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index 156888969366..d632e6447f06 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -181,26 +181,48 @@ static const struct iio_chan_spec_ext_info stk3310_ext_info[] = {
 	{ }
 };
 
+#define STK3310_LIGHT_CHANNEL {				\
+	.type = IIO_LIGHT,				\
+	.address = STK3310_REG_ALS_DATA_MSB,		\
+	.info_mask_separate =				\
+		BIT(IIO_CHAN_INFO_RAW) |		\
+		BIT(IIO_CHAN_INFO_SCALE) |		\
+		BIT(IIO_CHAN_INFO_INT_TIME),		\
+}
+
+#define STK3310_PROXIMITY_CHANNEL {			\
+	.type = IIO_PROXIMITY,				\
+	.address = STK3310_REG_PS_DATA_MSB,		\
+	.info_mask_separate =				\
+		BIT(IIO_CHAN_INFO_RAW) |		\
+		BIT(IIO_CHAN_INFO_SCALE) |		\
+		BIT(IIO_CHAN_INFO_INT_TIME),		\
+	.event_spec = stk3310_events,			\
+	.num_event_specs = ARRAY_SIZE(stk3310_events),	\
+	.ext_info = stk3310_ext_info,			\
+}
+
 static const struct iio_chan_spec stk3310_channels[] = {
-	{
-		.type = IIO_LIGHT,
-		.address = STK3310_REG_ALS_DATA_MSB,
-		.info_mask_separate =
-			BIT(IIO_CHAN_INFO_RAW) |
-			BIT(IIO_CHAN_INFO_SCALE) |
-			BIT(IIO_CHAN_INFO_INT_TIME),
-	},
-	{
-		.type = IIO_PROXIMITY,
-		.address = STK3310_REG_PS_DATA_MSB,
-		.info_mask_separate =
-			BIT(IIO_CHAN_INFO_RAW) |
-			BIT(IIO_CHAN_INFO_SCALE) |
-			BIT(IIO_CHAN_INFO_INT_TIME),
-		.event_spec = stk3310_events,
-		.num_event_specs = ARRAY_SIZE(stk3310_events),
-		.ext_info = stk3310_ext_info,
-	}
+	STK3310_LIGHT_CHANNEL,
+	STK3310_PROXIMITY_CHANNEL,
+};
+
+/**
+ * struct stk3310_chip_info - chip-specific data
+ * @name: device name reported to the IIO core
+ * @channels: channel specification
+ * @num_channels: number of channels
+ */
+struct stk3310_chip_info {
+	const char			*name;
+	const struct iio_chan_spec	*channels __counted_by_ptr(num_channels);
+	unsigned int			num_channels;
+};
+
+static const struct stk3310_chip_info stk3310_chip_info = {
+	.name = STK3310_DRIVER_NAME,
+	.channels = stk3310_channels,
+	.num_channels = ARRAY_SIZE(stk3310_channels),
 };
 
 static IIO_CONST_ATTR(in_illuminance_scale_available, STK3310_SCALE_AVAILABLE);
@@ -632,10 +654,16 @@ static irqreturn_t stk3310_irq_event_handler(int irq, void *private)
 
 static int stk3310_probe(struct i2c_client *client)
 {
+	const struct stk3310_chip_info *chip_info;
 	int ret;
 	struct iio_dev *indio_dev;
 	struct stk3310_data *data;
 
+	chip_info = i2c_get_match_data(client);
+	if (!chip_info)
+		return dev_err_probe(&client->dev, -ENODEV,
+				     "missing driver data\n");
+
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
 	if (!indio_dev)
 		return -ENOMEM;
@@ -654,10 +682,10 @@ static int stk3310_probe(struct i2c_client *client)
 		return ret;
 
 	indio_dev->info = &stk3310_info;
-	indio_dev->name = STK3310_DRIVER_NAME;
+	indio_dev->name = chip_info->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
-	indio_dev->channels = stk3310_channels;
-	indio_dev->num_channels = ARRAY_SIZE(stk3310_channels);
+	indio_dev->channels = chip_info->channels;
+	indio_dev->num_channels = chip_info->num_channels;
 
 	ret = stk3310_init(indio_dev);
 	if (ret < 0)
@@ -763,28 +791,27 @@ static DEFINE_SIMPLE_DEV_PM_OPS(stk3310_pm_ops, stk3310_suspend,
 				stk3310_resume);
 
 static const struct i2c_device_id stk3310_i2c_id[] = {
-	{ .name = "stk3013" },
-	{ .name = "stk3310" },
-	{ .name = "stk3311" },
-	{ .name = "stk3335" },
+	{ .name = "stk3013", .driver_data = (kernel_ulong_t)&stk3310_chip_info },
+	{ .name = "stk3310", .driver_data = (kernel_ulong_t)&stk3310_chip_info },
+	{ .name = "stk3311", .driver_data = (kernel_ulong_t)&stk3310_chip_info },
+	{ .name = "stk3335", .driver_data = (kernel_ulong_t)&stk3310_chip_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, stk3310_i2c_id);
 
 static const struct acpi_device_id stk3310_acpi_id[] = {
-	{"STK3013", 0},
-	{"STK3310", 0},
-	{"STK3311", 0},
+	{ .id = "STK3013", .driver_data = (kernel_ulong_t)&stk3310_chip_info },
+	{ .id = "STK3310", .driver_data = (kernel_ulong_t)&stk3310_chip_info },
+	{ .id = "STK3311", .driver_data = (kernel_ulong_t)&stk3310_chip_info },
 	{ }
 };
-
 MODULE_DEVICE_TABLE(acpi, stk3310_acpi_id);
 
 static const struct of_device_id stk3310_of_match[] = {
-	{ .compatible = "sensortek,stk3013", },
-	{ .compatible = "sensortek,stk3310", },
-	{ .compatible = "sensortek,stk3311", },
-	{ .compatible = "sensortek,stk3335", },
+	{ .compatible = "sensortek,stk3013", .data = &stk3310_chip_info },
+	{ .compatible = "sensortek,stk3310", .data = &stk3310_chip_info },
+	{ .compatible = "sensortek,stk3311", .data = &stk3310_chip_info },
+	{ .compatible = "sensortek,stk3335", .data = &stk3310_chip_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, stk3310_of_match);
-- 
2.55.0

Re: [PATCH v2 4/5] iio: light: stk3310: add per-chip match data

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] stk3310_write_event() modifies hardware registers and shared cached state without holding the driver lock, leading to state desynchronization.
--

commit 1a4ec350350bbed28440d2daf132d19b8c5712c9
Author: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>

    iio: light: stk3310: add per-chip match data
    
    Introduce a chip_info structure carrying the device name and channel
    specification, attach it to every i2c, OF and ACPI table entry, and
    let probe take it from the match data, failing when there is none.

> diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
> index 156888969366c..d632e6447f065 100644
> --- a/drivers/iio/light/stk3310.c
> +++ b/drivers/iio/light/stk3310.c

[Severity: High]
This is a pre-existing issue, but does stk3310_write_event() need to hold
data->lock to prevent concurrent access?

Looking at stk3310_write_event(), it modifies hardware registers and shared
cached state without holding the driver lock:

	ret = regmap_bulk_write(data->regmap, reg, &buf, sizeof(buf));
	if (ret < 0) {
		dev_err(&client->dev, "failed to set PS threshold!\n");
		return ret;
	}

	if (reg == STK3310_REG_THDH_PS)
		data->ps_thdh = val;
	else
		data->ps_thdl = val;

This allows a race between concurrent writers to cause a state
desynchronization between the hardware registers and the driver's cached
threshold values (data->ps_thdh and data->ps_thdl).

On system resume, the driver restores these incorrect cached thresholds to the
hardware, resulting in incorrect proximity event behavior.

Additionally, this function reads state that could concurrently be modified by
stk3310_write_raw() without a lock:

	ret = regmap_field_read(data->reg_ps_gain, &index);

This causes a TOCTOU race on threshold bounds checking.

Could we add a mutex_lock() to stk3310_write_event() to synchronize with
stk3310_write_raw() and prevent state desynchronization?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826175409.326131-1-jorijnvdgraaf@catcrafts.net?part=4

Re: [PATCH v2 4/5] iio: light: stk3310: add per-chip match data

[Andy Shevchenko]

On Wed, Aug 26, 2026 at 07:54:08PM +0200, Jorijn van der Graaf wrote:
> Introduce a chip_info structure carrying the device name and channel
> specification, attach it to every i2c, OF and ACPI table entry, and
> let probe take it from the match data, failing when there is none.
> Every firmware bind path carries match data, and with the id names in
> lower case a client instantiated through the sysfs new_device
> interface under a compatible-derived name receives it through the id
> table. The shared channel definitions move into macros.
> 
> The ACPI table entries change to named initializers, matching the
> other id tables.
> 
> This is a preparatory change for a variant that provides more channels
> than the existing parts. No functional change for firmware-described
> devices; a sysfs client under a name that binds without matching any
> id entry (the full compatible string) now fails probe with an error
> instead of probing as an stk3310.

...

> +/**
> + * struct stk3310_chip_info - chip-specific data
> + * @name: device name reported to the IIO core
> + * @channels: channel specification
> + * @num_channels: number of channels
> + */
> +struct stk3310_chip_info {
> +	const char			*name;
> +	const struct iio_chan_spec	*channels __counted_by_ptr(num_channels);
> +	unsigned int			num_channels;

I don't think we need tab-based indentation of the field names.

> +};

...

>  static int stk3310_probe(struct i2c_client *client)
>  {
> +	const struct stk3310_chip_info *chip_info;

	struct device *dev = &client->dev;

>  	int ret;
>  	struct iio_dev *indio_dev;
>  	struct stk3310_data *data;
>  
> +	chip_info = i2c_get_match_data(client);
> +	if (!chip_info)
> +		return dev_err_probe(&client->dev, -ENODEV,
> +				     "missing driver data\n");

Use -ENODATA

		return dev_err_probe(dev, -ENODATA, "missing driver data\n");

>  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>  	if (!indio_dev)
>  		return -ENOMEM;

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 5/5] iio: light: stk3310: support the Sensortek STK36C61

[Jorijn van der Graaf]

The Sensortek STK36C61 is a 3-in-1 ambient light / proximity / RGB
colour sensor (chip ID 0x95) found in the Fairphone 6. Its register
interface is compatible with the feature set this driver uses: the
STATE/FLAG bit layout, the data and threshold registers and the gain
and integration-time fields, verified on that device (the ALS and
proximity readings scale with their gain and integration-time fields,
thresholds written through the event interface read back from the
chip, and the FLAG near/far bit crosses with them). Add its chip ID to
the known-ID list and the device table entries.

Whenever the ALS engine runs, the chip also measures four colour
channels, laid out directly after the ALS data as 16-bit big-endian
values in R (0x15), G (0x17), B (0x19), C (0x1B) order; the R, G and B
assignments were each confirmed by the matching channel dominating
under red, green and blue illumination, and clear by its broadband
response. The ALS data register tracks the green channel exactly.

The colour controls, all verified on the device: R, G and B are gained
by the same ALSCTRL gain field the illuminance channel uses, clear by
a GAIN_F_C field in a GAINCTRL register at 0x4E (documented in the
datasheet of the STK37660, a sibling part with the same data register
layout; stepping it multiplies the clear count by ~4 per step while
the other channels hold still), and the whole colour block integrates
over the ALS integration time. Expose the channels with per-channel
scale and a shared integration time, as suggested by Jonathan Cameron.

The scale attributes reuse the driver's existing table rather than the
iio-gts helper. The green channel equals the ALS data, so its scale
must read identically to the pre-existing in_illuminance_scale, which
has always been the gain selector alone with the integration time
exposed separately; iio-gts would fold the integration time into the
scale, and its scale writes may retune the integration time behind the
illuminance channel's back. The pre-existing available-values files
stay constant attributes; converting them to read_avail is a separate
cleanup, as it changes the long-standing scale lists' text ("6.4"
becomes "6.400000").

Assisted-by: Claude:claude-fable-5
Signed-off-by: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>
---
 Documentation/ABI/testing/sysfs-bus-iio |   1 +
 drivers/iio/light/stk3310.c             | 101 ++++++++++++++++++++----
 2 files changed, 88 insertions(+), 14 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index a4f5595722ad..08a8de215814 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -507,6 +507,7 @@ What:		/sys/bus/iio/devices/iio:deviceX/in_intensity_z_scale
 What:		/sys/bus/iio/devices/iio:deviceX/in_intensity_red_scale
 What:		/sys/bus/iio/devices/iio:deviceX/in_intensity_green_scale
 What:		/sys/bus/iio/devices/iio:deviceX/in_intensity_blue_scale
+What:		/sys/bus/iio/devices/iio:deviceX/in_intensity_clear_scale
 What:		/sys/bus/iio/devices/iio:deviceX/in_concentration_co2_scale
 What:		/sys/bus/iio/devices/iio:deviceX/in_volumeflow_scale
 What:		/sys/bus/iio/devices/iio:deviceX/in_volumeflowY_scale
diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index d632e6447f06..efdb521881e6 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -40,7 +40,13 @@
 #define STK3310_REG_PS_DATA_LSB			0x12
 #define STK3310_REG_ALS_DATA_MSB		0x13
 #define STK3310_REG_ALS_DATA_LSB		0x14
+#define STK36C61_REG_RED_DATA_MSB		0x15
+#define STK36C61_REG_GREEN_DATA_MSB		0x17
+#define STK36C61_REG_BLUE_DATA_MSB		0x19
+#define STK36C61_REG_CLEAR_DATA_MSB		0x1B
+#define STK36C61_REG_CLEAR_DATA_LSB		0x1C
 #define STK3310_REG_ID				0x3E
+#define STK36C61_REG_GAINCTRL			0x4E
 #define STK3310_MAX_REG				0x80
 
 #define STK3310_STATE_EN_PS			BIT(0)
@@ -54,6 +60,7 @@
 #define STK3311S34_CHIP_ID_VAL			0x1E
 #define STK3311X_CHIP_ID_VAL			0x12
 #define STK3335_CHIP_ID_VAL			0x51
+#define STK36C61_CHIP_ID_VAL			0x95
 #define STK3310_PSINT_EN			0x01
 #define STK3310_PS_MAX_VAL			0xFFFF
 
@@ -83,6 +90,8 @@ static const struct reg_field stk3310_reg_field_als_gain =
 				REG_FIELD(STK3310_REG_ALSCTRL, 4, 5);
 static const struct reg_field stk3310_reg_field_ps_gain =
 				REG_FIELD(STK3310_REG_PSCTRL, 4, 5);
+static const struct reg_field stk3310_reg_field_clear_gain =
+				REG_FIELD(STK36C61_REG_GAINCTRL, 4, 5);
 static const struct reg_field stk3310_reg_field_als_it =
 				REG_FIELD(STK3310_REG_ALSCTRL, 0, 3);
 static const struct reg_field stk3310_reg_field_ps_it =
@@ -102,6 +111,7 @@ static const u8 stk3310_chip_ids[] = {
 	STK3311X_CHIP_ID_VAL,
 	STK3311_CHIP_ID_VAL,
 	STK3335_CHIP_ID_VAL,
+	STK36C61_CHIP_ID_VAL,
 };
 
 /* Estimate maximum proximity values with regard to measurement scale. */
@@ -132,6 +142,7 @@ struct stk3310_data {
 	struct regmap_field *reg_state;
 	struct regmap_field *reg_als_gain;
 	struct regmap_field *reg_ps_gain;
+	struct regmap_field *reg_clear_gain;
 	struct regmap_field *reg_als_it;
 	struct regmap_field *reg_ps_it;
 	struct regmap_field *reg_int_ps;
@@ -202,11 +213,35 @@ static const struct iio_chan_spec_ext_info stk3310_ext_info[] = {
 	.ext_info = stk3310_ext_info,			\
 }
 
+#define STK36C61_INTENSITY_CHANNEL(_mod, _reg) {	\
+	.type = IIO_INTENSITY,				\
+	.address = _reg,				\
+	.modified = 1,					\
+	.channel2 = IIO_MOD_LIGHT_##_mod,		\
+	.info_mask_separate =				\
+		BIT(IIO_CHAN_INFO_RAW) |		\
+		BIT(IIO_CHAN_INFO_SCALE),		\
+	.info_mask_shared_by_type =			\
+		BIT(IIO_CHAN_INFO_INT_TIME),		\
+	.info_mask_shared_by_type_available =		\
+		BIT(IIO_CHAN_INFO_SCALE) |		\
+		BIT(IIO_CHAN_INFO_INT_TIME),		\
+}
+
 static const struct iio_chan_spec stk3310_channels[] = {
 	STK3310_LIGHT_CHANNEL,
 	STK3310_PROXIMITY_CHANNEL,
 };
 
+static const struct iio_chan_spec stk36c61_channels[] = {
+	STK3310_LIGHT_CHANNEL,
+	STK3310_PROXIMITY_CHANNEL,
+	STK36C61_INTENSITY_CHANNEL(RED, STK36C61_REG_RED_DATA_MSB),
+	STK36C61_INTENSITY_CHANNEL(GREEN, STK36C61_REG_GREEN_DATA_MSB),
+	STK36C61_INTENSITY_CHANNEL(BLUE, STK36C61_REG_BLUE_DATA_MSB),
+	STK36C61_INTENSITY_CHANNEL(CLEAR, STK36C61_REG_CLEAR_DATA_MSB),
+};
+
 /**
  * struct stk3310_chip_info - chip-specific data
  * @name: device name reported to the IIO core
@@ -225,6 +260,12 @@ static const struct stk3310_chip_info stk3310_chip_info = {
 	.num_channels = ARRAY_SIZE(stk3310_channels),
 };
 
+static const struct stk3310_chip_info stk36c61_chip_info = {
+	.name = "stk36c61",
+	.channels = stk36c61_channels,
+	.num_channels = ARRAY_SIZE(stk36c61_channels),
+};
+
 static IIO_CONST_ATTR(in_illuminance_scale_available, STK3310_SCALE_AVAILABLE);
 
 static IIO_CONST_ATTR(in_proximity_scale_available, STK3310_SCALE_AVAILABLE);
@@ -401,7 +442,8 @@ static int stk3310_read_raw(struct iio_dev *indio_dev,
 	struct i2c_client *client = data->client;
 	struct regmap *map = data->regmap;
 
-	if (chan->type != IIO_LIGHT && chan->type != IIO_PROXIMITY)
+	if (chan->type != IIO_LIGHT && chan->type != IIO_PROXIMITY &&
+	    chan->type != IIO_INTENSITY)
 		return -EINVAL;
 
 	switch (mask) {
@@ -417,10 +459,10 @@ static int stk3310_read_raw(struct iio_dev *indio_dev,
 		mutex_unlock(&data->lock);
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_INT_TIME:
-		if (chan->type == IIO_LIGHT)
-			ret = regmap_field_read(data->reg_als_it, &index);
-		else
+		if (chan->type == IIO_PROXIMITY)
 			ret = regmap_field_read(data->reg_ps_it, &index);
+		else
+			ret = regmap_field_read(data->reg_als_it, &index);
 		if (ret < 0)
 			return ret;
 
@@ -428,10 +470,12 @@ static int stk3310_read_raw(struct iio_dev *indio_dev,
 		*val2 = stk3310_it_table[index][1];
 		return IIO_VAL_INT_PLUS_MICRO;
 	case IIO_CHAN_INFO_SCALE:
-		if (chan->type == IIO_LIGHT)
-			ret = regmap_field_read(data->reg_als_gain, &index);
-		else
+		if (chan->type == IIO_PROXIMITY)
 			ret = regmap_field_read(data->reg_ps_gain, &index);
+		else if (chan->channel2 == IIO_MOD_LIGHT_CLEAR)
+			ret = regmap_field_read(data->reg_clear_gain, &index);
+		else
+			ret = regmap_field_read(data->reg_als_gain, &index);
 		if (ret < 0)
 			return ret;
 
@@ -451,7 +495,8 @@ static int stk3310_write_raw(struct iio_dev *indio_dev,
 	int index;
 	struct stk3310_data *data = iio_priv(indio_dev);
 
-	if (chan->type != IIO_LIGHT && chan->type != IIO_PROXIMITY)
+	if (chan->type != IIO_LIGHT && chan->type != IIO_PROXIMITY &&
+	    chan->type != IIO_INTENSITY)
 		return -EINVAL;
 
 	switch (mask) {
@@ -462,10 +507,10 @@ static int stk3310_write_raw(struct iio_dev *indio_dev,
 		if (index < 0)
 			return -EINVAL;
 		mutex_lock(&data->lock);
-		if (chan->type == IIO_LIGHT)
-			ret = regmap_field_write(data->reg_als_it, index);
-		else
+		if (chan->type == IIO_PROXIMITY)
 			ret = regmap_field_write(data->reg_ps_it, index);
+		else
+			ret = regmap_field_write(data->reg_als_it, index);
 		if (ret < 0)
 			dev_err(&data->client->dev,
 				"sensor configuration failed\n");
@@ -479,10 +524,12 @@ static int stk3310_write_raw(struct iio_dev *indio_dev,
 		if (index < 0)
 			return -EINVAL;
 		mutex_lock(&data->lock);
-		if (chan->type == IIO_LIGHT)
-			ret = regmap_field_write(data->reg_als_gain, index);
-		else
+		if (chan->type == IIO_PROXIMITY)
 			ret = regmap_field_write(data->reg_ps_gain, index);
+		else if (chan->channel2 == IIO_MOD_LIGHT_CLEAR)
+			ret = regmap_field_write(data->reg_clear_gain, index);
+		else
+			ret = regmap_field_write(data->reg_als_gain, index);
 		if (ret < 0)
 			dev_err(&data->client->dev,
 				"sensor configuration failed\n");
@@ -493,9 +540,31 @@ static int stk3310_write_raw(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
+static int stk3310_read_avail(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      const int **vals, int *type, int *length,
+			      long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		*vals = (const int *)stk3310_scale_table;
+		*length = 2 * ARRAY_SIZE(stk3310_scale_table);
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_INT_TIME:
+		*vals = (const int *)stk3310_it_table;
+		*length = 2 * ARRAY_SIZE(stk3310_it_table);
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
 static const struct iio_info stk3310_info = {
 	.read_raw		= stk3310_read_raw,
 	.write_raw		= stk3310_write_raw,
+	.read_avail		= stk3310_read_avail,
 	.attrs			= &stk3310_attribute_group,
 	.read_event_value	= stk3310_read_event,
 	.write_event_value	= stk3310_write_event,
@@ -567,6 +636,7 @@ static bool stk3310_is_volatile_reg(struct device *dev, unsigned int reg)
 	switch (reg) {
 	case STK3310_REG_ALS_DATA_MSB:
 	case STK3310_REG_ALS_DATA_LSB:
+	case STK36C61_REG_RED_DATA_MSB ... STK36C61_REG_CLEAR_DATA_LSB:
 	case STK3310_REG_PS_DATA_LSB:
 	case STK3310_REG_PS_DATA_MSB:
 	case STK3310_REG_FLAG:
@@ -601,6 +671,7 @@ static int stk3310_regmap_init(struct stk3310_data *data)
 	STK3310_REGFIELD(state);
 	STK3310_REGFIELD(als_gain);
 	STK3310_REGFIELD(ps_gain);
+	STK3310_REGFIELD(clear_gain);
 	STK3310_REGFIELD(als_it);
 	STK3310_REGFIELD(ps_it);
 	STK3310_REGFIELD(int_ps);
@@ -795,6 +866,7 @@ static const struct i2c_device_id stk3310_i2c_id[] = {
 	{ .name = "stk3310", .driver_data = (kernel_ulong_t)&stk3310_chip_info },
 	{ .name = "stk3311", .driver_data = (kernel_ulong_t)&stk3310_chip_info },
 	{ .name = "stk3335", .driver_data = (kernel_ulong_t)&stk3310_chip_info },
+	{ .name = "stk36c61", .driver_data = (kernel_ulong_t)&stk36c61_chip_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, stk3310_i2c_id);
@@ -812,6 +884,7 @@ static const struct of_device_id stk3310_of_match[] = {
 	{ .compatible = "sensortek,stk3310", .data = &stk3310_chip_info },
 	{ .compatible = "sensortek,stk3311", .data = &stk3310_chip_info },
 	{ .compatible = "sensortek,stk3335", .data = &stk3310_chip_info },
+	{ .compatible = "sensortek,stk36c61", .data = &stk36c61_chip_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, stk3310_of_match);
-- 
2.55.0

Re: [PATCH v2 5/5] iio: light: stk3310: support the Sensortek STK36C61

[Andy Shevchenko]

On Wed, Aug 26, 2026 at 07:54:09PM +0200, Jorijn van der Graaf wrote:
> The Sensortek STK36C61 is a 3-in-1 ambient light / proximity / RGB
> colour sensor (chip ID 0x95) found in the Fairphone 6. Its register
> interface is compatible with the feature set this driver uses:

> the STATE/FLAG bit layout, the data and threshold registers and the gain
> and integration-time fields, verified on that device (the ALS and
> proximity readings scale with their gain and integration-time fields,
> thresholds written through the event interface read back from the
> chip, and the FLAG near/far bit crosses with them).

Do we need this paragraph in the commit message? To me sounds like a good
for the cover letter.

> Add its chip ID to the known-ID list and the device table entries.

> Whenever the ALS engine runs, the chip also measures four colour
> channels, laid out directly after the ALS data as 16-bit big-endian
> values in R (0x15), G (0x17), B (0x19), C (0x1B) order;

> the R, G and B assignments were each confirmed by the matching channel
> dominating under red, green and blue illumination, and clear by its broadband
> response. The ALS data register tracks the green channel exactly.

Isn't it too many details? One may decode that.

> The colour controls, all verified on the device: R, G and B are gained
> by the same ALSCTRL gain field the illuminance channel uses, clear by
> a GAIN_F_C field in a GAINCTRL register at 0x4E (documented in the
> datasheet of the STK37660, a sibling part with the same data register
> layout; stepping it multiplies the clear count by ~4 per step while
> the other channels hold still), and the whole colour block integrates
> over the ALS integration time. Expose the channels with per-channel
> scale and a shared integration time, as suggested by Jonathan Cameron.

Again, too detailed description. Try to squeeze that AI puke straight to
the point. If one needs a Datasheet, add Datasheet: tag with an URL.

> The scale attributes reuse the driver's existing table rather than the
> iio-gts helper. The green channel equals the ALS data, so its scale
> must read identically to the pre-existing in_illuminance_scale, which
> has always been the gain selector alone with the integration time
> exposed separately; iio-gts would fold the integration time into the
> scale, and its scale writes may retune the integration time behind the
> illuminance channel's back. The pre-existing available-values files
> stay constant attributes; converting them to read_avail is a separate
> cleanup, as it changes the long-standing scale lists' text ("6.4"
> becomes "6.400000").

-- 
With Best Regards,
Andy Shevchenko