* [PATCH 0/3] Add basic attributes for vcnl4040
@ 2022-09-20 18:09 Mårten Lindahl
2022-09-20 18:09 ` [PATCH 1/3] iio: light: vcnl4000: Preserve conf bits when toggle power Mårten Lindahl
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Mårten Lindahl @ 2022-09-20 18:09 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen
Cc: Paul Cercueil, Uwe Kleine-König, linux-iio, kernel,
Mårten Lindahl
Hi!
Currently there is no way for userspace to make any configuration of
the VCNL4040 sensors, but only the sensor readings are exported in
sysfs. To support basic control of the sensors such as enable/disable
and setting integration time value, sysfs attributes for this needs to
be exported.
To begin with the runtime power management turns both sensors (ALS, and
PS) on before reading the sensor register values and then switching
them off again. But when doing so it writes the whole register instead
of just switching the power on/off bit. This needs to be fixed in order
to make other persistent configurations.
Kind regards
Mårten Lindahl
Mårten Lindahl (3):
iio: light: vcnl4000: Preserve conf bits when toggle power
iio: light: vcnl4000: Add enable attributes for vcnl4040
iio: light: vcnl4000: Add ps_it attributes for vcnl4040
drivers/iio/light/vcnl4000.c | 205 ++++++++++++++++++++++++++++++++++-
1 file changed, 200 insertions(+), 5 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/3] iio: light: vcnl4000: Preserve conf bits when toggle power 2022-09-20 18:09 [PATCH 0/3] Add basic attributes for vcnl4040 Mårten Lindahl @ 2022-09-20 18:09 ` Mårten Lindahl 2022-09-20 22:23 ` Paul Cercueil 2022-09-20 18:09 ` [PATCH 2/3] iio: light: vcnl4000: Add enable attributes for vcnl4040 Mårten Lindahl 2022-09-20 18:09 ` [PATCH 3/3] iio: light: vcnl4000: Add ps_it " Mårten Lindahl 2 siblings, 1 reply; 12+ messages in thread From: Mårten Lindahl @ 2022-09-20 18:09 UTC (permalink / raw) To: Jonathan Cameron, Lars-Peter Clausen Cc: Paul Cercueil, Uwe Kleine-König, linux-iio, kernel, Mårten Lindahl As the vcnl4040 and vcnl4200 chip uses runtime power management for turning the ambient light and proximity sensors on/off, it overwrites the entire register each time. In ALS_CONF register bit fields ALS_IT, ALS_PERS, ALS_INT_EN are overwritten. In PS_CONF1 register bit fields PS_DUTY, PS_PERS, PS_IT, PS_HD, and PS_INT are overwritten. Add functions for preserving the affected bit fields when changing power state. Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> --- drivers/iio/light/vcnl4000.c | 53 ++++++++++++++++++++++++++++++++++-- 1 file changed, 51 insertions(+), 2 deletions(-) diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c index 3db4e26731bb..0b226c684957 100644 --- a/drivers/iio/light/vcnl4000.c +++ b/drivers/iio/light/vcnl4000.c @@ -74,6 +74,9 @@ #define VCNL4000_PROX_EN BIT(1) /* start proximity measurement */ #define VCNL4000_SELF_TIMED_EN BIT(0) /* start self-timed measurement */ +#define VCNL4040_ALS_CONF_ALS_SD BIT(0) /* Enable ambient light sensor */ +#define VCNL4040_PS_CONF1_PS_SD BIT(0) /* Enable proximity sensor */ + /* Bit masks for interrupt registers. */ #define VCNL4010_INT_THR_SEL BIT(0) /* Select threshold interrupt source */ #define VCNL4010_INT_THR_EN BIT(1) /* Threshold interrupt type */ @@ -188,16 +191,62 @@ static int vcnl4000_init(struct vcnl4000_data *data) return data->chip_spec->set_power_state(data, true); }; +static ssize_t vcnl4000_write_als_enable(struct vcnl4000_data *data, int val) +{ + int ret; + + switch (data->id) { + case VCNL4040: + case VCNL4200: + ret = i2c_smbus_read_word_data(data->client, VCNL4200_AL_CONF); + if (ret < 0) + return ret; + + if (val) + ret &= ~VCNL4040_ALS_CONF_ALS_SD; + else + ret |= VCNL4040_ALS_CONF_ALS_SD; + + return i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF, + ret); + default: + return -EINVAL; + } +} + +static ssize_t vcnl4000_write_ps_enable(struct vcnl4000_data *data, int val) +{ + int ret; + + switch (data->id) { + case VCNL4040: + case VCNL4200: + ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1); + if (ret < 0) + return ret; + + if (val) + ret &= ~VCNL4040_PS_CONF1_PS_SD; + else + ret |= VCNL4040_PS_CONF1_PS_SD; + + return i2c_smbus_write_word_data(data->client, + VCNL4200_PS_CONF1, ret); + default: + return -EINVAL; + } +} + static int vcnl4200_set_power_state(struct vcnl4000_data *data, bool on) { u16 val = on ? 0 /* power on */ : 1 /* shut down */; int ret; - ret = i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF, val); + ret = vcnl4000_write_als_enable(data, !val); if (ret < 0) return ret; - ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, val); + ret = vcnl4000_write_ps_enable(data, !val); if (ret < 0) return ret; -- 2.30.2 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] iio: light: vcnl4000: Preserve conf bits when toggle power 2022-09-20 18:09 ` [PATCH 1/3] iio: light: vcnl4000: Preserve conf bits when toggle power Mårten Lindahl @ 2022-09-20 22:23 ` Paul Cercueil 2022-09-22 12:18 ` Marten Lindahl 0 siblings, 1 reply; 12+ messages in thread From: Paul Cercueil @ 2022-09-20 22:23 UTC (permalink / raw) To: Mårten Lindahl Cc: Jonathan Cameron, Lars-Peter Clausen, Uwe Kleine-König, linux-iio, kernel Hi Mårten, Le mar., sept. 20 2022 at 20:09:56 +0200, Mårten Lindahl <marten.lindahl@axis.com> a écrit : > As the vcnl4040 and vcnl4200 chip uses runtime power management for > turning the ambient light and proximity sensors on/off, it overwrites > the entire register each time. In ALS_CONF register bit fields ALS_IT, > ALS_PERS, ALS_INT_EN are overwritten. In PS_CONF1 register bit fields > PS_DUTY, PS_PERS, PS_IT, PS_HD, and PS_INT are overwritten. > > Add functions for preserving the affected bit fields when changing > power > state. > > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> > --- > drivers/iio/light/vcnl4000.c | 53 > ++++++++++++++++++++++++++++++++++-- > 1 file changed, 51 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/light/vcnl4000.c > b/drivers/iio/light/vcnl4000.c > index 3db4e26731bb..0b226c684957 100644 > --- a/drivers/iio/light/vcnl4000.c > +++ b/drivers/iio/light/vcnl4000.c > @@ -74,6 +74,9 @@ > #define VCNL4000_PROX_EN BIT(1) /* start proximity measurement */ > #define VCNL4000_SELF_TIMED_EN BIT(0) /* start self-timed > measurement */ > > +#define VCNL4040_ALS_CONF_ALS_SD BIT(0) /* Enable ambient light > sensor */ > +#define VCNL4040_PS_CONF1_PS_SD BIT(0) /* Enable proximity sensor */ > + > /* Bit masks for interrupt registers. */ > #define VCNL4010_INT_THR_SEL BIT(0) /* Select threshold interrupt > source */ > #define VCNL4010_INT_THR_EN BIT(1) /* Threshold interrupt type */ > @@ -188,16 +191,62 @@ static int vcnl4000_init(struct vcnl4000_data > *data) > return data->chip_spec->set_power_state(data, true); > }; > > +static ssize_t vcnl4000_write_als_enable(struct vcnl4000_data *data, > int val) > +{ > + int ret; > + > + switch (data->id) { > + case VCNL4040: > + case VCNL4200: The switch isn't needed, since vcnl4200_set_power_state() is only called for compatible chips. > + ret = i2c_smbus_read_word_data(data->client, VCNL4200_AL_CONF); > + if (ret < 0) > + return ret; > + > + if (val) > + ret &= ~VCNL4040_ALS_CONF_ALS_SD; > + else > + ret |= VCNL4040_ALS_CONF_ALS_SD; > + > + return i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF, > + ret); Are you sure a race cannot happen here? I would assume it cannot, but that's still a bit fishy. This driver would benefit from a regmap conversion, but it's probably a bit too much to ask... > + default: > + return -EINVAL; > + } > +} > + > +static ssize_t vcnl4000_write_ps_enable(struct vcnl4000_data *data, > int val) > +{ > + int ret; > + > + switch (data->id) { > + case VCNL4040: > + case VCNL4200: > + ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1); > + if (ret < 0) > + return ret; > + > + if (val) > + ret &= ~VCNL4040_PS_CONF1_PS_SD; > + else > + ret |= VCNL4040_PS_CONF1_PS_SD; > + > + return i2c_smbus_write_word_data(data->client, > + VCNL4200_PS_CONF1, ret); > + default: > + return -EINVAL; > + } > +} > + > static int vcnl4200_set_power_state(struct vcnl4000_data *data, bool > on) > { > u16 val = on ? 0 /* power on */ : 1 /* shut down */; > int ret; > > - ret = i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF, > val); > + ret = vcnl4000_write_als_enable(data, !val); You set "val" to 0 if "on", and to 1 if "!on", then pass "!val", this is very confusing. You could just pass "on" here and below. Cheers, -Paul > if (ret < 0) > return ret; > > - ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, > val); > + ret = vcnl4000_write_ps_enable(data, !val); > if (ret < 0) > return ret; > > -- > 2.30.2 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] iio: light: vcnl4000: Preserve conf bits when toggle power 2022-09-20 22:23 ` Paul Cercueil @ 2022-09-22 12:18 ` Marten Lindahl 0 siblings, 0 replies; 12+ messages in thread From: Marten Lindahl @ 2022-09-22 12:18 UTC (permalink / raw) To: Paul Cercueil Cc: Mårten Lindahl, Jonathan Cameron, Lars-Peter Clausen, Uwe Kleine-König, linux-iio, kernel On Wed, Sep 21, 2022 at 12:23:26AM +0200, Paul Cercueil wrote: > Hi Mårten, > > Le mar., sept. 20 2022 at 20:09:56 +0200, Mårten Lindahl > <marten.lindahl@axis.com> a écrit : > > As the vcnl4040 and vcnl4200 chip uses runtime power management for > > turning the ambient light and proximity sensors on/off, it overwrites > > the entire register each time. In ALS_CONF register bit fields ALS_IT, > > ALS_PERS, ALS_INT_EN are overwritten. In PS_CONF1 register bit fields > > PS_DUTY, PS_PERS, PS_IT, PS_HD, and PS_INT are overwritten. > > > > Add functions for preserving the affected bit fields when changing > > power > > state. > > > > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> > > --- > > drivers/iio/light/vcnl4000.c | 53 > > ++++++++++++++++++++++++++++++++++-- > > 1 file changed, 51 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iio/light/vcnl4000.c > > b/drivers/iio/light/vcnl4000.c > > index 3db4e26731bb..0b226c684957 100644 > > --- a/drivers/iio/light/vcnl4000.c > > +++ b/drivers/iio/light/vcnl4000.c > > @@ -74,6 +74,9 @@ > > #define VCNL4000_PROX_EN BIT(1) /* start proximity measurement */ > > #define VCNL4000_SELF_TIMED_EN BIT(0) /* start self-timed > > measurement */ > > > > +#define VCNL4040_ALS_CONF_ALS_SD BIT(0) /* Enable ambient light > > sensor */ > > +#define VCNL4040_PS_CONF1_PS_SD BIT(0) /* Enable proximity sensor */ > > + > > /* Bit masks for interrupt registers. */ > > #define VCNL4010_INT_THR_SEL BIT(0) /* Select threshold interrupt > > source */ > > #define VCNL4010_INT_THR_EN BIT(1) /* Threshold interrupt type */ > > @@ -188,16 +191,62 @@ static int vcnl4000_init(struct vcnl4000_data > > *data) > > return data->chip_spec->set_power_state(data, true); > > }; > > > > +static ssize_t vcnl4000_write_als_enable(struct vcnl4000_data *data, > > int val) > > +{ > > + int ret; > > + > > + switch (data->id) { > > + case VCNL4040: > > + case VCNL4200: Hi Paul! Thanks for looking at this! > > The switch isn't needed, since vcnl4200_set_power_state() is only > called for compatible chips. Yes, your're right. I will remove the switch. > > > + ret = i2c_smbus_read_word_data(data->client, VCNL4200_AL_CONF); > > + if (ret < 0) > > + return ret; > > + > > + if (val) > > + ret &= ~VCNL4040_ALS_CONF_ALS_SD; > > + else > > + ret |= VCNL4040_ALS_CONF_ALS_SD; > > + > > + return i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF, > > + ret); > > Are you sure a race cannot happen here? > > I would assume it cannot, but that's still a bit fishy. Ok, I see what you mean. I cannot tell for sure, but I can guard the read and write with the mutex. > > This driver would benefit from a regmap conversion, but it's probably a > bit too much to ask... I can try to do this in a separate patch after these ones if that's ok? > > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static ssize_t vcnl4000_write_ps_enable(struct vcnl4000_data *data, > > int val) > > +{ > > + int ret; > > + > > + switch (data->id) { > > + case VCNL4040: > > + case VCNL4200: > > + ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1); > > + if (ret < 0) > > + return ret; > > + > > + if (val) > > + ret &= ~VCNL4040_PS_CONF1_PS_SD; > > + else > > + ret |= VCNL4040_PS_CONF1_PS_SD; > > + > > + return i2c_smbus_write_word_data(data->client, > > + VCNL4200_PS_CONF1, ret); > > + default: > > + return -EINVAL; > > + } > > +} > > + > > static int vcnl4200_set_power_state(struct vcnl4000_data *data, bool > > on) > > { > > u16 val = on ? 0 /* power on */ : 1 /* shut down */; > > int ret; > > > > - ret = i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF, > > val); > > + ret = vcnl4000_write_als_enable(data, !val); > > You set "val" to 0 if "on", and to 1 if "!on", then pass "!val", this > is very confusing. You could just pass "on" here and below. I agree! I will change it to only use the boolean 'on'. Kind reagards Mårten > > Cheers, > -Paul > > > if (ret < 0) > > return ret; > > > > - ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, > > val); > > + ret = vcnl4000_write_ps_enable(data, !val); > > if (ret < 0) > > return ret; > > > > -- > > 2.30.2 > > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] iio: light: vcnl4000: Add enable attributes for vcnl4040 2022-09-20 18:09 [PATCH 0/3] Add basic attributes for vcnl4040 Mårten Lindahl 2022-09-20 18:09 ` [PATCH 1/3] iio: light: vcnl4000: Preserve conf bits when toggle power Mårten Lindahl @ 2022-09-20 18:09 ` Mårten Lindahl 2022-09-20 22:01 ` Paul Cercueil 2022-09-20 18:09 ` [PATCH 3/3] iio: light: vcnl4000: Add ps_it " Mårten Lindahl 2 siblings, 1 reply; 12+ messages in thread From: Mårten Lindahl @ 2022-09-20 18:09 UTC (permalink / raw) To: Jonathan Cameron, Lars-Peter Clausen Cc: Paul Cercueil, Uwe Kleine-König, linux-iio, kernel, Mårten Lindahl Add channel attribute in_illuminance_en and in_proximity_en with read/write access for vcnl4040. If automatic runtime power management is turned off (power/control = on), both sensors can be kept on or off by userspace. Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> --- drivers/iio/light/vcnl4000.c | 79 ++++++++++++++++++++++++++++++++---- 1 file changed, 72 insertions(+), 7 deletions(-) diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c index 0b226c684957..9838f0868372 100644 --- a/drivers/iio/light/vcnl4000.c +++ b/drivers/iio/light/vcnl4000.c @@ -125,6 +125,9 @@ struct vcnl4000_data { enum vcnl4000_device_ids id; int rev; int al_scale; + bool als_enable; + bool ps_enable; + const struct vcnl4000_chip_spec *chip_spec; struct mutex vcnl4000_lock; struct vcnl4200_channel vcnl4200_al; @@ -202,10 +205,13 @@ static ssize_t vcnl4000_write_als_enable(struct vcnl4000_data *data, int val) if (ret < 0) return ret; - if (val) + if (val) { ret &= ~VCNL4040_ALS_CONF_ALS_SD; - else + data->als_enable = true; + } else { ret |= VCNL4040_ALS_CONF_ALS_SD; + data->als_enable = false; + } return i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF, ret); @@ -225,10 +231,13 @@ static ssize_t vcnl4000_write_ps_enable(struct vcnl4000_data *data, int val) if (ret < 0) return ret; - if (val) + if (val) { ret &= ~VCNL4040_PS_CONF1_PS_SD; - else + data->ps_enable = true; + } else { ret |= VCNL4040_PS_CONF1_PS_SD; + data->ps_enable = false; + } return i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, ret); @@ -283,6 +292,8 @@ static int vcnl4200_init(struct vcnl4000_data *data) dev_dbg(&data->client->dev, "device id 0x%x", id); data->rev = (ret >> 8) & 0xf; + data->als_enable = false; + data->ps_enable = false; data->vcnl4200_al.reg = VCNL4200_AL_DATA; data->vcnl4200_ps.reg = VCNL4200_PS_DATA; @@ -459,8 +470,12 @@ static bool vcnl4010_is_in_periodic_mode(struct vcnl4000_data *data) static int vcnl4000_set_pm_runtime_state(struct vcnl4000_data *data, bool on) { struct device *dev = &data->client->dev; + struct iio_dev *indio_dev = i2c_get_clientdata(data->client); int ret; + if (!indio_dev->dev.power.runtime_auto) + return 0; + if (on) { ret = pm_runtime_resume_and_get(dev); } else { @@ -507,6 +522,38 @@ static int vcnl4000_read_raw(struct iio_dev *indio_dev, *val = 0; *val2 = data->al_scale; return IIO_VAL_INT_PLUS_MICRO; + case IIO_CHAN_INFO_ENABLE: + switch (chan->type) { + case IIO_LIGHT: + *val = data->als_enable; + return IIO_VAL_INT; + case IIO_PROXIMITY: + *val = data->ps_enable; + return IIO_VAL_INT; + default: + return -EINVAL; + } + default: + return -EINVAL; + } +} + +static int vcnl4040_write_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int val, int val2, long mask) +{ + struct vcnl4000_data *data = iio_priv(indio_dev); + + switch (mask) { + case IIO_CHAN_INFO_ENABLE: + switch (chan->type) { + case IIO_LIGHT: + return vcnl4000_write_als_enable(data, val); + case IIO_PROXIMITY: + return vcnl4000_write_ps_enable(data, val); + default: + return -EINVAL; + } default: return -EINVAL; } @@ -845,6 +892,19 @@ static const struct iio_chan_spec vcnl4010_channels[] = { IIO_CHAN_SOFT_TIMESTAMP(1), }; +static const struct iio_chan_spec vcnl4040_channels[] = { + { + .type = IIO_LIGHT, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_ENABLE), + }, { + .type = IIO_PROXIMITY, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_ENABLE), + .ext_info = vcnl4000_ext_info, + } +}; + static const struct iio_info vcnl4000_info = { .read_raw = vcnl4000_read_raw, }; @@ -859,6 +919,11 @@ static const struct iio_info vcnl4010_info = { .write_event_config = vcnl4010_write_event_config, }; +static const struct iio_info vcnl4040_info = { + .read_raw = vcnl4000_read_raw, + .write_raw = vcnl4040_write_raw, +}; + static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = { [VCNL4000] = { .prod = "VCNL4000", @@ -888,9 +953,9 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = { .measure_light = vcnl4200_measure_light, .measure_proximity = vcnl4200_measure_proximity, .set_power_state = vcnl4200_set_power_state, - .channels = vcnl4000_channels, - .num_channels = ARRAY_SIZE(vcnl4000_channels), - .info = &vcnl4000_info, + .channels = vcnl4040_channels, + .num_channels = ARRAY_SIZE(vcnl4040_channels), + .info = &vcnl4040_info, .irq_support = false, }, [VCNL4200] = { -- 2.30.2 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] iio: light: vcnl4000: Add enable attributes for vcnl4040 2022-09-20 18:09 ` [PATCH 2/3] iio: light: vcnl4000: Add enable attributes for vcnl4040 Mårten Lindahl @ 2022-09-20 22:01 ` Paul Cercueil 2022-09-22 13:04 ` Marten Lindahl 0 siblings, 1 reply; 12+ messages in thread From: Paul Cercueil @ 2022-09-20 22:01 UTC (permalink / raw) To: Mårten Lindahl Cc: Jonathan Cameron, Lars-Peter Clausen, Uwe Kleine-König, linux-iio, kernel Hi Mårten, Le mar., sept. 20 2022 at 20:09:57 +0200, Mårten Lindahl <marten.lindahl@axis.com> a écrit : > Add channel attribute in_illuminance_en and in_proximity_en with > read/write access for vcnl4040. If automatic runtime power management > is > turned off (power/control = on), both sensors can be kept on or off by > userspace. I don't really understand this. If automatic runtime power management is turned OFF, I would expect both sensors to be kept ON always. It's not userspace's job to do power management of the chip. Why are these channel attributes needed? Cheers, -Paul > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> > --- > drivers/iio/light/vcnl4000.c | 79 > ++++++++++++++++++++++++++++++++---- > 1 file changed, 72 insertions(+), 7 deletions(-) > > diff --git a/drivers/iio/light/vcnl4000.c > b/drivers/iio/light/vcnl4000.c > index 0b226c684957..9838f0868372 100644 > --- a/drivers/iio/light/vcnl4000.c > +++ b/drivers/iio/light/vcnl4000.c > @@ -125,6 +125,9 @@ struct vcnl4000_data { > enum vcnl4000_device_ids id; > int rev; > int al_scale; > + bool als_enable; > + bool ps_enable; > + > const struct vcnl4000_chip_spec *chip_spec; > struct mutex vcnl4000_lock; > struct vcnl4200_channel vcnl4200_al; > @@ -202,10 +205,13 @@ static ssize_t vcnl4000_write_als_enable(struct > vcnl4000_data *data, int val) > if (ret < 0) > return ret; > > - if (val) > + if (val) { > ret &= ~VCNL4040_ALS_CONF_ALS_SD; > - else > + data->als_enable = true; > + } else { > ret |= VCNL4040_ALS_CONF_ALS_SD; > + data->als_enable = false; > + } > > return i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF, > ret); > @@ -225,10 +231,13 @@ static ssize_t vcnl4000_write_ps_enable(struct > vcnl4000_data *data, int val) > if (ret < 0) > return ret; > > - if (val) > + if (val) { > ret &= ~VCNL4040_PS_CONF1_PS_SD; > - else > + data->ps_enable = true; > + } else { > ret |= VCNL4040_PS_CONF1_PS_SD; > + data->ps_enable = false; > + } > > return i2c_smbus_write_word_data(data->client, > VCNL4200_PS_CONF1, ret); > @@ -283,6 +292,8 @@ static int vcnl4200_init(struct vcnl4000_data > *data) > dev_dbg(&data->client->dev, "device id 0x%x", id); > > data->rev = (ret >> 8) & 0xf; > + data->als_enable = false; > + data->ps_enable = false; > > data->vcnl4200_al.reg = VCNL4200_AL_DATA; > data->vcnl4200_ps.reg = VCNL4200_PS_DATA; > @@ -459,8 +470,12 @@ static bool vcnl4010_is_in_periodic_mode(struct > vcnl4000_data *data) > static int vcnl4000_set_pm_runtime_state(struct vcnl4000_data *data, > bool on) > { > struct device *dev = &data->client->dev; > + struct iio_dev *indio_dev = i2c_get_clientdata(data->client); > int ret; > > + if (!indio_dev->dev.power.runtime_auto) > + return 0; > + > if (on) { > ret = pm_runtime_resume_and_get(dev); > } else { > @@ -507,6 +522,38 @@ static int vcnl4000_read_raw(struct iio_dev > *indio_dev, > *val = 0; > *val2 = data->al_scale; > return IIO_VAL_INT_PLUS_MICRO; > + case IIO_CHAN_INFO_ENABLE: > + switch (chan->type) { > + case IIO_LIGHT: > + *val = data->als_enable; > + return IIO_VAL_INT; > + case IIO_PROXIMITY: > + *val = data->ps_enable; > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > + default: > + return -EINVAL; > + } > +} > + > +static int vcnl4040_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int val, int val2, long mask) > +{ > + struct vcnl4000_data *data = iio_priv(indio_dev); > + > + switch (mask) { > + case IIO_CHAN_INFO_ENABLE: > + switch (chan->type) { > + case IIO_LIGHT: > + return vcnl4000_write_als_enable(data, val); > + case IIO_PROXIMITY: > + return vcnl4000_write_ps_enable(data, val); > + default: > + return -EINVAL; > + } > default: > return -EINVAL; > } > @@ -845,6 +892,19 @@ static const struct iio_chan_spec > vcnl4010_channels[] = { > IIO_CHAN_SOFT_TIMESTAMP(1), > }; > > +static const struct iio_chan_spec vcnl4040_channels[] = { > + { > + .type = IIO_LIGHT, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_ENABLE), > + }, { > + .type = IIO_PROXIMITY, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_ENABLE), > + .ext_info = vcnl4000_ext_info, > + } > +}; > + > static const struct iio_info vcnl4000_info = { > .read_raw = vcnl4000_read_raw, > }; > @@ -859,6 +919,11 @@ static const struct iio_info vcnl4010_info = { > .write_event_config = vcnl4010_write_event_config, > }; > > +static const struct iio_info vcnl4040_info = { > + .read_raw = vcnl4000_read_raw, > + .write_raw = vcnl4040_write_raw, > +}; > + > static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = { > [VCNL4000] = { > .prod = "VCNL4000", > @@ -888,9 +953,9 @@ static const struct vcnl4000_chip_spec > vcnl4000_chip_spec_cfg[] = { > .measure_light = vcnl4200_measure_light, > .measure_proximity = vcnl4200_measure_proximity, > .set_power_state = vcnl4200_set_power_state, > - .channels = vcnl4000_channels, > - .num_channels = ARRAY_SIZE(vcnl4000_channels), > - .info = &vcnl4000_info, > + .channels = vcnl4040_channels, > + .num_channels = ARRAY_SIZE(vcnl4040_channels), > + .info = &vcnl4040_info, > .irq_support = false, > }, > [VCNL4200] = { > -- > 2.30.2 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] iio: light: vcnl4000: Add enable attributes for vcnl4040 2022-09-20 22:01 ` Paul Cercueil @ 2022-09-22 13:04 ` Marten Lindahl 2022-09-22 14:10 ` Paul Cercueil 0 siblings, 1 reply; 12+ messages in thread From: Marten Lindahl @ 2022-09-22 13:04 UTC (permalink / raw) To: Paul Cercueil Cc: Mårten Lindahl, Jonathan Cameron, Lars-Peter Clausen, Uwe Kleine-König, linux-iio, kernel On Wed, Sep 21, 2022 at 12:01:24AM +0200, Paul Cercueil wrote: > Hi Mårten, > > Le mar., sept. 20 2022 at 20:09:57 +0200, Mårten Lindahl > <marten.lindahl@axis.com> a écrit : > > Add channel attribute in_illuminance_en and in_proximity_en with > > read/write access for vcnl4040. If automatic runtime power management > > is > > turned off (power/control = on), both sensors can be kept on or off by > > userspace. > Hi Paul! > I don't really understand this. If automatic runtime power management > is turned OFF, I would expect both sensors to be kept ON always. > > It's not userspace's job to do power management of the chip. Why are > these channel attributes needed? I think I understand the problem here. I added the *_en attributes because I couldn't see any way to turn the sensors on without forcing it on during the *_raw read operation (with vcnl4000_set_pm_runtime_state(true)) after which it is turned off again (false). Even if the power/control is set to 'on', there will be no callback for changing the state to active. It seems this does not work in this driver (with or without my patches) and I was confused by how it was supposed to work. But after some digging I suspect there could be a bug in the driver since the sysfs control/* nodes seems to operate on the indio_dev->dev and not on the driver dev, which is used for the vcnl4000 driver pm_runtime operations. Setting the power/control to 'on' invokes the rpm_resume function which checks the dev.power.disable_depth attribute before it calls the resume_callback for setting the active state on the driver. But if the dev.power.disable_depth == 1 (which is the init value) the callback will not be called. And nothing happens. And I suspect the reason why the dev.power.disable_depth is 1 may be that it is not the vcnl4000 dev object that is being checked, but the indio_dev->dev object, which has not been configured for pm_runtime operations in the driver. Sorry for a long reply to your question, but I suspect that if the automatic pm_runtime for this driver can be disabled by the sysfs power/control, the *_en attributes wont be needed. I will look into why it does not work. Kind regards Mårten > > Cheers, > -Paul > > > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> > > --- > > drivers/iio/light/vcnl4000.c | 79 > > ++++++++++++++++++++++++++++++++---- > > 1 file changed, 72 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/iio/light/vcnl4000.c > > b/drivers/iio/light/vcnl4000.c > > index 0b226c684957..9838f0868372 100644 > > --- a/drivers/iio/light/vcnl4000.c > > +++ b/drivers/iio/light/vcnl4000.c > > @@ -125,6 +125,9 @@ struct vcnl4000_data { > > enum vcnl4000_device_ids id; > > int rev; > > int al_scale; > > + bool als_enable; > > + bool ps_enable; > > + > > const struct vcnl4000_chip_spec *chip_spec; > > struct mutex vcnl4000_lock; > > struct vcnl4200_channel vcnl4200_al; > > @@ -202,10 +205,13 @@ static ssize_t vcnl4000_write_als_enable(struct > > vcnl4000_data *data, int val) > > if (ret < 0) > > return ret; > > > > - if (val) > > + if (val) { > > ret &= ~VCNL4040_ALS_CONF_ALS_SD; > > - else > > + data->als_enable = true; > > + } else { > > ret |= VCNL4040_ALS_CONF_ALS_SD; > > + data->als_enable = false; > > + } > > > > return i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF, > > ret); > > @@ -225,10 +231,13 @@ static ssize_t vcnl4000_write_ps_enable(struct > > vcnl4000_data *data, int val) > > if (ret < 0) > > return ret; > > > > - if (val) > > + if (val) { > > ret &= ~VCNL4040_PS_CONF1_PS_SD; > > - else > > + data->ps_enable = true; > > + } else { > > ret |= VCNL4040_PS_CONF1_PS_SD; > > + data->ps_enable = false; > > + } > > > > return i2c_smbus_write_word_data(data->client, > > VCNL4200_PS_CONF1, ret); > > @@ -283,6 +292,8 @@ static int vcnl4200_init(struct vcnl4000_data > > *data) > > dev_dbg(&data->client->dev, "device id 0x%x", id); > > > > data->rev = (ret >> 8) & 0xf; > > + data->als_enable = false; > > + data->ps_enable = false; > > > > data->vcnl4200_al.reg = VCNL4200_AL_DATA; > > data->vcnl4200_ps.reg = VCNL4200_PS_DATA; > > @@ -459,8 +470,12 @@ static bool vcnl4010_is_in_periodic_mode(struct > > vcnl4000_data *data) > > static int vcnl4000_set_pm_runtime_state(struct vcnl4000_data *data, > > bool on) > > { > > struct device *dev = &data->client->dev; > > + struct iio_dev *indio_dev = i2c_get_clientdata(data->client); > > int ret; > > > > + if (!indio_dev->dev.power.runtime_auto) > > + return 0; > > + > > if (on) { > > ret = pm_runtime_resume_and_get(dev); > > } else { > > @@ -507,6 +522,38 @@ static int vcnl4000_read_raw(struct iio_dev > > *indio_dev, > > *val = 0; > > *val2 = data->al_scale; > > return IIO_VAL_INT_PLUS_MICRO; > > + case IIO_CHAN_INFO_ENABLE: > > + switch (chan->type) { > > + case IIO_LIGHT: > > + *val = data->als_enable; > > + return IIO_VAL_INT; > > + case IIO_PROXIMITY: > > + *val = data->ps_enable; > > + return IIO_VAL_INT; > > + default: > > + return -EINVAL; > > + } > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static int vcnl4040_write_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, > > + int val, int val2, long mask) > > +{ > > + struct vcnl4000_data *data = iio_priv(indio_dev); > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_ENABLE: > > + switch (chan->type) { > > + case IIO_LIGHT: > > + return vcnl4000_write_als_enable(data, val); > > + case IIO_PROXIMITY: > > + return vcnl4000_write_ps_enable(data, val); > > + default: > > + return -EINVAL; > > + } > > default: > > return -EINVAL; > > } > > @@ -845,6 +892,19 @@ static const struct iio_chan_spec > > vcnl4010_channels[] = { > > IIO_CHAN_SOFT_TIMESTAMP(1), > > }; > > > > +static const struct iio_chan_spec vcnl4040_channels[] = { > > + { > > + .type = IIO_LIGHT, > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > > + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_ENABLE), > > + }, { > > + .type = IIO_PROXIMITY, > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > > + BIT(IIO_CHAN_INFO_ENABLE), > > + .ext_info = vcnl4000_ext_info, > > + } > > +}; > > + > > static const struct iio_info vcnl4000_info = { > > .read_raw = vcnl4000_read_raw, > > }; > > @@ -859,6 +919,11 @@ static const struct iio_info vcnl4010_info = { > > .write_event_config = vcnl4010_write_event_config, > > }; > > > > +static const struct iio_info vcnl4040_info = { > > + .read_raw = vcnl4000_read_raw, > > + .write_raw = vcnl4040_write_raw, > > +}; > > + > > static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = { > > [VCNL4000] = { > > .prod = "VCNL4000", > > @@ -888,9 +953,9 @@ static const struct vcnl4000_chip_spec > > vcnl4000_chip_spec_cfg[] = { > > .measure_light = vcnl4200_measure_light, > > .measure_proximity = vcnl4200_measure_proximity, > > .set_power_state = vcnl4200_set_power_state, > > - .channels = vcnl4000_channels, > > - .num_channels = ARRAY_SIZE(vcnl4000_channels), > > - .info = &vcnl4000_info, > > + .channels = vcnl4040_channels, > > + .num_channels = ARRAY_SIZE(vcnl4040_channels), > > + .info = &vcnl4040_info, > > .irq_support = false, > > }, > > [VCNL4200] = { > > -- > > 2.30.2 > > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] iio: light: vcnl4000: Add enable attributes for vcnl4040 2022-09-22 13:04 ` Marten Lindahl @ 2022-09-22 14:10 ` Paul Cercueil 2022-09-22 18:39 ` Marten Lindahl 0 siblings, 1 reply; 12+ messages in thread From: Paul Cercueil @ 2022-09-22 14:10 UTC (permalink / raw) To: Marten Lindahl Cc: Mårten Lindahl, Jonathan Cameron, Lars-Peter Clausen, Uwe Kleine-König, linux-iio, kernel Le jeu., sept. 22 2022 at 15:04:47 +0200, Marten Lindahl <martenli@axis.com> a écrit : > On Wed, Sep 21, 2022 at 12:01:24AM +0200, Paul Cercueil wrote: >> Hi Mårten, >> >> Le mar., sept. 20 2022 at 20:09:57 +0200, Mårten Lindahl >> <marten.lindahl@axis.com> a écrit : >> > Add channel attribute in_illuminance_en and in_proximity_en with >> > read/write access for vcnl4040. If automatic runtime power >> management >> > is >> > turned off (power/control = on), both sensors can be kept on or >> off by >> > userspace. >> > > Hi Paul! > >> I don't really understand this. If automatic runtime power >> management >> is turned OFF, I would expect both sensors to be kept ON always. >> >> It's not userspace's job to do power management of the chip. Why are >> these channel attributes needed? > > I think I understand the problem here. I added the *_en attributes > because I couldn't see any way to turn the sensors on without forcing > it > on during the *_raw read operation (with > vcnl4000_set_pm_runtime_state(true)) > after which it is turned off again (false). What's wrong with doing that? > Even if the power/control is set to 'on', there will be no callback > for > changing the state to active. > > It seems this does not work in this driver (with or without my > patches) and I > was confused by how it was supposed to work. But after some digging I > suspect > there could be a bug in the driver since the sysfs control/* nodes > seems to > operate on the indio_dev->dev and not on the driver dev, which is > used for > the vcnl4000 driver pm_runtime operations. I believe this is normal. The devm_iio_device_alloc() creates a new device, whose parent is your i2c_client->dev. > Setting the power/control to 'on' invokes the rpm_resume function > which > checks the dev.power.disable_depth attribute before it calls the > resume_callback for setting the active state on the driver. But if the > dev.power.disable_depth == 1 (which is the init value) the callback > will not > be called. And nothing happens. And I suspect the reason why the > dev.power.disable_depth is 1 may be that it is not the vcnl4000 dev > object that is being checked, but the indio_dev->dev object, which > has not > been configured for pm_runtime operations in the driver. > > Sorry for a long reply to your question, but I suspect that if the > automatic pm_runtime for this driver can be disabled by the sysfs > power/control, the *_en attributes wont be needed. > > I will look into why it does not work. I still don't understand. Why do you *need* to disable runtime PM? -Paul > Kind regards > Mårten > >> >> Cheers, >> -Paul >> >> > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> >> > --- >> > drivers/iio/light/vcnl4000.c | 79 >> > ++++++++++++++++++++++++++++++++---- >> > 1 file changed, 72 insertions(+), 7 deletions(-) >> > >> > diff --git a/drivers/iio/light/vcnl4000.c >> > b/drivers/iio/light/vcnl4000.c >> > index 0b226c684957..9838f0868372 100644 >> > --- a/drivers/iio/light/vcnl4000.c >> > +++ b/drivers/iio/light/vcnl4000.c >> > @@ -125,6 +125,9 @@ struct vcnl4000_data { >> > enum vcnl4000_device_ids id; >> > int rev; >> > int al_scale; >> > + bool als_enable; >> > + bool ps_enable; >> > + >> > const struct vcnl4000_chip_spec *chip_spec; >> > struct mutex vcnl4000_lock; >> > struct vcnl4200_channel vcnl4200_al; >> > @@ -202,10 +205,13 @@ static ssize_t >> vcnl4000_write_als_enable(struct >> > vcnl4000_data *data, int val) >> > if (ret < 0) >> > return ret; >> > >> > - if (val) >> > + if (val) { >> > ret &= ~VCNL4040_ALS_CONF_ALS_SD; >> > - else >> > + data->als_enable = true; >> > + } else { >> > ret |= VCNL4040_ALS_CONF_ALS_SD; >> > + data->als_enable = false; >> > + } >> > >> > return i2c_smbus_write_word_data(data->client, >> VCNL4200_AL_CONF, >> > ret); >> > @@ -225,10 +231,13 @@ static ssize_t >> vcnl4000_write_ps_enable(struct >> > vcnl4000_data *data, int val) >> > if (ret < 0) >> > return ret; >> > >> > - if (val) >> > + if (val) { >> > ret &= ~VCNL4040_PS_CONF1_PS_SD; >> > - else >> > + data->ps_enable = true; >> > + } else { >> > ret |= VCNL4040_PS_CONF1_PS_SD; >> > + data->ps_enable = false; >> > + } >> > >> > return i2c_smbus_write_word_data(data->client, >> > VCNL4200_PS_CONF1, ret); >> > @@ -283,6 +292,8 @@ static int vcnl4200_init(struct vcnl4000_data >> > *data) >> > dev_dbg(&data->client->dev, "device id 0x%x", id); >> > >> > data->rev = (ret >> 8) & 0xf; >> > + data->als_enable = false; >> > + data->ps_enable = false; >> > >> > data->vcnl4200_al.reg = VCNL4200_AL_DATA; >> > data->vcnl4200_ps.reg = VCNL4200_PS_DATA; >> > @@ -459,8 +470,12 @@ static bool >> vcnl4010_is_in_periodic_mode(struct >> > vcnl4000_data *data) >> > static int vcnl4000_set_pm_runtime_state(struct vcnl4000_data >> *data, >> > bool on) >> > { >> > struct device *dev = &data->client->dev; >> > + struct iio_dev *indio_dev = i2c_get_clientdata(data->client); >> > int ret; >> > >> > + if (!indio_dev->dev.power.runtime_auto) >> > + return 0; >> > + >> > if (on) { >> > ret = pm_runtime_resume_and_get(dev); >> > } else { >> > @@ -507,6 +522,38 @@ static int vcnl4000_read_raw(struct iio_dev >> > *indio_dev, >> > *val = 0; >> > *val2 = data->al_scale; >> > return IIO_VAL_INT_PLUS_MICRO; >> > + case IIO_CHAN_INFO_ENABLE: >> > + switch (chan->type) { >> > + case IIO_LIGHT: >> > + *val = data->als_enable; >> > + return IIO_VAL_INT; >> > + case IIO_PROXIMITY: >> > + *val = data->ps_enable; >> > + return IIO_VAL_INT; >> > + default: >> > + return -EINVAL; >> > + } >> > + default: >> > + return -EINVAL; >> > + } >> > +} >> > + >> > +static int vcnl4040_write_raw(struct iio_dev *indio_dev, >> > + struct iio_chan_spec const *chan, >> > + int val, int val2, long mask) >> > +{ >> > + struct vcnl4000_data *data = iio_priv(indio_dev); >> > + >> > + switch (mask) { >> > + case IIO_CHAN_INFO_ENABLE: >> > + switch (chan->type) { >> > + case IIO_LIGHT: >> > + return vcnl4000_write_als_enable(data, val); >> > + case IIO_PROXIMITY: >> > + return vcnl4000_write_ps_enable(data, val); >> > + default: >> > + return -EINVAL; >> > + } >> > default: >> > return -EINVAL; >> > } >> > @@ -845,6 +892,19 @@ static const struct iio_chan_spec >> > vcnl4010_channels[] = { >> > IIO_CHAN_SOFT_TIMESTAMP(1), >> > }; >> > >> > +static const struct iio_chan_spec vcnl4040_channels[] = { >> > + { >> > + .type = IIO_LIGHT, >> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | >> > + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_ENABLE), >> > + }, { >> > + .type = IIO_PROXIMITY, >> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | >> > + BIT(IIO_CHAN_INFO_ENABLE), >> > + .ext_info = vcnl4000_ext_info, >> > + } >> > +}; >> > + >> > static const struct iio_info vcnl4000_info = { >> > .read_raw = vcnl4000_read_raw, >> > }; >> > @@ -859,6 +919,11 @@ static const struct iio_info vcnl4010_info = >> { >> > .write_event_config = vcnl4010_write_event_config, >> > }; >> > >> > +static const struct iio_info vcnl4040_info = { >> > + .read_raw = vcnl4000_read_raw, >> > + .write_raw = vcnl4040_write_raw, >> > +}; >> > + >> > static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] >> = { >> > [VCNL4000] = { >> > .prod = "VCNL4000", >> > @@ -888,9 +953,9 @@ static const struct vcnl4000_chip_spec >> > vcnl4000_chip_spec_cfg[] = { >> > .measure_light = vcnl4200_measure_light, >> > .measure_proximity = vcnl4200_measure_proximity, >> > .set_power_state = vcnl4200_set_power_state, >> > - .channels = vcnl4000_channels, >> > - .num_channels = ARRAY_SIZE(vcnl4000_channels), >> > - .info = &vcnl4000_info, >> > + .channels = vcnl4040_channels, >> > + .num_channels = ARRAY_SIZE(vcnl4040_channels), >> > + .info = &vcnl4040_info, >> > .irq_support = false, >> > }, >> > [VCNL4200] = { >> > -- >> > 2.30.2 >> > >> >> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] iio: light: vcnl4000: Add enable attributes for vcnl4040 2022-09-22 14:10 ` Paul Cercueil @ 2022-09-22 18:39 ` Marten Lindahl 0 siblings, 0 replies; 12+ messages in thread From: Marten Lindahl @ 2022-09-22 18:39 UTC (permalink / raw) To: Paul Cercueil Cc: Mårten Lindahl, Jonathan Cameron, Lars-Peter Clausen, Uwe Kleine-König, linux-iio, kernel On Thu, Sep 22, 2022 at 04:10:54PM +0200, Paul Cercueil wrote: > > > Le jeu., sept. 22 2022 at 15:04:47 +0200, Marten Lindahl > <martenli@axis.com> a écrit : > > On Wed, Sep 21, 2022 at 12:01:24AM +0200, Paul Cercueil wrote: > >> Hi Mårten, > >> > >> Le mar., sept. 20 2022 at 20:09:57 +0200, Mårten Lindahl > >> <marten.lindahl@axis.com> a écrit : > >> > Add channel attribute in_illuminance_en and in_proximity_en with > >> > read/write access for vcnl4040. If automatic runtime power > >> management > >> > is > >> > turned off (power/control = on), both sensors can be kept on or > >> off by > >> > userspace. > >> > > > > Hi Paul! > > > >> I don't really understand this. If automatic runtime power > >> management > >> is turned OFF, I would expect both sensors to be kept ON always. > >> > >> It's not userspace's job to do power management of the chip. Why are > >> these channel attributes needed? > > > > I think I understand the problem here. I added the *_en attributes > > because I couldn't see any way to turn the sensors on without forcing > > it > > on during the *_raw read operation (with > > vcnl4000_set_pm_runtime_state(true)) > > after which it is turned off again (false). > > What's wrong with doing that? Hi! No, nothing is wrong with that. I was just confused by the fact that power/control=on didn't have any effect on the device. > > > Even if the power/control is set to 'on', there will be no callback > > for > > changing the state to active. > > > > It seems this does not work in this driver (with or without my > > patches) and I > > was confused by how it was supposed to work. But after some digging I > > suspect > > there could be a bug in the driver since the sysfs control/* nodes > > seems to > > operate on the indio_dev->dev and not on the driver dev, which is > > used for > > the vcnl4000 driver pm_runtime operations. > > I believe this is normal. The devm_iio_device_alloc() creates a new > device, whose parent is your i2c_client->dev. Ok > > > Setting the power/control to 'on' invokes the rpm_resume function > > which > > checks the dev.power.disable_depth attribute before it calls the > > resume_callback for setting the active state on the driver. But if the > > dev.power.disable_depth == 1 (which is the init value) the callback > > will not > > be called. And nothing happens. And I suspect the reason why the > > dev.power.disable_depth is 1 may be that it is not the vcnl4000 dev > > object that is being checked, but the indio_dev->dev object, which > > has not > > been configured for pm_runtime operations in the driver. > > > > Sorry for a long reply to your question, but I suspect that if the > > automatic pm_runtime for this driver can be disabled by the sysfs > > power/control, the *_en attributes wont be needed. > > > > I will look into why it does not work. > > I still don't understand. Why do you *need* to disable runtime PM? There is no special reason to disable runtime PM for the usecases we have. The original reason behind this patch is a local patch from the 4.19 kernel when runtime PM did not exist in the driver. The local patch added *_en attributes for turning the sensors on/off. I will gladly skip this patch, since it has come clear to me that it doesn't bring any value to the current version of the driver. I will probably look at a fix for the power/control=on problem, but any fix for that will be a separate patch. Kind regards Mårten > > -Paul > > > Kind regards > > Mårten > > > >> > >> Cheers, > >> -Paul > >> > >> > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> > >> > --- > >> > drivers/iio/light/vcnl4000.c | 79 > >> > ++++++++++++++++++++++++++++++++---- > >> > 1 file changed, 72 insertions(+), 7 deletions(-) > >> > > >> > diff --git a/drivers/iio/light/vcnl4000.c > >> > b/drivers/iio/light/vcnl4000.c > >> > index 0b226c684957..9838f0868372 100644 > >> > --- a/drivers/iio/light/vcnl4000.c > >> > +++ b/drivers/iio/light/vcnl4000.c > >> > @@ -125,6 +125,9 @@ struct vcnl4000_data { > >> > enum vcnl4000_device_ids id; > >> > int rev; > >> > int al_scale; > >> > + bool als_enable; > >> > + bool ps_enable; > >> > + > >> > const struct vcnl4000_chip_spec *chip_spec; > >> > struct mutex vcnl4000_lock; > >> > struct vcnl4200_channel vcnl4200_al; > >> > @@ -202,10 +205,13 @@ static ssize_t > >> vcnl4000_write_als_enable(struct > >> > vcnl4000_data *data, int val) > >> > if (ret < 0) > >> > return ret; > >> > > >> > - if (val) > >> > + if (val) { > >> > ret &= ~VCNL4040_ALS_CONF_ALS_SD; > >> > - else > >> > + data->als_enable = true; > >> > + } else { > >> > ret |= VCNL4040_ALS_CONF_ALS_SD; > >> > + data->als_enable = false; > >> > + } > >> > > >> > return i2c_smbus_write_word_data(data->client, > >> VCNL4200_AL_CONF, > >> > ret); > >> > @@ -225,10 +231,13 @@ static ssize_t > >> vcnl4000_write_ps_enable(struct > >> > vcnl4000_data *data, int val) > >> > if (ret < 0) > >> > return ret; > >> > > >> > - if (val) > >> > + if (val) { > >> > ret &= ~VCNL4040_PS_CONF1_PS_SD; > >> > - else > >> > + data->ps_enable = true; > >> > + } else { > >> > ret |= VCNL4040_PS_CONF1_PS_SD; > >> > + data->ps_enable = false; > >> > + } > >> > > >> > return i2c_smbus_write_word_data(data->client, > >> > VCNL4200_PS_CONF1, ret); > >> > @@ -283,6 +292,8 @@ static int vcnl4200_init(struct vcnl4000_data > >> > *data) > >> > dev_dbg(&data->client->dev, "device id 0x%x", id); > >> > > >> > data->rev = (ret >> 8) & 0xf; > >> > + data->als_enable = false; > >> > + data->ps_enable = false; > >> > > >> > data->vcnl4200_al.reg = VCNL4200_AL_DATA; > >> > data->vcnl4200_ps.reg = VCNL4200_PS_DATA; > >> > @@ -459,8 +470,12 @@ static bool > >> vcnl4010_is_in_periodic_mode(struct > >> > vcnl4000_data *data) > >> > static int vcnl4000_set_pm_runtime_state(struct vcnl4000_data > >> *data, > >> > bool on) > >> > { > >> > struct device *dev = &data->client->dev; > >> > + struct iio_dev *indio_dev = i2c_get_clientdata(data->client); > >> > int ret; > >> > > >> > + if (!indio_dev->dev.power.runtime_auto) > >> > + return 0; > >> > + > >> > if (on) { > >> > ret = pm_runtime_resume_and_get(dev); > >> > } else { > >> > @@ -507,6 +522,38 @@ static int vcnl4000_read_raw(struct iio_dev > >> > *indio_dev, > >> > *val = 0; > >> > *val2 = data->al_scale; > >> > return IIO_VAL_INT_PLUS_MICRO; > >> > + case IIO_CHAN_INFO_ENABLE: > >> > + switch (chan->type) { > >> > + case IIO_LIGHT: > >> > + *val = data->als_enable; > >> > + return IIO_VAL_INT; > >> > + case IIO_PROXIMITY: > >> > + *val = data->ps_enable; > >> > + return IIO_VAL_INT; > >> > + default: > >> > + return -EINVAL; > >> > + } > >> > + default: > >> > + return -EINVAL; > >> > + } > >> > +} > >> > + > >> > +static int vcnl4040_write_raw(struct iio_dev *indio_dev, > >> > + struct iio_chan_spec const *chan, > >> > + int val, int val2, long mask) > >> > +{ > >> > + struct vcnl4000_data *data = iio_priv(indio_dev); > >> > + > >> > + switch (mask) { > >> > + case IIO_CHAN_INFO_ENABLE: > >> > + switch (chan->type) { > >> > + case IIO_LIGHT: > >> > + return vcnl4000_write_als_enable(data, val); > >> > + case IIO_PROXIMITY: > >> > + return vcnl4000_write_ps_enable(data, val); > >> > + default: > >> > + return -EINVAL; > >> > + } > >> > default: > >> > return -EINVAL; > >> > } > >> > @@ -845,6 +892,19 @@ static const struct iio_chan_spec > >> > vcnl4010_channels[] = { > >> > IIO_CHAN_SOFT_TIMESTAMP(1), > >> > }; > >> > > >> > +static const struct iio_chan_spec vcnl4040_channels[] = { > >> > + { > >> > + .type = IIO_LIGHT, > >> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > >> > + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_ENABLE), > >> > + }, { > >> > + .type = IIO_PROXIMITY, > >> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > >> > + BIT(IIO_CHAN_INFO_ENABLE), > >> > + .ext_info = vcnl4000_ext_info, > >> > + } > >> > +}; > >> > + > >> > static const struct iio_info vcnl4000_info = { > >> > .read_raw = vcnl4000_read_raw, > >> > }; > >> > @@ -859,6 +919,11 @@ static const struct iio_info vcnl4010_info = > >> { > >> > .write_event_config = vcnl4010_write_event_config, > >> > }; > >> > > >> > +static const struct iio_info vcnl4040_info = { > >> > + .read_raw = vcnl4000_read_raw, > >> > + .write_raw = vcnl4040_write_raw, > >> > +}; > >> > + > >> > static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] > >> = { > >> > [VCNL4000] = { > >> > .prod = "VCNL4000", > >> > @@ -888,9 +953,9 @@ static const struct vcnl4000_chip_spec > >> > vcnl4000_chip_spec_cfg[] = { > >> > .measure_light = vcnl4200_measure_light, > >> > .measure_proximity = vcnl4200_measure_proximity, > >> > .set_power_state = vcnl4200_set_power_state, > >> > - .channels = vcnl4000_channels, > >> > - .num_channels = ARRAY_SIZE(vcnl4000_channels), > >> > - .info = &vcnl4000_info, > >> > + .channels = vcnl4040_channels, > >> > + .num_channels = ARRAY_SIZE(vcnl4040_channels), > >> > + .info = &vcnl4040_info, > >> > .irq_support = false, > >> > }, > >> > [VCNL4200] = { > >> > -- > >> > 2.30.2 > >> > > >> > >> > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] iio: light: vcnl4000: Add ps_it attributes for vcnl4040 2022-09-20 18:09 [PATCH 0/3] Add basic attributes for vcnl4040 Mårten Lindahl 2022-09-20 18:09 ` [PATCH 1/3] iio: light: vcnl4000: Preserve conf bits when toggle power Mårten Lindahl 2022-09-20 18:09 ` [PATCH 2/3] iio: light: vcnl4000: Add enable attributes for vcnl4040 Mårten Lindahl @ 2022-09-20 18:09 ` Mårten Lindahl 2022-09-20 22:12 ` Paul Cercueil 2 siblings, 1 reply; 12+ messages in thread From: Mårten Lindahl @ 2022-09-20 18:09 UTC (permalink / raw) To: Jonathan Cameron, Lars-Peter Clausen Cc: Paul Cercueil, Uwe Kleine-König, linux-iio, kernel, Mårten Lindahl Add read/write attribute for proximity integration time, and a read attribute for available integration times for the vcnl4040 chip. Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> --- drivers/iio/light/vcnl4000.c | 83 +++++++++++++++++++++++++++++++++++- 1 file changed, 82 insertions(+), 1 deletion(-) diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c index 9838f0868372..7a207e48335d 100644 --- a/drivers/iio/light/vcnl4000.c +++ b/drivers/iio/light/vcnl4000.c @@ -76,6 +76,8 @@ #define VCNL4040_ALS_CONF_ALS_SD BIT(0) /* Enable ambient light sensor */ #define VCNL4040_PS_CONF1_PS_SD BIT(0) /* Enable proximity sensor */ +#define VCNL4040_PS_CONF2_PS_IT \ + (BIT(3) | BIT(2) | BIT(1)) /* Proximity integration time */ /* Bit masks for interrupt registers. */ #define VCNL4010_INT_THR_SEL BIT(0) /* Select threshold interrupt source */ @@ -103,6 +105,16 @@ static const int vcnl4010_prox_sampling_frequency[][2] = { {125, 0}, {250, 0}, }; +static const int vcnl4040_ps_it_times[][2] = { + {0, 100}, + {0, 150}, + {0, 200}, + {0, 250}, + {0, 300}, + {0, 350}, + {0, 400}, + {0, 800}, +}; #define VCNL4000_SLEEP_DELAY_MS 2000 /* before we enter pm_runtime_suspend */ @@ -486,6 +498,49 @@ static int vcnl4000_set_pm_runtime_state(struct vcnl4000_data *data, bool on) return ret; } +static int vcnl4040_read_ps_it(struct vcnl4000_data *data, int *val, int *val2) +{ + int ret; + + ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1); + if (ret < 0) + return ret; + + ret = (ret & VCNL4040_PS_CONF2_PS_IT) >> 1; + + if (ret >= ARRAY_SIZE(vcnl4040_ps_it_times)) + return -EINVAL; + + *val = vcnl4040_ps_it_times[ret][0]; + *val2 = vcnl4040_ps_it_times[ret][1]; + + return 0; +} + +static ssize_t vcnl4040_write_ps_it(struct vcnl4000_data *data, int val) +{ + unsigned int i; + int ret, index = -1; + + for (i = 0; i < ARRAY_SIZE(vcnl4040_ps_it_times); i++) { + if (val == vcnl4040_ps_it_times[i][1]) { + index = i; + break; + } + } + + if (index < 0) + return -EINVAL; + + ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1); + if (ret < 0) + return ret; + + ret = (ret & ~VCNL4040_PS_CONF2_PS_IT) | (index << 1); + + return i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, ret); +} + static int vcnl4000_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, int *val, int *val2, long mask) @@ -533,6 +588,13 @@ static int vcnl4000_read_raw(struct iio_dev *indio_dev, default: return -EINVAL; } + case IIO_CHAN_INFO_INT_TIME: + if (chan->type != IIO_PROXIMITY) + return -EINVAL; + ret = vcnl4040_read_ps_it(data, val, val2); + if (ret < 0) + return ret; + return IIO_VAL_INT_PLUS_MICRO; default: return -EINVAL; } @@ -554,6 +616,12 @@ static int vcnl4040_write_raw(struct iio_dev *indio_dev, default: return -EINVAL; } + case IIO_CHAN_INFO_INT_TIME: + if (val != 0) + return -EINVAL; + if (chan->type != IIO_PROXIMITY) + return -EINVAL; + return vcnl4040_write_ps_it(data, val2); default: return -EINVAL; } @@ -900,11 +968,23 @@ static const struct iio_chan_spec vcnl4040_channels[] = { }, { .type = IIO_PROXIMITY, .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | - BIT(IIO_CHAN_INFO_ENABLE), + BIT(IIO_CHAN_INFO_ENABLE) | BIT(IIO_CHAN_INFO_INT_TIME), .ext_info = vcnl4000_ext_info, } }; +static IIO_CONST_ATTR(in_proximity_integration_time_available, + "0.000100 0.000150 0.000200 0.000250 0.000300 0.000350 0.000400 0.000800"); + +static struct attribute *vcnl4040_attributes[] = { + &iio_const_attr_in_proximity_integration_time_available.dev_attr.attr, + NULL, +}; + +static const struct attribute_group vcnl4040_attribute_group = { + .attrs = vcnl4040_attributes, +}; + static const struct iio_info vcnl4000_info = { .read_raw = vcnl4000_read_raw, }; @@ -922,6 +1002,7 @@ static const struct iio_info vcnl4010_info = { static const struct iio_info vcnl4040_info = { .read_raw = vcnl4000_read_raw, .write_raw = vcnl4040_write_raw, + .attrs = &vcnl4040_attribute_group, }; static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = { -- 2.30.2 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] iio: light: vcnl4000: Add ps_it attributes for vcnl4040 2022-09-20 18:09 ` [PATCH 3/3] iio: light: vcnl4000: Add ps_it " Mårten Lindahl @ 2022-09-20 22:12 ` Paul Cercueil 2022-09-22 13:31 ` Marten Lindahl 0 siblings, 1 reply; 12+ messages in thread From: Paul Cercueil @ 2022-09-20 22:12 UTC (permalink / raw) To: Mårten Lindahl Cc: Jonathan Cameron, Lars-Peter Clausen, Uwe Kleine-König, linux-iio, kernel Hi Mårten, Le mar., sept. 20 2022 at 20:09:58 +0200, Mårten Lindahl <marten.lindahl@axis.com> a écrit : > Add read/write attribute for proximity integration time, and a read > attribute for available integration times for the vcnl4040 chip. > > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> > --- > drivers/iio/light/vcnl4000.c | 83 > +++++++++++++++++++++++++++++++++++- > 1 file changed, 82 insertions(+), 1 deletion(-) > > diff --git a/drivers/iio/light/vcnl4000.c > b/drivers/iio/light/vcnl4000.c > index 9838f0868372..7a207e48335d 100644 > --- a/drivers/iio/light/vcnl4000.c > +++ b/drivers/iio/light/vcnl4000.c > @@ -76,6 +76,8 @@ > > #define VCNL4040_ALS_CONF_ALS_SD BIT(0) /* Enable ambient light > sensor */ > #define VCNL4040_PS_CONF1_PS_SD BIT(0) /* Enable proximity sensor */ > +#define VCNL4040_PS_CONF2_PS_IT \ > + (BIT(3) | BIT(2) | BIT(1)) /* Proximity integration time */ Use the GENMASK() macro. > > /* Bit masks for interrupt registers. */ > #define VCNL4010_INT_THR_SEL BIT(0) /* Select threshold interrupt > source */ > @@ -103,6 +105,16 @@ static const int > vcnl4010_prox_sampling_frequency[][2] = { > {125, 0}, > {250, 0}, > }; > +static const int vcnl4040_ps_it_times[][2] = { > + {0, 100}, > + {0, 150}, > + {0, 200}, > + {0, 250}, > + {0, 300}, > + {0, 350}, > + {0, 400}, > + {0, 800}, > +}; > > #define VCNL4000_SLEEP_DELAY_MS 2000 /* before we enter > pm_runtime_suspend */ > > @@ -486,6 +498,49 @@ static int vcnl4000_set_pm_runtime_state(struct > vcnl4000_data *data, bool on) > return ret; > } > > +static int vcnl4040_read_ps_it(struct vcnl4000_data *data, int *val, > int *val2) > +{ > + int ret; > + > + ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1); > + if (ret < 0) > + return ret; > + > + ret = (ret & VCNL4040_PS_CONF2_PS_IT) >> 1; Use the FIELD_GET() macro. > + > + if (ret >= ARRAY_SIZE(vcnl4040_ps_it_times)) > + return -EINVAL; > + > + *val = vcnl4040_ps_it_times[ret][0]; > + *val2 = vcnl4040_ps_it_times[ret][1]; > + > + return 0; > +} > + > +static ssize_t vcnl4040_write_ps_it(struct vcnl4000_data *data, int > val) > +{ > + unsigned int i; > + int ret, index = -1; > + > + for (i = 0; i < ARRAY_SIZE(vcnl4040_ps_it_times); i++) { > + if (val == vcnl4040_ps_it_times[i][1]) { > + index = i; > + break; > + } > + } > + > + if (index < 0) > + return -EINVAL; > + > + ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1); > + if (ret < 0) > + return ret; > + > + ret = (ret & ~VCNL4040_PS_CONF2_PS_IT) | (index << 1); Use the FIELD_PREP() macro. > + > + return i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, > ret); > +} > + > static int vcnl4000_read_raw(struct iio_dev *indio_dev, > struct iio_chan_spec const *chan, > int *val, int *val2, long mask) > @@ -533,6 +588,13 @@ static int vcnl4000_read_raw(struct iio_dev > *indio_dev, > default: > return -EINVAL; > } > + case IIO_CHAN_INFO_INT_TIME: > + if (chan->type != IIO_PROXIMITY) > + return -EINVAL; > + ret = vcnl4040_read_ps_it(data, val, val2); > + if (ret < 0) > + return ret; > + return IIO_VAL_INT_PLUS_MICRO; > default: > return -EINVAL; > } > @@ -554,6 +616,12 @@ static int vcnl4040_write_raw(struct iio_dev > *indio_dev, > default: > return -EINVAL; > } > + case IIO_CHAN_INFO_INT_TIME: > + if (val != 0) > + return -EINVAL; > + if (chan->type != IIO_PROXIMITY) > + return -EINVAL; > + return vcnl4040_write_ps_it(data, val2); > default: > return -EINVAL; > } > @@ -900,11 +968,23 @@ static const struct iio_chan_spec > vcnl4040_channels[] = { > }, { > .type = IIO_PROXIMITY, > .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > - BIT(IIO_CHAN_INFO_ENABLE), > + BIT(IIO_CHAN_INFO_ENABLE) | BIT(IIO_CHAN_INFO_INT_TIME), > .ext_info = vcnl4000_ext_info, > } > }; > > +static IIO_CONST_ATTR(in_proximity_integration_time_available, > + "0.000100 0.000150 0.000200 0.000250 0.000300 0.000350 0.000400 > 0.000800"); Hmm, this is not how you define a "_available" attribute. You need to add a .info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME) to your iio_chan_spec, then implement the .read_avail callback in your iio_info structure. Cheers, -Paul > + > +static struct attribute *vcnl4040_attributes[] = { > + &iio_const_attr_in_proximity_integration_time_available.dev_attr.attr, > + NULL, > +}; > + > +static const struct attribute_group vcnl4040_attribute_group = { > + .attrs = vcnl4040_attributes, > +}; > + > static const struct iio_info vcnl4000_info = { > .read_raw = vcnl4000_read_raw, > }; > @@ -922,6 +1002,7 @@ static const struct iio_info vcnl4010_info = { > static const struct iio_info vcnl4040_info = { > .read_raw = vcnl4000_read_raw, > .write_raw = vcnl4040_write_raw, > + .attrs = &vcnl4040_attribute_group, > }; > > static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = { > -- > 2.30.2 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] iio: light: vcnl4000: Add ps_it attributes for vcnl4040 2022-09-20 22:12 ` Paul Cercueil @ 2022-09-22 13:31 ` Marten Lindahl 0 siblings, 0 replies; 12+ messages in thread From: Marten Lindahl @ 2022-09-22 13:31 UTC (permalink / raw) To: Paul Cercueil Cc: Mårten Lindahl, Jonathan Cameron, Lars-Peter Clausen, Uwe Kleine-König, linux-iio, kernel On Wed, Sep 21, 2022 at 12:12:27AM +0200, Paul Cercueil wrote: > Hi Mårten, > > Le mar., sept. 20 2022 at 20:09:58 +0200, Mårten Lindahl > <marten.lindahl@axis.com> a écrit : > > Add read/write attribute for proximity integration time, and a read > > attribute for available integration times for the vcnl4040 chip. > > > > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> > > --- > > drivers/iio/light/vcnl4000.c | 83 > > +++++++++++++++++++++++++++++++++++- > > 1 file changed, 82 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/iio/light/vcnl4000.c > > b/drivers/iio/light/vcnl4000.c > > index 9838f0868372..7a207e48335d 100644 > > --- a/drivers/iio/light/vcnl4000.c > > +++ b/drivers/iio/light/vcnl4000.c > > @@ -76,6 +76,8 @@ > > > > #define VCNL4040_ALS_CONF_ALS_SD BIT(0) /* Enable ambient light > > sensor */ > > #define VCNL4040_PS_CONF1_PS_SD BIT(0) /* Enable proximity sensor */ > > +#define VCNL4040_PS_CONF2_PS_IT \ > > + (BIT(3) | BIT(2) | BIT(1)) /* Proximity integration time */ Hi Paul! > > Use the GENMASK() macro. > Will do! > > > > /* Bit masks for interrupt registers. */ > > #define VCNL4010_INT_THR_SEL BIT(0) /* Select threshold interrupt > > source */ > > @@ -103,6 +105,16 @@ static const int > > vcnl4010_prox_sampling_frequency[][2] = { > > {125, 0}, > > {250, 0}, > > }; > > +static const int vcnl4040_ps_it_times[][2] = { > > + {0, 100}, > > + {0, 150}, > > + {0, 200}, > > + {0, 250}, > > + {0, 300}, > > + {0, 350}, > > + {0, 400}, > > + {0, 800}, > > +}; > > > > #define VCNL4000_SLEEP_DELAY_MS 2000 /* before we enter > > pm_runtime_suspend */ > > > > @@ -486,6 +498,49 @@ static int vcnl4000_set_pm_runtime_state(struct > > vcnl4000_data *data, bool on) > > return ret; > > } > > > > +static int vcnl4040_read_ps_it(struct vcnl4000_data *data, int *val, > > int *val2) > > +{ > > + int ret; > > + > > + ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1); > > + if (ret < 0) > > + return ret; > > + > > + ret = (ret & VCNL4040_PS_CONF2_PS_IT) >> 1; > > Use the FIELD_GET() macro. > Will do! > > + > > + if (ret >= ARRAY_SIZE(vcnl4040_ps_it_times)) > > + return -EINVAL; > > + > > + *val = vcnl4040_ps_it_times[ret][0]; > > + *val2 = vcnl4040_ps_it_times[ret][1]; > > + > > + return 0; > > +} > > + > > +static ssize_t vcnl4040_write_ps_it(struct vcnl4000_data *data, int > > val) > > +{ > > + unsigned int i; > > + int ret, index = -1; > > + > > + for (i = 0; i < ARRAY_SIZE(vcnl4040_ps_it_times); i++) { > > + if (val == vcnl4040_ps_it_times[i][1]) { > > + index = i; > > + break; > > + } > > + } > > + > > + if (index < 0) > > + return -EINVAL; > > + > > + ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1); > > + if (ret < 0) > > + return ret; > > + > > + ret = (ret & ~VCNL4040_PS_CONF2_PS_IT) | (index << 1); > > Use the FIELD_PREP() macro. > Will do! > > + > > + return i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, > > ret); > > +} > > + > > static int vcnl4000_read_raw(struct iio_dev *indio_dev, > > struct iio_chan_spec const *chan, > > int *val, int *val2, long mask) > > @@ -533,6 +588,13 @@ static int vcnl4000_read_raw(struct iio_dev > > *indio_dev, > > default: > > return -EINVAL; > > } > > + case IIO_CHAN_INFO_INT_TIME: > > + if (chan->type != IIO_PROXIMITY) > > + return -EINVAL; > > + ret = vcnl4040_read_ps_it(data, val, val2); > > + if (ret < 0) > > + return ret; > > + return IIO_VAL_INT_PLUS_MICRO; > > default: > > return -EINVAL; > > } > > @@ -554,6 +616,12 @@ static int vcnl4040_write_raw(struct iio_dev > > *indio_dev, > > default: > > return -EINVAL; > > } > > + case IIO_CHAN_INFO_INT_TIME: > > + if (val != 0) > > + return -EINVAL; > > + if (chan->type != IIO_PROXIMITY) > > + return -EINVAL; > > + return vcnl4040_write_ps_it(data, val2); > > default: > > return -EINVAL; > > } > > @@ -900,11 +968,23 @@ static const struct iio_chan_spec > > vcnl4040_channels[] = { > > }, { > > .type = IIO_PROXIMITY, > > .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > > - BIT(IIO_CHAN_INFO_ENABLE), > > + BIT(IIO_CHAN_INFO_ENABLE) | BIT(IIO_CHAN_INFO_INT_TIME), > > .ext_info = vcnl4000_ext_info, > > } > > }; > > > > +static IIO_CONST_ATTR(in_proximity_integration_time_available, > > + "0.000100 0.000150 0.000200 0.000250 0.000300 0.000350 0.000400 > > 0.000800"); > > Hmm, this is not how you define a "_available" attribute. > > You need to add a > .info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME) > > to your iio_chan_spec, then implement the .read_avail callback in your > iio_info structure. Ok, I didn't notice that possibility. Thanks, I will do so! Kind regards Mårten > > Cheers, > -Paul > > > + > > +static struct attribute *vcnl4040_attributes[] = { > > + &iio_const_attr_in_proximity_integration_time_available.dev_attr.attr, > > + NULL, > > +}; > > + > > +static const struct attribute_group vcnl4040_attribute_group = { > > + .attrs = vcnl4040_attributes, > > +}; > > + > > static const struct iio_info vcnl4000_info = { > > .read_raw = vcnl4000_read_raw, > > }; > > @@ -922,6 +1002,7 @@ static const struct iio_info vcnl4010_info = { > > static const struct iio_info vcnl4040_info = { > > .read_raw = vcnl4000_read_raw, > > .write_raw = vcnl4040_write_raw, > > + .attrs = &vcnl4040_attribute_group, > > }; > > > > static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = { > > -- > > 2.30.2 > > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-09-22 18:40 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-20 18:09 [PATCH 0/3] Add basic attributes for vcnl4040 Mårten Lindahl 2022-09-20 18:09 ` [PATCH 1/3] iio: light: vcnl4000: Preserve conf bits when toggle power Mårten Lindahl 2022-09-20 22:23 ` Paul Cercueil 2022-09-22 12:18 ` Marten Lindahl 2022-09-20 18:09 ` [PATCH 2/3] iio: light: vcnl4000: Add enable attributes for vcnl4040 Mårten Lindahl 2022-09-20 22:01 ` Paul Cercueil 2022-09-22 13:04 ` Marten Lindahl 2022-09-22 14:10 ` Paul Cercueil 2022-09-22 18:39 ` Marten Lindahl 2022-09-20 18:09 ` [PATCH 3/3] iio: light: vcnl4000: Add ps_it " Mårten Lindahl 2022-09-20 22:12 ` Paul Cercueil 2022-09-22 13:31 ` Marten Lindahl
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).