| Message ID | 20231023-imx214-v1-1-b33f1bbd1fcf@apitzsch.eu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: i2c: imx214: Extend with sensor size and firmware information | expand |
Quoting André Apitzsch (2023-10-23 22:47:50) > Code refinement, no functional changes. > > Signed-off-by: André Apitzsch <git@apitzsch.eu> > --- > drivers/media/i2c/imx214.c | 24 +++++++++++++++++++----- > 1 file changed, 19 insertions(+), 5 deletions(-) > > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c > index 4f77ea02cc27..9218c149d4c8 100644 > --- a/drivers/media/i2c/imx214.c > +++ b/drivers/media/i2c/imx214.c > @@ -19,12 +19,23 @@ > #include <media/v4l2-fwnode.h> > #include <media/v4l2-subdev.h> > > +#define IMX214_REG_MODE_SELECT 0x0100 > +#define IMX214_MODE_STANDBY 0x00 > +#define IMX214_MODE_STREAMING 0x01 > + > #define IMX214_DEFAULT_CLK_FREQ 24000000 > #define IMX214_DEFAULT_LINK_FREQ 480000000 > #define IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ * 8LL) / 10) > #define IMX214_FPS 30 > #define IMX214_MBUS_CODE MEDIA_BUS_FMT_SRGGB10_1X10 > > +/* Exposure control */ > +#define IMX214_REG_EXPOSURE 0x0202 > +#define IMX214_EXPOSURE_MIN 0 > +#define IMX214_EXPOSURE_MAX 3184 > +#define IMX214_EXPOSURE_STEP 1 > +#define IMX214_EXPOSURE_DEFAULT 0x0c70 I like this change, and I see that 0x0c70 was directly moved here from the code below. But could we replace 0xc70 here with 3184 please so that it's /far/ clearer that the Exposure Default == Exposure Max which is otherwise hidden? -- Regards Kieran > + > static const char * const imx214_supply_name[] = { > "vdda", > "vddd", > @@ -665,7 +676,7 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl) > case V4L2_CID_EXPOSURE: > vals[1] = ctrl->val; > vals[0] = ctrl->val >> 8; > - ret = regmap_bulk_write(imx214->regmap, 0x202, vals, 2); > + ret = regmap_bulk_write(imx214->regmap, IMX214_REG_EXPOSURE, vals, 2); > if (ret < 0) > dev_err(imx214->dev, "Error %d\n", ret); > ret = 0; > @@ -743,7 +754,7 @@ static int imx214_start_streaming(struct imx214 *imx214) > dev_err(imx214->dev, "could not sync v4l2 controls\n"); > goto error; > } > - ret = regmap_write(imx214->regmap, 0x100, 1); > + ret = regmap_write(imx214->regmap, IMX214_REG_MODE_SELECT, IMX214_MODE_STREAMING); > if (ret < 0) { > dev_err(imx214->dev, "could not sent start table %d\n", ret); > goto error; > @@ -761,7 +772,7 @@ static int imx214_stop_streaming(struct imx214 *imx214) > { > int ret; > > - ret = regmap_write(imx214->regmap, 0x100, 0); > + ret = regmap_write(imx214->regmap, IMX214_REG_MODE_SELECT, IMX214_MODE_STANDBY); > if (ret < 0) > dev_err(imx214->dev, "could not sent stop table %d\n", ret); > > @@ -991,9 +1002,12 @@ static int imx214_probe(struct i2c_client *client) > * > * Yours sincerely, Ricardo. > */ > - imx214->exposure = v4l2_ctrl_new_std(&imx214->ctrls, &imx214_ctrl_ops, > + imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops, > V4L2_CID_EXPOSURE, > - 0, 3184, 1, 0x0c70); > + IMX214_EXPOSURE_MIN, > + IMX214_EXPOSURE_MAX, > + IMX214_EXPOSURE_STEP, > + IMX214_EXPOSURE_DEFAULT); > > imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls, > NULL, > > -- > 2.42.0 >
Am Montag, dem 23.10.2023 um 23:44 +0100 schrieb Kieran Bingham: > Quoting André Apitzsch (2023-10-23 22:47:50) > > Code refinement, no functional changes. > > > > Signed-off-by: André Apitzsch <git@apitzsch.eu> > > --- > > drivers/media/i2c/imx214.c | 24 +++++++++++++++++++----- > > 1 file changed, 19 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/media/i2c/imx214.c > > b/drivers/media/i2c/imx214.c > > index 4f77ea02cc27..9218c149d4c8 100644 > > --- a/drivers/media/i2c/imx214.c > > +++ b/drivers/media/i2c/imx214.c > > @@ -19,12 +19,23 @@ > > #include <media/v4l2-fwnode.h> > > #include <media/v4l2-subdev.h> > > > > +#define IMX214_REG_MODE_SELECT 0x0100 > > +#define IMX214_MODE_STANDBY 0x00 > > +#define IMX214_MODE_STREAMING 0x01 > > + > > #define IMX214_DEFAULT_CLK_FREQ 24000000 > > #define IMX214_DEFAULT_LINK_FREQ 480000000 > > #define IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ * > > 8LL) / 10) > > #define IMX214_FPS 30 > > #define IMX214_MBUS_CODE MEDIA_BUS_FMT_SRGGB10_1X10 > > > > +/* Exposure control */ > > +#define IMX214_REG_EXPOSURE 0x0202 > > +#define IMX214_EXPOSURE_MIN 0 > > +#define IMX214_EXPOSURE_MAX 3184 > > +#define IMX214_EXPOSURE_STEP 1 > > +#define IMX214_EXPOSURE_DEFAULT 0x0c70 > > I like this change, and I see that 0x0c70 was directly moved here > from > the code below. But could we replace 0xc70 here with 3184 please so > that > it's /far/ clearer that the Exposure Default == Exposure Max which is > otherwise hidden? > Hi Kieran, I can do that. But I propose to replace 3184 with 0x0c70 instead. Because that matches the entries used in the reg_8 lists mode_4096x2304[] and mode_1920x1080[]. {0x0202, 0x0C}, {0x0203, 0x70}, What do you think? Regards, André > -- > Regards > > Kieran >
Quoting André Apitzsch (2023-10-24 08:30:01) > Am Montag, dem 23.10.2023 um 23:44 +0100 schrieb Kieran Bingham: > > Quoting André Apitzsch (2023-10-23 22:47:50) > > > Code refinement, no functional changes. > > > > > > Signed-off-by: André Apitzsch <git@apitzsch.eu> > > > --- > > > drivers/media/i2c/imx214.c | 24 +++++++++++++++++++----- > > > 1 file changed, 19 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/media/i2c/imx214.c > > > b/drivers/media/i2c/imx214.c > > > index 4f77ea02cc27..9218c149d4c8 100644 > > > --- a/drivers/media/i2c/imx214.c > > > +++ b/drivers/media/i2c/imx214.c > > > @@ -19,12 +19,23 @@ > > > #include <media/v4l2-fwnode.h> > > > #include <media/v4l2-subdev.h> > > > > > > +#define IMX214_REG_MODE_SELECT 0x0100 > > > +#define IMX214_MODE_STANDBY 0x00 > > > +#define IMX214_MODE_STREAMING 0x01 > > > + > > > #define IMX214_DEFAULT_CLK_FREQ 24000000 > > > #define IMX214_DEFAULT_LINK_FREQ 480000000 > > > #define IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ * > > > 8LL) / 10) > > > #define IMX214_FPS 30 > > > #define IMX214_MBUS_CODE MEDIA_BUS_FMT_SRGGB10_1X10 > > > > > > +/* Exposure control */ > > > +#define IMX214_REG_EXPOSURE 0x0202 > > > +#define IMX214_EXPOSURE_MIN 0 > > > +#define IMX214_EXPOSURE_MAX 3184 > > > +#define IMX214_EXPOSURE_STEP 1 > > > +#define IMX214_EXPOSURE_DEFAULT 0x0c70 > > > > I like this change, and I see that 0x0c70 was directly moved here > > from > > the code below. But could we replace 0xc70 here with 3184 please so > > that > > it's /far/ clearer that the Exposure Default == Exposure Max which is > > otherwise hidden? > > > Hi Kieran, > > I can do that. But I propose to replace 3184 with 0x0c70 instead. > Because that matches the entries used in the reg_8 lists > mode_4096x2304[] and mode_1920x1080[]. > > {0x0202, 0x0C}, > {0x0203, 0x70}, > > What do you think? I think exposure values are easier to read as integers than hex values. This is the 'Coarse Integration Time' register with a unit of 'lines'. If you have lots of time, or wish to delve deeper - we could talk about splitting out those register tables to use CCI and/or more readable functions ;-) Do you have the datasheet for this sensor - or are you just working from the information within this driver? What device are you using to test this driver? -- Kieran > Regards, > André
Am Dienstag, dem 24.10.2023 um 13:52 +0100 schrieb Kieran Bingham: > Quoting André Apitzsch (2023-10-24 08:30:01) > > Am Montag, dem 23.10.2023 um 23:44 +0100 schrieb Kieran Bingham: > > > Quoting André Apitzsch (2023-10-23 22:47:50) > > > > Code refinement, no functional changes. > > > > > > > > Signed-off-by: André Apitzsch <git@apitzsch.eu> > > > > --- > > > > drivers/media/i2c/imx214.c | 24 +++++++++++++++++++----- > > > > 1 file changed, 19 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/media/i2c/imx214.c > > > > b/drivers/media/i2c/imx214.c > > > > index 4f77ea02cc27..9218c149d4c8 100644 > > > > --- a/drivers/media/i2c/imx214.c > > > > +++ b/drivers/media/i2c/imx214.c > > > > @@ -19,12 +19,23 @@ > > > > #include <media/v4l2-fwnode.h> > > > > #include <media/v4l2-subdev.h> > > > > > > > > +#define IMX214_REG_MODE_SELECT 0x0100 > > > > +#define IMX214_MODE_STANDBY 0x00 > > > > +#define IMX214_MODE_STREAMING 0x01 > > > > + > > > > #define IMX214_DEFAULT_CLK_FREQ 24000000 > > > > #define IMX214_DEFAULT_LINK_FREQ 480000000 > > > > #define IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ * > > > > 8LL) / 10) > > > > #define IMX214_FPS 30 > > > > #define IMX214_MBUS_CODE MEDIA_BUS_FMT_SRGGB10_1X10 > > > > > > > > +/* Exposure control */ > > > > +#define IMX214_REG_EXPOSURE 0x0202 > > > > +#define IMX214_EXPOSURE_MIN 0 > > > > +#define IMX214_EXPOSURE_MAX 3184 > > > > +#define IMX214_EXPOSURE_STEP 1 > > > > +#define IMX214_EXPOSURE_DEFAULT 0x0c70 > > > > > > I like this change, and I see that 0x0c70 was directly moved here > > > from > > > the code below. But could we replace 0xc70 here with 3184 please > > > so > > > that > > > it's /far/ clearer that the Exposure Default == Exposure Max > > > which is > > > otherwise hidden? > > > > > Hi Kieran, > > > > I can do that. But I propose to replace 3184 with 0x0c70 instead. > > Because that matches the entries used in the reg_8 lists > > mode_4096x2304[] and mode_1920x1080[]. > > > > {0x0202, 0x0C}, > > {0x0203, 0x70}, > > > > What do you think? > > I think exposure values are easier to read as integers than hex > values. Okay, integer it is. > > This is the 'Coarse Integration Time' register with a unit of > 'lines'. > > If you have lots of time, or wish to delve deeper - we could talk > about splitting out those register tables to use CCI and/or more > readable functions ;-) I will consider it. > > Do you have the datasheet for this sensor - or are you just working > from the information within this driver? I don't have the datasheet, yet (maybe somebody could provide it), so I have to work with the information within this driver and downstream implementations, like the one by nvidia [1]. > > What device are you using to test this driver? > The sensor is part of my mobile, BQ Aquaris M5 (Longcheer L9100). Best regards, André [1] https://github.com/InES-HPMM/linux-l4t/blob/9150157d2b1f73aefe01e9a07e3b062a45d57247/drivers/media/i2c/imx214_mode_tbls.h > -- > Kieran > > > Regards, > > André
Hi André Thanks for your patch! I usually do not care about creating a define for something that I use only once... but if you think this is more clear, let it be :) On Mon, Oct 23, 2023 at 11:49 PM André Apitzsch <git@apitzsch.eu> wrote: > > Code refinement, no functional changes. > > Signed-off-by: André Apitzsch <git@apitzsch.eu> Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/i2c/imx214.c | 24 +++++++++++++++++++----- > 1 file changed, 19 insertions(+), 5 deletions(-) > > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c > index 4f77ea02cc27..9218c149d4c8 100644 > --- a/drivers/media/i2c/imx214.c > +++ b/drivers/media/i2c/imx214.c > @@ -19,12 +19,23 @@ > #include <media/v4l2-fwnode.h> > #include <media/v4l2-subdev.h> > > +#define IMX214_REG_MODE_SELECT 0x0100 > +#define IMX214_MODE_STANDBY 0x00 > +#define IMX214_MODE_STREAMING 0x01 > + > #define IMX214_DEFAULT_CLK_FREQ 24000000 > #define IMX214_DEFAULT_LINK_FREQ 480000000 > #define IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ * 8LL) / 10) > #define IMX214_FPS 30 > #define IMX214_MBUS_CODE MEDIA_BUS_FMT_SRGGB10_1X10 > > +/* Exposure control */ > +#define IMX214_REG_EXPOSURE 0x0202 > +#define IMX214_EXPOSURE_MIN 0 > +#define IMX214_EXPOSURE_MAX 3184 > +#define IMX214_EXPOSURE_STEP 1 > +#define IMX214_EXPOSURE_DEFAULT 0x0c70 > + > static const char * const imx214_supply_name[] = { > "vdda", > "vddd", > @@ -665,7 +676,7 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl) > case V4L2_CID_EXPOSURE: > vals[1] = ctrl->val; > vals[0] = ctrl->val >> 8; > - ret = regmap_bulk_write(imx214->regmap, 0x202, vals, 2); > + ret = regmap_bulk_write(imx214->regmap, IMX214_REG_EXPOSURE, vals, 2); > if (ret < 0) > dev_err(imx214->dev, "Error %d\n", ret); > ret = 0; > @@ -743,7 +754,7 @@ static int imx214_start_streaming(struct imx214 *imx214) > dev_err(imx214->dev, "could not sync v4l2 controls\n"); > goto error; > } > - ret = regmap_write(imx214->regmap, 0x100, 1); > + ret = regmap_write(imx214->regmap, IMX214_REG_MODE_SELECT, IMX214_MODE_STREAMING); > if (ret < 0) { > dev_err(imx214->dev, "could not sent start table %d\n", ret); > goto error; > @@ -761,7 +772,7 @@ static int imx214_stop_streaming(struct imx214 *imx214) > { > int ret; > > - ret = regmap_write(imx214->regmap, 0x100, 0); > + ret = regmap_write(imx214->regmap, IMX214_REG_MODE_SELECT, IMX214_MODE_STANDBY); > if (ret < 0) > dev_err(imx214->dev, "could not sent stop table %d\n", ret); > > @@ -991,9 +1002,12 @@ static int imx214_probe(struct i2c_client *client) > * > * Yours sincerely, Ricardo. > */ > - imx214->exposure = v4l2_ctrl_new_std(&imx214->ctrls, &imx214_ctrl_ops, > + imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops, > V4L2_CID_EXPOSURE, > - 0, 3184, 1, 0x0c70); > + IMX214_EXPOSURE_MIN, > + IMX214_EXPOSURE_MAX, > + IMX214_EXPOSURE_STEP, > + IMX214_EXPOSURE_DEFAULT); > > imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls, > NULL, > > -- > 2.42.0 >
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c index 4f77ea02cc27..9218c149d4c8 100644 --- a/drivers/media/i2c/imx214.c +++ b/drivers/media/i2c/imx214.c @@ -19,12 +19,23 @@ #include <media/v4l2-fwnode.h> #include <media/v4l2-subdev.h> +#define IMX214_REG_MODE_SELECT 0x0100 +#define IMX214_MODE_STANDBY 0x00 +#define IMX214_MODE_STREAMING 0x01 + #define IMX214_DEFAULT_CLK_FREQ 24000000 #define IMX214_DEFAULT_LINK_FREQ 480000000 #define IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ * 8LL) / 10) #define IMX214_FPS 30 #define IMX214_MBUS_CODE MEDIA_BUS_FMT_SRGGB10_1X10 +/* Exposure control */ +#define IMX214_REG_EXPOSURE 0x0202 +#define IMX214_EXPOSURE_MIN 0 +#define IMX214_EXPOSURE_MAX 3184 +#define IMX214_EXPOSURE_STEP 1 +#define IMX214_EXPOSURE_DEFAULT 0x0c70 + static const char * const imx214_supply_name[] = { "vdda", "vddd", @@ -665,7 +676,7 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl) case V4L2_CID_EXPOSURE: vals[1] = ctrl->val; vals[0] = ctrl->val >> 8; - ret = regmap_bulk_write(imx214->regmap, 0x202, vals, 2); + ret = regmap_bulk_write(imx214->regmap, IMX214_REG_EXPOSURE, vals, 2); if (ret < 0) dev_err(imx214->dev, "Error %d\n", ret); ret = 0; @@ -743,7 +754,7 @@ static int imx214_start_streaming(struct imx214 *imx214) dev_err(imx214->dev, "could not sync v4l2 controls\n"); goto error; } - ret = regmap_write(imx214->regmap, 0x100, 1); + ret = regmap_write(imx214->regmap, IMX214_REG_MODE_SELECT, IMX214_MODE_STREAMING); if (ret < 0) { dev_err(imx214->dev, "could not sent start table %d\n", ret); goto error; @@ -761,7 +772,7 @@ static int imx214_stop_streaming(struct imx214 *imx214) { int ret; - ret = regmap_write(imx214->regmap, 0x100, 0); + ret = regmap_write(imx214->regmap, IMX214_REG_MODE_SELECT, IMX214_MODE_STANDBY); if (ret < 0) dev_err(imx214->dev, "could not sent stop table %d\n", ret); @@ -991,9 +1002,12 @@ static int imx214_probe(struct i2c_client *client) * * Yours sincerely, Ricardo. */ - imx214->exposure = v4l2_ctrl_new_std(&imx214->ctrls, &imx214_ctrl_ops, + imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops, V4L2_CID_EXPOSURE, - 0, 3184, 1, 0x0c70); + IMX214_EXPOSURE_MIN, + IMX214_EXPOSURE_MAX, + IMX214_EXPOSURE_STEP, + IMX214_EXPOSURE_DEFAULT); imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls, NULL,
Code refinement, no functional changes. Signed-off-by: André Apitzsch <git@apitzsch.eu> --- drivers/media/i2c/imx214.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-)