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