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
Hi!
> > > >> +++ 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.
Ok, how about this one?
Pavel
omap3isp: add rest of CSI1 support
CSI1 needs one more bit to be set up. Do just that.
It is not as straightforward as I'd like, see the comments in the code
for explanation.
Signed-off-by: Pavel Machek <[email protected]>
index ca09523..b6e055e 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -21,6 +23,7 @@
#include <linux/mutex.h>
#include <linux/uaccess.h>
#include <linux/regulator/consumer.h>
+#include <linux/regmap.h>
#include "isp.h"
#include "ispreg.h"
@@ -160,6 +163,22 @@ static int ccp2_if_enable(struct isp_ccp2_device *ccp2, u8 enable)
return ret;
}
+ if (isp->phy_type == ISP_PHY_TYPE_3430) {
+ struct media_pad *pad;
+ struct v4l2_subdev *sensor;
+ const struct isp_ccp2_cfg *buscfg;
+
+ pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
+ sensor = media_entity_to_v4l2_subdev(pad->entity);
+ /* Struct isp_bus_cfg has union inside */
+ buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;
+
+ csiphy_routing_cfg_3430(&isp->isp_csiphy2,
+ ISP_INTERFACE_CCP2B_PHY1,
+ enable, !!buscfg->phy_layer,
+ buscfg->strobe_clk_pol);
+ }
+
/* Enable/Disable all the LCx channels */
for (i = 0; i < CCP2_LCx_CHANS_NUM; i++)
isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_LCx_CTRL(i),
@@ -1137,10 +1159,19 @@ int omap3isp_ccp2_init(struct isp_device *isp)
if (isp->revision == ISP_REVISION_2_0) {
ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
if (IS_ERR(ccp2->vdds_csib)) {
+ if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
dev_dbg(isp->dev,
"Could not get regulator vdds_csib\n");
ccp2->vdds_csib = NULL;
}
+ /*
+ * If we set up ccp2->phy here,
+ * omap3isp_csiphy_acquire() will go ahead and assume
+ * csi2, dereferencing some null pointers.
+ *
+ * ccp2->phy = &isp->isp_csiphy2;
+ */
} else if (isp->revision == ISP_REVISION_15_0) {
ccp2->phy = &isp->isp_csiphy1;
}
diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
index 871d4fe..897097b 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -68,8 +68,8 @@ static void csiphy_routing_cfg_3630(struct isp_csiphy *phy,
regmap_write(phy->isp->syscon, phy->isp->syscon_offset, reg);
}
-static void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
- bool ccp2_strobe)
+void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
+ bool ccp2_strobe, bool strobe_clk_pol)
{
u32 csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ
| OMAP343X_CONTROL_CSIRXFE_RESET;
@@ -85,6 +85,9 @@ static void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
if (ccp2_strobe)
csirxfe |= OMAP343X_CONTROL_CSIRXFE_SELFORM;
+
+ if (strobe_clk_pol)
+ csirxfe |= OMAP343X_CONTROL_CSIRXFE_CSIB_INV;
regmap_write(phy->isp->syscon, phy->isp->syscon_offset, csirxfe);
}
@@ -108,7 +111,7 @@ static void csiphy_routing_cfg(struct isp_csiphy *phy,
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)
- return csiphy_routing_cfg_3430(phy, iface, on, ccp2_strobe);
+ return csiphy_routing_cfg_3430(phy, iface, on, ccp2_strobe, false);
}
/*
diff --git a/drivers/media/platform/omap3isp/ispcsiphy.h b/drivers/media/platform/omap3isp/ispcsiphy.h
index 28b63b2..88c5c1e8 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.h
+++ b/drivers/media/platform/omap3isp/ispcsiphy.h
@@ -40,4 +40,7 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy);
void omap3isp_csiphy_release(struct isp_csiphy *phy);
int omap3isp_csiphy_init(struct isp_device *isp);
+void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
+ bool ccp2_strobe, bool strobe_clk_pol);
+
#endif /* OMAP3_ISP_CSI_PHY_H */
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi!
> > Ok, how about this one?
> > omap3isp: add rest of CSI1 support
> >
> > CSI1 needs one more bit to be set up. Do just that.
> >
> > It is not as straightforward as I'd like, see the comments in the code
> > for explanation.
...
> > + if (isp->phy_type == ISP_PHY_TYPE_3430) {
> > + struct media_pad *pad;
> > + struct v4l2_subdev *sensor;
> > + const struct isp_ccp2_cfg *buscfg;
> > +
> > + pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
> > + sensor = media_entity_to_v4l2_subdev(pad->entity);
> > + /* Struct isp_bus_cfg has union inside */
> > + buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;
> > +
> > + csiphy_routing_cfg_3430(&isp->isp_csiphy2,
> > + ISP_INTERFACE_CCP2B_PHY1,
> > + enable, !!buscfg->phy_layer,
> > + buscfg->strobe_clk_pol);
>
> You should do this through omap3isp_csiphy_acquire(), and not call
> csiphy_routing_cfg_3430() directly from here.
Well, unfortunately omap3isp_csiphy_acquire() does have csi2
assumptions hard-coded :-(.
This will probably fail.
rval = omap3isp_csi2_reset(phy->csi2);
if (rval < 0)
goto done;
And this will oops:
static int omap3isp_csiphy_config(struct isp_csiphy *phy)
{
struct isp_csi2_device *csi2 = phy->csi2;
struct isp_pipeline *pipe = to_isp_pipeline(&csi2->subdev.entity);
struct isp_bus_cfg *buscfg = pipe->external->host_priv;
> > @@ -1137,10 +1159,19 @@ int omap3isp_ccp2_init(struct isp_device *isp)
> > if (isp->revision == ISP_REVISION_2_0) {
> > ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
> > if (IS_ERR(ccp2->vdds_csib)) {
> > + if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER)
> > + return -EPROBE_DEFER;
>
> This should go to a separate patch.
Ok, easy enough.
> > dev_dbg(isp->dev,
> > "Could not get regulator vdds_csib\n");
> > ccp2->vdds_csib = NULL;
> > }
> > + /*
> > + * If we set up ccp2->phy here,
> > + * omap3isp_csiphy_acquire() will go ahead and assume
> > + * csi2, dereferencing some null pointers.
> > + *
> > + * ccp2->phy = &isp->isp_csiphy2;
>
> That needs to be fixed separately.
See analysis above. Yes, it would be nice to fix it. Can you provide
some hints how to do that? Maybe even patch to test? :-).
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
If regulator returns -EPROBE_DEFER, we need to return it too, so that
omap3isp will be re-probed when regulator is ready.
Signed-off-by: Pavel Machek <[email protected]>
diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
index ca09523..b6e055e 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -1137,10 +1159,12 @@ int omap3isp_ccp2_init(struct isp_device *isp)
if (isp->revision == ISP_REVISION_2_0) {
ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
if (IS_ERR(ccp2->vdds_csib)) {
+ if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
dev_dbg(isp->dev,
"Could not get regulator vdds_csib\n");
ccp2->vdds_csib = NULL;
}
} else if (isp->revision == ISP_REVISION_15_0) {
ccp2->phy = &isp->isp_csiphy1;
}
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi Pavel,
On Thu, Mar 02, 2017 at 11:16:04AM +0100, Pavel Machek wrote:
> Hi!
>
> > > > >> +++ 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.
>
> Ok, how about this one?
>
> Pavel
>
> omap3isp: add rest of CSI1 support
>
> CSI1 needs one more bit to be set up. Do just that.
>
> It is not as straightforward as I'd like, see the comments in the code
> for explanation.
>
> Signed-off-by: Pavel Machek <[email protected]>
>
> index ca09523..b6e055e 100644
> --- a/drivers/media/platform/omap3isp/ispccp2.c
> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> @@ -21,6 +23,7 @@
> #include <linux/mutex.h>
> #include <linux/uaccess.h>
> #include <linux/regulator/consumer.h>
> +#include <linux/regmap.h>
>
> #include "isp.h"
> #include "ispreg.h"
> @@ -160,6 +163,22 @@ static int ccp2_if_enable(struct isp_ccp2_device *ccp2, u8 enable)
> return ret;
> }
>
> + if (isp->phy_type == ISP_PHY_TYPE_3430) {
> + struct media_pad *pad;
> + struct v4l2_subdev *sensor;
> + const struct isp_ccp2_cfg *buscfg;
> +
> + pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
> + sensor = media_entity_to_v4l2_subdev(pad->entity);
> + /* Struct isp_bus_cfg has union inside */
> + buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;
> +
> + csiphy_routing_cfg_3430(&isp->isp_csiphy2,
> + ISP_INTERFACE_CCP2B_PHY1,
> + enable, !!buscfg->phy_layer,
> + buscfg->strobe_clk_pol);
You should do this through omap3isp_csiphy_acquire(), and not call
csiphy_routing_cfg_3430() directly from here.
> + }
> +
> /* Enable/Disable all the LCx channels */
> for (i = 0; i < CCP2_LCx_CHANS_NUM; i++)
> isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_LCx_CTRL(i),
> @@ -1137,10 +1159,19 @@ int omap3isp_ccp2_init(struct isp_device *isp)
> if (isp->revision == ISP_REVISION_2_0) {
> ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
> if (IS_ERR(ccp2->vdds_csib)) {
> + if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
This should go to a separate patch.
> dev_dbg(isp->dev,
> "Could not get regulator vdds_csib\n");
> ccp2->vdds_csib = NULL;
> }
> + /*
> + * If we set up ccp2->phy here,
> + * omap3isp_csiphy_acquire() will go ahead and assume
> + * csi2, dereferencing some null pointers.
> + *
> + * ccp2->phy = &isp->isp_csiphy2;
That needs to be fixed separately.
> + */
> } else if (isp->revision == ISP_REVISION_15_0) {
> ccp2->phy = &isp->isp_csiphy1;
> }
> diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
> index 871d4fe..897097b 100644
> --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> @@ -68,8 +68,8 @@ static void csiphy_routing_cfg_3630(struct isp_csiphy *phy,
> regmap_write(phy->isp->syscon, phy->isp->syscon_offset, reg);
> }
>
> -static void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
> - bool ccp2_strobe)
> +void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
> + bool ccp2_strobe, bool strobe_clk_pol)
> {
> u32 csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ
> | OMAP343X_CONTROL_CSIRXFE_RESET;
> @@ -85,6 +85,9 @@ static void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
>
> if (ccp2_strobe)
> csirxfe |= OMAP343X_CONTROL_CSIRXFE_SELFORM;
> +
> + if (strobe_clk_pol)
> + csirxfe |= OMAP343X_CONTROL_CSIRXFE_CSIB_INV;
>
> regmap_write(phy->isp->syscon, phy->isp->syscon_offset, csirxfe);
> }
> @@ -108,7 +111,7 @@ static void csiphy_routing_cfg(struct isp_csiphy *phy,
> 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)
> - return csiphy_routing_cfg_3430(phy, iface, on, ccp2_strobe);
> + return csiphy_routing_cfg_3430(phy, iface, on, ccp2_strobe, false);
> }
>
> /*
> diff --git a/drivers/media/platform/omap3isp/ispcsiphy.h b/drivers/media/platform/omap3isp/ispcsiphy.h
> index 28b63b2..88c5c1e8 100644
> --- a/drivers/media/platform/omap3isp/ispcsiphy.h
> +++ b/drivers/media/platform/omap3isp/ispcsiphy.h
> @@ -40,4 +40,7 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy);
> void omap3isp_csiphy_release(struct isp_csiphy *phy);
> int omap3isp_csiphy_init(struct isp_device *isp);
>
> +void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
> + bool ccp2_strobe, bool strobe_clk_pol);
> +
> #endif /* OMAP3_ISP_CSI_PHY_H */
>
>
>
--
Kind regards,
Sakari Ailus
e-mail: [email protected] XMPP: [email protected]
Hi Pavel,
Thank you for the patch.
On Thursday 02 Mar 2017 13:45:32 Pavel Machek wrote:
> If regulator returns -EPROBE_DEFER, we need to return it too, so that
> omap3isp will be re-probed when regulator is ready.
>
> Signed-off-by: Pavel Machek <[email protected]>
>
> diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> b/drivers/media/platform/omap3isp/ispccp2.c index ca09523..b6e055e 100644
> --- a/drivers/media/platform/omap3isp/ispccp2.c
> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> @@ -1137,10 +1159,12 @@ int omap3isp_ccp2_init(struct isp_device *isp)
> if (isp->revision == ISP_REVISION_2_0) {
> ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
> if (IS_ERR(ccp2->vdds_csib)) {
> + if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
This looks good to me, but it will result in the caller printing a "CCP2
initialization failed" error message, which I'm not sure is right. Maybe we
should move that message to the omap3isp_ccp2_init() function ?
In any case, this change is fine, so
Reviewed-by: Laurent Pinchart <[email protected]>
> dev_dbg(isp->dev,
> "Could not get regulator vdds_csib\n");
> ccp2->vdds_csib = NULL;
> }
> } else if (isp->revision == ISP_REVISION_15_0) {
> ccp2->phy = &isp->isp_csiphy1;
> }
--
Regards,
Laurent Pinchart
Hi Pavel,
On Thu, Mar 02, 2017 at 01:38:48PM +0100, Pavel Machek wrote:
> Hi!
>
> > > Ok, how about this one?
> > > omap3isp: add rest of CSI1 support
> > >
> > > CSI1 needs one more bit to be set up. Do just that.
> > >
> > > It is not as straightforward as I'd like, see the comments in the code
> > > for explanation.
> ...
> > > + if (isp->phy_type == ISP_PHY_TYPE_3430) {
> > > + struct media_pad *pad;
> > > + struct v4l2_subdev *sensor;
> > > + const struct isp_ccp2_cfg *buscfg;
> > > +
> > > + pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
> > > + sensor = media_entity_to_v4l2_subdev(pad->entity);
> > > + /* Struct isp_bus_cfg has union inside */
> > > + buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;
> > > +
> > > + csiphy_routing_cfg_3430(&isp->isp_csiphy2,
> > > + ISP_INTERFACE_CCP2B_PHY1,
> > > + enable, !!buscfg->phy_layer,
> > > + buscfg->strobe_clk_pol);
> >
> > You should do this through omap3isp_csiphy_acquire(), and not call
> > csiphy_routing_cfg_3430() directly from here.
>
> Well, unfortunately omap3isp_csiphy_acquire() does have csi2
> assumptions hard-coded :-(.
>
> This will probably fail.
>
> rval = omap3isp_csi2_reset(phy->csi2);
> if (rval < 0)
> goto done;
Yes. It needs to be fixed. :-)
>
> And this will oops:
>
> static int omap3isp_csiphy_config(struct isp_csiphy *phy)
> {
> struct isp_csi2_device *csi2 = phy->csi2;
> struct isp_pipeline *pipe = to_isp_pipeline(&csi2->subdev.entity);
> struct isp_bus_cfg *buscfg = pipe->external->host_priv;
There seems to be some more work left, yes. :-I
>
> > > @@ -1137,10 +1159,19 @@ int omap3isp_ccp2_init(struct isp_device *isp)
> > > if (isp->revision == ISP_REVISION_2_0) {
> > > ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
> > > if (IS_ERR(ccp2->vdds_csib)) {
> > > + if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER)
> > > + return -EPROBE_DEFER;
> >
> > This should go to a separate patch.
>
> Ok, easy enough.
>
> > > dev_dbg(isp->dev,
> > > "Could not get regulator vdds_csib\n");
> > > ccp2->vdds_csib = NULL;
> > > }
> > > + /*
> > > + * If we set up ccp2->phy here,
> > > + * omap3isp_csiphy_acquire() will go ahead and assume
> > > + * csi2, dereferencing some null pointers.
> > > + *
> > > + * ccp2->phy = &isp->isp_csiphy2;
> >
> > That needs to be fixed separately.
>
> See analysis above. Yes, it would be nice to fix it. Can you provide
> some hints how to do that? Maybe even patch to test? :-).
If I only will have the time. Let's see if I can find some time this
week-end.
--
Kind regards,
Sakari Ailus
e-mail: [email protected] XMPP: [email protected]
Hi Pavel,
On Thu, Mar 02, 2017 at 01:38:48PM +0100, Pavel Machek wrote:
> Hi!
>
> > > Ok, how about this one?
> > > omap3isp: add rest of CSI1 support
> > >
> > > CSI1 needs one more bit to be set up. Do just that.
> > >
> > > It is not as straightforward as I'd like, see the comments in the code
> > > for explanation.
> ...
> > > + if (isp->phy_type == ISP_PHY_TYPE_3430) {
> > > + struct media_pad *pad;
> > > + struct v4l2_subdev *sensor;
> > > + const struct isp_ccp2_cfg *buscfg;
> > > +
> > > + pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
> > > + sensor = media_entity_to_v4l2_subdev(pad->entity);
> > > + /* Struct isp_bus_cfg has union inside */
> > > + buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;
> > > +
> > > + csiphy_routing_cfg_3430(&isp->isp_csiphy2,
> > > + ISP_INTERFACE_CCP2B_PHY1,
> > > + enable, !!buscfg->phy_layer,
> > > + buscfg->strobe_clk_pol);
> >
> > You should do this through omap3isp_csiphy_acquire(), and not call
> > csiphy_routing_cfg_3430() directly from here.
>
> Well, unfortunately omap3isp_csiphy_acquire() does have csi2
> assumptions hard-coded :-(.
>
> This will probably fail.
>
> rval = omap3isp_csi2_reset(phy->csi2);
> if (rval < 0)
> goto done;
Could you try to two patches I've applied on the ccp2 branch (I'll remove
them if there are issues).
That's compile tested for now only.
--
Regards,
Sakari Ailus
e-mail: [email protected] XMPP: [email protected]
On Thu, Mar 02, 2017 at 04:46:42PM +0200, Laurent Pinchart wrote:
> Hi Pavel,
>
> Thank you for the patch.
>
> On Thursday 02 Mar 2017 13:45:32 Pavel Machek wrote:
> > If regulator returns -EPROBE_DEFER, we need to return it too, so that
> > omap3isp will be re-probed when regulator is ready.
> >
> > Signed-off-by: Pavel Machek <[email protected]>
> >
> > diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> > b/drivers/media/platform/omap3isp/ispccp2.c index ca09523..b6e055e 100644
> > --- a/drivers/media/platform/omap3isp/ispccp2.c
> > +++ b/drivers/media/platform/omap3isp/ispccp2.c
> > @@ -1137,10 +1159,12 @@ int omap3isp_ccp2_init(struct isp_device *isp)
> > if (isp->revision == ISP_REVISION_2_0) {
> > ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
> > if (IS_ERR(ccp2->vdds_csib)) {
> > + if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER)
> > + return -EPROBE_DEFER;
>
> This looks good to me, but it will result in the caller printing a "CCP2
> initialization failed" error message, which I'm not sure is right. Maybe we
> should move that message to the omap3isp_ccp2_init() function ?
>
> In any case, this change is fine, so
>
> Reviewed-by: Laurent Pinchart <[email protected]>
Applied to ccp2 branch with the error message moved. The old message is
still printed if the error code is different.
--
Sakari Ailus
e-mail: [email protected] XMPP: [email protected]
On Sat, Mar 04, 2017 at 03:03:18PM +0200, Sakari Ailus wrote:
> Hi Pavel,
>
> On Thu, Mar 02, 2017 at 01:38:48PM +0100, Pavel Machek wrote:
> > Hi!
> >
> > > > Ok, how about this one?
> > > > omap3isp: add rest of CSI1 support
> > > >
> > > > CSI1 needs one more bit to be set up. Do just that.
> > > >
> > > > It is not as straightforward as I'd like, see the comments in the code
> > > > for explanation.
> > ...
> > > > + if (isp->phy_type == ISP_PHY_TYPE_3430) {
> > > > + struct media_pad *pad;
> > > > + struct v4l2_subdev *sensor;
> > > > + const struct isp_ccp2_cfg *buscfg;
> > > > +
> > > > + pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
> > > > + sensor = media_entity_to_v4l2_subdev(pad->entity);
> > > > + /* Struct isp_bus_cfg has union inside */
> > > > + buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;
> > > > +
> > > > + csiphy_routing_cfg_3430(&isp->isp_csiphy2,
> > > > + ISP_INTERFACE_CCP2B_PHY1,
> > > > + enable, !!buscfg->phy_layer,
> > > > + buscfg->strobe_clk_pol);
> > >
> > > You should do this through omap3isp_csiphy_acquire(), and not call
> > > csiphy_routing_cfg_3430() directly from here.
> >
> > Well, unfortunately omap3isp_csiphy_acquire() does have csi2
> > assumptions hard-coded :-(.
> >
> > This will probably fail.
> >
> > rval = omap3isp_csi2_reset(phy->csi2);
> > if (rval < 0)
> > goto done;
>
> Could you try to two patches I've applied on the ccp2 branch (I'll remove
> them if there are issues).
>
> That's compile tested for now only.
One more thing. What's needed for configuring the PHY for CCP2?
For instance, is the CSI-2 PHY regulator still needed in
omap3isp_csiphy_acquire()? One way to do this might go to see the original
driver for N900; I don't have the TRM at hand right now.
--
Kind regards,
Sakari Ailus
e-mail: [email protected] XMPP: [email protected]
Hi Sakari,
On Saturday 04 Mar 2017 17:39:46 Sakari Ailus wrote:
> On Sat, Mar 04, 2017 at 03:03:18PM +0200, Sakari Ailus wrote:
> > On Thu, Mar 02, 2017 at 01:38:48PM +0100, Pavel Machek wrote:
> >>
> >>>> Ok, how about this one?
> >>>> omap3isp: add rest of CSI1 support
> >>>>
> >>>> CSI1 needs one more bit to be set up. Do just that.
> >>>>
> >>>> It is not as straightforward as I'd like, see the comments in the
> >>>> code for explanation.
> >>
> >> ...
> >>
> >>>> + if (isp->phy_type == ISP_PHY_TYPE_3430) {
> >>>> + struct media_pad *pad;
> >>>> + struct v4l2_subdev *sensor;
> >>>> + const struct isp_ccp2_cfg *buscfg;
> >>>> +
> >>>> + pad = media_entity_remote_pad(&ccp2
> >>>> ->pads[CCP2_PAD_SINK]);
> >>>> + sensor = media_entity_to_v4l2_subdev(pad->entity);
> >>>> + /* Struct isp_bus_cfg has union inside */
> >>>> + buscfg = &((struct isp_bus_cfg *)sensor->host_priv)
> >>>> ->bus.ccp2;
> >>>> +
> >>>> + csiphy_routing_cfg_3430(&isp->isp_csiphy2,
> >>>> + ISP_INTERFACE_CCP2B_PHY1,
> >>> > + enable, !!buscfg->phy_layer,
> >>> > + buscfg->strobe_clk_pol);
> >>>
> >>> You should do this through omap3isp_csiphy_acquire(), and not call
> >>> csiphy_routing_cfg_3430() directly from here.
> >>
> >> Well, unfortunately omap3isp_csiphy_acquire() does have csi2
> >> assumptions hard-coded :-(.
> >>
> >> This will probably fail.
> >>
> >> rval = omap3isp_csi2_reset(phy->csi2);
> >> if (rval < 0)
> >> goto done;
> >
> > Could you try to two patches I've applied on the ccp2 branch (I'll remove
> > them if there are issues).
> >
> > That's compile tested for now only.
>
> One more thing. What's needed for configuring the PHY for CCP2?
>
> For instance, is the CSI-2 PHY regulator still needed in
> omap3isp_csiphy_acquire()? One way to do this might go to see the original
> driver for N900; I don't have the TRM at hand right now.
The OMAP34xx TRM and data manual both mention separate VDDS power supplies for
the CSIb and CSI2 I/O complexes.
vdds_csi2 CSI2 Complex I/O
vdds_csib CSIb Complex I/O
On OMAP36xx, we instead have
vdda_csiphy1 Input power for camera PHY buffer
vdda_csiphy2 Input power for camera PHY buffer
We need to enable the vds_csib regulator to operate the CSI1/CCP2 PHY, but
that regulator gets enabled in ispccp2.c as that module is powered by the
vdds_csib supply on OMAP34xx. However, it won't hurt to do so, and the code
could be simpler if we manage the regulators the same way on OMAP34xx and
OMAP36xx.
--
Regards,
Laurent Pinchart
Hi!
> > > > + if (isp->phy_type == ISP_PHY_TYPE_3430) {
> > > > + struct media_pad *pad;
> > > > + struct v4l2_subdev *sensor;
> > > > + const struct isp_ccp2_cfg *buscfg;
> > > > +
> > > > + pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
> > > > + sensor = media_entity_to_v4l2_subdev(pad->entity);
> > > > + /* Struct isp_bus_cfg has union inside */
> > > > + buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;
> > > > +
> > > > + csiphy_routing_cfg_3430(&isp->isp_csiphy2,
> > > > + ISP_INTERFACE_CCP2B_PHY1,
> > > > + enable, !!buscfg->phy_layer,
> > > > + buscfg->strobe_clk_pol);
> > >
> > > You should do this through omap3isp_csiphy_acquire(), and not call
> > > csiphy_routing_cfg_3430() directly from here.
> >
> > Well, unfortunately omap3isp_csiphy_acquire() does have csi2
> > assumptions hard-coded :-(.
> >
> > This will probably fail.
> >
> > rval = omap3isp_csi2_reset(phy->csi2);
> > if (rval < 0)
> > goto done;
>
> Could you try to two patches I've applied on the ccp2 branch (I'll remove
> them if there are issues).
>
> That's compile tested for now only.
Thanks! They seem to be step in right direction. I still need to call
csiphy_routing_cfg_3430() directly for camera to work, but at least it
does not crash if I set up the phy pointer. I'll debug it some more.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi!
> > This will probably fail.
> >
> > rval = omap3isp_csi2_reset(phy->csi2);
> > if (rval < 0)
> > goto done;
>
> Could you try to two patches I've applied on the ccp2 branch (I'll remove
> them if there are issues).
>
> That's compile tested for now only.
They help a lot. Now I can use similar code paths...
Not yet a mergeable patch, but already better than it was.
Thanks and best regards,
Pavel
diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
index 24a9fc5..79838bd 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -21,6 +23,7 @@
#include <linux/mutex.h>
#include <linux/uaccess.h>
#include <linux/regulator/consumer.h>
+#include <linux/regmap.h>
#include "isp.h"
#include "ispreg.h"
@@ -1149,6 +1170,7 @@ int omap3isp_ccp2_init(struct isp_device *isp)
"Could not get regulator vdds_csib\n");
ccp2->vdds_csib = NULL;
}
+ ccp2->phy = &isp->isp_csiphy2;
} else if (isp->revision == ISP_REVISION_15_0) {
ccp2->phy = &isp->isp_csiphy1;
}
diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
index 50c0f64..94461df 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -68,8 +68,8 @@ static void csiphy_routing_cfg_3630(struct isp_csiphy *phy,
regmap_write(phy->isp->syscon, phy->isp->syscon_offset, reg);
}
-static void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
- bool ccp2_strobe)
+void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
+ bool ccp2_strobe, bool strobe_clk_pol)
{
u32 csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ
| OMAP343X_CONTROL_CSIRXFE_RESET;
@@ -85,6 +85,9 @@ static void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
if (ccp2_strobe)
csirxfe |= OMAP343X_CONTROL_CSIRXFE_SELFORM;
+
+ if (strobe_clk_pol)
+ csirxfe |= OMAP343X_CONTROL_CSIRXFE_CSIB_INV;
regmap_write(phy->isp->syscon, phy->isp->syscon_offset, csirxfe);
}
@@ -108,7 +111,7 @@ static void csiphy_routing_cfg(struct isp_csiphy *phy,
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)
- return csiphy_routing_cfg_3430(phy, iface, on, ccp2_strobe);
+ return csiphy_routing_cfg_3430(phy, iface, on, ccp2_strobe, false);
}
/*
@@ -197,27 +200,40 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
}
if (buscfg->interface == ISP_INTERFACE_CCP2B_PHY1
- || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2)
+ || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2) {
lanes = &buscfg->bus.ccp2.lanecfg;
- else
+ phy->num_data_lanes = 1;
+ } else
lanes = &buscfg->bus.csi2.lanecfg;
+ printk("lane verification... %d\n", phy->num_data_lanes);
+
/* Clock and data lanes verification */
for (i = 0; i < phy->num_data_lanes; i++) {
- if (lanes->data[i].pol > 1 || lanes->data[i].pos > 3)
- return -EINVAL;
+ if (lanes->data[i].pol > 1 || lanes->data[i].pos > 3) {
+ printk("Bad cfg\n");
+ //return -EINVAL;
+ }
- if (used_lanes & (1 << lanes->data[i].pos))
- return -EINVAL;
+ if (used_lanes & (1 << lanes->data[i].pos)) {
+ printk("Already used\n");
+ //return -EINVAL;
+ }
used_lanes |= 1 << lanes->data[i].pos;
}
- if (lanes->clk.pol > 1 || lanes->clk.pos > 3)
- return -EINVAL;
+ printk("used lanes... %d\n", used_lanes);
- if (lanes->clk.pos == 0 || used_lanes & (1 << lanes->clk.pos))
- return -EINVAL;
+ if (lanes->clk.pol > 1 || lanes->clk.pos > 3) {
+ printk("Bad clock\n");
+ //return -EINVAL;
+ }
+
+ if (lanes->clk.pos == 0 || used_lanes & (1 << lanes->clk.pos)) {
+ printk("Reused clock\n");
+ //return -EINVAL;
+ }
/*
* The PHY configuration is lost in off mode, that's not an
@@ -302,13 +318,16 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
if (rval < 0)
goto done;
- rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
- if (rval) {
- regulator_disable(phy->vdd);
- goto done;
+ if (phy->isp->revision == ISP_REVISION_15_0) {
+ rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
+ if (rval) {
+ regulator_disable(phy->vdd);
+ goto done;
+ }
+
+ csiphy_power_autoswitch_enable(phy, true);
}
- csiphy_power_autoswitch_enable(phy, true);
phy->phy_in_use = 1;
done:
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Fix clock lane parsing in v4l2-fwnode.
Signed-off-by: Pavel Machek <[email protected]>
diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index d6666d3..44036b8 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -167,7 +167,7 @@ void v4l2_fwnode_endpoint_parse_csi1_bus(struct fwnode_handle *fwn,
bus->data_lane = v;
if (!fwnode_property_read_u32(fwn, "clock-lanes", &v))
- bus->data_lane = v;
+ bus->clock_lane = v;
if (bus_type == V4L2_FWNODE_BUS_TYPE_CCP2)
vfwn->bus_type = V4L2_MBUS_CCP2;
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
omap3isp: add rest of CSI1 support
CSI1 needs one more bit to be set up. Do just that.
Signed-off-by: Pavel Machek <[email protected]>
---
Hmm. Looking at that... num_data_lanes probably should be modified in
local variable, not globally like this. Should I do that?
Anything else that needs fixing?
index 24a9fc5..6feba36 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -21,6 +23,7 @@
#include <linux/mutex.h>
#include <linux/uaccess.h>
#include <linux/regulator/consumer.h>
+#include <linux/regmap.h>
#include "isp.h"
#include "ispreg.h"
@@ -1149,6 +1152,7 @@ int omap3isp_ccp2_init(struct isp_device *isp)
"Could not get regulator vdds_csib\n");
ccp2->vdds_csib = NULL;
}
+ ccp2->phy = &isp->isp_csiphy2;
} else if (isp->revision == ISP_REVISION_15_0) {
ccp2->phy = &isp->isp_csiphy1;
}
diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
index 50c0f64..cd6351b 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -197,9 +200,10 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
}
if (buscfg->interface == ISP_INTERFACE_CCP2B_PHY1
- || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2)
+ || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2) {
lanes = &buscfg->bus.ccp2.lanecfg;
- else
+ phy->num_data_lanes = 1;
+ } else
lanes = &buscfg->bus.csi2.lanecfg;
/* Clock and data lanes verification */
@@ -302,13 +306,16 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
if (rval < 0)
goto done;
- rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
- if (rval) {
- regulator_disable(phy->vdd);
- goto done;
+ if (phy->isp->revision == ISP_REVISION_15_0) {
+ rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
+ if (rval) {
+ regulator_disable(phy->vdd);
+ goto done;
+ }
+
+ csiphy_power_autoswitch_enable(phy, true);
}
- csiphy_power_autoswitch_enable(phy, true);
phy->phy_in_use = 1;
done:
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Mon 2017-03-06 08:56:59, Pavel Machek wrote:
> omap3isp: add rest of CSI1 support
>
> CSI1 needs one more bit to be set up. Do just that.
>
> Signed-off-by: Pavel Machek <[email protected]>
>
> ---
>
> Hmm. Looking at that... num_data_lanes probably should be modified in
> local variable, not globally like this. Should I do that?
>
> Anything else that needs fixing?
Ping? Feedback here would be nice. This is last "interesting" piece of
the hardware support...
Best regards,
Pavel
> index 24a9fc5..6feba36 100644
> --- a/drivers/media/platform/omap3isp/ispccp2.c
> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> @@ -21,6 +23,7 @@
> #include <linux/mutex.h>
> #include <linux/uaccess.h>
> #include <linux/regulator/consumer.h>
> +#include <linux/regmap.h>
>
> #include "isp.h"
> #include "ispreg.h"
> @@ -1149,6 +1152,7 @@ int omap3isp_ccp2_init(struct isp_device *isp)
> "Could not get regulator vdds_csib\n");
> ccp2->vdds_csib = NULL;
> }
> + ccp2->phy = &isp->isp_csiphy2;
> } else if (isp->revision == ISP_REVISION_15_0) {
> ccp2->phy = &isp->isp_csiphy1;
> }
> diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
> index 50c0f64..cd6351b 100644
> --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> @@ -197,9 +200,10 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
> }
>
> if (buscfg->interface == ISP_INTERFACE_CCP2B_PHY1
> - || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2)
> + || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2) {
> lanes = &buscfg->bus.ccp2.lanecfg;
> - else
> + phy->num_data_lanes = 1;
> + } else
> lanes = &buscfg->bus.csi2.lanecfg;
>
> /* Clock and data lanes verification */
> @@ -302,13 +306,16 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
> if (rval < 0)
> goto done;
>
> - rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
> - if (rval) {
> - regulator_disable(phy->vdd);
> - goto done;
> + if (phy->isp->revision == ISP_REVISION_15_0) {
> + rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
> + if (rval) {
> + regulator_disable(phy->vdd);
> + goto done;
> + }
> +
> + csiphy_power_autoswitch_enable(phy, true);
> }
>
> - csiphy_power_autoswitch_enable(phy, true);
> phy->phy_in_use = 1;
>
> done:
>
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Mon, Mar 06, 2017 at 08:23:24AM +0100, Pavel Machek wrote:
> Fix clock lane parsing in v4l2-fwnode.
>
> Signed-off-by: Pavel Machek <[email protected]>
>
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index d6666d3..44036b8 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -167,7 +167,7 @@ void v4l2_fwnode_endpoint_parse_csi1_bus(struct fwnode_handle *fwn,
> bus->data_lane = v;
>
> if (!fwnode_property_read_u32(fwn, "clock-lanes", &v))
> - bus->data_lane = v;
> + bus->clock_lane = v;
Oh my. Did I really write it like that?
I'll merge your fix next Monday. Thanks!
--
Sakari Ailus
e-mail: [email protected] XMPP: [email protected]
On Fri 2017-03-10 14:41:31, Pavel Machek wrote:
> On Mon 2017-03-06 08:56:59, Pavel Machek wrote:
> > omap3isp: add rest of CSI1 support
> >
> > CSI1 needs one more bit to be set up. Do just that.
> >
> > Signed-off-by: Pavel Machek <[email protected]>
> >
> > ---
> >
> > Hmm. Looking at that... num_data_lanes probably should be modified in
> > local variable, not globally like this. Should I do that?
> >
> > Anything else that needs fixing?
>
> Ping? Feedback here would be nice. This is last "interesting" piece of
> the hardware support...
Any news here? You complained that I was not pushy enough in the past
;-).
Pavel
> > index 24a9fc5..6feba36 100644
> > --- a/drivers/media/platform/omap3isp/ispccp2.c
> > +++ b/drivers/media/platform/omap3isp/ispccp2.c
> > @@ -21,6 +23,7 @@
> > #include <linux/mutex.h>
> > #include <linux/uaccess.h>
> > #include <linux/regulator/consumer.h>
> > +#include <linux/regmap.h>
> >
> > #include "isp.h"
> > #include "ispreg.h"
> > @@ -1149,6 +1152,7 @@ int omap3isp_ccp2_init(struct isp_device *isp)
> > "Could not get regulator vdds_csib\n");
> > ccp2->vdds_csib = NULL;
> > }
> > + ccp2->phy = &isp->isp_csiphy2;
> > } else if (isp->revision == ISP_REVISION_15_0) {
> > ccp2->phy = &isp->isp_csiphy1;
> > }
> > diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
> > index 50c0f64..cd6351b 100644
> > --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> > +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> > @@ -197,9 +200,10 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
> > }
> >
> > if (buscfg->interface == ISP_INTERFACE_CCP2B_PHY1
> > - || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2)
> > + || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2) {
> > lanes = &buscfg->bus.ccp2.lanecfg;
> > - else
> > + phy->num_data_lanes = 1;
> > + } else
> > lanes = &buscfg->bus.csi2.lanecfg;
> >
> > /* Clock and data lanes verification */
> > @@ -302,13 +306,16 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
> > if (rval < 0)
> > goto done;
> >
> > - rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
> > - if (rval) {
> > - regulator_disable(phy->vdd);
> > - goto done;
> > + if (phy->isp->revision == ISP_REVISION_15_0) {
> > + rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
> > + if (rval) {
> > + regulator_disable(phy->vdd);
> > + goto done;
> > + }
> > +
> > + csiphy_power_autoswitch_enable(phy, true);
> > }
> >
> > - csiphy_power_autoswitch_enable(phy, true);
> > phy->phy_in_use = 1;
> >
> > done:
> >
>
>
>
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi!
Currently, ispvideo.c does not support enum_format. This causes
problems for example for libv4l2.
Now, I'm pretty sure patch below is not the right fix. But it fixes
libv4l2 problem for me.
Pointer to right solution welcome.
Regards,
Pavel
diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index 218e6d7..2ce0327 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -772,6 +772,44 @@ isp_video_try_format(struct file *file, void *fh, struct v4l2_format *format)
}
static int
+isp_video_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *format)
+{
+ struct isp_video *video = video_drvdata(file);
+ struct v4l2_subdev_format fmt;
+ struct v4l2_subdev *subdev;
+ u32 pad;
+ int ret;
+
+ printk("ispvideo: enum_fmt\n");
+
+ subdev = isp_video_remote_subdev(video, &pad);
+ if (subdev == NULL) {
+ printk("No subdev\n");
+ //return -EINVAL;
+ }
+
+ //isp_video_pix_to_mbus(&format->fmt.pix, &fmt.format);
+ if (format->index)
+ return -EINVAL;
+ format->type = video->type;
+ format->flags = 0;
+ strcpy(format->description, "subdev description");
+ format->pixelformat = V4L2_PIX_FMT_SGRBG10;
+
+ printk("Returning SRGBG10\n");
+#if 0
+ fmt.pad = pad;
+ fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+ ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
+ if (ret)
+ return ret == -ENOIOCTLCMD ? -ENOTTY : ret;
+
+ isp_video_mbus_to_pix(video, &fmt.format, &format->fmt.pix);
+#endif
+ return 0;
+}
+
+static int
isp_video_get_selection(struct file *file, void *fh, struct v4l2_selection *sel)
{
struct isp_video *video = video_drvdata(file);
@@ -1276,6 +1314,7 @@ static const struct v4l2_ioctl_ops isp_video_ioctl_ops = {
.vidioc_g_fmt_vid_cap = isp_video_get_format,
.vidioc_s_fmt_vid_cap = isp_video_set_format,
.vidioc_try_fmt_vid_cap = isp_video_try_format,
+ .vidioc_enum_fmt_vid_cap = isp_video_enum_format,
.vidioc_g_fmt_vid_out = isp_video_get_format,
.vidioc_s_fmt_vid_out = isp_video_set_format,
.vidioc_try_fmt_vid_out = isp_video_try_format,
On Sat 2017-03-04 20:44:50, Laurent Pinchart wrote:
> Hi Sakari,
>
> On Saturday 04 Mar 2017 17:39:46 Sakari Ailus wrote:
> > On Sat, Mar 04, 2017 at 03:03:18PM +0200, Sakari Ailus wrote:
> > > On Thu, Mar 02, 2017 at 01:38:48PM +0100, Pavel Machek wrote:
> > >>
> > >>>> Ok, how about this one?
> > >>>> omap3isp: add rest of CSI1 support
> > >>>>
> > >>>> CSI1 needs one more bit to be set up. Do just that.
> > >>>>
> > >>>> It is not as straightforward as I'd like, see the comments in the
> > >>>> code for explanation.
> > >>
> > >> ...
> > >>
> > >>>> + if (isp->phy_type == ISP_PHY_TYPE_3430) {
> > >>>> + struct media_pad *pad;
> > >>>> + struct v4l2_subdev *sensor;
> > >>>> + const struct isp_ccp2_cfg *buscfg;
> > >>>> +
> > >>>> + pad = media_entity_remote_pad(&ccp2
> > >>>> ->pads[CCP2_PAD_SINK]);
> > >>>> + sensor = media_entity_to_v4l2_subdev(pad->entity);
> > >>>> + /* Struct isp_bus_cfg has union inside */
> > >>>> + buscfg = &((struct isp_bus_cfg *)sensor->host_priv)
> > >>>> ->bus.ccp2;
> > >>>> +
> > >>>> + csiphy_routing_cfg_3430(&isp->isp_csiphy2,
> > >>>> + ISP_INTERFACE_CCP2B_PHY1,
> > >>> > + enable, !!buscfg->phy_layer,
> > >>> > + buscfg->strobe_clk_pol);
> > >>>
> > >>> You should do this through omap3isp_csiphy_acquire(), and not call
> > >>> csiphy_routing_cfg_3430() directly from here.
> > >>
> > >> Well, unfortunately omap3isp_csiphy_acquire() does have csi2
> > >> assumptions hard-coded :-(.
> > >>
> > >> This will probably fail.
> > >>
> > >> rval = omap3isp_csi2_reset(phy->csi2);
> > >> if (rval < 0)
> > >> goto done;
> > >
> > > Could you try to two patches I've applied on the ccp2 branch (I'll remove
> > > them if there are issues).
> > >
> > > That's compile tested for now only.
> >
> > One more thing. What's needed for configuring the PHY for CCP2?
> >
> > For instance, is the CSI-2 PHY regulator still needed in
> > omap3isp_csiphy_acquire()? One way to do this might go to see the original
> > driver for N900; I don't have the TRM at hand right now.
>
> The OMAP34xx TRM and data manual both mention separate VDDS power supplies for
> the CSIb and CSI2 I/O complexes.
>
> vdds_csi2 CSI2 Complex I/O
> vdds_csib CSIb Complex I/O
>
> On OMAP36xx, we instead have
>
> vdda_csiphy1 Input power for camera PHY buffer
> vdda_csiphy2 Input power for camera PHY buffer
>
> We need to enable the vds_csib regulator to operate the CSI1/CCP2 PHY, but
> that regulator gets enabled in ispccp2.c as that module is powered by the
> vdds_csib supply on OMAP34xx. However, it won't hurt to do so, and the code
> could be simpler if we manage the regulators the same way on OMAP34xx and
> OMAP36xx.
>
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi Pavel,
On Wed, Apr 26, 2017 at 11:19:33PM +0200, Pavel Machek wrote:
> Hi!
>
> Currently, ispvideo.c does not support enum_format. This causes
> problems for example for libv4l2.
>
> Now, I'm pretty sure patch below is not the right fix. But it fixes
> libv4l2 problem for me.
>
> Pointer to right solution welcome.
The issue with providing ENUM_FMT support on MC-enabled drivers is that the
media bus format has an effect to which pixel formats are available.
What has been discussed in the past but what remains unimplemented is to add
the media bus code to v4l2_fmtdesc for user to choose. That would allow
meaningful ENUM_FMT support.
For users such as libv4l2, i.e. code == 0 --- it should just tell the user
what it can do with the active media bus format.
--
Kind regards,
Sakari Ailus
e-mail: [email protected] XMPP: [email protected]
Hi!
Are there any news about the fwnode branch?
I have quite usable camera, but it is still based on
982e8e40390d26430ef106fede41594139a4111c (that's v4.10). It would be
good to see fwnode stuff upstream... are there any plans for that?
Is there stable branch to which I could move the stuff?
Thanks,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi Pavel,
On Tue, Jun 13, 2017 at 02:22:40PM +0200, Pavel Machek wrote:
> Hi!
>
> Are there any news about the fwnode branch?
>
> I have quite usable camera, but it is still based on
> 982e8e40390d26430ef106fede41594139a4111c (that's v4.10). It would be
> good to see fwnode stuff upstream... are there any plans for that?
>
> Is there stable branch to which I could move the stuff?
What's relevant for most V4L2 drivers is in linux-media right now.
There are new features that will take some time to get in. The trouble has
been, and continue to be, that the patches need to go through various trees
so it'll take some time for them to be merged.
I expect to have most of them in during the next merge window.
--
Kind regards,
Sakari Ailus
e-mail: [email protected] XMPP: [email protected]
Hi!
> > Are there any news about the fwnode branch?
> >
> > I have quite usable camera, but it is still based on
> > 982e8e40390d26430ef106fede41594139a4111c (that's v4.10). It would be
> > good to see fwnode stuff upstream... are there any plans for that?
> >
> > Is there stable branch to which I could move the stuff?
>
> What's relevant for most V4L2 drivers is in linux-media right now.
>
> There are new features that will take some time to get in. The trouble has
> been, and continue to be, that the patches need to go through various trees
> so it'll take some time for them to be merged.
>
> I expect to have most of them in during the next merge window.
So git://linuxtv.org/media_tree.git branch master is the right one to
work one?
Thanks,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi, Pavel!
On Tue, Jun 13, 2017 at 11:09:00PM +0200, Pavel Machek wrote:
> Hi!
>
> > > Are there any news about the fwnode branch?
> > >
> > > I have quite usable camera, but it is still based on
> > > 982e8e40390d26430ef106fede41594139a4111c (that's v4.10). It would be
> > > good to see fwnode stuff upstream... are there any plans for that?
> > >
> > > Is there stable branch to which I could move the stuff?
> >
> > What's relevant for most V4L2 drivers is in linux-media right now.
> >
> > There are new features that will take some time to get in. The trouble has
> > been, and continue to be, that the patches need to go through various trees
> > so it'll take some time for them to be merged.
> >
> > I expect to have most of them in during the next merge window.
>
> So git://linuxtv.org/media_tree.git branch master is the right one to
> work one?
I also pushed the rebased ccp2 branch there:
<URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=ccp2>
It's now right on the top of media-tree master.
--
Regards,
Sakari Ailus
e-mail: [email protected] XMPP: [email protected]
Hi!
> > > > Are there any news about the fwnode branch?
> > > >
> > > > I have quite usable camera, but it is still based on
> > > > 982e8e40390d26430ef106fede41594139a4111c (that's v4.10). It would be
> > > > good to see fwnode stuff upstream... are there any plans for that?
> > > >
> > > > Is there stable branch to which I could move the stuff?
> > >
> > > What's relevant for most V4L2 drivers is in linux-media right now.
> > >
> > > There are new features that will take some time to get in. The trouble has
> > > been, and continue to be, that the patches need to go through various trees
> > > so it'll take some time for them to be merged.
> > >
> > > I expect to have most of them in during the next merge window.
> >
> > So git://linuxtv.org/media_tree.git branch master is the right one to
> > work one?
>
> I also pushed the rebased ccp2 branch there:
>
> <URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=ccp2>
>
> It's now right on the top of media-tree master.
Thanks, that's what I was looking for.
Unfortunately, it does not compile.
CC drivers/media/platform/omap3isp/ispcsiphy.o
drivers/media/platform/omap3isp/isp.c: In function
'isp_fwnode_parse':
drivers/media/platform/omap3isp/isp.c:2029:35: error: 'fwn'
undeclared (first use in this function)
drivers/media/platform/omap3isp/isp.c:2029:35: note: each undeclared
identifier is reported only once for each function it appears in
drivers/media/platform/omap3isp/isp.c:2029:2: error: incompatible
type for argument 2 of 'v4l2_fwnode_endpoint_parse'
In file included from drivers/media/platform/omap3isp/isp.c:67:0:
./include/media/v4l2-fwnode.h:112:5: note: expected 'struct
v4l2_fwnode_endpoint *' but argument is of type 'struct
v4l2_fwnode_endpoint'
scripts/Makefile.build:302: recipe for target
'drivers/media/platform/omap3isp/isp.o' failed
make[4]: *** [drivers/media/platform/omap3isp/isp.o] Error 1
make[4]: *** Waiting for unfinished jobs....
scripts/Makefile.build:561: recipe for target
'drivers/media/platform/omap3isp' failed
make[3]: *** [drivers/media/platform/omap3isp] Error 2
You can get my config if needed. Now let me try to fix it... It was
not too bad, good.
commit 364340e7aa037535a65d2ef2a1711c97d233fede
Author: Pavel <[email protected]>
Date: Wed Jun 14 21:40:37 2017 +0200
Fix compilation of omap3isp/isp.c.
Signed-off-by: Pavel Machek <[email protected]>
diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 4ca3fc9..b80debf 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2026,7 +2026,7 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
isd->bus = buscfg;
- ret = v4l2_fwnode_endpoint_parse(fwn, vep);
+ ret = v4l2_fwnode_endpoint_parse(fwnode, &vep);
if (ret)
return ret;
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Wed, Jun 14, 2017 at 09:41:29PM +0200, Pavel Machek wrote:
> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> index 4ca3fc9..b80debf 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2026,7 +2026,7 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
>
> isd->bus = buscfg;
>
> - ret = v4l2_fwnode_endpoint_parse(fwn, vep);
> + ret = v4l2_fwnode_endpoint_parse(fwnode, &vep);
> if (ret)
> return ret;
I just pushed the fix there.
Btw. I think we should probably drop the change allocating the sub-device
configuration separately. It's better to associate the lens, flash and
eeprom (where it exists) to the sensor than to the CSI-2 receiver. In that
case there are no async sub-devices without bus configuration.
--
Sakari Ailus
e-mail: [email protected] XMPP: [email protected]
Hi!
Ok, so I played a bit, and now I have working camera in v4.12-rc3.
https://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-n900.git/
camera-fw5-3 is recommended branch to play with.
Sakari, should I attempt to clean/send you patches, or would it be
better to wait till ccp2 branch is merged upstream? There's one
compile fix, I'll submit that one in following email.
I even have patches for v4l2-utils, so digital camera can be used as
... digital camera :-). (With rather slow autofocus, and 1Mpix only at
the moment, but hey, its a start, and I already have _one_ nice
picture from it.)
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Fix compilation of isp.c
Signed-off-by: Pavel Machek <[email protected]>
diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 4ca3fc9..b80debf 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2026,7 +2026,7 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
isd->bus = buscfg;
- ret = v4l2_fwnode_endpoint_parse(fwn, vep);
+ ret = v4l2_fwnode_endpoint_parse(fwnode, &vep);
if (ret)
return ret;
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Fri 2017-06-16 01:07:00, Sakari Ailus wrote:
> On Wed, Jun 14, 2017 at 09:41:29PM +0200, Pavel Machek wrote:
> > diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> > index 4ca3fc9..b80debf 100644
> > --- a/drivers/media/platform/omap3isp/isp.c
> > +++ b/drivers/media/platform/omap3isp/isp.c
> > @@ -2026,7 +2026,7 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
> >
> > isd->bus = buscfg;
> >
> > - ret = v4l2_fwnode_endpoint_parse(fwn, vep);
> > + ret = v4l2_fwnode_endpoint_parse(fwnode, &vep);
> > if (ret)
> > return ret;
>
> I just pushed the fix there.
>
> Btw. I think we should probably drop the change allocating the sub-device
> configuration separately. It's better to associate the lens, flash and
> eeprom (where it exists) to the sensor than to the CSI-2 receiver. In that
> case there are no async sub-devices without bus configuration.
Actually I thought about that a bit, and am not sure about that.
CSI-2 receiver may not be good place to associate lens and flash with,
agreed.
But is sensor a good place? In particular, phones with two cameras
cooperating (for example one black&white and one color) are getting
common. It seems to be true that each sensor has a lens and autofocus
motor associated, but flash LED is common, and both sensors are
designed to work as one device.
But yes, that's still better than placing it at CSI-2 receiver. But I
guess we should make sure that flash LED can associated with more than
one sensor, and maybe we should have some kind of "camera package"
entity.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On 06/16/2017 12:23 AM, Pavel Machek wrote:
>
> Fix compilation of isp.c
>
> Signed-off-by: Pavel Machek <[email protected]>
>
> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> index 4ca3fc9..b80debf 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2026,7 +2026,7 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
>
> isd->bus = buscfg;
>
> - ret = v4l2_fwnode_endpoint_parse(fwn, vep);
> + ret = v4l2_fwnode_endpoint_parse(fwnode, &vep);
> if (ret)
> return ret;
>
>
You're using something old since the media tree master already uses &vep.
Regards,
Hans
On Fri, Jun 16, 2017 at 10:03:41AM +0200, Hans Verkuil wrote:
> On 06/16/2017 12:23 AM, Pavel Machek wrote:
> >
> >Fix compilation of isp.c
> >Signed-off-by: Pavel Machek <[email protected]>
> >
> >diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> >index 4ca3fc9..b80debf 100644
> >--- a/drivers/media/platform/omap3isp/isp.c
> >+++ b/drivers/media/platform/omap3isp/isp.c
> >@@ -2026,7 +2026,7 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
> > isd->bus = buscfg;
> >- ret = v4l2_fwnode_endpoint_parse(fwn, vep);
> >+ ret = v4l2_fwnode_endpoint_parse(fwnode, &vep);
> > if (ret)
> > return ret;
> >
>
> You're using something old since the media tree master already uses &vep.
Well, yes and no. Pavel is using my ccp2 support branch I recently rebased.
:-)
--
Sakari Ailus
e-mail: [email protected] XMPP: [email protected]
Hi!
> > > > Are there any news about the fwnode branch?
> > > >
> > > > I have quite usable camera, but it is still based on
> > > > 982e8e40390d26430ef106fede41594139a4111c (that's v4.10). It would be
> > > > good to see fwnode stuff upstream... are there any plans for that?
> > > >
> > > > Is there stable branch to which I could move the stuff?
> > >
> > > What's relevant for most V4L2 drivers is in linux-media right now.
> > >
> > > There are new features that will take some time to get in. The trouble has
> > > been, and continue to be, that the patches need to go through various trees
> > > so it'll take some time for them to be merged.
> > >
> > > I expect to have most of them in during the next merge window.
> >
> > So git://linuxtv.org/media_tree.git branch master is the right one to
> > work one?
>
> I also pushed the rebased ccp2 branch there:
>
> <URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=ccp2>
>
> It's now right on the top of media-tree master.
Is ccp2 branch expected to go into 4.13, too?
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Tue, Jul 04, 2017 at 05:08:19PM +0200, Pavel Machek wrote:
> Hi!
>
> > > > > Are there any news about the fwnode branch?
> > > > >
> > > > > I have quite usable camera, but it is still based on
> > > > > 982e8e40390d26430ef106fede41594139a4111c (that's v4.10). It would be
> > > > > good to see fwnode stuff upstream... are there any plans for that?
> > > > >
> > > > > Is there stable branch to which I could move the stuff?
> > > >
> > > > What's relevant for most V4L2 drivers is in linux-media right now.
> > > >
> > > > There are new features that will take some time to get in. The trouble has
> > > > been, and continue to be, that the patches need to go through various trees
> > > > so it'll take some time for them to be merged.
> > > >
> > > > I expect to have most of them in during the next merge window.
> > >
> > > So git://linuxtv.org/media_tree.git branch master is the right one to
> > > work one?
> >
> > I also pushed the rebased ccp2 branch there:
> >
> > <URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=ccp2>
> >
> > It's now right on the top of media-tree master.
>
> Is ccp2 branch expected to go into 4.13, too?
Hi Pavel,
What I've done is just rebased the ccp2 branch. In other words, the patches
in that branch are no more ready than they were.
To get these merged we should ideally
1) Make sure there will be no regressions,
2) clean things up in the omap3isp; which resources are needed and when
(e.g. regulators, PHY configuration) isn't clear at the moment and
2) have one driver using the implementation.
At least 1) is needed. I think a number of framework patches could be
mergeable before 2) and 3) are done. I can prepare a set later this week.
But even that'd be likely for 4.14, not 4.13.
--
Kind regards,
Sakari Ailus
e-mail: [email protected] XMPP: [email protected]
Hi!
> > > > > I expect to have most of them in during the next merge window.
> > > >
> > > > So git://linuxtv.org/media_tree.git branch master is the right one to
> > > > work one?
> > >
> > > I also pushed the rebased ccp2 branch there:
> > >
> > > <URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=ccp2>
> > >
> > > It's now right on the top of media-tree master.
> >
> > Is ccp2 branch expected to go into 4.13, too?
>
> Hi Pavel,
>
> What I've done is just rebased the ccp2 branch. In other words, the patches
> in that branch are no more ready than they were.
I thought they were ready even back then :-).
> To get these merged we should ideally
>
> 1) Make sure there will be no regressions,
Well, all I have running recent kernels is N900. If ccp branch works
for you on N9, that's probably as much testing as we can get.
> 2) clean things up in the omap3isp; which resources are needed and when
> (e.g. regulators, PHY configuration) isn't clear at the moment and
>
> 2) have one driver using the implementation.
>
> At least 1) is needed. I think a number of framework patches could be
> mergeable before 2) and 3) are done. I can prepare a set later this week.
> But even that'd be likely for 4.14, not 4.13.
Yep, it is too late for v4.13 now. But getting stuff ready for v4.14
would be good.
I started looking through the patches; I believe they are safe, but it
is probably better to review the series you've just mailed.
The driver using the implementation -- yes, I have it all working on
n900 (incuding userland, I can actually take photos.) I can post the
series, or better link to kernel.org.
Right now, my goal would be to get sensor working on N900 with
mainline (without flash and focus).
I'd very much like any comment on patch attached below.
Age Commit message (Expand) Author Files Lines
2017-06-16 omap3isp: Destroy CSI-2 phy mutexes in error and module
2017-06-16 omap3isp: Skip CSI-2 receiver initialisation in CCP2
2017-06-16 omap3isp: Correctly put the last iterated endpoint
2017-06-16 omap3isp: Always initialise isp and mutex for csiphy1
2017-06-16 omap3isp: Return -EPROBE_DEFER if the required
2017-06-16 omap3isp: Correctly set IO_OUT_SEL and VP_CLK_POL for CCP2
2017-06-16 omap3isp: Make external sub-device bus configuration a
2017-06-15 omap3isp: Parse CSI1 configuration from the device tree
2017-06-15 omap3isp: Check for valid port in endpoints Sakari
2017-06-15 omap3isp: Ignore endpoints with invalid configuration
# Nothing changes for bus_type == V4L2_MBUS_CSI2. FIXME: Is bus_type
set correctly?
2017-06-15 smiapp: add CCP2 support Pavel Machek 1
# bus_type will be guess, so no code changes on existing system:
2017-06-15 v4l: Add support for CSI-1 and CCP2 busses Sakari
# Reads unused value -> can't break anything:
2017-06-13 v4l: fwnode: Obtain data bus type from FW Sakari
# No code changes -> totally safe:
2017-06-13 v4l: fwnode: Call CSI2 bus csi2, not csi Sakari
2017-06-13 dt: bindings: Add strobe property for CCP2 Sakari
2017-06-13 dt: bindings: Explicitly specify bus type
Best regards,
Pavel
commit 1220492dd4c1872c8036caa573680f95aabc69bc
Author: Pavel <[email protected]>
Date: Tue Feb 28 12:02:26 2017 +0100
omap3isp: add CSI1 support
Use proper code path for csi1/ccp2 support.
Signed-off-by: Pavel Machek <[email protected]>
diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
index 24a9fc5..47210b1 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -1149,6 +1149,7 @@ int omap3isp_ccp2_init(struct isp_device *isp)
"Could not get regulator vdds_csib\n");
ccp2->vdds_csib = NULL;
}
+ ccp2->phy = &isp->isp_csiphy2;
} else if (isp->revision == ISP_REVISION_15_0) {
ccp2->phy = &isp->isp_csiphy1;
}
diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
index 50c0f64..862fdd3 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -197,9 +197,10 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
}
if (buscfg->interface == ISP_INTERFACE_CCP2B_PHY1
- || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2)
+ || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2) {
lanes = &buscfg->bus.ccp2.lanecfg;
- else
+ phy->num_data_lanes = 1;
+ } else
lanes = &buscfg->bus.csi2.lanecfg;
/* Clock and data lanes verification */
@@ -302,13 +303,16 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
if (rval < 0)
goto done;
- rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
- if (rval) {
- regulator_disable(phy->vdd);
- goto done;
+ if (phy->isp->revision == ISP_REVISION_15_0) {
+ rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
+ if (rval) {
+ regulator_disable(phy->vdd);
+ goto done;
+ }
+
+ csiphy_power_autoswitch_enable(phy, true);
}
- csiphy_power_autoswitch_enable(phy, true);
phy->phy_in_use = 1;
done:
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi Pavel,
On Thu, Jul 06, 2017 at 12:38:51PM +0200, Pavel Machek wrote:
> Hi!
>
> > > > > > I expect to have most of them in during the next merge window.
> > > > >
> > > > > So git://linuxtv.org/media_tree.git branch master is the right one to
> > > > > work one?
> > > >
> > > > I also pushed the rebased ccp2 branch there:
> > > >
> > > > <URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=ccp2>
> > > >
> > > > It's now right on the top of media-tree master.
> > >
> > > Is ccp2 branch expected to go into 4.13, too?
> >
> > Hi Pavel,
> >
> > What I've done is just rebased the ccp2 branch. In other words, the patches
> > in that branch are no more ready than they were.
>
> I thought they were ready even back then :-).
>
> > To get these merged we should ideally
> >
> > 1) Make sure there will be no regressions,
>
> Well, all I have running recent kernels is N900. If ccp branch works
> for you on N9, that's probably as much testing as we can get.
>
> > 2) clean things up in the omap3isp; which resources are needed and when
> > (e.g. regulators, PHY configuration) isn't clear at the moment and
> >
> > 2) have one driver using the implementation.
> >
> > At least 1) is needed. I think a number of framework patches could be
> > mergeable before 2) and 3) are done. I can prepare a set later this week.
> > But even that'd be likely for 4.14, not 4.13.
>
> Yep, it is too late for v4.13 now. But getting stuff ready for v4.14
> would be good.
>
> I started looking through the patches; I believe they are safe, but it
> is probably better to review the series you've just mailed.
>
> The driver using the implementation -- yes, I have it all working on
> n900 (incuding userland, I can actually take photos.) I can post the
> series, or better link to kernel.org.
>
> Right now, my goal would be to get sensor working on N900 with
> mainline (without flash and focus).
>
> I'd very much like any comment on patch attached below.
>
> Age Commit message (Expand) Author Files Lines
> 2017-06-16 omap3isp: Destroy CSI-2 phy mutexes in error and module
> 2017-06-16 omap3isp: Skip CSI-2 receiver initialisation in CCP2
> 2017-06-16 omap3isp: Correctly put the last iterated endpoint
> 2017-06-16 omap3isp: Always initialise isp and mutex for csiphy1
> 2017-06-16 omap3isp: Return -EPROBE_DEFER if the required
> 2017-06-16 omap3isp: Correctly set IO_OUT_SEL and VP_CLK_POL for CCP2
> 2017-06-16 omap3isp: Make external sub-device bus configuration a
> 2017-06-15 omap3isp: Parse CSI1 configuration from the device tree
> 2017-06-15 omap3isp: Check for valid port in endpoints Sakari
> 2017-06-15 omap3isp: Ignore endpoints with invalid configuration
>
> # Nothing changes for bus_type == V4L2_MBUS_CSI2. FIXME: Is bus_type
> set correctly?
>
> 2017-06-15 smiapp: add CCP2 support Pavel Machek 1
>
> # bus_type will be guess, so no code changes on existing system:
>
> 2017-06-15 v4l: Add support for CSI-1 and CCP2 busses Sakari
>
> # Reads unused value -> can't break anything:
>
> 2017-06-13 v4l: fwnode: Obtain data bus type from FW Sakari
>
> # No code changes -> totally safe:
>
> 2017-06-13 v4l: fwnode: Call CSI2 bus csi2, not csi Sakari
> 2017-06-13 dt: bindings: Add strobe property for CCP2 Sakari
> 2017-06-13 dt: bindings: Explicitly specify bus type
>
> Best regards,
> Pavel
>
> commit 1220492dd4c1872c8036caa573680f95aabc69bc
> Author: Pavel <[email protected]>
> Date: Tue Feb 28 12:02:26 2017 +0100
>
> omap3isp: add CSI1 support
>
> Use proper code path for csi1/ccp2 support.
>
> Signed-off-by: Pavel Machek <[email protected]>
>
> diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
> index 24a9fc5..47210b1 100644
> --- a/drivers/media/platform/omap3isp/ispccp2.c
> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> @@ -1149,6 +1149,7 @@ int omap3isp_ccp2_init(struct isp_device *isp)
> "Could not get regulator vdds_csib\n");
> ccp2->vdds_csib = NULL;
> }
> + ccp2->phy = &isp->isp_csiphy2;
> } else if (isp->revision == ISP_REVISION_15_0) {
> ccp2->phy = &isp->isp_csiphy1;
> }
> diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
> index 50c0f64..862fdd3 100644
> --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> @@ -197,9 +197,10 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
> }
>
> if (buscfg->interface == ISP_INTERFACE_CCP2B_PHY1
> - || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2)
> + || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2) {
> lanes = &buscfg->bus.ccp2.lanecfg;
> - else
> + phy->num_data_lanes = 1;
> + } else
> lanes = &buscfg->bus.csi2.lanecfg;
>
> /* Clock and data lanes verification */
> @@ -302,13 +303,16 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
> if (rval < 0)
> goto done;
>
> - rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
> - if (rval) {
> - regulator_disable(phy->vdd);
> - goto done;
> + if (phy->isp->revision == ISP_REVISION_15_0) {
Shouldn't you make the related changes to omap3isp_csiphy_release() as
well?
Other than that the patch looks good to me.
> + rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
> + if (rval) {
> + regulator_disable(phy->vdd);
> + goto done;
> + }
> +
> + csiphy_power_autoswitch_enable(phy, true);
> }
>
> - csiphy_power_autoswitch_enable(phy, true);
> phy->phy_in_use = 1;
>
> done:
>
--
Kind regards,
Sakari Ailus
e-mail: [email protected] XMPP: [email protected]
Hi!
> > > 1) Make sure there will be no regressions,
> >
> > Well, all I have running recent kernels is N900. If ccp branch works
> > for you on N9, that's probably as much testing as we can get.
> >
> > > 2) clean things up in the omap3isp; which resources are needed and when
> > > (e.g. regulators, PHY configuration) isn't clear at the moment and
> > >
> > > 2) have one driver using the implementation.
> > >
> > > At least 1) is needed. I think a number of framework patches could be
> > > mergeable before 2) and 3) are done. I can prepare a set later this week.
> > > But even that'd be likely for 4.14, not 4.13.
> >
> > Yep, it is too late for v4.13 now. But getting stuff ready for v4.14
> > would be good.
...
> > @@ -302,13 +303,16 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
> > if (rval < 0)
> > goto done;
> >
> > - rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
> > - if (rval) {
> > - regulator_disable(phy->vdd);
> > - goto done;
> > + if (phy->isp->revision == ISP_REVISION_15_0) {
>
> Shouldn't you make the related changes to omap3isp_csiphy_release() as
> well?
>
> Other than that the patch looks good to me.
Ah, yes, that needs to be fixed. Thanks for review.
I'll refresh the series. I believe we now have everything neccessary
to have useful driver for 4.14. Series is still based on 4.12-rc3, I
can rebase it when there's better base.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi!
> What I've done is just rebased the ccp2 branch. In other words, the patches
> in that branch are no more ready than they were.
>
> To get these merged we should ideally
>
> 1) Make sure there will be no regressions,
I grepped dts trees a bit... where is omap3isp currently used?
Anything besides N9 and N950?
Does the capture mode currently work for you?
Because as far as I can tell, formatter is disabled, so video is in
wrong format for the userspace.
So something like patch below is needed; (of course after adjusting
the comment etc.)
Thanks,
Pavel
commit eb81524b8b44bbff2518b272cb3de304157bd3ba
Author: Pavel <[email protected]>
Date: Mon Feb 13 21:26:51 2017 +0100
omap3isp: fix VP2SDR bit so capture (not preview) works
This is neccessary for capture (not preview) to work properly on
N900. Why is unknown.
diff --git a/drivers/media/platform/omap3isp/ispccdc.c b/drivers/media/platform/omap3isp/ispccdc.c
index 7207558..2fb755f 100644
--- a/drivers/media/platform/omap3isp/ispccdc.c
+++ b/drivers/media/platform/omap3isp/ispccdc.c
@@ -1186,7 +1186,8 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
/* Use the raw, unprocessed data when writing to memory. The H3A and
* histogram modules are still fed with lens shading corrected data.
*/
- syn_mode &= ~ISPCCDC_SYN_MODE_VP2SDR;
+// syn_mode &= ~ISPCCDC_SYN_MODE_VP2SDR;
+ syn_mode |= ISPCCDC_SYN_MODE_VP2SDR;
if (ccdc->output & CCDC_OUTPUT_MEMORY)
syn_mode |= ISPCCDC_SYN_MODE_WEN;
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html