subject:"\[PATCH\] omap3isp\: add support for CSI1 bus"

Re: [PATCH] omap3isp: add support for CSI1 bus

2017-03-02 Thread Pavel Machek

Hi!

> > >> +
> > >> +static int isp_of_parse_node_endpoint(struct device *dev,
> > >> +  struct device_node *node,
> > >> +  struct isp_async_subdev *isd)
> > >> +{
> > >> +struct isp_bus_cfg *buscfg;
> > >> +struct v4l2_of_endpoint vep;
> > >> +int ret;
> > >> +
> > >> +isd->bus = devm_kzalloc(dev, sizeof(*isd->bus), GFP_KERNEL);
> > > 
> > > Why do you now need to allocate this manually ?
> > 
> > bus is now a pointer.
> 
> I've seen that, but why have you changed it ?

subdev support. Needs to go into separate patch. Will be done shortly.

> > >> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> > >> @@ -160,6 +163,33 @@ static int ccp2_if_enable(struct isp_ccp2_device
> > >> *ccp2, u8 enable) return ret;
> > >> 
> > >>  }
> > >> 
> > >> +if (isp->revision == ISP_REVISION_2_0) {
> > > 
> > > The isp_csiphy.c code checks phy->isp->phy_type for the same purpose,
> > > shouldn't you use that too ?
> > 
> > Do you want me to do phy->isp->phy_type == ISP_PHY_TYPE_3430 check
> > here? Can do...
> 
> Yes that's what I meant.

Ok, that's something I can do.

But code is still somewhat "interesting". Code in omap3isp_csiphy_acquire()
assumes csi2, and I don't need most of it.. so I'll just not use it,
but it looks strange. I'll post new patch shortly.

Best regards,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH] omap3isp: add support for CSI1 bus

2017-02-19 Thread Laurent Pinchart

Hi Pavel,

On Wednesday 15 Feb 2017 10:42:29 Pavel Machek wrote:
> Hi!
> 
> >> diff --git a/drivers/media/platform/omap3isp/isp.c
> >> b/drivers/media/platform/omap3isp/isp.c index 0321d84..88bc7c6 100644
> >> --- a/drivers/media/platform/omap3isp/isp.c
> >> +++ b/drivers/media/platform/omap3isp/isp.c
> >> @@ -2024,21 +2024,92 @@ enum isp_of_phy {
> >>ISP_OF_PHY_CSIPHY2,
> >>  };
> >> 
> >> -static int isp_of_parse_node(struct device *dev, struct device_node
> >> *node,
> >> -   struct isp_async_subdev *isd)
> >> +void __isp_of_parse_node_csi1(struct device *dev,
> >> + struct isp_ccp2_cfg *buscfg,
> >> + struct v4l2_of_endpoint *vep)
> > 
> > This function isn't use anywhere else, you can merge it with
> > isp_of_parse_node_csi1().
> 
> I'd prefer not to. First, it will be used separately in future, and
> second, expresions would be uglier.

Where will it be used ? As for the uglier part, I don't agree, otherwise I 
wouldn't have proposed it.

