| Message ID | 20221022092015.208592-3-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: ar0521: Add analog gain, rework clock tree | expand |
Hi Jacopo On Sat, 22 Oct 2022 at 11:47, Jacopo Mondi <jacopo@jmondi.org> wrote: > > Add support for V4L2_CID_ANALOG_GAIN. The control programs the global > gain register which applies to all color channels. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > drivers/media/i2c/ar0521.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c > index 0daa61df2603..ba169f0218a9 100644 > --- a/drivers/media/i2c/ar0521.c > +++ b/drivers/media/i2c/ar0521.c > @@ -35,6 +35,11 @@ > #define AR0521_HEIGHT_BLANKING_MIN 38u /* must be even */ > #define AR0521_TOTAL_WIDTH_MIN 2968u > > +#define AR0521_ANA_GAIN_MIN 0x00 > +#define AR0521_ANA_GAIN_MAX 0x3f The register reference I have says it is u3.4 format, which would make the max value 0x7f rather than 0x3f. Functionally it makes no real difference, but a max gain of nearly x7 15/16ths is preferable to nearly x3 15/16ths. If it is u3.4 I'd have expected the minimum to be 0x10 to avoid a gain of less than x1 (does it even work?) Looking at the listed m0, c0, m1, c1 values for gain (1, 0, 0, and 4 respectively), that maps to a formula: gain = code / 4 Min and max codes are 0 and 0x1f, so that implies it will do a gain of less than x1, and goes up to x7.75. So much contradictory information!! I'm happy to add a R-B tag for the code side, but the limits seem to be a little all over the place. I'd be looking at taking some test images with fixed exposure time at each gain code to work out what value is actually x1, x2, x4, etc. Doing so does require knowing the black level and applying an appropriate correction to the raw values, or extrapolating from the results. Dave > +#define AR0521_ANA_GAIN_STEP 0x01 > +#define AR0521_ANA_GAIN_DEFAULT 0x00 > + > /* AR0521 registers */ > #define AR0521_REG_VT_PIX_CLK_DIV 0x0300 > #define AR0521_REG_FRAME_LENGTH_LINES 0x0340 > @@ -50,6 +55,8 @@ > #define AR0521_REG_RESET_RESTART BIT(1) > #define AR0521_REG_RESET_INIT BIT(0) > > +#define AR0521_REG_ANA_GAIN_CODE_GLOBAL 0x3028 > + > #define AR0521_REG_GREEN1_GAIN 0x3056 > #define AR0521_REG_BLUE_GAIN 0x3058 > #define AR0521_REG_RED_GAIN 0x305A > @@ -456,6 +463,10 @@ static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl) > case V4L2_CID_VBLANK: > ret = ar0521_set_geometry(sensor); > break; > + case V4L2_CID_ANALOGUE_GAIN: > + ret = ar0521_write_reg(sensor, AR0521_REG_ANA_GAIN_CODE_GLOBAL, > + ctrl->val); > + break; > case V4L2_CID_GAIN: > case V4L2_CID_RED_BALANCE: > case V4L2_CID_BLUE_BALANCE: > @@ -499,6 +510,11 @@ static int ar0521_init_controls(struct ar0521_dev *sensor) > /* We can use our own mutex for the ctrl lock */ > hdl->lock = &sensor->lock; > > + /* Analog gain */ > + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN, > + AR0521_ANA_GAIN_MIN, AR0521_ANA_GAIN_MAX, > + AR0521_ANA_GAIN_STEP, AR0521_ANA_GAIN_DEFAULT); > + > /* Manual gain */ > ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, 0, 511, 1, 0); > ctrls->red_balance = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_RED_BALANCE, > -- > 2.37.3 >
Hi Dave, On Mon, Oct 24, 2022 at 01:13:58PM +0100, Dave Stevenson wrote: > Hi Jacopo > > On Sat, 22 Oct 2022 at 11:47, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Add support for V4L2_CID_ANALOG_GAIN. The control programs the global > > gain register which applies to all color channels. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > drivers/media/i2c/ar0521.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c > > index 0daa61df2603..ba169f0218a9 100644 > > --- a/drivers/media/i2c/ar0521.c > > +++ b/drivers/media/i2c/ar0521.c > > @@ -35,6 +35,11 @@ > > #define AR0521_HEIGHT_BLANKING_MIN 38u /* must be even */ > > #define AR0521_TOTAL_WIDTH_MIN 2968u > > > > +#define AR0521_ANA_GAIN_MIN 0x00 > > +#define AR0521_ANA_GAIN_MAX 0x3f > > The register reference I have says it is u3.4 format, which would make > the max value 0x7f rather than 0x3f. > > Functionally it makes no real difference, but a max gain of nearly x7 > 15/16ths is preferable to nearly x3 15/16ths. > > If it is u3.4 I'd have expected the minimum to be 0x10 to avoid a gain > of less than x1 (does it even work?) > The value of the 0x3280 register is written in the 7 low bits of the 0x305e register. Whatever is written to 0x305e is similalrly reflected in the 0x3280 one. The 0x305e[6:4] bits are the coarse analog gain value and the lower 4 bits are the fine analog gain value. I wonder if u3.4 is used there to describe the that, even if u3.4 has a different meaning > Looking at the listed m0, c0, m1, c1 values for gain (1, 0, 0, and 4 > respectively), that maps to a formula: > gain = code / 4 > Min and max codes are 0 and 0x1f, so that implies it will do a gain of > less than x1, and goes up to x7.75. The sensor does not use the CCS gain model, but an exponential piecewise function documented as a table of register values/gains in the application manual. The sensor exposes a list of CCS-compatible registers, including "0x0204 analogue_gain_code_global", from which I presume one should contorl the gain if the CCS compatible model works. From my tests, writing to that register is a no-op. I presume the CCS compatible interface is not plumbed, or maybe it depdends on the FW version, or else :) > > So much contradictory information!! > Yes :) > I'm happy to add a R-B tag for the code side, but the limits seem to > be a little all over the place. I'd be looking at taking some test > images with fixed exposure time at each gain code to work out what > value is actually x1, x2, x4, etc. Doing so does require knowing the > black level and applying an appropriate correction to the raw values, > or extrapolating from the results. > > Dave > > > +#define AR0521_ANA_GAIN_STEP 0x01 > > +#define AR0521_ANA_GAIN_DEFAULT 0x00 > > + > > /* AR0521 registers */ > > #define AR0521_REG_VT_PIX_CLK_DIV 0x0300 > > #define AR0521_REG_FRAME_LENGTH_LINES 0x0340 > > @@ -50,6 +55,8 @@ > > #define AR0521_REG_RESET_RESTART BIT(1) > > #define AR0521_REG_RESET_INIT BIT(0) > > > > +#define AR0521_REG_ANA_GAIN_CODE_GLOBAL 0x3028 > > + > > #define AR0521_REG_GREEN1_GAIN 0x3056 > > #define AR0521_REG_BLUE_GAIN 0x3058 > > #define AR0521_REG_RED_GAIN 0x305A > > @@ -456,6 +463,10 @@ static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl) > > case V4L2_CID_VBLANK: > > ret = ar0521_set_geometry(sensor); > > break; > > + case V4L2_CID_ANALOGUE_GAIN: > > + ret = ar0521_write_reg(sensor, AR0521_REG_ANA_GAIN_CODE_GLOBAL, > > + ctrl->val); > > + break; > > case V4L2_CID_GAIN: > > case V4L2_CID_RED_BALANCE: > > case V4L2_CID_BLUE_BALANCE: > > @@ -499,6 +510,11 @@ static int ar0521_init_controls(struct ar0521_dev *sensor) > > /* We can use our own mutex for the ctrl lock */ > > hdl->lock = &sensor->lock; > > > > + /* Analog gain */ > > + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN, > > + AR0521_ANA_GAIN_MIN, AR0521_ANA_GAIN_MAX, > > + AR0521_ANA_GAIN_STEP, AR0521_ANA_GAIN_DEFAULT); > > + > > /* Manual gain */ > > ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, 0, 511, 1, 0); > > ctrls->red_balance = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_RED_BALANCE, > > -- > > 2.37.3 > >
Hi Jacopo, On Mon, Oct 24, 2022 at 02:31:05PM +0200, Jacopo Mondi wrote: > Hi Dave, > > On Mon, Oct 24, 2022 at 01:13:58PM +0100, Dave Stevenson wrote: > > Hi Jacopo > > > > On Sat, 22 Oct 2022 at 11:47, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > Add support for V4L2_CID_ANALOG_GAIN. The control programs the global > > > gain register which applies to all color channels. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > drivers/media/i2c/ar0521.c | 16 ++++++++++++++++ > > > 1 file changed, 16 insertions(+) > > > > > > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c > > > index 0daa61df2603..ba169f0218a9 100644 > > > --- a/drivers/media/i2c/ar0521.c > > > +++ b/drivers/media/i2c/ar0521.c > > > @@ -35,6 +35,11 @@ > > > #define AR0521_HEIGHT_BLANKING_MIN 38u /* must be even */ > > > #define AR0521_TOTAL_WIDTH_MIN 2968u > > > > > > +#define AR0521_ANA_GAIN_MIN 0x00 > > > +#define AR0521_ANA_GAIN_MAX 0x3f > > > > The register reference I have says it is u3.4 format, which would make > > the max value 0x7f rather than 0x3f. > > > > Functionally it makes no real difference, but a max gain of nearly x7 > > 15/16ths is preferable to nearly x3 15/16ths. > > > > If it is u3.4 I'd have expected the minimum to be 0x10 to avoid a gain > > of less than x1 (does it even work?) > > > > The value of the 0x3280 register is written in the 7 low bits of the > 0x305e register. Whatever is written to 0x305e is similalrly reflected > in the 0x3280 one. The 0x305e[6:4] bits are the coarse analog gain > value and the lower 4 bits are the fine analog gain value. I wonder if > u3.4 is used there to describe the that, even if u3.4 has a different > meaning > > > Looking at the listed m0, c0, m1, c1 values for gain (1, 0, 0, and 4 > > respectively), that maps to a formula: > > gain = code / 4 > > Min and max codes are 0 and 0x1f, so that implies it will do a gain of > > less than x1, and goes up to x7.75. > > The sensor does not use the CCS gain model, but an exponential > piecewise function documented as a table of register values/gains in > the application manual. > > The sensor exposes a list of CCS-compatible registers, including > "0x0204 analogue_gain_code_global", from which I presume one should > contorl the gain if the CCS compatible model works. From my tests, > writing to that register is a no-op. > > I presume the CCS compatible interface is not plumbed, or maybe it > depdends on the FW version, or else :) It is also possible this is a sensor developed based on an earlier one that was compatible with e.g. SMIA. If the newer CCS-only registers aren't defined, it is unlikely it would be compatible.
diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c index 0daa61df2603..ba169f0218a9 100644 --- a/drivers/media/i2c/ar0521.c +++ b/drivers/media/i2c/ar0521.c @@ -35,6 +35,11 @@ #define AR0521_HEIGHT_BLANKING_MIN 38u /* must be even */ #define AR0521_TOTAL_WIDTH_MIN 2968u +#define AR0521_ANA_GAIN_MIN 0x00 +#define AR0521_ANA_GAIN_MAX 0x3f +#define AR0521_ANA_GAIN_STEP 0x01 +#define AR0521_ANA_GAIN_DEFAULT 0x00 + /* AR0521 registers */ #define AR0521_REG_VT_PIX_CLK_DIV 0x0300 #define AR0521_REG_FRAME_LENGTH_LINES 0x0340 @@ -50,6 +55,8 @@ #define AR0521_REG_RESET_RESTART BIT(1) #define AR0521_REG_RESET_INIT BIT(0) +#define AR0521_REG_ANA_GAIN_CODE_GLOBAL 0x3028 + #define AR0521_REG_GREEN1_GAIN 0x3056 #define AR0521_REG_BLUE_GAIN 0x3058 #define AR0521_REG_RED_GAIN 0x305A @@ -456,6 +463,10 @@ static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl) case V4L2_CID_VBLANK: ret = ar0521_set_geometry(sensor); break; + case V4L2_CID_ANALOGUE_GAIN: + ret = ar0521_write_reg(sensor, AR0521_REG_ANA_GAIN_CODE_GLOBAL, + ctrl->val); + break; case V4L2_CID_GAIN: case V4L2_CID_RED_BALANCE: case V4L2_CID_BLUE_BALANCE: @@ -499,6 +510,11 @@ static int ar0521_init_controls(struct ar0521_dev *sensor) /* We can use our own mutex for the ctrl lock */ hdl->lock = &sensor->lock; + /* Analog gain */ + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN, + AR0521_ANA_GAIN_MIN, AR0521_ANA_GAIN_MAX, + AR0521_ANA_GAIN_STEP, AR0521_ANA_GAIN_DEFAULT); + /* Manual gain */ ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, 0, 511, 1, 0); ctrls->red_balance = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_RED_BALANCE,
Add support for V4L2_CID_ANALOG_GAIN. The control programs the global gain register which applies to all color channels. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- drivers/media/i2c/ar0521.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)