| Message ID | 20181127100253.30845-1-m.felsch@pengutronix.de |
|---|---|
| Headers | show |
| Series | media: mt9m111 features | expand |
Hi Sakari, On Tue, 2018-11-27 at 15:19 +0200, Sakari Ailus wrote: > Hi Marco, > > On Tue, Nov 27, 2018 at 11:02:53AM +0100, Marco Felsch wrote: > > From: Enrico Scholz <enrico.scholz@sigma-chemnitz.de> > > > > 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 <enrico.scholz@sigma-chemnitz.de> > > (m.grzeschik@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.grzeschik@pengutronix.de> > > (m.felsch@pengutronix.de: use fwnode helpers) > > (m.felsch@pengutronix.de: mv fw parsing into own function) > > (m.felsch@pengutronix.de: adapt commit msg) > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > Applied with the following diff: > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c > index 2ef332b9b914..b6011bfddde8 100644 > --- a/drivers/media/i2c/mt9m111.c > +++ b/drivers/media/i2c/mt9m111.c > @@ -1172,24 +1172,24 @@ static int mt9m111_video_probe(struct i2c_client *client) > > static int mt9m111_probe_fw(struct i2c_client *client, struct mt9m111 *mt9m111) > { > - struct v4l2_fwnode_endpoint *bus_cfg; > + struct v4l2_fwnode_endpoint bus_cfg = { > + .bus_type = V4L2_MBUS_PARALLEL > + }; > struct fwnode_handle *np; > - int ret = 0; > + int ret; > > np = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL); > if (!np) > return -EINVAL; > > - bus_cfg = v4l2_fwnode_endpoint_alloc_parse(np); > - if (IS_ERR(bus_cfg)) { > - ret = PTR_ERR(bus_cfg); > + ret = v4l2_fwnode_endpoint_alloc_parse(np, &bus_cfg); Should that be + ret = v4l2_fwnode_endpoint_parse(np, &bus_cfg); intead? > + if (ret) > goto out_put_fw; > - } > > - mt9m111->pclk_sample = !!(bus_cfg->bus.parallel.flags & > + mt9m111->pclk_sample = !!(bus_cfg.bus.parallel.flags & > V4L2_MBUS_PCLK_SAMPLE_RISING); > > - v4l2_fwnode_endpoint_free(bus_cfg); > + v4l2_fwnode_endpoint_free(&bus_cfg); > > out_put_fw: > fwnode_handle_put(np); > > Please base on current media tree master on the next time. Thanks. regards Philipp
Hi Sakari, On 18-11-27 15:19, Sakari Ailus wrote: > Hi Marco, > > On Tue, Nov 27, 2018 at 11:02:53AM +0100, Marco Felsch wrote: > > From: Enrico Scholz <enrico.scholz@sigma-chemnitz.de> > > > > 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 <enrico.scholz@sigma-chemnitz.de> > > (m.grzeschik@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.grzeschik@pengutronix.de> > > (m.felsch@pengutronix.de: use fwnode helpers) > > (m.felsch@pengutronix.de: mv fw parsing into own function) > > (m.felsch@pengutronix.de: adapt commit msg) > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > Applied with the following diff: > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c > index 2ef332b9b914..b6011bfddde8 100644 > --- a/drivers/media/i2c/mt9m111.c > +++ b/drivers/media/i2c/mt9m111.c > @@ -1172,24 +1172,24 @@ static int mt9m111_video_probe(struct i2c_client *client) > > static int mt9m111_probe_fw(struct i2c_client *client, struct mt9m111 *mt9m111) > { > - struct v4l2_fwnode_endpoint *bus_cfg; > + struct v4l2_fwnode_endpoint bus_cfg = { > + .bus_type = V4L2_MBUS_PARALLEL > + }; > struct fwnode_handle *np; > - int ret = 0; > + int ret; > > np = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL); > if (!np) > return -EINVAL; > > - bus_cfg = v4l2_fwnode_endpoint_alloc_parse(np); > - if (IS_ERR(bus_cfg)) { > - ret = PTR_ERR(bus_cfg); > + ret = v4l2_fwnode_endpoint_alloc_parse(np, &bus_cfg); > + if (ret) > goto out_put_fw; > - } > > - mt9m111->pclk_sample = !!(bus_cfg->bus.parallel.flags & > + mt9m111->pclk_sample = !!(bus_cfg.bus.parallel.flags & > V4L2_MBUS_PCLK_SAMPLE_RISING); > > - v4l2_fwnode_endpoint_free(bus_cfg); > + v4l2_fwnode_endpoint_free(&bus_cfg); > > out_put_fw: > fwnode_handle_put(np); > > Please base on current media tree master on the next time. Thanks. Sorry, thanks for the inline fix :) Kind regards, Marco > -- > Kind regards, > > Sakari Ailus > sakari.ailus@linux.intel.com
Hi Sakari, On Tue, 2018-11-27 at 15:50 +0200, Sakari Ailus wrote: > Hi Philipp, > > On Tue, Nov 27, 2018 at 02:39:27PM +0100, Philipp Zabel wrote: > > Hi Sakari, > > > > On Tue, 2018-11-27 at 15:19 +0200, Sakari Ailus wrote: > > > Hi Marco, > > > > > > On Tue, Nov 27, 2018 at 11:02:53AM +0100, Marco Felsch wrote: > > > > From: Enrico Scholz <enrico.scholz@sigma-chemnitz.de> > > > > > > > > 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 <enrico.scholz@sigma-chemnitz.de> > > > > (m.grzeschik@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.grzeschik@pengutronix.de> > > > > (m.felsch@pengutronix.de: use fwnode helpers) > > > > (m.felsch@pengutronix.de: mv fw parsing into own function) > > > > (m.felsch@pengutronix.de: adapt commit msg) > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > > > > > Applied with the following diff: > > > > > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c > > > index 2ef332b9b914..b6011bfddde8 100644 > > > --- a/drivers/media/i2c/mt9m111.c > > > +++ b/drivers/media/i2c/mt9m111.c > > > @@ -1172,24 +1172,24 @@ static int mt9m111_video_probe(struct i2c_client *client) > > > > > > static int mt9m111_probe_fw(struct i2c_client *client, struct mt9m111 *mt9m111) > > > { > > > - struct v4l2_fwnode_endpoint *bus_cfg; > > > + struct v4l2_fwnode_endpoint bus_cfg = { > > > + .bus_type = V4L2_MBUS_PARALLEL > > > + }; > > > struct fwnode_handle *np; > > > - int ret = 0; > > > + int ret; > > > > > > np = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL); > > > if (!np) > > > return -EINVAL; > > > > > > - bus_cfg = v4l2_fwnode_endpoint_alloc_parse(np); > > > - if (IS_ERR(bus_cfg)) { > > > - ret = PTR_ERR(bus_cfg); > > > + ret = v4l2_fwnode_endpoint_alloc_parse(np, &bus_cfg); > > > > Should that be > > > > + ret = v4l2_fwnode_endpoint_parse(np, &bus_cfg); > > > > intead? > > Could be. I'd expect the driver to need the link frequency at some point > after which you'd need the variable size properties anyway. But that's not > the case now. I don't think the link-frequencies property will be used, this is just a parallel device. But Marco chose to use _alloc_parse because of what the v4l2_fwnode_endpoint_parse() documentation says: /* * NOTE: This function does not parse properties the size of which is variable * without a low fixed limit. Please use v4l2_fwnode_endpoint_alloc_parse() in * new drivers instead. */ So maybe we want to use v4l2_fwnode_endpoint_alloc_parse() always. There is no unnecessary allocation, just a lookup of the non-existing link- frequencies property. regards Philipp
On Tue, Nov 27, 2018 at 03:12:29PM +0100, Philipp Zabel wrote: > Hi Sakari, > > On Tue, 2018-11-27 at 15:50 +0200, Sakari Ailus wrote: > > Hi Philipp, > > > > On Tue, Nov 27, 2018 at 02:39:27PM +0100, Philipp Zabel wrote: > > > Hi Sakari, > > > > > > On Tue, 2018-11-27 at 15:19 +0200, Sakari Ailus wrote: > > > > Hi Marco, > > > > > > > > On Tue, Nov 27, 2018 at 11:02:53AM +0100, Marco Felsch wrote: > > > > > From: Enrico Scholz <enrico.scholz@sigma-chemnitz.de> > > > > > > > > > > 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 <enrico.scholz@sigma-chemnitz.de> > > > > > (m.grzeschik@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.grzeschik@pengutronix.de> > > > > > (m.felsch@pengutronix.de: use fwnode helpers) > > > > > (m.felsch@pengutronix.de: mv fw parsing into own function) > > > > > (m.felsch@pengutronix.de: adapt commit msg) > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > > > > > > > Applied with the following diff: > > > > > > > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c > > > > index 2ef332b9b914..b6011bfddde8 100644 > > > > --- a/drivers/media/i2c/mt9m111.c > > > > +++ b/drivers/media/i2c/mt9m111.c > > > > @@ -1172,24 +1172,24 @@ static int mt9m111_video_probe(struct i2c_client *client) > > > > > > > > static int mt9m111_probe_fw(struct i2c_client *client, struct mt9m111 *mt9m111) > > > > { > > > > - struct v4l2_fwnode_endpoint *bus_cfg; > > > > + struct v4l2_fwnode_endpoint bus_cfg = { > > > > + .bus_type = V4L2_MBUS_PARALLEL > > > > + }; > > > > struct fwnode_handle *np; > > > > - int ret = 0; > > > > + int ret; > > > > > > > > np = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL); > > > > if (!np) > > > > return -EINVAL; > > > > > > > > - bus_cfg = v4l2_fwnode_endpoint_alloc_parse(np); > > > > - if (IS_ERR(bus_cfg)) { > > > > - ret = PTR_ERR(bus_cfg); > > > > + ret = v4l2_fwnode_endpoint_alloc_parse(np, &bus_cfg); > > > > > > Should that be > > > > > > + ret = v4l2_fwnode_endpoint_parse(np, &bus_cfg); > > > > > > intead? > > > > Could be. I'd expect the driver to need the link frequency at some point > > after which you'd need the variable size properties anyway. But that's not > > the case now. > > I don't think the link-frequencies property will be used, this is just a > parallel device. But Marco chose to use _alloc_parse because of what the > v4l2_fwnode_endpoint_parse() documentation says: > > /* > * NOTE: This function does not parse properties the size of which is variable > * without a low fixed limit. Please use v4l2_fwnode_endpoint_alloc_parse() in > * new drivers instead. > */ > > So maybe we want to use v4l2_fwnode_endpoint_alloc_parse() always. There > is no unnecessary allocation, just a lookup of the non-existing link- > frequencies property. There could be other properties in the future. When I wrote that, I guess I ignored that the link frequency might not be relevant for some devices such as CSI-2 receivers. I think it'd make sense to remove the latter sentence; I can send a patch. The first sentence that tells the limitations of the function is enough IMO.
On 18-11-27 23:15, Sakari Ailus wrote: > On Tue, Nov 27, 2018 at 11:02:53AM +0100, Marco Felsch wrote: > > From: Enrico Scholz <enrico.scholz@sigma-chemnitz.de> > > > > 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 <enrico.scholz@sigma-chemnitz.de> > > (m.grzeschik@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.grzeschik@pengutronix.de> > > (m.felsch@pengutronix.de: use fwnode helpers) > > (m.felsch@pengutronix.de: mv fw parsing into own function) > > (m.felsch@pengutronix.de: adapt commit msg) > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > This one as well: Sorry for that, I forget to adapt the Kconfig to often. Thanks for your fix. Kind regards, Marco > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > index 005fc2bd0d05..902c3cabf44c 100644 > --- a/drivers/media/i2c/Kconfig > +++ b/drivers/media/i2c/Kconfig > @@ -859,6 +859,7 @@ config VIDEO_MT9M032 > config VIDEO_MT9M111 > tristate "mt9m111, mt9m112 and mt9m131 support" > depends on I2C && VIDEO_V4L2 > + select V4L2_FWNODE > help > This driver supports MT9M111, MT9M112 and MT9M131 cameras from > Micron/Aptina > > -- > Sakari Ailus > sakari.ailus@linux.intel.com >
On Wed, Nov 28, 2018 at 09:29:01AM +0100, Marco Felsch wrote: > On 18-11-27 23:15, Sakari Ailus wrote: > > On Tue, Nov 27, 2018 at 11:02:53AM +0100, Marco Felsch wrote: > > > From: Enrico Scholz <enrico.scholz@sigma-chemnitz.de> > > > > > > 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 <enrico.scholz@sigma-chemnitz.de> > > > (m.grzeschik@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.grzeschik@pengutronix.de> > > > (m.felsch@pengutronix.de: use fwnode helpers) > > > (m.felsch@pengutronix.de: mv fw parsing into own function) > > > (m.felsch@pengutronix.de: adapt commit msg) > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > > > This one as well: > > Sorry for that, I forget to adapt the Kconfig to often. Thanks for your > fix. No worries. I hope we'll have automated compile testing in not too distant future...
Hi, this v3 integrate the review of my v2 [1]. I reordered the series as mentioned by Sakari. The patches are rebased on top of the actual media-tree/master. [1] https://www.mail-archive.com/linux-media@vger.kernel.org/msg135932.html Regards, Marco Enrico Scholz (1): media: mt9m111: allow to setup pixclk polarity Marco Felsch (3): media: mt9m111: add s_stream callback dt-bindings: media: mt9m111: adapt documentation to be more clear dt-bindings: media: mt9m111: add pclk-sample property Michael Grzeschik (2): media: mt9m111: add streaming check to set_fmt media: mt9m111: add support to select formats and fps for {Q,SXGA} .../devicetree/bindings/media/i2c/mt9m111.txt | 13 +- drivers/media/i2c/mt9m111.c | 224 +++++++++++++++++- 2 files changed, 232 insertions(+), 5 deletions(-)