> >> +{
> >> +  buscfg->lanecfg.clk.pos = vep->bus.mipi_csi1.clock_lane;
> >> +  buscfg->lanecfg.clk.pol =
> >> +  vep->bus.mipi_csi1.lane_polarity[0];
> >> +  dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> >> +  buscfg->lanecfg.clk.pol,
> >> +  buscfg->lanecfg.clk.pos);
> >> +
> >> +  buscfg->lanecfg.data[0].pos = vep->bus.mipi_csi2.data_lanes[0];
> >> +  buscfg->lanecfg.data[0].pol =
> >> +  vep->bus.mipi_csi2.lane_polarities[1];
> > 
> > bus.mipi_csi2 ?
> 
> Good catch. Fixed.
> 
> >> -  ret = v4l2_of_parse_endpoint(node, );
> >> -  if (ret)
> >> -  return ret;
> >> +  if (vep->base.port == ISP_OF_PHY_CSIPHY1)
> >> +  buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
> >> +  else
> >> +  buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
> > 
> > I would keep this code in the caller to avoid code duplication with
> > isp_of_parse_node_csi1().
> 
> Take a closer look. Code in _csi1 is different.
>
> >>break;
> >>
> >>default:
> >> +  return -1;
> > 
> > Please use the appropriate error code.
> 
> Ok.
> 
> >> +  return 0;
> >> +}
> >> +
> >> +static int isp_of_parse_node_endpoint(struct device *dev,
> >> +struct device_node *node,
> >> +struct isp_async_subdev *isd)
> >> +{
> >> +  struct isp_bus_cfg *buscfg;
> >> +  struct v4l2_of_endpoint vep;
> >> +  int ret;
> >> +
> >> +  isd->bus = devm_kzalloc(dev, sizeof(*isd->bus), GFP_KERNEL);
> > 
> > Why do you now need to allocate this manually ?
> 
> bus is now a pointer.

I've seen that, but why have you changed it ?

> >> +  dev_dbg(dev, "parsing endpoint %s, interface %u\n", node->full_name,
> >> +  vep.base.port);
> >> +
> >> +  if (isp_endpoint_to_buscfg(dev, vep, buscfg))
> > 
> > I'm fine splitting the CSI1/CSI2 parsing code to separate functions, but I
> > don't think there's a need to split isp_endpoint_to_buscfg(). You can keep
> > that part inline.
> 
> I'd prefer smaller functions here. I tried to read the original and it
> was not too easy.

This function became a kzalloc (which I still don't see why you need it), a 
call to v4l2_of_parse_endpoint(), and then isp_endpoint_to_buscfg(). That's 
too small to be a function of its own. Please merge 
isp_of_parse_node_endpoint() and isp_endpoint_to_buscfg().

> >> diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> >> b/drivers/media/platform/omap3isp/ispccp2.c index ca09523..4edb55a
> >> 100644
> >> --- a/drivers/media/platform/omap3isp/ispccp2.c
> >> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> >> @@ -160,6 +163,33 @@ static int ccp2_if_enable(struct isp_ccp2_device
> >> *ccp2, u8 enable) return ret;
> >> 
> >>}
> >> 
> >> +  if (isp->revision == ISP_REVISION_2_0) {
> > 
> > The isp_csiphy.c code checks phy->isp->phy_type for the same purpose,
> > shouldn't you use that too ?
> 
> Do you want me to do phy->isp->phy_type == ISP_PHY_TYPE_3430 check
> here? Can do...

Yes that's what I meant.

> >> +  buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;
> >> +
> >> +
> > 
> > One blank line is enough.
> 
> Ok.
> 
> >> +  if (enable) {
> >> +  csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ |
> >> +OMAP343X_CONTROL_CSIRXFE_RESET;
> >> +
> >> +  if (buscfg->phy_layer)
> >> +  csirxfe |= OMAP343X_CONTROL_CSIRXFE_SELFORM;
> >> +
> >> +  if (buscfg->strobe_clk_pol)
> >> +  csirxfe |= OMAP343X_CONTROL_CSIRXFE_CSIB_INV;
> >> +  } else
> >> +  csirxfe = 0;
> > 
> > You need curly braces for the else statement too.
> 
> Easy enough.
> 
> >> +
> >> +  regmap_write(isp->syscon, isp->syscon_offset, csirxfe);
> > 
> > Isn't this already configured by csiphy_routing_cfg_3430(), called through
> > omap3isp_csiphy_acquire() ? You'll need to add support for the
> >

Re: [PATCH] omap3isp: add support for CSI1 bus

2017-02-15 Thread Pavel Machek

On Wed 2017-02-15 17:57:46, Sebastian Reichel wrote:
> Hi,
> 
> On Wed, Feb 15, 2017 at 11:23:01AM +0100, Pavel Machek wrote:
> > It seems csiphy_routing_cfg_3430 is not called at all. I added
> > printks, but they don't trigger. If you have an idea what is going on
> > there, it would help...
> 
> You added printk to csiphy_routing_cfg_3630 instead of csiphy_routing_cfg_3430
> and N900 has OMAP3430. Function should be called when you start (or
> stop) using the camera:
> 
> csiphy_routing_cfg_3430(...)
> csiphy_routing_cfg(...)
> omap3isp_csiphy_config(...)
> omap3isp_csiphy_acquire(...) & omap3isp_csiphy_release(...)
> ccp2_s_stream(...)

Take another look, I believe I added printk to both of them.

Thanks for the expected backtrace, that should help figuring it out.

> -- Sebastian
> 
> > diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c 
> > b/drivers/media/platform/omap3isp/ispcsiphy.c
> > index 6b814e1..fe9303a 100644
> > --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> > +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> > @@ -30,6 +30,8 @@ static void csiphy_routing_cfg_3630(struct isp_csiphy 
> > *phy,
> > u32 reg;
> > u32 shift, mode;
> >  
> > +   printk("routing cfg 3630: iface %d, %d\n", iface, 
> > ISP_INTERFACE_CCP2B_PHY1);
> > +   
> > regmap_read(phy->isp->syscon, phy->isp->syscon_offset, );
> >  
> > switch (iface) {
> > @@ -74,6 +76,9 @@ static void csiphy_routing_cfg_3430(struct isp_csiphy 
> > *phy, u32 iface, bool on,
> > u32 csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ
> > | OMAP343X_CONTROL_CSIRXFE_RESET;
> >  
> > +   /* FIXME: can this be used instead of if (isp->revision) in ispccp2.c? 
> > */
> > +   
> > +   printk("routing cfg: iface %d, %d\n", iface, ISP_INTERFACE_CCP2B_PHY1);
> > /* Only the CCP2B on PHY1 is configurable. */
> > if (iface != ISP_INTERFACE_CCP2B_PHY1)
> > return;
> > @@ -105,6 +110,7 @@ static void csiphy_routing_cfg(struct isp_csiphy *phy,
> >enum isp_interface_type iface, bool on,
> >bool ccp2_strobe)
> >  {
> > +   printk("csiphy_routing_cfg\n");
> > if (phy->isp->phy_type == ISP_PHY_TYPE_3630 && on)
> > return csiphy_routing_cfg_3630(phy, iface, ccp2_strobe);
> > if (phy->isp->phy_type == ISP_PHY_TYPE_3430)



-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH] omap3isp: add support for CSI1 bus

2017-02-15 Thread Sebastian Reichel

Hi,

On Wed, Feb 15, 2017 at 11:23:01AM +0100, Pavel Machek wrote:
> It seems csiphy_routing_cfg_3430 is not called at all. I added
> printks, but they don't trigger. If you have an idea what is going on
> there, it would help...

You added printk to csiphy_routing_cfg_3630 instead of csiphy_routing_cfg_3430
and N900 has OMAP3430. Function should be called when you start (or
stop) using the camera:

csiphy_routing_cfg_3430(...)
csiphy_routing_cfg(...)
omap3isp_csiphy_config(...)
omap3isp_csiphy_acquire(...) & omap3isp_csiphy_release(...)
ccp2_s_stream(...)

-- Sebastian

> diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c 
> b/drivers/media/platform/omap3isp/ispcsiphy.c
> index 6b814e1..fe9303a 100644
> --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> @@ -30,6 +30,8 @@ static void csiphy_routing_cfg_3630(struct isp_csiphy *phy,
>   u32 reg;
>   u32 shift, mode;
>  
> + printk("routing cfg 3630: iface %d, %d\n", iface, 
> ISP_INTERFACE_CCP2B_PHY1);
> + 
>   regmap_read(phy->isp->syscon, phy->isp->syscon_offset, );
>  
>   switch (iface) {
> @@ -74,6 +76,9 @@ static void csiphy_routing_cfg_3430(struct isp_csiphy *phy, 
> u32 iface, bool on,
>   u32 csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ
>   | OMAP343X_CONTROL_CSIRXFE_RESET;
>  
> + /* FIXME: can this be used instead of if (isp->revision) in ispccp2.c? 
> */
> + 
> + printk("routing cfg: iface %d, %d\n", iface, ISP_INTERFACE_CCP2B_PHY1);
>   /* Only the CCP2B on PHY1 is configurable. */
>   if (iface != ISP_INTERFACE_CCP2B_PHY1)
>   return;
> @@ -105,6 +110,7 @@ static void csiphy_routing_cfg(struct isp_csiphy *phy,
>  enum isp_interface_type iface, bool on,
>  bool ccp2_strobe)
>  {
> + printk("csiphy_routing_cfg\n");
>   if (phy->isp->phy_type == ISP_PHY_TYPE_3630 && on)
>   return csiphy_routing_cfg_3630(phy, iface, ccp2_strobe);
>   if (phy->isp->phy_type == ISP_PHY_TYPE_3430)


signature.asc
Description: PGP signature

Re: [PATCH] omap3isp: add support for CSI1 bus

2017-02-15 Thread Pavel Machek

Hi!

> > +   if (enable) {
> > +   csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ |
> > + OMAP343X_CONTROL_CSIRXFE_RESET;
> > +
> > +   if (buscfg->phy_layer)
> > +   csirxfe |= OMAP343X_CONTROL_CSIRXFE_SELFORM;
> > +
> > +   if (buscfg->strobe_clk_pol)
> > +   csirxfe |= OMAP343X_CONTROL_CSIRXFE_CSIB_INV;
> > +   } else
> > +   csirxfe = 0;
> 
> You need curly braces for the else statement too.
> 
> > +
> > +   regmap_write(isp->syscon, isp->syscon_offset, csirxfe);
> 
> Isn't this already configured by csiphy_routing_cfg_3430(), called through 
> omap3isp_csiphy_acquire() ? You'll need to add support for the strobe/clock 
> polarity there, but the rest should already be handled.

It seems csiphy_routing_cfg_3430 is not called at all. I added
printks, but they don't trigger. If you have an idea what is going on
there, it would help...
Pavel

diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c 
b/drivers/media/platform/omap3isp/ispcsiphy.c
index 6b814e1..fe9303a 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -30,6 +30,8 @@ static void csiphy_routing_cfg_3630(struct isp_csiphy *phy,
u32 reg;
u32 shift, mode;
 
+   printk("routing cfg 3630: iface %d, %d\n", iface, 
ISP_INTERFACE_CCP2B_PHY1);
+   
regmap_read(phy->isp->syscon, phy->isp->syscon_offset, );
 
switch (iface) {
@@ -74,6 +76,9 @@ static void csiphy_routing_cfg_3430(struct isp_csiphy *phy, 
u32 iface, bool on,
u32 csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ
| OMAP343X_CONTROL_CSIRXFE_RESET;
 
+   /* FIXME: can this be used instead of if (isp->revision) in ispccp2.c? 
*/
+   
+   printk("routing cfg: iface %d, %d\n", iface, ISP_INTERFACE_CCP2B_PHY1);
/* Only the CCP2B on PHY1 is configurable. */
if (iface != ISP_INTERFACE_CCP2B_PHY1)
return;
@@ -105,6 +110,7 @@ static void csiphy_routing_cfg(struct isp_csiphy *phy,
   enum isp_interface_type iface, bool on,
   bool ccp2_strobe)
 {
+   printk("csiphy_routing_cfg\n");
if (phy->isp->phy_type == ISP_PHY_TYPE_3630 && on)
return csiphy_routing_cfg_3630(phy, iface, ccp2_strobe);
if (phy->isp->phy_type == ISP_PHY_TYPE_3430)



-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH] omap3isp: add support for CSI1 bus

2017-02-15 Thread Pavel Machek

Hi!

> > diff --git a/drivers/media/platform/omap3isp/isp.c
> > b/drivers/media/platform/omap3isp/isp.c index 0321d84..88bc7c6 100644
> > --- a/drivers/media/platform/omap3isp/isp.c
> > +++ b/drivers/media/platform/omap3isp/isp.c
> > @@ -2024,21 +2024,92 @@ enum isp_of_phy {
> > ISP_OF_PHY_CSIPHY2,
> >  };
> > 
> > -static int isp_of_parse_node(struct device *dev, struct device_node *node,
> > -struct isp_async_subdev *isd)
> > +void __isp_of_parse_node_csi1(struct device *dev,
> > +  struct isp_ccp2_cfg *buscfg,
> > +  struct v4l2_of_endpoint *vep)
> 
> This function isn't use anywhere else, you can merge it with 
> isp_of_parse_node_csi1().

I'd prefer not to. First, it will be used separately in future, and
second, expresions would be uglier.

> > +{
> > +   buscfg->lanecfg.clk.pos = vep->bus.mipi_csi1.clock_lane;
> > +   buscfg->lanecfg.clk.pol =
> > +   vep->bus.mipi_csi1.lane_polarity[0];
> > +   dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> > +   buscfg->lanecfg.clk.pol,
> > +   buscfg->lanecfg.clk.pos);
> > +
> > +   buscfg->lanecfg.data[0].pos = vep->bus.mipi_csi2.data_lanes[0];
> > +   buscfg->lanecfg.data[0].pol =
> > +   vep->bus.mipi_csi2.lane_polarities[1];
> 
> bus.mipi_csi2 ?

Good catch. Fixed.

> > -   ret = v4l2_of_parse_endpoint(node, );
> > -   if (ret)
> > -   return ret;
> > +   if (vep->base.port == ISP_OF_PHY_CSIPHY1)
> > +   buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
> > +   else
> > +   buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
> 
> I would keep this code in the caller to avoid code duplication with 
> isp_of_parse_node_csi1().

Take a closer look. Code in _csi1 is different.

> > break;
> > 
> > default:
> > +   return -1;
> 
> Please use the appropriate error code.

Ok.

> > +   return 0;
> > +}
> > +
> > +static int isp_of_parse_node_endpoint(struct device *dev,
> > + struct device_node *node,
> > + struct isp_async_subdev *isd)
> > +{
> > +   struct isp_bus_cfg *buscfg;
> > +   struct v4l2_of_endpoint vep;
> > +   int ret;
> > +
> > +   isd->bus = devm_kzalloc(dev, sizeof(*isd->bus), GFP_KERNEL);
> 
> Why do you now need to allocate this manually ?

bus is now a pointer.

> > +   dev_dbg(dev, "parsing endpoint %s, interface %u\n", node->full_name,
> > +   vep.base.port);
> > +
> > +   if (isp_endpoint_to_buscfg(dev, vep, buscfg))
> 
> I'm fine splitting the CSI1/CSI2 parsing code to separate functions, but I 
> don't think there's a need to split isp_endpoint_to_buscfg(). You can keep 
> that part inline.

I'd prefer smaller functions here. I tried to read the original and it
was not too easy.

> > diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> > b/drivers/media/platform/omap3isp/ispccp2.c index ca09523..4edb55a 100644
> > --- a/drivers/media/platform/omap3isp/ispccp2.c
> > +++ b/drivers/media/platform/omap3isp/ispccp2.c
> > @@ -160,6 +163,33 @@ static int ccp2_if_enable(struct isp_ccp2_device *ccp2,
> > u8 enable) return ret;
> > }
> > 
> > +   if (isp->revision == ISP_REVISION_2_0) {
> 
> The isp_csiphy.c code checks phy->isp->phy_type for the same purpose, 
> shouldn't you use that too ?

Do you want me to do phy->isp->phy_type == ISP_PHY_TYPE_3430 check
here? Can do...

> > +   buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;
> > +
> > +
> 
> One blank line is enough.

Ok.

> > +   if (enable) {
> > +   csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ |
> > + OMAP343X_CONTROL_CSIRXFE_RESET;
> > +
> > +   if (buscfg->phy_layer)
> > +   csirxfe |= OMAP343X_CONTROL_CSIRXFE_SELFORM;
> > +
> > +   if (buscfg->strobe_clk_pol)
> > +   csirxfe |= OMAP343X_CONTROL_CSIRXFE_CSIB_INV;
> > +   } else
> > +   csirxfe = 0;
> 
> You need curly braces for the else statement too.

Easy enough.

> > +
> > +   regmap_write(isp->syscon, isp->syscon_offset, csirxfe);
> 
> Isn't this already configured by csiphy_routing_cfg_3430(), called through 
> omap3isp_csiphy_acquire() ? You'll need to add support for the strobe/clock 
> polarity there, but the rest should already be handled.

Let me check...

> > @@ -69,11 +69,15 @@
> >   * @V4L2_MBUS_PARALLEL:parallel interface with hsync and vsync
> >   * @V4L2_MBUS_BT656:   parallel interface with embedded 
> > synchronisation, can
> >   * also be used for BT.1120
> > + * @V4L2_MBUS_CSI1:MIPI CSI-1 serial interface
> > + * @V4L2_MBUS_CCP2:CCP2 (Compact Camera Port 2)
> 
> It would help if you could provide, in comments or in the commit message, a 
> few pointers to information about CSI-1 and CCP2.

There's not much good information :-(.

Re: [PATCH] omap3isp: add support for CSI1 bus

2017-02-08 Thread Laurent Pinchart

Hi Pavel,

Thank you for the patch.

On Wednesday 08 Feb 2017 13:57:38 Pavel Machek wrote:
> Obtain the CSI1/CCP2 bus parameters from the OF node.
> 
> ISP CSI1 module needs all the bits correctly set to work.
> 
> OMAP3430 needs various syscon CONTROL_CSIRXFE bits set in order to
> operate. Implement the missing functionality.
> 
> Signed-off-by: Sakari Ailus 
> Signed-off-by: Ivaylo Dimitrov 
> Signed-off-by: Pavel Machek 
> 
> ---
> 
> > How about the rest? :-) I guess we could get the CCP2 support in omap3isp
> > without the video bus switch. It'd be nice to have this in a single
> > patchset.
> 
> Ok, here you go, what about this?
> 
>   Pavel
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index 0321d84..88bc7c6 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2024,21 +2024,92 @@ enum isp_of_phy {
>   ISP_OF_PHY_CSIPHY2,
>  };
> 
> -static int isp_of_parse_node(struct device *dev, struct device_node *node,
> -  struct isp_async_subdev *isd)
> +void __isp_of_parse_node_csi1(struct device *dev,
> +struct isp_ccp2_cfg *buscfg,
> +struct v4l2_of_endpoint *vep)

This function isn't use anywhere else, you can merge it with 
isp_of_parse_node_csi1().

> +{
> + buscfg->lanecfg.clk.pos = vep->bus.mipi_csi1.clock_lane;
> + buscfg->lanecfg.clk.pol =
> + vep->bus.mipi_csi1.lane_polarity[0];
> + dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> + buscfg->lanecfg.clk.pol,
> + buscfg->lanecfg.clk.pos);
> +
> + buscfg->lanecfg.data[0].pos = vep->bus.mipi_csi2.data_lanes[0];
> + buscfg->lanecfg.data[0].pol =
> + vep->bus.mipi_csi2.lane_polarities[1];

bus.mipi_csi2 ?

> + dev_dbg(dev, "data lane polarity %u, pos %u\n",
> + buscfg->lanecfg.data[0].pol,
> + buscfg->lanecfg.data[0].pos);
> +
> + buscfg->strobe_clk_pol = vep->bus.mipi_csi1.clock_inv;
> + buscfg->phy_layer = vep->bus.mipi_csi1.strobe;
> + buscfg->ccp2_mode = vep->bus_type == V4L2_MBUS_CCP2;
> +
> + dev_dbg(dev, "clock_inv %u strobe %u ccp2 %u\n",
> + buscfg->strobe_clk_pol,
> + buscfg->phy_layer,
> + buscfg->ccp2_mode);
> + /*
> +  * FIXME: now we assume the CRC is always there.
> +  * Implement a way to obtain this information from the
> +  * sensor. Frame descriptors, perhaps?
> +  */
> + buscfg->crc = 1;
> +
> + buscfg->vp_clk_pol = 1;
> +}
> +
> +static void isp_of_parse_node_csi1(struct device *dev,
> +struct isp_bus_cfg *buscfg,
> +struct v4l2_of_endpoint *vep)
> +{
> + if (vep->base.port == ISP_OF_PHY_CSIPHY1)
> + buscfg->interface = ISP_INTERFACE_CCP2B_PHY1;
> + else
> + buscfg->interface = ISP_INTERFACE_CCP2B_PHY2;
> + __isp_of_parse_node_csi1(dev, >bus.ccp2, vep);
> +}
> +
> +static void isp_of_parse_node_csi2(struct device *dev,
> +struct isp_bus_cfg *buscfg,
> +struct v4l2_of_endpoint *vep)
>  {
> - struct isp_bus_cfg *buscfg = >bus;
> - struct v4l2_of_endpoint vep;
>   unsigned int i;
> - int ret;
> 
> - ret = v4l2_of_parse_endpoint(node, );
> - if (ret)
> - return ret;
> + if (vep->base.port == ISP_OF_PHY_CSIPHY1)
> + buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
> + else
> + buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;

I would keep this code in the caller to avoid code duplication with 
isp_of_parse_node_csi1().

> + buscfg->bus.csi2.lanecfg.clk.pos = vep->bus.mipi_csi2.clock_lane;
> + buscfg->bus.csi2.lanecfg.clk.pol =
> + vep->bus.mipi_csi2.lane_polarities[0];
> + dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> + buscfg->bus.csi2.lanecfg.clk.pol,
> + buscfg->bus.csi2.lanecfg.clk.pos);
> +
> + for (i = 0; i < ISP_CSIPHY2_NUM_DATA_LANES; i++) {
> + buscfg->bus.csi2.lanecfg.data[i].pos =
> + vep->bus.mipi_csi2.data_lanes[i];
> + buscfg->bus.csi2.lanecfg.data[i].pol =
> + vep->bus.mipi_csi2.lane_polarities[i + 1];
> + dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
> + buscfg->bus.csi2.lanecfg.data[i].pol,
> + buscfg->bus.csi2.lanecfg.data[i].pos);
> + }
> 
> - dev_dbg(dev, "parsing endpoint %s, interface %u\n", node->full_name,
> - vep.base.port);
> + /*
> +  * FIXME: now we assume the CRC is always there.
> +  * Implement a way to obtain this information from the
> +  * sensor. Frame

Re: [PATCH] omap3isp: add support for CSI1 bus

2017-02-08 Thread kbuild test robot

Hi Pavel,

[auto build test WARNING on v4.9-rc8]
[cannot apply to linuxtv-media/master next-20170208]
[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/Pavel-Machek/omap3isp-add-support-for-CSI1-bus/20170208-49
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   make[3]: warning: jobserver unavailable: using -j1.  Add '+' to parent make 
rule.
   include/linux/init.h:1: warning: no structured comments found
   include/linux/workqueue.h:392: warning: No description found for parameter 
'...'
   include/linux/workqueue.h:392: warning: Excess function parameter 'args' 
description in 'alloc_workqueue'
   include/linux/workqueue.h:413: warning: No description found for parameter 
'...'
   include/linux/workqueue.h:413: warning: Excess function parameter 'args' 
description in 'alloc_ordered_workqueue'
   include/linux/kthread.h:26: warning: No description found for parameter '...'
   kernel/sys.c:1: warning: no structured comments found
   drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found
   include/linux/fence-array.h:61: warning: No description found for parameter 
'fence'
   include/sound/core.h:324: warning: No description found for parameter '...'
   include/sound/core.h:335: warning: No description found for parameter '...'
   include/sound/core.h:388: warning: No description found for parameter '...'
   include/media/media-entity.h:1054: warning: No description found for 
parameter '...'
>> include/media/v4l2-of.h:69: warning: No description found for parameter 
>> 'lane_polarity[2]'
   include/net/mac80211.h:3207: ERROR: Unexpected indentation.
   include/net/mac80211.h:3210: WARNING: Block quote ends without a blank line; 
unexpected unindent.
   include/net/mac80211.h:3212: ERROR: Unexpected indentation.
   include/net/mac80211.h:3213: WARNING: Block quote ends without a blank line; 
unexpected unindent.
   include/net/mac80211.h:1772: ERROR: Unexpected indentation.
   include/net/mac80211.h:1776: WARNING: Block quote ends without a blank line; 
unexpected unindent.
   kernel/sched/fair.c:7259: WARNING: Inline emphasis start-string without 
end-string.
   kernel/time/timer.c:1240: ERROR: Unexpected indentation.
   kernel/time/timer.c:1242: ERROR: Unexpected indentation.
   kernel/time/timer.c:1243: WARNING: Block quote ends without a blank line; 
unexpected unindent.
   include/linux/wait.h:121: WARNING: Block quote ends without a blank line; 
unexpected unindent.
   include/linux/wait.h:124: ERROR: Unexpected indentation.
   include/linux/wait.h:126: WARNING: Block quote ends without a blank line; 
unexpected unindent.
   kernel/time/hrtimer.c:1021: WARNING: Block quote ends without a blank line; 
unexpected unindent.
   kernel/signal.c:317: WARNING: Inline literal start-string without end-string.
   drivers/base/firmware_class.c:1348: WARNING: Bullet list ends without a 
blank line; unexpected unindent.
   drivers/message/fusion/mptbase.c:5054: WARNING: Definition list ends without 
a blank line; unexpected unindent.
   drivers/tty/serial/serial_core.c:1893: WARNING: Definition list ends without 
a blank line; unexpected unindent.
   include/linux/spi/spi.h:369: ERROR: Unexpected indentation.
   WARNING: dvipng command 'dvipng' cannot be run (needed for math display), 
check the imgmath_dvipng setting

vim +69 include/media/v4l2-of.h

53  };
54  
55  /**
56   * struct v4l2_of_bus_csi1 - CSI-1/CCP2 data bus structure
57   * @clock_inv: polarity of clock/strobe signal
58   * false - not inverted, true - inverted
59   * @strobe: false - data/clock, true - data/strobe
60   * @data_lane: the number of the data lane
61   * @clock_lane: the number of the clock lane
62   */
63  struct v4l2_of_bus_mipi_csi1 {
64  bool clock_inv;
65  bool strobe;
66  bool lane_polarity[2];
67  unsigned char data_lane;
68  unsigned char clock_lane;
  > 69  };
70  
71  /**
72   * struct v4l2_of_endpoint - the endpoint data structure
73   * @base: struct of_endpoint containing port, id, and local of_node
74   * @bus_type: bus type
75   * @bus: bus configuration data structure
76   * @link_frequencies: array of supported link frequencies
77   * @nr_of_link_frequencies: number of elements in link_frequenccies 
array

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

[PATCH] omap3isp: add support for CSI1 bus

2017-02-08 Thread Pavel Machek


Obtain the CSI1/CCP2 bus parameters from the OF node.

ISP CSI1 module needs all the bits correctly set to work.

OMAP3430 needs various syscon CONTROL_CSIRXFE bits set in order to
operate. Implement the missing functionality.

Signed-off-by: Sakari Ailus 
Signed-off-by: Ivaylo Dimitrov 
Signed-off-by: Pavel Machek 

---

> How about the rest? :-) I guess we could get the CCP2 support in omap3isp
> without the video bus switch. It'd be nice to have this in a single
> patchset.

Ok, here you go, what about this?

Pavel

diff --git a/drivers/media/platform/omap3isp/isp.c 
b/drivers/media/platform/omap3isp/isp.c
index 0321d84..88bc7c6 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2024,21 +2024,92 @@ enum isp_of_phy {
ISP_OF_PHY_CSIPHY2,
 };
 
-static int isp_of_parse_node(struct device *dev, struct device_node *node,
-struct isp_async_subdev *isd)
+void __isp_of_parse_node_csi1(struct device *dev,
+  struct isp_ccp2_cfg *buscfg,
+  struct v4l2_of_endpoint *vep)
+{
+   buscfg->lanecfg.clk.pos = vep->bus.mipi_csi1.clock_lane;
+   buscfg->lanecfg.clk.pol =
+   vep->bus.mipi_csi1.lane_polarity[0];
+   dev_dbg(dev, "clock lane polarity %u, pos %u\n",
+   buscfg->lanecfg.clk.pol,
+   buscfg->lanecfg.clk.pos);
+
+   buscfg->lanecfg.data[0].pos = vep->bus.mipi_csi2.data_lanes[0];
+   buscfg->lanecfg.data[0].pol =
+   vep->bus.mipi_csi2.lane_polarities[1];
+   dev_dbg(dev, "data lane polarity %u, pos %u\n",
+   buscfg->lanecfg.data[0].pol,
+   buscfg->lanecfg.data[0].pos);
+
+   buscfg->strobe_clk_pol = vep->bus.mipi_csi1.clock_inv;
+   buscfg->phy_layer = vep->bus.mipi_csi1.strobe;
+   buscfg->ccp2_mode = vep->bus_type == V4L2_MBUS_CCP2;
+
+   dev_dbg(dev, "clock_inv %u strobe %u ccp2 %u\n",
+   buscfg->strobe_clk_pol,
+   buscfg->phy_layer,
+   buscfg->ccp2_mode);
+   /*
+* FIXME: now we assume the CRC is always there.
+* Implement a way to obtain this information from the
+* sensor. Frame descriptors, perhaps?
+*/
+   buscfg->crc = 1;
+
+   buscfg->vp_clk_pol = 1;
+}
+
+static void isp_of_parse_node_csi1(struct device *dev,
+  struct isp_bus_cfg *buscfg,
+  struct v4l2_of_endpoint *vep)
+{
+   if (vep->base.port == ISP_OF_PHY_CSIPHY1)
+   buscfg->interface = ISP_INTERFACE_CCP2B_PHY1;
+   else
+   buscfg->interface = ISP_INTERFACE_CCP2B_PHY2;
+   __isp_of_parse_node_csi1(dev, >bus.ccp2, vep);
+}
+
+static void isp_of_parse_node_csi2(struct device *dev,
+  struct isp_bus_cfg *buscfg,
+  struct v4l2_of_endpoint *vep)
 {
-   struct isp_bus_cfg *buscfg = >bus;
-   struct v4l2_of_endpoint vep;
unsigned int i;
-   int ret;
 
-   ret = v4l2_of_parse_endpoint(node, );
-   if (ret)
-   return ret;
+   if (vep->base.port == ISP_OF_PHY_CSIPHY1)
+   buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
+   else
+   buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
+   buscfg->bus.csi2.lanecfg.clk.pos = vep->bus.mipi_csi2.clock_lane;
+   buscfg->bus.csi2.lanecfg.clk.pol =
+   vep->bus.mipi_csi2.lane_polarities[0];
+   dev_dbg(dev, "clock lane polarity %u, pos %u\n",
+   buscfg->bus.csi2.lanecfg.clk.pol,
+   buscfg->bus.csi2.lanecfg.clk.pos);
+
+   for (i = 0; i < ISP_CSIPHY2_NUM_DATA_LANES; i++) {
+   buscfg->bus.csi2.lanecfg.data[i].pos =
+   vep->bus.mipi_csi2.data_lanes[i];
+   buscfg->bus.csi2.lanecfg.data[i].pol =
+   vep->bus.mipi_csi2.lane_polarities[i + 1];
+   dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
+   buscfg->bus.csi2.lanecfg.data[i].pol,
+   buscfg->bus.csi2.lanecfg.data[i].pos);
+   }
 
-   dev_dbg(dev, "parsing endpoint %s, interface %u\n", node->full_name,
-   vep.base.port);
+   /*
+* FIXME: now we assume the CRC is always there.
+* Implement a way to obtain this information from the
+* sensor. Frame descriptors, perhaps?
+*/
+   buscfg->bus.csi2.crc = 1;
+}
 
+static int isp_endpoint_to_buscfg(struct device *dev,
+ struct v4l2_of_endpoint vep,
+ struct isp_bus_cfg *buscfg)
+{
switch (vep.base.port) {
case ISP_OF_PHY_PARALLEL:
buscfg->interface = ISP_INTERFACE_PARALLEL;
@@

Re: [PATCH] omap3isp: add support for CSI1 bus

Re: [PATCH] omap3isp: add support for CSI1 bus

Re: [PATCH] omap3isp: add support for CSI1 bus

Re: [PATCH] omap3isp: add support for CSI1 bus

Re: [PATCH] omap3isp: add support for CSI1 bus

Re: [PATCH] omap3isp: add support for CSI1 bus

Re: [PATCH] omap3isp: add support for CSI1 bus

Re: [PATCH] omap3isp: add support for CSI1 bus

[PATCH] omap3isp: add support for CSI1 bus

9 matches

Site Navigation

Mail list logo

Footer information