Hello Jorijn, few questions inline.
On 08/26, Jorijn van der Graaf wrote:
> The Sensortek STK36C61 is a 3-in-1 ambient light / proximity / RGB
> colour sensor (chip ID 0x95) found in the Fairphone 6. Its register
> interface is compatible with the feature set this driver uses: the
> STATE/FLAG bit layout, the data and threshold registers and the gain
> and integration-time fields, verified on that device (the ALS and
> proximity readings scale with their gain and integration-time fields,
> thresholds written through the event interface read back from the
> chip, and the FLAG near/far bit crosses with them). Add its chip ID to
> the known-ID list and the device table entries.
>
...
>
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Jorijn van der Graaf <[email protected]>
> ---
...
> @@ -417,10 +459,10 @@ static int stk3310_read_raw(struct iio_dev *indio_dev,
> mutex_unlock(&data->lock);
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_INT_TIME:
> - if (chan->type == IIO_LIGHT)
> - ret = regmap_field_read(data->reg_als_it, &index);
> - else
> + if (chan->type == IIO_PROXIMITY)
> ret = regmap_field_read(data->reg_ps_it, &index);
> + else
> + ret = regmap_field_read(data->reg_als_it, &index);
The above seems unnecessary. Why changing the comparison from IIO_LIGHT to
IIO_PROXIMITY?
After the proposed update we would have the integration time for both light and
intensity channels being read from the same register field?
> if (ret < 0)
> return ret;
>
> @@ -428,10 +470,12 @@ static int stk3310_read_raw(struct iio_dev *indio_dev,
> *val2 = stk3310_it_table[index][1];
> return IIO_VAL_INT_PLUS_MICRO;
> case IIO_CHAN_INFO_SCALE:
> - if (chan->type == IIO_LIGHT)
> - ret = regmap_field_read(data->reg_als_gain, &index);
> - else
> + if (chan->type == IIO_PROXIMITY)
> ret = regmap_field_read(data->reg_ps_gain, &index);
> + else if (chan->channel2 == IIO_MOD_LIGHT_CLEAR)
> + ret = regmap_field_read(data->reg_clear_gain, &index);
> + else
> + ret = regmap_field_read(data->reg_als_gain, &index);
Similar question here. What do we accomplish by comparing to proximity instead
of light? Is the gain info the same for light and intensity red/green/blue
channels?
> if (ret < 0)
> return ret;
>
> @@ -451,7 +495,8 @@ static int stk3310_write_raw(struct iio_dev *indio_dev,
> int index;
> struct stk3310_data *data = iio_priv(indio_dev);
>
> - if (chan->type != IIO_LIGHT && chan->type != IIO_PROXIMITY)
> + if (chan->type != IIO_LIGHT && chan->type != IIO_PROXIMITY &&
> + chan->type != IIO_INTENSITY)
> return -EINVAL;
>
> switch (mask) {
> @@ -462,10 +507,10 @@ static int stk3310_write_raw(struct iio_dev *indio_dev,
> if (index < 0)
> return -EINVAL;
> mutex_lock(&data->lock);
> - if (chan->type == IIO_LIGHT)
> - ret = regmap_field_write(data->reg_als_it, index);
> - else
> + if (chan->type == IIO_PROXIMITY)
> ret = regmap_field_write(data->reg_ps_it, index);
> + else
> + ret = regmap_field_write(data->reg_als_it, index);
Same pattern, same questions.
> if (ret < 0)
> dev_err(&data->client->dev,
> "sensor configuration failed\n");
> @@ -479,10 +524,12 @@ static int stk3310_write_raw(struct iio_dev *indio_dev,
> if (index < 0)
> return -EINVAL;
> mutex_lock(&data->lock);
> - if (chan->type == IIO_LIGHT)
> - ret = regmap_field_write(data->reg_als_gain, index);
> - else
> + if (chan->type == IIO_PROXIMITY)
> ret = regmap_field_write(data->reg_ps_gain, index);
> + else if (chan->channel2 == IIO_MOD_LIGHT_CLEAR)
> + ret = regmap_field_write(data->reg_clear_gain, index);
> + else
> + ret = regmap_field_write(data->reg_als_gain, index);
And here.
> if (ret < 0)
> dev_err(&data->client->dev,
> "sensor configuration failed\n");
With best regards,
Marcelo
