[libcamera-devel] [PATCH] imx258: Read analog gain register values

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

As this is meant for testing but not integration, adding a [DNI] or
similar prefix in the subject line would be nice.

On Mon, Jul 19, 2021 at 03:44:37PM +0530, Umang Jain wrote:
> Also copy and tweak imx258_read_reg() to allow signed int values.
> (These constants are signed ints).
> 
> ---
>  drivers/media/i2c/imx258.c | 78 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 78 insertions(+)
> 
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index f86ae18bc104..d029706f4d51 100644
> --- a/drivers/media/i2c/imx258.c
> +++ b/drivers/media/i2c/imx258.c
> @@ -21,6 +21,13 @@
>  #define IMX258_REG_CHIP_ID		0x0016
>  #define IMX258_CHIP_ID			0x0258
>  
> +/* Gain constant registers */
> +#define IMX258_REG_GAIN_CAP		0x0080
> +#define IMX258_REG_C0			0x008E

Lowercase for hex constants please.

> +#define IMX258_REG_M0			0x008C
> +#define IMX258_REG_C1			0x0092
> +#define IMX258_REG_M1			0x0090

It would be useful to also read the limits.

> +
>  /* V_TIMING internal */
>  #define IMX258_VTS_30FPS		0x0c98
>  #define IMX258_VTS_30FPS_2K		0x0638
> @@ -650,6 +657,38 @@ static int imx258_read_reg(struct imx258 *imx258, u16 reg, u32 len, u32 *val)
>  	return 0;
>  }
>  
> +static int imx258_read_reg_signed(struct imx258 *imx258, u16 reg, u32 len, int16_t *val)

The 16-bit signed type in the kernel is s16.

> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
> +	struct i2c_msg msgs[2];
> +	u8 addr_buf[2] = { reg >> 8, reg & 0xff };
> +	u8 data_buf[4] = { 0, };
> +	int ret;
> +
> +	if (len > 4)
> +		return -EINVAL;
> +
> +	/* Write register address */
> +	msgs[0].addr = client->addr;
> +	msgs[0].flags = 0;
> +	msgs[0].len = ARRAY_SIZE(addr_buf);
> +	msgs[0].buf = addr_buf;
> +
> +	/* Read data from register */
> +	msgs[1].addr = client->addr;
> +	msgs[1].flags = I2C_M_RD;
> +	msgs[1].len = len;
> +	msgs[1].buf = &data_buf[4 - len];
> +
> +	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> +	if (ret != ARRAY_SIZE(msgs))
> +		return -EIO;
> +
> +	*val = get_unaligned_be32(data_buf);
> +
> +	return 0;
> +}

Instead of duplicating all the code, you could implement this as a
wrapper around imx258_read_reg().

> +
>  /* Write registers up to 2 at a time */
>  static int imx258_write_reg(struct imx258 *imx258, u16 reg, u32 len, u32 val)
>  {
> @@ -1055,6 +1094,7 @@ static int imx258_identify_module(struct imx258 *imx258)
>  	struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
>  	int ret;
>  	u32 val;
> +	int16_t value;
>  
>  	ret = imx258_read_reg(imx258, IMX258_REG_CHIP_ID,
>  			      IMX258_REG_VALUE_16BIT, &val);
> @@ -1070,6 +1110,44 @@ static int imx258_identify_module(struct imx258 *imx258)
>  		return -EIO;
>  	}
>  
> +	// Hijack this to also read analog gain registers and print values
> +	dev_info(&client->dev, "Reading analog gain registers...\n");
> +
> +	ret = imx258_read_reg(imx258, IMX258_REG_GAIN_CAP, IMX258_REG_VALUE_16BIT, &val);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to read GAIN_CAP %x\n",
> +			IMX258_REG_GAIN_CAP);
> +	}
> +	dev_info(&client->dev, "Value of analog Gain capability: %d\n", val);

Another option would be to write

	dev_info(&client->dev, "Value of analog Gain capability: %d\n", (s16)val);

and drop the imx258_read_reg_signed() function completely.

> +
> +	ret = imx258_read_reg_signed(imx258, IMX258_REG_C0, IMX258_REG_VALUE_16BIT, &value);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to read C0 %x\n",
> +			IMX258_REG_C0);
> +	}
> +	dev_info(&client->dev, "Value of C0: %d\n", value);
> +
> +	ret = imx258_read_reg_signed(imx258, IMX258_REG_M0, IMX258_REG_VALUE_16BIT, &value);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to read M0 %x\n",
> +			IMX258_REG_M0);
> +	}
> +	dev_info(&client->dev, "Value of M0: %d\n", value);
> +
> +	ret = imx258_read_reg_signed(imx258, IMX258_REG_C1, IMX258_REG_VALUE_16BIT, &value);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to read C1 %x\n",
> +			IMX258_REG_C1);
> +	}
> +	dev_info(&client->dev, "Value of C1: %d\n", value);
> +
> +	ret = imx258_read_reg_signed(imx258, IMX258_REG_M1, IMX258_REG_VALUE_16BIT, &value);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to read M1 %x\n",
> +			IMX258_REG_M1);
> +	}
> +	dev_info(&client->dev, "Value of M1: %d\n", value);
> +
>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart