Re: [PATCH 2/3] iio: light: vcnl4000: Add enable attributes for vcnl4040

[Paul Cercueil <paul@xxxxxxxxxxxxxxx>]

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.

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