From: Christian Hemp <[email protected]>
Aside from 12 bit monochrome or color format the sensor implicitly
supports 10 and 8 bit formats as well by simply dropping the
corresponding LSBs.
Signed-off-by: Christian Hemp <[email protected]>
[[email protected]: simplified by dropping v4l2_colorspace handling]
Signed-off-by: Jan Luebbe <[email protected]>
Signed-off-by: Stefan Riedmueller <[email protected]>
---
drivers/media/i2c/mt9p031.c | 50 +++++++++++++++++++++++++++++--------
1 file changed, 40 insertions(+), 10 deletions(-)
diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index dc23b9ed510a..0002dd299ffa 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -116,6 +116,18 @@ enum mt9p031_model {
MT9P031_MODEL_MONOCHROME,
};
+static const u32 mt9p031_color_fmts[] = {
+ MEDIA_BUS_FMT_SGRBG8_1X8,
+ MEDIA_BUS_FMT_SGRBG10_1X10,
+ MEDIA_BUS_FMT_SGRBG12_1X12,
+};
+
+static const u32 mt9p031_monochrome_fmts[] = {
+ MEDIA_BUS_FMT_Y8_1X8,
+ MEDIA_BUS_FMT_Y10_1X10,
+ MEDIA_BUS_FMT_Y12_1X12,
+};
+
struct mt9p031 {
struct v4l2_subdev subdev;
struct media_pad pad;
@@ -138,6 +150,9 @@ struct mt9p031 {
struct v4l2_ctrl *blc_auto;
struct v4l2_ctrl *blc_offset;
+ const u32 *fmts;
+ int num_fmts;
+
/* Registers cache */
u16 output_control;
u16 mode2;
@@ -148,6 +163,17 @@ static struct mt9p031 *to_mt9p031(struct v4l2_subdev *sd)
return container_of(sd, struct mt9p031, subdev);
}
+static const u32 mt9p031_find_datafmt(struct mt9p031 *mt9p031, u32 code)
+{
+ int i;
+
+ for (i = 0; i < mt9p031->num_fmts; i++)
+ if (mt9p031->fmts[i] == code)
+ return mt9p031->fmts[i];
+
+ return mt9p031->fmts[mt9p031->num_fmts-1];
+}
+
static int mt9p031_read(struct i2c_client *client, u8 reg)
{
return i2c_smbus_read_word_swapped(client, reg);
@@ -476,10 +502,11 @@ static int mt9p031_enum_mbus_code(struct v4l2_subdev *subdev,
{
struct mt9p031 *mt9p031 = to_mt9p031(subdev);
- if (code->pad || code->index)
+ if (code->pad || code->index >= mt9p031->num_fmts)
return -EINVAL;
- code->code = mt9p031->format.code;
+ code->code = mt9p031->fmts[code->index];
+
return 0;
}
@@ -573,6 +600,8 @@ static int mt9p031_set_format(struct v4l2_subdev *subdev,
__format->width = __crop->width / hratio;
__format->height = __crop->height / vratio;
+ __format->code = mt9p031_find_datafmt(mt9p031, format->format.code);
+
format->format = *__format;
return 0;
@@ -951,10 +980,7 @@ static int mt9p031_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
format = v4l2_subdev_get_try_format(subdev, fh->pad, 0);
- if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
- format->code = MEDIA_BUS_FMT_Y12_1X12;
- else
- format->code = MEDIA_BUS_FMT_SGRBG12_1X12;
+ format->code = mt9p031_find_datafmt(mt9p031, 0);
format->width = MT9P031_WINDOW_WIDTH_DEF;
format->height = MT9P031_WINDOW_HEIGHT_DEF;
@@ -1121,10 +1147,14 @@ static int mt9p031_probe(struct i2c_client *client,
mt9p031->crop.left = MT9P031_COLUMN_START_DEF;
mt9p031->crop.top = MT9P031_ROW_START_DEF;
- if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
- mt9p031->format.code = MEDIA_BUS_FMT_Y12_1X12;
- else
- mt9p031->format.code = MEDIA_BUS_FMT_SGRBG12_1X12;
+ if (mt9p031->model == MT9P031_MODEL_MONOCHROME) {
+ mt9p031->fmts = mt9p031_monochrome_fmts;
+ mt9p031->num_fmts = ARRAY_SIZE(mt9p031_monochrome_fmts);
+ } else {
+ mt9p031->fmts = mt9p031_color_fmts;
+ mt9p031->num_fmts = ARRAY_SIZE(mt9p031_color_fmts);
+ }
+ mt9p031->format.code = mt9p031_find_datafmt(mt9p031, 0);
mt9p031->format.width = MT9P031_WINDOW_WIDTH_DEF;
mt9p031->format.height = MT9P031_WINDOW_HEIGHT_DEF;
--
2.25.1
From: Enrico Scholz <[email protected]>
The real and requested clock can differ and because it is used to
calculate PLL values, the real clock rate should be read.
Signed-off-by: Enrico Scholz <[email protected]>
Signed-off-by: Stefan Riedmueller <[email protected]>
---
drivers/media/i2c/mt9p031.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index 0002dd299ffa..ce192be4531f 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -255,6 +255,7 @@ static int mt9p031_clk_setup(struct mt9p031 *mt9p031)
struct i2c_client *client = v4l2_get_subdevdata(&mt9p031->subdev);
struct mt9p031_platform_data *pdata = mt9p031->pdata;
+ unsigned long ext_freq;
int ret;
mt9p031->clk = devm_clk_get(&client->dev, NULL);
@@ -265,13 +266,15 @@ static int mt9p031_clk_setup(struct mt9p031 *mt9p031)
if (ret < 0)
return ret;
+ ext_freq = clk_get_rate(mt9p031->clk);
+
/* If the external clock frequency is out of bounds for the PLL use the
* pixel clock divider only and disable the PLL.
*/
- if (pdata->ext_freq > limits.ext_clock_max) {
+ if (ext_freq > limits.ext_clock_max) {
unsigned int div;
- div = DIV_ROUND_UP(pdata->ext_freq, pdata->target_freq);
+ div = DIV_ROUND_UP(ext_freq, pdata->target_freq);
div = roundup_pow_of_two(div) / 2;
mt9p031->clk_div = min_t(unsigned int, div, 64);
@@ -280,7 +283,7 @@ static int mt9p031_clk_setup(struct mt9p031 *mt9p031)
return 0;
}
- mt9p031->pll.ext_clock = pdata->ext_freq;
+ mt9p031->pll.ext_clock = ext_freq;
mt9p031->pll.pix_clock = pdata->target_freq;
mt9p031->use_pll = true;
--
2.25.1
From: Enrico Scholz <[email protected]>
Implement g_register and s_register v4l2_subdev_core_ops to access
camera register directly from userspace for debug purposes.
Signed-off-by: Enrico Scholz <[email protected]>
Signed-off-by: Stefan Riedmueller <[email protected]>
---
drivers/media/i2c/mt9p031.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index ce192be4531f..f5d6a7890c47 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -703,6 +703,30 @@ static int mt9p031_restore_blc(struct mt9p031 *mt9p031)
return 0;
}
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+static int mt9p031_g_register(struct v4l2_subdev *sd,
+ struct v4l2_dbg_register *reg)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+ int ret;
+
+ ret = mt9p031_read(client, reg->reg);
+ if (ret < 0)
+ return ret;
+
+ reg->val = ret;
+ return 0;
+}
+
+static int mt9p031_s_register(struct v4l2_subdev *sd,
+ struct v4l2_dbg_register const *reg)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+ return mt9p031_write(client, reg->reg, reg->val);
+}
+#endif
+
static int mt9p031_s_ctrl(struct v4l2_ctrl *ctrl)
{
struct mt9p031 *mt9p031 =
@@ -1000,6 +1024,10 @@ static int mt9p031_close(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
static const struct v4l2_subdev_core_ops mt9p031_subdev_core_ops = {
.s_power = mt9p031_set_power,
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+ .s_register = mt9p031_s_register,
+ .g_register = mt9p031_g_register,
+#endif
};
static const struct v4l2_subdev_video_ops mt9p031_subdev_video_ops = {
--
2.25.1
From: Christian Hemp <[email protected]>
Evaluate the desired pixel clock polarity from the device tree.
Signed-off-by: Christian Hemp <[email protected]>
Signed-off-by: Stefan Riedmueller <[email protected]>
---
drivers/media/i2c/Kconfig | 1 +
drivers/media/i2c/mt9p031.c | 19 ++++++++++++++++++-
include/media/i2c/mt9p031.h | 1 +
3 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index c7ba76fee599..7c026daeacf0 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -1103,6 +1103,7 @@ config VIDEO_MT9P031
select MEDIA_CONTROLLER
select VIDEO_V4L2_SUBDEV_API
select VIDEO_APTINA_PLL
+ select V4L2_FWNODE
help
This is a Video4Linux2 sensor driver for the Aptina
(Micron) mt9p031 5 Mpixel camera.
diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index f5d6a7890c47..8f8ee37a2dd2 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -27,6 +27,7 @@
#include <media/v4l2-async.h>
#include <media/v4l2-ctrls.h>
#include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
#include <media/v4l2-subdev.h>
#include "aptina-pll.h"
@@ -399,6 +400,14 @@ static int __mt9p031_set_power(struct mt9p031 *mt9p031, bool on)
return ret;
}
+ /* Configure the pixel clock polarity */
+ if (mt9p031->pdata && mt9p031->pdata->pixclk_pol) {
+ ret = mt9p031_write(client, MT9P031_PIXEL_CLOCK_CONTROL,
+ MT9P031_PIXEL_CLOCK_INVERT);
+ if (ret < 0)
+ return ret;
+ }
+
return v4l2_ctrl_handler_setup(&mt9p031->ctrls);
}
@@ -1062,7 +1071,8 @@ static const struct v4l2_subdev_internal_ops mt9p031_subdev_internal_ops = {
static struct mt9p031_platform_data *
mt9p031_get_pdata(struct i2c_client *client)
{
- struct mt9p031_platform_data *pdata;
+ struct mt9p031_platform_data *pdata = NULL;
+ struct v4l2_fwnode_endpoint endpoint;
struct device_node *np;
if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node)
@@ -1072,6 +1082,10 @@ mt9p031_get_pdata(struct i2c_client *client)
if (!np)
return NULL;
+ endpoint.bus_type = V4L2_MBUS_UNKNOWN;
+ if (v4l2_fwnode_endpoint_parse(of_fwnode_handle(np), &endpoint) < 0)
+ goto done;
+
pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
if (!pdata)
goto done;
@@ -1079,6 +1093,9 @@ mt9p031_get_pdata(struct i2c_client *client)
of_property_read_u32(np, "input-clock-frequency", &pdata->ext_freq);
of_property_read_u32(np, "pixel-clock-frequency", &pdata->target_freq);
+ pdata->pixclk_pol = !!(endpoint.bus.parallel.flags &
+ V4L2_MBUS_PCLK_SAMPLE_RISING);
+
done:
of_node_put(np);
return pdata;
diff --git a/include/media/i2c/mt9p031.h b/include/media/i2c/mt9p031.h
index 7c29c53aa988..f933cd0be8e5 100644
--- a/include/media/i2c/mt9p031.h
+++ b/include/media/i2c/mt9p031.h
@@ -10,6 +10,7 @@ struct v4l2_subdev;
* @target_freq: Pixel clock frequency
*/
struct mt9p031_platform_data {
+ unsigned int pixclk_pol:1;
int ext_freq;
int target_freq;
};
--
2.25.1
From: Dirk Bender <[email protected]>
To prevent corrupted frames after starting and stopping the sensor it's
datasheet specifies a specific pause sequence to follow:
Stopping:
Set Pause_Restart Bit -> Set Restart Bit -> Set Chip_Enable Off
Restarting:
Set Chip_Enable On -> Clear Pause_Restart Bit
The Restart Bit is cleared automatically and must not be cleared
manually as this would cause undefined behavior.
Signed-off-by: Dirk Bender <[email protected]>
Signed-off-by: Stefan Riedmueller <[email protected]>
---
drivers/media/i2c/mt9p031.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index 8f8ee37a2dd2..2f2daf95dcd3 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -80,6 +80,8 @@
#define MT9P031_PIXEL_CLOCK_SHIFT(n) ((n) << 8)
#define MT9P031_PIXEL_CLOCK_DIVIDE(n) ((n) << 0)
#define MT9P031_FRAME_RESTART 0x0b
+#define MT9P031_FRAME_RESTART_SET (1 << 0)
+#define MT9P031_FRAME_PAUSE_RESTART_SET (1 << 1)
#define MT9P031_SHUTTER_DELAY 0x0c
#define MT9P031_RST 0x0d
#define MT9P031_RST_ENABLE 1
@@ -483,9 +485,25 @@ static int mt9p031_set_params(struct mt9p031 *mt9p031)
static int mt9p031_s_stream(struct v4l2_subdev *subdev, int enable)
{
struct mt9p031 *mt9p031 = to_mt9p031(subdev);
+ struct i2c_client *client = v4l2_get_subdevdata(subdev);
+ int val;
int ret;
if (!enable) {
+ val = mt9p031_read(client, MT9P031_FRAME_RESTART);
+
+ /* enable pause restart */
+ val |= MT9P031_FRAME_PAUSE_RESTART_SET;
+ ret = mt9p031_write(client, MT9P031_FRAME_RESTART, val);
+ if (ret < 0)
+ return ret;
+
+ /* enable restart + keep pause restart set */
+ val |= MT9P031_FRAME_RESTART_SET;
+ ret = mt9p031_write(client, MT9P031_FRAME_RESTART, val);
+ if (ret < 0)
+ return ret;
+
/* Stop sensor readout */
ret = mt9p031_set_output_control(mt9p031,
MT9P031_OUTPUT_CONTROL_CEN, 0);
@@ -505,6 +523,13 @@ static int mt9p031_s_stream(struct v4l2_subdev *subdev, int enable)
if (ret < 0)
return ret;
+ val = mt9p031_read(client, MT9P031_FRAME_RESTART);
+ /* disable reset + pause restart */
+ val &= ~MT9P031_FRAME_PAUSE_RESTART_SET;
+ ret = mt9p031_write(client, MT9P031_FRAME_RESTART, val);
+ if (ret < 0)
+ return ret;
+
return mt9p031_pll_enable(mt9p031);
}
--
2.25.1
Hi Stefan,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v5.9-rc6 next-20200924]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Stefan-Riedmueller/media-mt9p031-Add-support-for-8-bit-and-10-bit-formats/20200925-155251
base: git://linuxtv.org/media_tree.git master
config: arc-allyesconfig (attached as .config)
compiler: arceb-elf-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/1863bce6d7b4791c048a25ec07ff8aec8b4847cf
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Stefan-Riedmueller/media-mt9p031-Add-support-for-8-bit-and-10-bit-formats/20200925-155251
git checkout 1863bce6d7b4791c048a25ec07ff8aec8b4847cf
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/media/i2c/mt9p031.c:166:8: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
166 | static const u32 mt9p031_find_datafmt(struct mt9p031 *mt9p031, u32 code)
| ^~~~~
vim +166 drivers/media/i2c/mt9p031.c
165
> 166 static const u32 mt9p031_find_datafmt(struct mt9p031 *mt9p031, u32 code)
167 {
168 int i;
169
170 for (i = 0; i < mt9p031->num_fmts; i++)
171 if (mt9p031->fmts[i] == code)
172 return mt9p031->fmts[i];
173
174 return mt9p031->fmts[mt9p031->num_fmts-1];
175 }
176
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Stefan,
Thanks for the patchset.
On Fri, Sep 25, 2020 at 09:50:28AM +0200, Stefan Riedmueller wrote:
> From: Christian Hemp <[email protected]>
>
> Evaluate the desired pixel clock polarity from the device tree.
>
> Signed-off-by: Christian Hemp <[email protected]>
> Signed-off-by: Stefan Riedmueller <[email protected]>
> ---
> drivers/media/i2c/Kconfig | 1 +
> drivers/media/i2c/mt9p031.c | 19 ++++++++++++++++++-
> include/media/i2c/mt9p031.h | 1 +
> 3 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index c7ba76fee599..7c026daeacf0 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -1103,6 +1103,7 @@ config VIDEO_MT9P031
> select MEDIA_CONTROLLER
> select VIDEO_V4L2_SUBDEV_API
> select VIDEO_APTINA_PLL
> + select V4L2_FWNODE
> help
> This is a Video4Linux2 sensor driver for the Aptina
> (Micron) mt9p031 5 Mpixel camera.
> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index f5d6a7890c47..8f8ee37a2dd2 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -27,6 +27,7 @@
> #include <media/v4l2-async.h>
> #include <media/v4l2-ctrls.h>
> #include <media/v4l2-device.h>
> +#include <media/v4l2-fwnode.h>
> #include <media/v4l2-subdev.h>
>
> #include "aptina-pll.h"
> @@ -399,6 +400,14 @@ static int __mt9p031_set_power(struct mt9p031 *mt9p031, bool on)
> return ret;
> }
>
> + /* Configure the pixel clock polarity */
> + if (mt9p031->pdata && mt9p031->pdata->pixclk_pol) {
> + ret = mt9p031_write(client, MT9P031_PIXEL_CLOCK_CONTROL,
> + MT9P031_PIXEL_CLOCK_INVERT);
> + if (ret < 0)
> + return ret;
> + }
> +
> return v4l2_ctrl_handler_setup(&mt9p031->ctrls);
> }
>
> @@ -1062,7 +1071,8 @@ static const struct v4l2_subdev_internal_ops mt9p031_subdev_internal_ops = {
> static struct mt9p031_platform_data *
> mt9p031_get_pdata(struct i2c_client *client)
> {
> - struct mt9p031_platform_data *pdata;
> + struct mt9p031_platform_data *pdata = NULL;
> + struct v4l2_fwnode_endpoint endpoint;
Could you initialise the bus_type field to a valid value? I suppose this
sensor only supports one of them? That way you'll also initialise the rest
of the struct fields to zero.
> struct device_node *np;
>
> if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node)
> @@ -1072,6 +1082,10 @@ mt9p031_get_pdata(struct i2c_client *client)
> if (!np)
> return NULL;
>
> + endpoint.bus_type = V4L2_MBUS_UNKNOWN;
> + if (v4l2_fwnode_endpoint_parse(of_fwnode_handle(np), &endpoint) < 0)
> + goto done;
> +
> pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> if (!pdata)
> goto done;
> @@ -1079,6 +1093,9 @@ mt9p031_get_pdata(struct i2c_client *client)
> of_property_read_u32(np, "input-clock-frequency", &pdata->ext_freq);
> of_property_read_u32(np, "pixel-clock-frequency", &pdata->target_freq);
>
> + pdata->pixclk_pol = !!(endpoint.bus.parallel.flags &
> + V4L2_MBUS_PCLK_SAMPLE_RISING);
> +
> done:
> of_node_put(np);
> return pdata;
> diff --git a/include/media/i2c/mt9p031.h b/include/media/i2c/mt9p031.h
> index 7c29c53aa988..f933cd0be8e5 100644
> --- a/include/media/i2c/mt9p031.h
> +++ b/include/media/i2c/mt9p031.h
> @@ -10,6 +10,7 @@ struct v4l2_subdev;
> * @target_freq: Pixel clock frequency
> */
> struct mt9p031_platform_data {
> + unsigned int pixclk_pol:1;
> int ext_freq;
> int target_freq;
> };
--
Regards,
Sakari Ailus
Hi Stefan,
On Fri, Sep 25, 2020 at 09:50:25AM +0200, Stefan Riedmueller wrote:
> From: Christian Hemp <[email protected]>
>
> Aside from 12 bit monochrome or color format the sensor implicitly
> supports 10 and 8 bit formats as well by simply dropping the
> corresponding LSBs.
>
> Signed-off-by: Christian Hemp <[email protected]>
> [[email protected]: simplified by dropping v4l2_colorspace handling]
> Signed-off-by: Jan Luebbe <[email protected]>
> Signed-off-by: Stefan Riedmueller <[email protected]>
> ---
> drivers/media/i2c/mt9p031.c | 50 +++++++++++++++++++++++++++++--------
> 1 file changed, 40 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index dc23b9ed510a..0002dd299ffa 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -116,6 +116,18 @@ enum mt9p031_model {
> MT9P031_MODEL_MONOCHROME,
> };
>
> +static const u32 mt9p031_color_fmts[] = {
> + MEDIA_BUS_FMT_SGRBG8_1X8,
> + MEDIA_BUS_FMT_SGRBG10_1X10,
> + MEDIA_BUS_FMT_SGRBG12_1X12,
> +};
> +
> +static const u32 mt9p031_monochrome_fmts[] = {
> + MEDIA_BUS_FMT_Y8_1X8,
> + MEDIA_BUS_FMT_Y10_1X10,
> + MEDIA_BUS_FMT_Y12_1X12,
> +};
> +
> struct mt9p031 {
> struct v4l2_subdev subdev;
> struct media_pad pad;
> @@ -138,6 +150,9 @@ struct mt9p031 {
> struct v4l2_ctrl *blc_auto;
> struct v4l2_ctrl *blc_offset;
>
> + const u32 *fmts;
> + int num_fmts;
Unsigned int, please.
> +
> /* Registers cache */
> u16 output_control;
> u16 mode2;
> @@ -148,6 +163,17 @@ static struct mt9p031 *to_mt9p031(struct v4l2_subdev *sd)
> return container_of(sd, struct mt9p031, subdev);
> }
>
> +static const u32 mt9p031_find_datafmt(struct mt9p031 *mt9p031, u32 code)
> +{
> + int i;
Same here.
> +
> + for (i = 0; i < mt9p031->num_fmts; i++)
> + if (mt9p031->fmts[i] == code)
> + return mt9p031->fmts[i];
> +
> + return mt9p031->fmts[mt9p031->num_fmts-1];
> +}
> +
> static int mt9p031_read(struct i2c_client *client, u8 reg)
> {
> return i2c_smbus_read_word_swapped(client, reg);
> @@ -476,10 +502,11 @@ static int mt9p031_enum_mbus_code(struct v4l2_subdev *subdev,
> {
> struct mt9p031 *mt9p031 = to_mt9p031(subdev);
>
> - if (code->pad || code->index)
> + if (code->pad || code->index >= mt9p031->num_fmts)
> return -EINVAL;
>
> - code->code = mt9p031->format.code;
> + code->code = mt9p031->fmts[code->index];
> +
> return 0;
> }
>
> @@ -573,6 +600,8 @@ static int mt9p031_set_format(struct v4l2_subdev *subdev,
> __format->width = __crop->width / hratio;
> __format->height = __crop->height / vratio;
>
> + __format->code = mt9p031_find_datafmt(mt9p031, format->format.code);
> +
> format->format = *__format;
>
> return 0;
> @@ -951,10 +980,7 @@ static int mt9p031_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
>
> format = v4l2_subdev_get_try_format(subdev, fh->pad, 0);
>
> - if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
> - format->code = MEDIA_BUS_FMT_Y12_1X12;
> - else
> - format->code = MEDIA_BUS_FMT_SGRBG12_1X12;
> + format->code = mt9p031_find_datafmt(mt9p031, 0);
>
> format->width = MT9P031_WINDOW_WIDTH_DEF;
> format->height = MT9P031_WINDOW_HEIGHT_DEF;
> @@ -1121,10 +1147,14 @@ static int mt9p031_probe(struct i2c_client *client,
> mt9p031->crop.left = MT9P031_COLUMN_START_DEF;
> mt9p031->crop.top = MT9P031_ROW_START_DEF;
>
> - if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
> - mt9p031->format.code = MEDIA_BUS_FMT_Y12_1X12;
> - else
> - mt9p031->format.code = MEDIA_BUS_FMT_SGRBG12_1X12;
> + if (mt9p031->model == MT9P031_MODEL_MONOCHROME) {
> + mt9p031->fmts = mt9p031_monochrome_fmts;
> + mt9p031->num_fmts = ARRAY_SIZE(mt9p031_monochrome_fmts);
> + } else {
> + mt9p031->fmts = mt9p031_color_fmts;
> + mt9p031->num_fmts = ARRAY_SIZE(mt9p031_color_fmts);
> + }
> + mt9p031->format.code = mt9p031_find_datafmt(mt9p031, 0);
>
> mt9p031->format.width = MT9P031_WINDOW_WIDTH_DEF;
> mt9p031->format.height = MT9P031_WINDOW_HEIGHT_DEF;
--
Kind regards,
Sakari Ailus
Hi Sakari,
thanks for your review.
On 25.09.20 22:07, Sakari Ailus wrote:
> Hi Stefan,
>
> Thanks for the patchset.
>
> On Fri, Sep 25, 2020 at 09:50:28AM +0200, Stefan Riedmueller wrote:
>> From: Christian Hemp <[email protected]>
>>
>> Evaluate the desired pixel clock polarity from the device tree.
>>
>> Signed-off-by: Christian Hemp <[email protected]>
>> Signed-off-by: Stefan Riedmueller <[email protected]>
>> ---
>> drivers/media/i2c/Kconfig | 1 +
>> drivers/media/i2c/mt9p031.c | 19 ++++++++++++++++++-
>> include/media/i2c/mt9p031.h | 1 +
>> 3 files changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>> index c7ba76fee599..7c026daeacf0 100644
>> --- a/drivers/media/i2c/Kconfig
>> +++ b/drivers/media/i2c/Kconfig
>> @@ -1103,6 +1103,7 @@ config VIDEO_MT9P031
>> select MEDIA_CONTROLLER
>> select VIDEO_V4L2_SUBDEV_API
>> select VIDEO_APTINA_PLL
>> + select V4L2_FWNODE
>> help
>> This is a Video4Linux2 sensor driver for the Aptina
>> (Micron) mt9p031 5 Mpixel camera.
>> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
>> index f5d6a7890c47..8f8ee37a2dd2 100644
>> --- a/drivers/media/i2c/mt9p031.c
>> +++ b/drivers/media/i2c/mt9p031.c
>> @@ -27,6 +27,7 @@
>> #include <media/v4l2-async.h>
>> #include <media/v4l2-ctrls.h>
>> #include <media/v4l2-device.h>
>> +#include <media/v4l2-fwnode.h>
>> #include <media/v4l2-subdev.h>
>>
>> #include "aptina-pll.h"
>> @@ -399,6 +400,14 @@ static int __mt9p031_set_power(struct mt9p031 *mt9p031, bool on)
>> return ret;
>> }
>>
>> + /* Configure the pixel clock polarity */
>> + if (mt9p031->pdata && mt9p031->pdata->pixclk_pol) {
>> + ret = mt9p031_write(client, MT9P031_PIXEL_CLOCK_CONTROL,
>> + MT9P031_PIXEL_CLOCK_INVERT);
>> + if (ret < 0)
>> + return ret;
>> + }
>> +
>> return v4l2_ctrl_handler_setup(&mt9p031->ctrls);
>> }
>>
>> @@ -1062,7 +1071,8 @@ static const struct v4l2_subdev_internal_ops mt9p031_subdev_internal_ops = {
>> static struct mt9p031_platform_data *
>> mt9p031_get_pdata(struct i2c_client *client)
>> {
>> - struct mt9p031_platform_data *pdata;
>> + struct mt9p031_platform_data *pdata = NULL;
>> + struct v4l2_fwnode_endpoint endpoint;
>
> Could you initialise the bus_type field to a valid value? I suppose this
> sensor only supports one of them? That way you'll also initialise the rest
> of the struct fields to zero.
Yes, I'll do that and send a v2.
Thanks,
Stefan
>
>> struct device_node *np;
>>
>> if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node)
>> @@ -1072,6 +1082,10 @@ mt9p031_get_pdata(struct i2c_client *client)
>> if (!np)
>> return NULL;
>>
>> + endpoint.bus_type = V4L2_MBUS_UNKNOWN;
>> + if (v4l2_fwnode_endpoint_parse(of_fwnode_handle(np), &endpoint) < 0)
>> + goto done;
>> +
>> pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
>> if (!pdata)
>> goto done;
>> @@ -1079,6 +1093,9 @@ mt9p031_get_pdata(struct i2c_client *client)
>> of_property_read_u32(np, "input-clock-frequency", &pdata->ext_freq);
>> of_property_read_u32(np, "pixel-clock-frequency", &pdata->target_freq);
>>
>> + pdata->pixclk_pol = !!(endpoint.bus.parallel.flags &
>> + V4L2_MBUS_PCLK_SAMPLE_RISING);
>> +
>> done:
>> of_node_put(np);
>> return pdata;
>> diff --git a/include/media/i2c/mt9p031.h b/include/media/i2c/mt9p031.h
>> index 7c29c53aa988..f933cd0be8e5 100644
>> --- a/include/media/i2c/mt9p031.h
>> +++ b/include/media/i2c/mt9p031.h
>> @@ -10,6 +10,7 @@ struct v4l2_subdev;
>> * @target_freq: Pixel clock frequency
>> */
>> struct mt9p031_platform_data {
>> + unsigned int pixclk_pol:1;
>> int ext_freq;
>> int target_freq;
>> };
>
Hi Sakari,
On 25.09.20 22:11, Sakari Ailus wrote:
> Hi Stefan,
>
> On Fri, Sep 25, 2020 at 09:50:25AM +0200, Stefan Riedmueller wrote:
>> From: Christian Hemp <[email protected]>
>>
>> Aside from 12 bit monochrome or color format the sensor implicitly
>> supports 10 and 8 bit formats as well by simply dropping the
>> corresponding LSBs.
>>
>> Signed-off-by: Christian Hemp <[email protected]>
>> [[email protected]: simplified by dropping v4l2_colorspace handling]
>> Signed-off-by: Jan Luebbe <[email protected]>
>> Signed-off-by: Stefan Riedmueller <[email protected]>
>> ---
>> drivers/media/i2c/mt9p031.c | 50 +++++++++++++++++++++++++++++--------
>> 1 file changed, 40 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
>> index dc23b9ed510a..0002dd299ffa 100644
>> --- a/drivers/media/i2c/mt9p031.c
>> +++ b/drivers/media/i2c/mt9p031.c
>> @@ -116,6 +116,18 @@ enum mt9p031_model {
>> MT9P031_MODEL_MONOCHROME,
>> };
>>
>> +static const u32 mt9p031_color_fmts[] = {
>> + MEDIA_BUS_FMT_SGRBG8_1X8,
>> + MEDIA_BUS_FMT_SGRBG10_1X10,
>> + MEDIA_BUS_FMT_SGRBG12_1X12,
>> +};
>> +
>> +static const u32 mt9p031_monochrome_fmts[] = {
>> + MEDIA_BUS_FMT_Y8_1X8,
>> + MEDIA_BUS_FMT_Y10_1X10,
>> + MEDIA_BUS_FMT_Y12_1X12,
>> +};
>> +
>> struct mt9p031 {
>> struct v4l2_subdev subdev;
>> struct media_pad pad;
>> @@ -138,6 +150,9 @@ struct mt9p031 {
>> struct v4l2_ctrl *blc_auto;
>> struct v4l2_ctrl *blc_offset;
>>
>> + const u32 *fmts;
>> + int num_fmts;
>
> Unsigned int, please.
>
>> +
>> /* Registers cache */
>> u16 output_control;
>> u16 mode2;
>> @@ -148,6 +163,17 @@ static struct mt9p031 *to_mt9p031(struct v4l2_subdev *sd)
>> return container_of(sd, struct mt9p031, subdev);
>> }
>>
>> +static const u32 mt9p031_find_datafmt(struct mt9p031 *mt9p031, u32 code)
>> +{
>> + int i;
>
> Same here.
I'll fix it in v2.
Thanks,
Stefan
>
>> +
>> + for (i = 0; i < mt9p031->num_fmts; i++)
>> + if (mt9p031->fmts[i] == code)
>> + return mt9p031->fmts[i];
>> +
>> + return mt9p031->fmts[mt9p031->num_fmts-1];
>> +}
>> +
>> static int mt9p031_read(struct i2c_client *client, u8 reg)
>> {
>> return i2c_smbus_read_word_swapped(client, reg);
>> @@ -476,10 +502,11 @@ static int mt9p031_enum_mbus_code(struct v4l2_subdev *subdev,
>> {
>> struct mt9p031 *mt9p031 = to_mt9p031(subdev);
>>
>> - if (code->pad || code->index)
>> + if (code->pad || code->index >= mt9p031->num_fmts)
>> return -EINVAL;
>>
>> - code->code = mt9p031->format.code;
>> + code->code = mt9p031->fmts[code->index];
>> +
>> return 0;
>> }
>>
>> @@ -573,6 +600,8 @@ static int mt9p031_set_format(struct v4l2_subdev *subdev,
>> __format->width = __crop->width / hratio;
>> __format->height = __crop->height / vratio;
>>
>> + __format->code = mt9p031_find_datafmt(mt9p031, format->format.code);
>> +
>> format->format = *__format;
>>
>> return 0;
>> @@ -951,10 +980,7 @@ static int mt9p031_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
>>
>> format = v4l2_subdev_get_try_format(subdev, fh->pad, 0);
>>
>> - if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
>> - format->code = MEDIA_BUS_FMT_Y12_1X12;
>> - else
>> - format->code = MEDIA_BUS_FMT_SGRBG12_1X12;
>> + format->code = mt9p031_find_datafmt(mt9p031, 0);
>>
>> format->width = MT9P031_WINDOW_WIDTH_DEF;
>> format->height = MT9P031_WINDOW_HEIGHT_DEF;
>> @@ -1121,10 +1147,14 @@ static int mt9p031_probe(struct i2c_client *client,
>> mt9p031->crop.left = MT9P031_COLUMN_START_DEF;
>> mt9p031->crop.top = MT9P031_ROW_START_DEF;
>>
>> - if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
>> - mt9p031->format.code = MEDIA_BUS_FMT_Y12_1X12;
>> - else
>> - mt9p031->format.code = MEDIA_BUS_FMT_SGRBG12_1X12;
>> + if (mt9p031->model == MT9P031_MODEL_MONOCHROME) {
>> + mt9p031->fmts = mt9p031_monochrome_fmts;
>> + mt9p031->num_fmts = ARRAY_SIZE(mt9p031_monochrome_fmts);
>> + } else {
>> + mt9p031->fmts = mt9p031_color_fmts;
>> + mt9p031->num_fmts = ARRAY_SIZE(mt9p031_color_fmts);
>> + }
>> + mt9p031->format.code = mt9p031_find_datafmt(mt9p031, 0);
>>
>> mt9p031->format.width = MT9P031_WINDOW_WIDTH_DEF;
>> mt9p031->format.height = MT9P031_WINDOW_HEIGHT_DEF;
>