* [PATCH] iio: light: vcnl4000: Set ps high definition for 4040/4200 @ 2023-12-21 16:33 Mårten Lindahl 2023-12-26 16:19 ` Jonathan Cameron 0 siblings, 1 reply; 4+ messages in thread From: Mårten Lindahl @ 2023-12-21 16:33 UTC (permalink / raw) To: Jonathan Cameron, Lars-Peter Clausen Cc: linux-iio, linux-kernel, kernel, Mårten Lindahl The vcnl4040/vcnl4200 proximity sensor defaults to 12 bit data resolution, but the chip also supports 16 bit data resolution, which is called proximity high definition (PS_HD). Make the vcnl4040/vcnl4200 proximity sensor use the high definition for all data readings. Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> --- drivers/iio/light/vcnl4000.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c index fdf763a04b0b..4846f3b698b5 100644 --- a/drivers/iio/light/vcnl4000.c +++ b/drivers/iio/light/vcnl4000.c @@ -90,6 +90,7 @@ #define VCNL4040_PS_CONF1_PS_SHUTDOWN BIT(0) #define VCNL4040_PS_CONF2_PS_IT GENMASK(3, 1) /* Proximity integration time */ #define VCNL4040_CONF1_PS_PERS GENMASK(5, 4) /* Proximity interrupt persistence setting */ +#define VCNL4040_PS_CONF2_PS_HD BIT(11) /* Proximity high definition */ #define VCNL4040_PS_CONF2_PS_INT GENMASK(9, 8) /* Proximity interrupt mode */ #define VCNL4040_PS_CONF3_MPS GENMASK(6, 5) /* Proximity multi pulse number */ #define VCNL4040_PS_MS_LED_I GENMASK(10, 8) /* Proximity current */ @@ -345,6 +346,7 @@ static int vcnl4200_set_power_state(struct vcnl4000_data *data, bool on) static int vcnl4200_init(struct vcnl4000_data *data) { int ret, id; + u16 regval; ret = i2c_smbus_read_word_data(data->client, VCNL4200_DEV_ID); if (ret < 0) @@ -389,6 +391,17 @@ static int vcnl4200_init(struct vcnl4000_data *data) mutex_init(&data->vcnl4200_al.lock); mutex_init(&data->vcnl4200_ps.lock); + /* Use 16 bits proximity sensor readings */ + ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1); + if (ret >= 0) { + regval = (ret | VCNL4040_PS_CONF2_PS_HD); + ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, + regval); + } + + if (ret < 0) + dev_info(&data->client->dev, "Default to 12 bits sensor data"); + ret = data->chip_spec->set_power_state(data, true); if (ret < 0) return ret; --- base-commit: a39b6ac3781d46ba18193c9dbb2110f31e9bffe9 change-id: 20231221-vcnl4000-ps-hd-863f4f8fcea7 Best regards, -- Mårten Lindahl <marten.lindahl@axis.com> ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] iio: light: vcnl4000: Set ps high definition for 4040/4200 2023-12-21 16:33 [PATCH] iio: light: vcnl4000: Set ps high definition for 4040/4200 Mårten Lindahl @ 2023-12-26 16:19 ` Jonathan Cameron 2024-01-09 18:25 ` Mårten Lindahl 0 siblings, 1 reply; 4+ messages in thread From: Jonathan Cameron @ 2023-12-26 16:19 UTC (permalink / raw) To: Mårten Lindahl; +Cc: Lars-Peter Clausen, linux-iio, linux-kernel, kernel On Thu, 21 Dec 2023 17:33:09 +0100 Mårten Lindahl <marten.lindahl@axis.com> wrote: > The vcnl4040/vcnl4200 proximity sensor defaults to 12 bit data > resolution, but the chip also supports 16 bit data resolution, which is > called proximity high definition (PS_HD). > > Make the vcnl4040/vcnl4200 proximity sensor use the high definition for > all data readings. > > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> Hmm. Was about to apply this and had a nasty thought. Whilst proximity isn't 'scaled' as such because there is no absolute scale applied, I assume this change divides the effective scale (so what someone may be applying in userspace) by 16? So this might cause someone a visible userspace regression? If so we may have to report it IIO_VAL_FRACTIONAL with the bottom set to 16 so we end up with a suitable fixed point value from sysfs. Jonathan > --- > drivers/iio/light/vcnl4000.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c > index fdf763a04b0b..4846f3b698b5 100644 > --- a/drivers/iio/light/vcnl4000.c > +++ b/drivers/iio/light/vcnl4000.c > @@ -90,6 +90,7 @@ > #define VCNL4040_PS_CONF1_PS_SHUTDOWN BIT(0) > #define VCNL4040_PS_CONF2_PS_IT GENMASK(3, 1) /* Proximity integration time */ > #define VCNL4040_CONF1_PS_PERS GENMASK(5, 4) /* Proximity interrupt persistence setting */ > +#define VCNL4040_PS_CONF2_PS_HD BIT(11) /* Proximity high definition */ > #define VCNL4040_PS_CONF2_PS_INT GENMASK(9, 8) /* Proximity interrupt mode */ > #define VCNL4040_PS_CONF3_MPS GENMASK(6, 5) /* Proximity multi pulse number */ > #define VCNL4040_PS_MS_LED_I GENMASK(10, 8) /* Proximity current */ > @@ -345,6 +346,7 @@ static int vcnl4200_set_power_state(struct vcnl4000_data *data, bool on) > static int vcnl4200_init(struct vcnl4000_data *data) > { > int ret, id; > + u16 regval; > > ret = i2c_smbus_read_word_data(data->client, VCNL4200_DEV_ID); > if (ret < 0) > @@ -389,6 +391,17 @@ static int vcnl4200_init(struct vcnl4000_data *data) > mutex_init(&data->vcnl4200_al.lock); > mutex_init(&data->vcnl4200_ps.lock); > > + /* Use 16 bits proximity sensor readings */ > + ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1); > + if (ret >= 0) { > + regval = (ret | VCNL4040_PS_CONF2_PS_HD); > + ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, > + regval); > + } > + > + if (ret < 0) > + dev_info(&data->client->dev, "Default to 12 bits sensor data"); > + > ret = data->chip_spec->set_power_state(data, true); > if (ret < 0) > return ret; > > --- > base-commit: a39b6ac3781d46ba18193c9dbb2110f31e9bffe9 > change-id: 20231221-vcnl4000-ps-hd-863f4f8fcea7 > > Best regards, ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] iio: light: vcnl4000: Set ps high definition for 4040/4200 2023-12-26 16:19 ` Jonathan Cameron @ 2024-01-09 18:25 ` Mårten Lindahl 2024-01-10 9:23 ` Jonathan Cameron 0 siblings, 1 reply; 4+ messages in thread From: Mårten Lindahl @ 2024-01-09 18:25 UTC (permalink / raw) To: Jonathan Cameron; +Cc: Lars-Peter Clausen, linux-iio, linux-kernel, kernel On 12/26/23 17:19, Jonathan Cameron wrote: > On Thu, 21 Dec 2023 17:33:09 +0100 > Mårten Lindahl<marten.lindahl@axis.com> wrote: > >> The vcnl4040/vcnl4200 proximity sensor defaults to 12 bit data >> resolution, but the chip also supports 16 bit data resolution, which is >> called proximity high definition (PS_HD). >> >> Make the vcnl4040/vcnl4200 proximity sensor use the high definition for >> all data readings. >> >> Signed-off-by: Mårten Lindahl<marten.lindahl@axis.com> Hi Jonathan! > Hmm. Was about to apply this and had a nasty thought. Whilst proximity isn't > 'scaled' as such because there is no absolute scale applied, I assume this change > divides the effective scale (so what someone may be applying in userspace) by 16? > > So this might cause someone a visible userspace regression? > > If so we may have to report it IIO_VAL_FRACTIONAL with the bottom set to 16 > so we end up with a suitable fixed point value from sysfs. > > Jonathan Yes, your assumption is correct. And I found that this can easily pass unnoticed, at least on our HW. To get full 16 bit data width the sensor needs higher proximity integration time, current, and sample rate, or else it will give a raw output very close to the 12 bit steps with a maximum around 4150 compared to 12 bit max 4095. With our HW only few would notice the difference. Increasing integration time/current/sample rate is already supported from sysfs, and to get the proper scaling I can use the IIO_VAL_FRACTIONAL as you suggest, but then I also need to set these higher values from the beginning (where PS_HD is set) or else sysfs in_proximity_raw will give output "259.375000000" for 4150, instead of "4095.937500000" for 65535. If not, userspace will have to change the values manually before first read can be done. Regarding the changed output format, is it ok to change it from "4095" to "4095.937500000", without making users upset? Kind regards Mårten >> --- >> drivers/iio/light/vcnl4000.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c >> index fdf763a04b0b..4846f3b698b5 100644 >> --- a/drivers/iio/light/vcnl4000.c >> +++ b/drivers/iio/light/vcnl4000.c >> @@ -90,6 +90,7 @@ >> #define VCNL4040_PS_CONF1_PS_SHUTDOWN BIT(0) >> #define VCNL4040_PS_CONF2_PS_IT GENMASK(3, 1) /* Proximity integration time */ >> #define VCNL4040_CONF1_PS_PERS GENMASK(5, 4) /* Proximity interrupt persistence setting */ >> +#define VCNL4040_PS_CONF2_PS_HD BIT(11) /* Proximity high definition */ >> #define VCNL4040_PS_CONF2_PS_INT GENMASK(9, 8) /* Proximity interrupt mode */ >> #define VCNL4040_PS_CONF3_MPS GENMASK(6, 5) /* Proximity multi pulse number */ >> #define VCNL4040_PS_MS_LED_I GENMASK(10, 8) /* Proximity current */ >> @@ -345,6 +346,7 @@ static int vcnl4200_set_power_state(struct vcnl4000_data *data, bool on) >> static int vcnl4200_init(struct vcnl4000_data *data) >> { >> int ret, id; >> + u16 regval; >> >> ret = i2c_smbus_read_word_data(data->client, VCNL4200_DEV_ID); >> if (ret < 0) >> @@ -389,6 +391,17 @@ static int vcnl4200_init(struct vcnl4000_data *data) >> mutex_init(&data->vcnl4200_al.lock); >> mutex_init(&data->vcnl4200_ps.lock); >> >> + /* Use 16 bits proximity sensor readings */ >> + ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1); >> + if (ret >= 0) { >> + regval = (ret | VCNL4040_PS_CONF2_PS_HD); >> + ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, >> + regval); >> + } >> + >> + if (ret < 0) >> + dev_info(&data->client->dev, "Default to 12 bits sensor data"); >> + >> ret = data->chip_spec->set_power_state(data, true); >> if (ret < 0) >> return ret; >> >> --- >> base-commit: a39b6ac3781d46ba18193c9dbb2110f31e9bffe9 >> change-id: 20231221-vcnl4000-ps-hd-863f4f8fcea7 >> >> Best regards, ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] iio: light: vcnl4000: Set ps high definition for 4040/4200 2024-01-09 18:25 ` Mårten Lindahl @ 2024-01-10 9:23 ` Jonathan Cameron 0 siblings, 0 replies; 4+ messages in thread From: Jonathan Cameron @ 2024-01-10 9:23 UTC (permalink / raw) To: Mårten Lindahl Cc: Jonathan Cameron, Lars-Peter Clausen, linux-iio, linux-kernel, kernel On Tue, 9 Jan 2024 19:25:25 +0100 Mårten Lindahl <martenli@axis.com> wrote: > On 12/26/23 17:19, Jonathan Cameron wrote: > > On Thu, 21 Dec 2023 17:33:09 +0100 > > Mårten Lindahl<marten.lindahl@axis.com> wrote: > > > >> The vcnl4040/vcnl4200 proximity sensor defaults to 12 bit data > >> resolution, but the chip also supports 16 bit data resolution, which is > >> called proximity high definition (PS_HD). > >> > >> Make the vcnl4040/vcnl4200 proximity sensor use the high definition for > >> all data readings. > >> > >> Signed-off-by: Mårten Lindahl<marten.lindahl@axis.com> > Hi Jonathan! > > Hmm. Was about to apply this and had a nasty thought. Whilst proximity isn't > > 'scaled' as such because there is no absolute scale applied, I assume this change > > divides the effective scale (so what someone may be applying in userspace) by 16? > > > > So this might cause someone a visible userspace regression? > > > > If so we may have to report it IIO_VAL_FRACTIONAL with the bottom set to 16 > > so we end up with a suitable fixed point value from sysfs. > > > > Jonathan > > Yes, your assumption is correct. And I found that this can easily pass > unnoticed, at least on our HW. To get full 16 bit data width the sensor > needs higher proximity integration time, current, and sample rate, or > else it will give a raw output very close to the 12 bit steps with a > maximum around 4150 compared to 12 bit max 4095. With our HW only few > would notice the difference. > > Increasing integration time/current/sample rate is already supported > from sysfs, and to get the proper scaling I can use the > IIO_VAL_FRACTIONAL as you suggest, but then I also need to set these > higher values from the beginning (where PS_HD is set) or else sysfs > in_proximity_raw will give output "259.375000000" for 4150, instead of > "4095.937500000" for 65535. If not, userspace will have to change the > values manually before first read can be done. Ok. Then I think we are fine. > > Regarding the changed output format, is it ok to change it from "4095" > to "4095.937500000", without making users upset? Should be fine. Generic userspace code already has to deal with fixed point values (treating them as if they were fp) as other drivers will produce this anyway. We 'might' break a flakey script, but this is in the category of hoping no one notices! Jonathan > > Kind regards > > Mårten > > >> --- > >> drivers/iio/light/vcnl4000.c | 13 +++++++++++++ > >> 1 file changed, 13 insertions(+) > >> > >> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c > >> index fdf763a04b0b..4846f3b698b5 100644 > >> --- a/drivers/iio/light/vcnl4000.c > >> +++ b/drivers/iio/light/vcnl4000.c > >> @@ -90,6 +90,7 @@ > >> #define VCNL4040_PS_CONF1_PS_SHUTDOWN BIT(0) > >> #define VCNL4040_PS_CONF2_PS_IT GENMASK(3, 1) /* Proximity integration time */ > >> #define VCNL4040_CONF1_PS_PERS GENMASK(5, 4) /* Proximity interrupt persistence setting */ > >> +#define VCNL4040_PS_CONF2_PS_HD BIT(11) /* Proximity high definition */ > >> #define VCNL4040_PS_CONF2_PS_INT GENMASK(9, 8) /* Proximity interrupt mode */ > >> #define VCNL4040_PS_CONF3_MPS GENMASK(6, 5) /* Proximity multi pulse number */ > >> #define VCNL4040_PS_MS_LED_I GENMASK(10, 8) /* Proximity current */ > >> @@ -345,6 +346,7 @@ static int vcnl4200_set_power_state(struct vcnl4000_data *data, bool on) > >> static int vcnl4200_init(struct vcnl4000_data *data) > >> { > >> int ret, id; > >> + u16 regval; > >> > >> ret = i2c_smbus_read_word_data(data->client, VCNL4200_DEV_ID); > >> if (ret < 0) > >> @@ -389,6 +391,17 @@ static int vcnl4200_init(struct vcnl4000_data *data) > >> mutex_init(&data->vcnl4200_al.lock); > >> mutex_init(&data->vcnl4200_ps.lock); > >> > >> + /* Use 16 bits proximity sensor readings */ > >> + ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1); > >> + if (ret >= 0) { > >> + regval = (ret | VCNL4040_PS_CONF2_PS_HD); > >> + ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, > >> + regval); > >> + } > >> + > >> + if (ret < 0) > >> + dev_info(&data->client->dev, "Default to 12 bits sensor data"); > >> + > >> ret = data->chip_spec->set_power_state(data, true); > >> if (ret < 0) > >> return ret; > >> > >> --- > >> base-commit: a39b6ac3781d46ba18193c9dbb2110f31e9bffe9 > >> change-id: 20231221-vcnl4000-ps-hd-863f4f8fcea7 > >> > >> Best regards, > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-01-10 9:24 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-12-21 16:33 [PATCH] iio: light: vcnl4000: Set ps high definition for 4040/4200 Mårten Lindahl 2023-12-26 16:19 ` Jonathan Cameron 2024-01-09 18:25 ` Mårten Lindahl 2024-01-10 9:23 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox