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@xxxxxxxx> 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@xxxxxxxx>
> > ---
> > 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
> >
>
>
