On Tue, 11 Feb 2020 16:37:59 +0100
Tomas Novotny <tomas@xxxxxxxxxx> wrote:
> Hi Jonathan,
>
> On Sat, 8 Feb 2020 14:53:54 +0000
> Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> > On Wed, 5 Feb 2020 11:16:52 +0100
> > Tomas Novotny <tomas@xxxxxxxxxx> wrote:
> >
> > > Both vcnl4040 and vcnl4200 chips start with ambient light and proximity
> > > channels disabled. The driver enables both channels during
> > > initialization. The channels may be enabled or disabled with this
> > > change.
> > >
> > > Disabled ambient light channel returns the last measured value on read.
> > > This differs from proximity, which returns 0. Channels return these
> > > values with the configured sampling rate.
> > >
> > > The driver doesn't configure some defaults during probe now. Only the
> > > enable bit is handled.
> > >
> > > Signed-off-by: Tomas Novotny <tomas@xxxxxxxxxx>
> >
> > As a general rule we don't do this. The enable controls for input devices are
> > only there for things like step counters that will count from point of being
> > enabled until you read them.
>
> ok.
>
> > For light sensors etc it's a userspace control that no standard code knows
> > how to use. So if it make sense to enable / disable under normal conditions
> > it should either be done for every reading with increased delays, or you
> > should look at doing it with runtime power management. Typical choice
> > is to turn off if no one has read from the sensor for N seconds.
> >
> > runtime pm reduces the exposed complexity of the interfaces from userspace
> > and also leads to power savings when a program is taking readings, but not
> > that often.
>
> yes, the PM way is much neater and I will drop this patch in favour of it.
>
> By the way, what about disabled CONFIG_PM? Although it is not my case, is
> there a way how to disable the sensor?
Nope. If people are running without power management, then they can't
really expect anything other than everything being turned on all the time.
>
> Thanks,
>
> Tomas
>
> > Thanks,
> >
> > Jonathan
> >
> > > ---
> > > drivers/iio/light/vcnl4000.c | 118 +++++++++++++++++++++++++++++++++++++------
> > > 1 file changed, 102 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> > > index 9f673e89084a..f351b100a165 100644
> > > --- a/drivers/iio/light/vcnl4000.c
> > > +++ b/drivers/iio/light/vcnl4000.c
> > > @@ -58,6 +58,10 @@
> > > #define VCNL4000_AL_OD BIT(4) /* start on-demand ALS measurement */
> > > #define VCNL4000_PS_OD BIT(3) /* start on-demand proximity measurement */
> > >
> > > +/* Bit masks for various configuration registers */
> > > +#define VCNL4200_AL_SD BIT(0) /* Ambient light shutdown */
> > > +#define VCNL4200_PS_SD BIT(0) /* Proximity shutdown */
> > > +
> > > enum vcnl4000_device_ids {
> > > VCNL4000,
> > > VCNL4010,
> > > @@ -86,10 +90,16 @@ struct vcnl4000_data {
> > >
> > > struct vcnl4000_chip_spec {
> > > const char *prod;
> > > + const struct iio_chan_spec *channels;
> > > + size_t num_channels;
> > > const struct regmap_config *regmap_config;
> > > int (*init)(struct vcnl4000_data *data);
> > > int (*measure_light)(struct vcnl4000_data *data, int *val);
> > > int (*measure_proximity)(struct vcnl4000_data *data, int *val);
> > > + int (*is_enabled)(struct vcnl4000_data *data, enum iio_chan_type type,
> > > + int *val);
> > > + int (*enable)(struct vcnl4000_data *data, enum iio_chan_type type,
> > > + bool val);
> > > };
> > >
> > > static const struct i2c_device_id vcnl4000_id[] = {
> > > @@ -102,6 +112,30 @@ static const struct i2c_device_id vcnl4000_id[] = {
> > > };
> > > MODULE_DEVICE_TABLE(i2c, vcnl4000_id);
> > >
> > > +static const struct iio_chan_spec vcnl4000_channels[] = {
> > > + {
> > > + .type = IIO_LIGHT,
> > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > > + BIT(IIO_CHAN_INFO_SCALE),
> > > + }, {
> > > + .type = IIO_PROXIMITY,
> > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > > + }
> > > +};
> > > +
> > > +static const struct iio_chan_spec vcnl4200_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),
> > > + }
> > > +};
> > > +
> > > static const struct regmap_config vcnl4000_regmap_config = {
> > > .reg_bits = 8,
> > > .val_bits = 8,
> > > @@ -171,11 +205,10 @@ static int vcnl4200_init(struct vcnl4000_data *data)
> > >
> > > data->rev = (ret >> 8) & 0xf;
> > >
> > > - /* Set defaults and enable both channels */
> > > - ret = regmap_write(data->regmap, VCNL4200_AL_CONF, 0);
> > > + ret = data->chip_spec->enable(data, IIO_LIGHT, true);
> > > if (ret < 0)
> > > return ret;
> > > - ret = regmap_write(data->regmap, VCNL4200_PS_CONF1, 0);
> > > + ret = data->chip_spec->enable(data, IIO_PROXIMITY, true);
> > > if (ret < 0)
> > > return ret;
> > >
> > > @@ -300,9 +333,46 @@ static int vcnl4200_measure_proximity(struct vcnl4000_data *data, int *val)
> > > return vcnl4200_measure(data, &data->vcnl4200_ps, val);
> > > }
> > >
> > > +static int vcnl4200_is_enabled(struct vcnl4000_data *data,
> > > + enum iio_chan_type type, int *val)
> > > +{
> > > + int ret;
> > > +
> > > + switch (type) {
> > > + case IIO_LIGHT:
> > > + ret = regmap_read(data->regmap, VCNL4200_AL_CONF, val);
> > > + *val = !(*val & VCNL4200_AL_SD);
> > > + break;
> > > + case IIO_PROXIMITY:
> > > + ret = regmap_read(data->regmap, VCNL4200_PS_CONF1, val);
> > > + *val = !(*val & VCNL4200_PS_SD);
> > > + break;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > + return ret < 0 ? ret : IIO_VAL_INT;
> > > +}
> > > +
> > > +static int vcnl4200_enable(struct vcnl4000_data *data, enum iio_chan_type type,
> > > + bool val)
> > > +{
> > > + switch (type) {
> > > + case IIO_LIGHT:
> > > + return regmap_update_bits(data->regmap, VCNL4200_AL_CONF,
> > > + VCNL4200_AL_SD, !val);
> > > + case IIO_PROXIMITY:
> > > + return regmap_update_bits(data->regmap, VCNL4200_PS_CONF1,
> > > + VCNL4200_PS_SD, !val);
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +}
> > > +
> > > static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> > > [VCNL4000] = {
> > > .prod = "VCNL4000",
> > > + .channels = vcnl4000_channels,
> > > + .num_channels = ARRAY_SIZE(vcnl4000_channels),
> > > .regmap_config = &vcnl4000_regmap_config,
> > > .init = vcnl4000_init,
> > > .measure_light = vcnl4000_measure_light,
> > > @@ -310,6 +380,8 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> > > },
> > > [VCNL4010] = {
> > > .prod = "VCNL4010/4020",
> > > + .channels = vcnl4000_channels,
> > > + .num_channels = ARRAY_SIZE(vcnl4000_channels),
> > > .regmap_config = &vcnl4000_regmap_config,
> > > .init = vcnl4000_init,
> > > .measure_light = vcnl4000_measure_light,
> > > @@ -317,31 +389,28 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> > > },
> > > [VCNL4040] = {
> > > .prod = "VCNL4040",
> > > + .channels = vcnl4200_channels,
> > > + .num_channels = ARRAY_SIZE(vcnl4200_channels),
> > > .regmap_config = &vcnl4200_regmap_config,
> > > .init = vcnl4200_init,
> > > .measure_light = vcnl4200_measure_light,
> > > .measure_proximity = vcnl4200_measure_proximity,
> > > + .is_enabled = vcnl4200_is_enabled,
> > > + .enable = vcnl4200_enable,
> > > },
> > > [VCNL4200] = {
> > > .prod = "VCNL4200",
> > > + .channels = vcnl4200_channels,
> > > + .num_channels = ARRAY_SIZE(vcnl4200_channels),
> > > .regmap_config = &vcnl4200_regmap_config,
> > > .init = vcnl4200_init,
> > > .measure_light = vcnl4200_measure_light,
> > > .measure_proximity = vcnl4200_measure_proximity,
> > > + .is_enabled = vcnl4200_is_enabled,
> > > + .enable = vcnl4200_enable,
> > > },
> > > };
> > >
> > > -static const struct iio_chan_spec vcnl4000_channels[] = {
> > > - {
> > > - .type = IIO_LIGHT,
> > > - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > > - BIT(IIO_CHAN_INFO_SCALE),
> > > - }, {
> > > - .type = IIO_PROXIMITY,
> > > - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > > - }
> > > -};
> > > -
> > > static int vcnl4000_read_raw(struct iio_dev *indio_dev,
> > > struct iio_chan_spec const *chan,
> > > int *val, int *val2, long mask)
> > > @@ -372,6 +441,22 @@ 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:
> > > + return data->chip_spec->is_enabled(data, chan->type, val);
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +}
> > > +
> > > +static int vcnl4000_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:
> > > + return data->chip_spec->enable(data, chan->type, val);
> > > default:
> > > return -EINVAL;
> > > }
> > > @@ -379,6 +464,7 @@ static int vcnl4000_read_raw(struct iio_dev *indio_dev,
> > >
> > > static const struct iio_info vcnl4000_info = {
> > > .read_raw = vcnl4000_read_raw,
> > > + .write_raw = vcnl4000_write_raw,
> > > };
> > >
> > > static int vcnl4000_probe(struct i2c_client *client,
> > > @@ -412,8 +498,8 @@ static int vcnl4000_probe(struct i2c_client *client,
> > >
> > > indio_dev->dev.parent = &client->dev;
> > > indio_dev->info = &vcnl4000_info;
> > > - indio_dev->channels = vcnl4000_channels;
> > > - indio_dev->num_channels = ARRAY_SIZE(vcnl4000_channels);
> > > + indio_dev->channels = data->chip_spec->channels;
> > > + indio_dev->num_channels = data->chip_spec->num_channels;
> > > indio_dev->name = VCNL4000_DRV_NAME;
> > > indio_dev->modes = INDIO_DIRECT_MODE;
> > >
> >
