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?
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;
> >
>
