Re: [PATCH 4/4] media: mt9m111: allow to setup pixclk polarity
Hi Sakari,
On 18-10-20 00:24, Sakari Ailus wrote:
> Hi Marco,
>
> Thanks for the patchset.
>
> On Fri, Oct 19, 2018 at 05:50:27PM +0200, Marco Felsch wrote:
> > From: Enrico Scholz
> >
> > The chip can be configured to output data transitions on the
> > rising or falling edge of PIXCLK (Datasheet R58:1[9]), default is on the
> > falling edge.
> >
> > Parsing the fw-node is made in a subfunction to bundle all (future)
> > dt-parsing / fw-parsing stuff.
>
> Could you rebase this on current mediatree master, please?
Of course.
> >
> > Signed-off-by: Enrico Scholz
> > (m.grzesc...@pengutronix.de: Fix inverting clock. INV_PIX_CLOCK bit is set
> > per default. Set bit to 0 (enable mask bit without value) to enable
> > falling edge sampling.)
> > Signed-off-by: Michael Grzeschik
> > (m.fel...@pengutronix.de: use fwnode helpers)
> > (m.fel...@pengutronix.de: mv of parsing into own function)
> > (m.fel...@pengutronix.de: adapt commit msg)
> > Signed-off-by: Marco Felsch
> > Reviewed-by: Philipp Zabel
> > ---
> > drivers/media/i2c/mt9m111.c | 52 -
> > 1 file changed, 51 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > index 13080c6c1ba3..5d45bc9ea0cb 100644
> > --- a/drivers/media/i2c/mt9m111.c
> > +++ b/drivers/media/i2c/mt9m111.c
> > @@ -15,12 +15,14 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > #include
> > #include
> > #include
> > +#include
> >
> > /*
> > * MT9M111, MT9M112 and MT9M131:
> > @@ -236,6 +238,8 @@ struct mt9m111 {
> > const struct mt9m111_datafmt *fmt;
> > int lastpage; /* PageMap cache value */
> > bool is_streaming;
> > + /* user point of view - 0: falling 1: rising edge */
> > + unsigned int pclk_sample:1;
> > #ifdef CONFIG_MEDIA_CONTROLLER
> > struct media_pad pad;
> > #endif
> > @@ -586,6 +590,10 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
> > return -EINVAL;
> > }
> >
> > + /* receiver samples on falling edge, chip-hw default is rising */
>
> Could you add DT binding documentation that would cover this? The existing
> documentation is, well, rather vague. Which properties are relevant for the
> hardware, are they mandatory or optional and if they're optional, then are
> there relevant default values?
Okay, I will ad a dt-binding patch.
Regards,
Marco
> > + if (mt9m111->pclk_sample == 0)
> > + mask_outfmt2 |= MT9M111_OUTFMT_INV_PIX_CLOCK;
> > +
> > ret = mt9m111_reg_mask(client, context_a.output_fmt_ctrl2,
> >data_outfmt2, mask_outfmt2);
> > if (!ret)
> > @@ -1045,9 +1053,15 @@ static int mt9m111_s_stream(struct v4l2_subdev *sd,
> > int enable)
> > static int mt9m111_g_mbus_config(struct v4l2_subdev *sd,
> > struct v4l2_mbus_config *cfg)
> > {
> > - cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
> > + struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
> > +
> > + cfg->flags = V4L2_MBUS_MASTER |
> > V4L2_MBUS_HSYNC_ACTIVE_HIGH | V4L2_MBUS_VSYNC_ACTIVE_HIGH |
> > V4L2_MBUS_DATA_ACTIVE_HIGH;
> > +
> > + cfg->flags |= mt9m111->pclk_sample ? V4L2_MBUS_PCLK_SAMPLE_FALLING :
> > + V4L2_MBUS_PCLK_SAMPLE_RISING;
> > +
> > cfg->type = V4L2_MBUS_PARALLEL;
> >
> > return 0;
> > @@ -1117,6 +1131,33 @@ static int mt9m111_video_probe(struct i2c_client
> > *client)
> > return ret;
> > }
> >
> > +#ifdef CONFIG_OF
> > +static int mt9m111_probe_of(struct i2c_client *client, struct mt9m111
> > *mt9m111)
> > +{
> > + struct v4l2_fwnode_endpoint *bus_cfg;
> > + struct device_node *np;
> > + int ret = 0;
> > +
> > + np = of_graph_get_next_endpoint(client->dev.of_node, NULL);
> > + if (!np)
> > + return -EINVAL;
> > +
> > + bus_cfg = v4l2_fwnode_endpoint_alloc_parse(of_fwnode_handle(np));
> > + if (IS_ERR(bus_cfg)) {
> > + ret = PTR_ERR(bus_cfg);
> > + goto out_of_put;
> > + }
> > +
> > + mt9m111->pclk_sample = !!(bus_cfg->bus.parallel.flags &
> > + V4L2_MBUS_PCLK_SAMPLE_RISING);
> > +
> > + v4l2_fwnode_endpoint_free(bus_cfg);
> > +out_of_put:
> > + of_node_put(np);
> > + return ret;
> > +}
> > +#endif
> > +
> > static int mt9m111_probe(struct i2c_client *client,
> > const struct i2c_device_id *did)
> > {
> > @@ -1141,6 +1182,15 @@ static int mt9m111_probe(struct i2c_client *client,
> > /* Default HIGHPOWER context */
> > mt9m111->ctx = _b;
> >
> > + if (IS_ENABLED(CONFIG_OF)) {
> > + ret = mt9m111_probe_of(client, mt9m111);
> > + if (ret)
> > + return ret;
> > + } else {
> > + /* use default chip hardware values */
> > + mt9m111->pclk_sample = 1;
> > + }
> > +
> > v4l2_i2c_subdev_init(>subdev, client, _subdev_ops);
> >
Re: [PATCH 4/4] media: mt9m111: allow to setup pixclk polarity
Hi Marco,
Thanks for the patchset.
On Fri, Oct 19, 2018 at 05:50:27PM +0200, Marco Felsch wrote:
> From: Enrico Scholz
>
> The chip can be configured to output data transitions on the
> rising or falling edge of PIXCLK (Datasheet R58:1[9]), default is on the
> falling edge.
>
> Parsing the fw-node is made in a subfunction to bundle all (future)
> dt-parsing / fw-parsing stuff.
Could you rebase this on current mediatree master, please?
>
> Signed-off-by: Enrico Scholz
> (m.grzesc...@pengutronix.de: Fix inverting clock. INV_PIX_CLOCK bit is set
> per default. Set bit to 0 (enable mask bit without value) to enable
> falling edge sampling.)
> Signed-off-by: Michael Grzeschik
> (m.fel...@pengutronix.de: use fwnode helpers)
> (m.fel...@pengutronix.de: mv of parsing into own function)
> (m.fel...@pengutronix.de: adapt commit msg)
> Signed-off-by: Marco Felsch
> Reviewed-by: Philipp Zabel
> ---
> drivers/media/i2c/mt9m111.c | 52 -
> 1 file changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index 13080c6c1ba3..5d45bc9ea0cb 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -15,12 +15,14 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> #include
> #include
> #include
> +#include
>
> /*
> * MT9M111, MT9M112 and MT9M131:
> @@ -236,6 +238,8 @@ struct mt9m111 {
> const struct mt9m111_datafmt *fmt;
> int lastpage; /* PageMap cache value */
> bool is_streaming;
> + /* user point of view - 0: falling 1: rising edge */
> + unsigned int pclk_sample:1;
> #ifdef CONFIG_MEDIA_CONTROLLER
> struct media_pad pad;
> #endif
> @@ -586,6 +590,10 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
> return -EINVAL;
> }
>
> + /* receiver samples on falling edge, chip-hw default is rising */
Could you add DT binding documentation that would cover this? The existing
documentation is, well, rather vague. Which properties are relevant for the
hardware, are they mandatory or optional and if they're optional, then are
there relevant default values?
> + if (mt9m111->pclk_sample == 0)
> + mask_outfmt2 |= MT9M111_OUTFMT_INV_PIX_CLOCK;
> +
> ret = mt9m111_reg_mask(client, context_a.output_fmt_ctrl2,
> data_outfmt2, mask_outfmt2);
> if (!ret)
> @@ -1045,9 +1053,15 @@ static int mt9m111_s_stream(struct v4l2_subdev *sd,
> int enable)
> static int mt9m111_g_mbus_config(struct v4l2_subdev *sd,
> struct v4l2_mbus_config *cfg)
> {
> - cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
> + struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
> +
> + cfg->flags = V4L2_MBUS_MASTER |
> V4L2_MBUS_HSYNC_ACTIVE_HIGH | V4L2_MBUS_VSYNC_ACTIVE_HIGH |
> V4L2_MBUS_DATA_ACTIVE_HIGH;
> +
> + cfg->flags |= mt9m111->pclk_sample ? V4L2_MBUS_PCLK_SAMPLE_FALLING :
> + V4L2_MBUS_PCLK_SAMPLE_RISING;
> +
> cfg->type = V4L2_MBUS_PARALLEL;
>
> return 0;
> @@ -1117,6 +1131,33 @@ static int mt9m111_video_probe(struct i2c_client
> *client)
> return ret;
> }
>
> +#ifdef CONFIG_OF
> +static int mt9m111_probe_of(struct i2c_client *client, struct mt9m111
> *mt9m111)
> +{
> + struct v4l2_fwnode_endpoint *bus_cfg;
> + struct device_node *np;
> + int ret = 0;
> +
> + np = of_graph_get_next_endpoint(client->dev.of_node, NULL);
> + if (!np)
> + return -EINVAL;
> +
> + bus_cfg = v4l2_fwnode_endpoint_alloc_parse(of_fwnode_handle(np));
> + if (IS_ERR(bus_cfg)) {
> + ret = PTR_ERR(bus_cfg);
> + goto out_of_put;
> + }
> +
> + mt9m111->pclk_sample = !!(bus_cfg->bus.parallel.flags &
> + V4L2_MBUS_PCLK_SAMPLE_RISING);
> +
> + v4l2_fwnode_endpoint_free(bus_cfg);
> +out_of_put:
> + of_node_put(np);
> + return ret;
> +}
> +#endif
> +
> static int mt9m111_probe(struct i2c_client *client,
>const struct i2c_device_id *did)
> {
> @@ -1141,6 +1182,15 @@ static int mt9m111_probe(struct i2c_client *client,
> /* Default HIGHPOWER context */
> mt9m111->ctx = _b;
>
> + if (IS_ENABLED(CONFIG_OF)) {
> + ret = mt9m111_probe_of(client, mt9m111);
> + if (ret)
> + return ret;
> + } else {
> + /* use default chip hardware values */
> + mt9m111->pclk_sample = 1;
> + }
> +
> v4l2_i2c_subdev_init(>subdev, client, _subdev_ops);
> mt9m111->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>
--
Regards,
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH 4/4] media: mt9m111: allow to setup pixclk polarity
Hi Enrico,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.19-rc8 next-20181019]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Marco-Felsch/media-mt9m111-features/20181020-022716
base: git://linuxtv.org/media_tree.git master
config: x86_64-randconfig-x000-201841 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/media//i2c/mt9m111.c: In function 'mt9m111_probe':
>> drivers/media//i2c/mt9m111.c:1185:9: error: implicit declaration of function
>> 'mt9m111_probe_of'; did you mean 'mt9m111_probe'?
>> [-Werror=implicit-function-declaration]
ret = mt9m111_probe_of(client, mt9m111);
^~~~
mt9m111_probe
cc1: some warnings being treated as errors
vim +1185 drivers/media//i2c/mt9m111.c
1159
1160 static int mt9m111_probe(struct i2c_client *client,
1161 const struct i2c_device_id *did)
1162 {
1163 struct mt9m111 *mt9m111;
1164 struct i2c_adapter *adapter =
to_i2c_adapter(client->dev.parent);
1165 int ret;
1166
1167 if (!i2c_check_functionality(adapter,
I2C_FUNC_SMBUS_WORD_DATA)) {
1168 dev_warn(>dev,
1169 "I2C-Adapter doesn't support
I2C_FUNC_SMBUS_WORD\n");
1170 return -EIO;
1171 }
1172
1173 mt9m111 = devm_kzalloc(>dev, sizeof(struct mt9m111),
GFP_KERNEL);
1174 if (!mt9m111)
1175 return -ENOMEM;
1176
1177 mt9m111->clk = v4l2_clk_get(>dev, "mclk");
1178 if (IS_ERR(mt9m111->clk))
1179 return PTR_ERR(mt9m111->clk);
1180
1181 /* Default HIGHPOWER context */
1182 mt9m111->ctx = _b;
1183
1184 if (IS_ENABLED(CONFIG_OF)) {
> 1185 ret = mt9m111_probe_of(client, mt9m111);
1186 if (ret)
1187 return ret;
1188 } else {
1189 /* use default chip hardware values */
1190 mt9m111->pclk_sample = 1;
1191 }
1192
1193 v4l2_i2c_subdev_init(>subdev, client,
_subdev_ops);
1194 mt9m111->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
1195
1196 v4l2_ctrl_handler_init(>hdl, 5);
1197 v4l2_ctrl_new_std(>hdl, _ctrl_ops,
1198 V4L2_CID_VFLIP, 0, 1, 1, 0);
1199 v4l2_ctrl_new_std(>hdl, _ctrl_ops,
1200 V4L2_CID_HFLIP, 0, 1, 1, 0);
1201 v4l2_ctrl_new_std(>hdl, _ctrl_ops,
1202 V4L2_CID_AUTO_WHITE_BALANCE, 0, 1, 1, 1);
1203 mt9m111->gain = v4l2_ctrl_new_std(>hdl,
_ctrl_ops,
1204 V4L2_CID_GAIN, 0, 63 * 2 * 2, 1, 32);
1205 v4l2_ctrl_new_std_menu(>hdl,
1206 _ctrl_ops, V4L2_CID_EXPOSURE_AUTO, 1, 0,
1207 V4L2_EXPOSURE_AUTO);
1208 v4l2_ctrl_new_std_menu_items(>hdl,
1209 _ctrl_ops, V4L2_CID_TEST_PATTERN,
1210 ARRAY_SIZE(mt9m111_test_pattern_menu) - 1, 0, 0,
1211 mt9m111_test_pattern_menu);
1212 mt9m111->subdev.ctrl_handler = >hdl;
1213 if (mt9m111->hdl.error) {
1214 ret = mt9m111->hdl.error;
1215 goto out_clkput;
1216 }
1217
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
[PATCH 4/4] media: mt9m111: allow to setup pixclk polarity
From: Enrico Scholz
The chip can be configured to output data transitions on the
rising or falling edge of PIXCLK (Datasheet R58:1[9]), default is on the
falling edge.
Parsing the fw-node is made in a subfunction to bundle all (future)
dt-parsing / fw-parsing stuff.
Signed-off-by: Enrico Scholz
(m.grzesc...@pengutronix.de: Fix inverting clock. INV_PIX_CLOCK bit is set
per default. Set bit to 0 (enable mask bit without value) to enable
falling edge sampling.)
Signed-off-by: Michael Grzeschik
(m.fel...@pengutronix.de: use fwnode helpers)
(m.fel...@pengutronix.de: mv of parsing into own function)
(m.fel...@pengutronix.de: adapt commit msg)
Signed-off-by: Marco Felsch
Reviewed-by: Philipp Zabel
---
drivers/media/i2c/mt9m111.c | 52 -
1 file changed, 51 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index 13080c6c1ba3..5d45bc9ea0cb 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -15,12 +15,14 @@
#include
#include
#include
+#include
#include
#include
#include
#include
#include
+#include
/*
* MT9M111, MT9M112 and MT9M131:
@@ -236,6 +238,8 @@ struct mt9m111 {
const struct mt9m111_datafmt *fmt;
int lastpage; /* PageMap cache value */
bool is_streaming;
+ /* user point of view - 0: falling 1: rising edge */
+ unsigned int pclk_sample:1;
#ifdef CONFIG_MEDIA_CONTROLLER
struct media_pad pad;
#endif
@@ -586,6 +590,10 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
return -EINVAL;
}
+ /* receiver samples on falling edge, chip-hw default is rising */
+ if (mt9m111->pclk_sample == 0)
+ mask_outfmt2 |= MT9M111_OUTFMT_INV_PIX_CLOCK;
+
ret = mt9m111_reg_mask(client, context_a.output_fmt_ctrl2,
data_outfmt2, mask_outfmt2);
if (!ret)
@@ -1045,9 +1053,15 @@ static int mt9m111_s_stream(struct v4l2_subdev *sd, int
enable)
static int mt9m111_g_mbus_config(struct v4l2_subdev *sd,
struct v4l2_mbus_config *cfg)
{
- cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
+ struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
+
+ cfg->flags = V4L2_MBUS_MASTER |
V4L2_MBUS_HSYNC_ACTIVE_HIGH | V4L2_MBUS_VSYNC_ACTIVE_HIGH |
V4L2_MBUS_DATA_ACTIVE_HIGH;
+
+ cfg->flags |= mt9m111->pclk_sample ? V4L2_MBUS_PCLK_SAMPLE_FALLING :
+ V4L2_MBUS_PCLK_SAMPLE_RISING;
+
cfg->type = V4L2_MBUS_PARALLEL;
return 0;
@@ -1117,6 +1131,33 @@ static int mt9m111_video_probe(struct i2c_client *client)
return ret;
}
+#ifdef CONFIG_OF
+static int mt9m111_probe_of(struct i2c_client *client, struct mt9m111 *mt9m111)
+{
+ struct v4l2_fwnode_endpoint *bus_cfg;
+ struct device_node *np;
+ int ret = 0;
+
+ np = of_graph_get_next_endpoint(client->dev.of_node, NULL);
+ if (!np)
+ return -EINVAL;
+
+ bus_cfg = v4l2_fwnode_endpoint_alloc_parse(of_fwnode_handle(np));
+ if (IS_ERR(bus_cfg)) {
+ ret = PTR_ERR(bus_cfg);
+ goto out_of_put;
+ }
+
+ mt9m111->pclk_sample = !!(bus_cfg->bus.parallel.flags &
+ V4L2_MBUS_PCLK_SAMPLE_RISING);
+
+ v4l2_fwnode_endpoint_free(bus_cfg);
+out_of_put:
+ of_node_put(np);
+ return ret;
+}
+#endif
+
static int mt9m111_probe(struct i2c_client *client,
const struct i2c_device_id *did)
{
@@ -1141,6 +1182,15 @@ static int mt9m111_probe(struct i2c_client *client,
/* Default HIGHPOWER context */
mt9m111->ctx = _b;
+ if (IS_ENABLED(CONFIG_OF)) {
+ ret = mt9m111_probe_of(client, mt9m111);
+ if (ret)
+ return ret;
+ } else {
+ /* use default chip hardware values */
+ mt9m111->pclk_sample = 1;
+ }
+
v4l2_i2c_subdev_init(>subdev, client, _subdev_ops);
mt9m111->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
--
2.19.0
