Re: [PATCH v2 5/5] iio: light: stk3310: support the Sensortek STK36C61

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