Re: [PATCH 05/23] media: atomisp: gc0310: Use V4L2_CID_ANALOGUE_GAIN for gain control

[Hans de Goede <hansg@kernel.org>]

Hi Kieran,

Thank you for the reviews.

On 17-May-25 23:09, Kieran Bingham wrote:
> Quoting Hans de Goede (2025-05-17 12:40:48)
>> Use V4L2_CID_ANALOGUE_GAIN for gain control, as expected by userspace.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/staging/media/atomisp/i2c/atomisp-gc0310.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
>> index ee039f3be4da..756e56f639b7 100644
>> --- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
>> +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
>> @@ -289,7 +289,7 @@ static int gc0310_s_ctrl(struct v4l2_ctrl *ctrl)
>>                 ret = cci_write(sensor->regmap, GC0310_AEC_PK_EXPO_REG,
>>                                 ctrl->val, NULL);
>>                 break;
>> -       case V4L2_CID_GAIN:
>> +       case V4L2_CID_ANALOGUE_GAIN:
>>                 ret = gc0310_gain_set(sensor, ctrl->val);
>>                 break;
>>         default:
>> @@ -533,7 +533,7 @@ static int gc0310_init_controls(struct gc0310_device *sensor)
>>  
>>         /* 32 steps at base gain 1 + 64 half steps at base gain 2 */
> 
> sounds like a curious gain model...
> 
> Will be interesting when we get the sensor calibration tools up and
> running to plot this. (Or is there already a public datasheet
> documenting this?)

There is a datasheet but it does not document much other then
register names.
 
> Is there a split here between analogue gain and digital gain ? Or is it
> all expected to be 'analogue gain' ?

here is the actual method setting the gain:

        /* Taken from original driver, this never sets dgain lower then 32? */

        /* Change 0 - 95 to 32 - 127 */
        gain += 32;

        if (gain < 64) {
                again = 0x0; /* sqrt(2) */
                dgain = gain;
        } else {
                again = 0x2; /* 2 * sqrt(2) */
                dgain = gain / 2;
        }

        cci_write(sensor->regmap, GC0310_AGC_ADJ_REG, again, &ret);
        cci_write(sensor->regmap, GC0310_DGC_ADJ_REG, dgain, &ret);

The 32 half steps come from the dgain = gain / 2 for steps 32 - 95 .

Note the again / dgain names here are confusing. The data sheet describes
the 2 registers which are being written as follows:

AGC:
P0:0x48 ANALOG_COL_gain-col_code 4 bit wide default 0x00 RO [7:4] ANALOG_COL_gain [2:0] col_code

DGC:
P0:0x71 Auto_pregain 8 bit wide default 0x20 RO Auto_pregain

Note DGC is described as read-only in the datasheet, maybe the datesheet is off by one
and we should actually look at register 0x70 in the datasheet:

P0:0x70 Global_gain 8 bit wide default 0x40 RW Global_gain

Either way based on the function calculations I get the feeling that
the first register is actually setting some fixed analog pre-multiplier
and the second register is the actual analog gain.

Despite the names used in the driver (inherited from Android kernels) it
does not feel to me like one of the 2 registers is a digital gain
register.

So I'm going to keep this as is for now and once we've gain calibration
tooling up and running make a plot with the current gain code and then
modify the driver to make this fit one of the standard gain models.

Regards,

Hans