| Message ID | 20220920180958.2308229-3-marten.lindahl@axis.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add basic attributes for vcnl4040 | expand |
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 >
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 > > > >
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 >> > >> >>
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 > >> > > >> > >> > >
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] = {
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(-)