On Thu, 12 Feb 2026 16:42:48 +0200 Erikas Bitovtas <[email protected]> wrote:
> Add support for Capella's CM36686 and CM36672P sensors. Capella > CM36686 is an ambient light and proximity sensor that is fully > compatible with VCNL4040 and can be used as is. For CM36672P, which is > a proximity-only sensor, also remove the IIO_LIGHT channel. > > Signed-off-by: Erikas Bitovtas <[email protected]> Hi Erikas, The following isn't necessarily suggesting a change, more something that might cause people to have a bit of a surprise. Whilst the binding having a fallback compatible is the right thing to do here, I wonder if you care that that when you use the drive via the fallback that the reported device name ends up not matching with the part you have (cm36686)? Sometimes we add specific chip_info for a part just to resolve that naming issue even if it's otherwise fully compatible. That means that an older kernel will support the device (via the fallback) but a newer kernel will also give it the name people might expect. It is an interesting corner case on whether that is preferable given it is an ABI change as they upgrade their kernel, but the name then ends up being what they want. There isn't really a right answer to this. One note below. > --- > drivers/iio/light/vcnl4000.c | 40 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c > index a36c23813679..1f8f4e4586f4 100644 > --- a/drivers/iio/light/vcnl4000.c > +++ b/drivers/iio/light/vcnl4000.c > @@ -185,6 +185,7 @@ static const int vcnl4040_ps_oversampling_ratio[] = {1, > 2, 4, 8}; > #define VCNL4000_SLEEP_DELAY_MS 2000 /* before we enter > pm_runtime_suspend */ > > enum vcnl4000_device_ids { > + CM36672P, > VCNL4000, > VCNL4010, > VCNL4040, > @@ -235,6 +236,8 @@ struct vcnl4000_chip_spec { > }; > > static const struct i2c_device_id vcnl4000_id[] = { > + { "cm36672p", CM36672P }, > + { "cm36686", VCNL4040 }, > { "vcnl4000", VCNL4000 }, > { "vcnl4010", VCNL4010 }, > { "vcnl4020", VCNL4010 }, > @@ -1842,6 +1845,22 @@ static const struct iio_chan_spec vcnl4040_channels[] > = { > } > }; ... > [VCNL4000] = { > .prod = "VCNL4000", > .init = vcnl4000_init, > @@ -2033,6 +2065,14 @@ static int vcnl4000_probe(struct i2c_client *client) > } > > static const struct of_device_id vcnl_4000_of_match[] = { > + { > + .compatible = "capella,cm36672p", > + .data = (void *)CM36672P, > + }, > + { > + .compatible = "capella,cm36686", > + .data = (void *)VCNL4040, Is this necessary? I 'think' if you drop it we'll match instead on the vcnl4040 fallback and then the access to the data will be through the stripped name only bit of the compatible (first entry, not the fallback so cm36686 in this case). So you do need the cm36686 entry in the i2c_device_id table above. Probably better to keep this here to avoid having to reason this out - but perhaps a comment to that affect would be useful (assuming you verify my reasoning). As Andy suggested moving away from enum values an towards direct pointers to the chip_info structures + drop the i2c_client_get_device_id() in favour of i2c_get_match_data() which uses the right firmware entry to get the data in all cases is the right long term solution and avoids an association being necessary between the two tables. Jonathan > + }, > { > .compatible = "vishay,vcnl4000", > .data = (void *)VCNL4000, >
