Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Laurent Pinchart]

Hi Niklas,

On Monday, 16 April 2018 00:26:06 EEST Niklas Söderlund wrote:
> Hi Laurent,
> 
> Thanks for your feedback.
> 
> I have addressed all your comment's but one for the next version. Please
> indicate if you are fine with this and I can still keep your review tag
> 
> :-)
> 
> On 2018-04-04 18:15:16 +0300, Laurent Pinchart wrote:
> 
> [snip]
> 
> > > +static int rcar_csi2_start(struct rcar_csi2 *priv)
> > > +{
> > > + const struct rcar_csi2_format *format;
> > > + u32 phycnt, phypll, vcdt = 0, vcdt2 = 0;
> > > + unsigned int i;
> > > + int ret;
> > > +
> > > + dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
> > > + priv->mf.width, priv->mf.height,
> > > + priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i');
> > > +
> > > + /* Code is validated in set_fmt */
> > > + format = rcar_csi2_code_to_fmt(priv->mf.code);
> > 
> > You could store the format pointer iin the rcar_csi2 structure to avoid
> > looking it up here.
> 
> I could do that, but then I would duplicate the storage of the code
> between the two cached values priv->mf and priv-> rcar_csi2>. I could find that acceptable but if you don't strongly
> disagree I would prefer to keep the current way of looking it up here
> :-)

I'm OK with that, even if I think that duplicating the code would be a very 
small price to pay for not having to look up the format information structure.

> [snip]
> 
> > > +static int rcar_csi2_probe_resources(struct rcar_csi2 *priv,
> > > +  struct platform_device *pdev)
> > > +{
> > > + struct resource *res;
> > > + int irq;
> > > +
> > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > + priv->base = devm_ioremap_resource(>dev, res);
> > > + if (IS_ERR(priv->base))
> > > + return PTR_ERR(priv->base);
> > > +
> > > + irq = platform_get_irq(pdev, 0);
> > > + if (irq < 0)
> > > + return irq;
> > 
> > You don't seem to use the IRQ. Is this meant to catch invalid DT that
> > don't
> > specify an IRQ, to make sure we'll always have one available when we'll
> > need to later ?
> 
> Yes, as you deducted this is currently only to catch invalid DT. In the
> DT documentation I list it as a mandatory property. I think there might
> be potential use-case to at some point add interrupt support of for the
> some of the error interrupts which can be enabled, specially now when we
> seen similar patches for VIN floating around.
> 
> > > +
> > > + return 0;
> > > +
> > > +error:
> > > + v4l2_async_notifier_unregister(>notifier);
> > > + v4l2_async_notifier_cleanup(>notifier);
> > > +
> > > + return ret;
> > > +}
> > 
> > [snip]
> > 
> > With these small issues fixed and Kieran's and Maxime's comments addressed
> > as you see fit,
> > 
> > Reviewed-by: Laurent Pinchart 
> 
> Thanks, I will hold of adding it until you indicate if you are OK with
> the one comment I'm not fully agreeing with you on.


-- 
Regards,

Laurent Pinchart

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Niklas Söderlund]

Hi Jacopo,

On 2018-04-16 01:16:35 +0200, Niklas Söderlund wrote:

[snip]

> > > +
> > > + /* Set frequency range if we have it */
> > > + if (priv->info->csi0clkfreqrange)
> > > + rcar_csi2_write(priv, CSI0CLKFCPR_REG,
> > > + CSI0CLKFREQRANGE(priv->info->csi0clkfreqrange));
> > > +
> > > + rcar_csi2_write(priv, PHYCNT_REG, phycnt);
> > > + rcar_csi2_write(priv, LINKCNT_REG, LINKCNT_MONITOR_EN |
> > > + LINKCNT_REG_MONI_PACT_EN | LINKCNT_ICLK_NONSTOP);
> > > + rcar_csi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ);
> > > + rcar_csi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ |
> > > + PHYCNT_RSTZ);
> > 
> > Nit: from tables 25.[17-20] it seems to me you do not have to re-issue
> > PHYCNT_SHUTDOWNZ when writing PHYCNT_RSTZ to PHYCNT_REG.
> 
> You are correct, I miss read ' Here, the ENABLE_0 to ENABLE_3 and
> ENABLECLK values set above should be retained' as all previous PHYCNT 
> bits should be retained not just the ones explicitly listed. I will give 
> this a test and if it still works I will remove it for the next version.

This change breaks capture and LP-11 is never detected. So I will 
continue to retain the PHYCNT_SHUTDOWNZ here.

-- 
Regards,
Niklas Söderlund

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[jacopo mondi]

Hi Niklas,

On Mon, Apr 16, 2018 at 01:16:35AM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your feedback.
>
> Comments I have snipped out from this reply are addressed, thanks for
> bringing them to my attention!
>
> On 2018-04-05 11:10:01 +0200, Jacopo Mondi wrote:
>
> [snip]
>
> > > +static int rcar_csi2_wait_phy_start(struct rcar_csi2 *priv)
> > > +{
> > > + int timeout;
> > > +
> > > + /* Wait for the clock and data lanes to enter LP-11 state. */
> > > + for (timeout = 100; timeout > 0; timeout--) {
> > > + const u32 lane_mask = (1 << priv->lanes) - 1;
> > > +
> > > + if ((rcar_csi2_read(priv, PHCLM_REG) & 1) == 1 &&
> >
> > Nitpicking:
> > if ((rcar_csi2_read(priv, PHCLM_REG) & 0x01) &&
> >
> > Don't you prefer to provide defines also for bit fields instead of
> > using magic values? In this case something like
> > PHCLM_REG_STOPSTATE_CLK would do.
>
> Thanks addressed per your and Kieran's suggestion.
>
> >
> > Also, from tables 25.[17-20] it seems to me that for H3 and V3 you
> > have to set INSTATE to an hardcoded value after having validated PHDLM.
> > Maybe it is not necessary, just pointing it out.
>
> I assume you mean Figures 25.[17-20] and not Tables as the last table in
> chapter 25 is Table 25.15 and the register in question is INTSTATE :-)
> And to clarify this is documented for H3 which this driver supports and
> V3H and M3-N which this driver dose not yet support. And the constant
> you are to set it to is ULPS_START | UPLS_END.

Yes, Figures, not Tables, sorry about this.

>
> This is a good catch as this was introduced in a later version of the
> datasheet and the current code where the ULPS_START | UPLS_END is set
> before confirming LP-11 have kept on working. Check the
> priv->info->clear_ulps usage in rcar_csi2_start(). I do think it's
> better to follow the flow-chart in the new datasheet so I will move this
> to the end of rcar_csi2_start() to reflect that (provided that the end
> result still works :-) Thanks for pointing this out!
>

I see...

Actually, I don't see M3-N in the manual version I'm looking at.
Anyway, I just hope this per-soc specificities are limited.

> [snip]
>
> > > +static int rcar_csi2_start(struct rcar_csi2 *priv)
> > > +{
> > > + const struct rcar_csi2_format *format;
> > > + u32 phycnt, phypll, vcdt = 0, vcdt2 = 0;
> > > + unsigned int i;
> > > + int ret;
> > > +
> > > + dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
> > > + priv->mf.width, priv->mf.height,
> > > + priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i');
> > > +
> > > + /* Code is validated in set_fmt */
> > > + format = rcar_csi2_code_to_fmt(priv->mf.code);
> > > +
> > > + /*
> > > +  * Enable all Virtual Channels
> > > +  *
> > > +  * NOTE: It's not possible to get individual datatype for each
> > > +  *   source virtual channel. Once this is possible in V4L2
> > > +  *   it should be used here.
> > > +  */
> > > + for (i = 0; i < 4; i++) {
> > > + u32 vcdt_part;
> > > +
> > > + vcdt_part = VCDT_SEL_VC(i) | VCDT_VCDTN_EN | VCDT_SEL_DTN_ON |
> > > + VCDT_SEL_DT(format->datatype);
> > > +
> > > + /* Store in correct reg and offset */
> > > + if (i < 2)
> > > + vcdt |= vcdt_part << ((i % 2) * 16);
> > > + else
> > > + vcdt2 |= vcdt_part << ((i % 2) * 16);
> > > + }
> > > +
> > > + switch (priv->lanes) {
> > > + case 1:
> > > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0;
> > > + break;
> > > + case 2:
> > > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0;
> > > + break;
> > > + case 4:
> > > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2 |
> > > + PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0;
> > > + break;
> >
> > Even simpler this could be written as
> >
> > phycnt = PHYCNT_ENABLECLK | (1 << priv->lanes) - 1;
>
> Fixed per your and Geert's suggestion.
>
> >
> > > + default:
> > > + return -EINVAL;
> >
> > Can this happen? You have validated priv->lanes already when parsing
> > DT
>
> This can't happen but I like to have a catch all in any case, but since
> I took yours and Geert's suggestion above this issue goes away :-)
>

Does gcc complains about the missing default case?

> >
> > > + }
> > > +
> > > + ret = rcar_csi2_calc_phypll(priv, format->bpp, );
> > > + if (ret)
> > > + return ret;
> > > +
> > > + /* Clear Ultra Low Power interrupt */
> > > + if (priv->info->clear_ulps)
> > > + rcar_csi2_write(priv, INTSTATE_REG,
> > > + INTSTATE_INT_ULPS_START |
> > > + INTSTATE_INT_ULPS_END);
> > > +
> > > + /* Init */
> > > + rcar_csi2_write(priv, TREF_REG, TREF_TREF);
> > > + rcar_csi2_reset(priv);
> > > + rcar_csi2_write(priv, PHTC_REG, 0);
> > > +
> > > + /* Configure */
> > > + rcar_csi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 |
> > > + FLD_FLD_EN3 | 

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Sakari Ailus]

Hi Niklas,

On Sun, Apr 15, 2018 at 10:47:37PM +0200, Niklas Söderlund wrote:
> Hi Sakari,
> 
> Thanks for your feedback.
> 
> On 2018-04-04 23:13:57 +0300, Sakari Ailus wrote:
> 
> [snip]
> 
> > > > +   pm_runtime_enable(>dev);
> > > 
> > > Is CONFIG_PM mandatory on Renesas SoCs? If not, you end up with the
> > > device uninitialised at probe, and pm_runtime_get_sync will not
> > > initialise it either if CONFIG_PM is not enabled. I guess you could
> > > call your runtime_resume function unconditionally, and mark the device
> > > as active in runtime_pm using pm_runtime_set_active.
> > 
> > There doesn't seem to be any runtime_resume function. Was there supposed
> > to be one?
> 
> No there is not suppose to be one.

Ok.

> 
> > 
> > Assuming runtime PM would actually do something here, you might add
> > pm_runtime_idle() to power the device off after probing.
> > 
> > I guess pm_runtime_set_active() should precede pm_runtime_enable().
> 
> The CSI-2 is in the always on power domain so the calls to 
> pm_runtime_get_sync() and pm_runtime_put() are there in the s_stream() 
> callback to enable and disable the module clock. I'm no expert on PM but 
> in my testing the pm_ calls in this driver seems to be correct.
> 
> 1. In probe I call pm_runtime_enable(). And rudimentary tests shows the 
>clock is off (but I might miss something) as I wish it to be until 
>stream on time.
> 2. In s_stream() I call pm_runtime_get_sync() before writing any 
>register when starting a stream. And likewise I call pm_runtime_put() 
>when stopping and I no longer need to write to a register.
> 3. In remove() I call pm_runtime_disable().
> 
> Am I missing something obvious here?

Looking at the code again, it seems fine in this respect.

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Niklas Söderlund]

Hi Jacopo,

Thanks for your feedback.

Comments I have snipped out from this reply are addressed, thanks for 
bringing them to my attention!

On 2018-04-05 11:10:01 +0200, Jacopo Mondi wrote:

[snip]

> > +static int rcar_csi2_wait_phy_start(struct rcar_csi2 *priv)
> > +{
> > +   int timeout;
> > +
> > +   /* Wait for the clock and data lanes to enter LP-11 state. */
> > +   for (timeout = 100; timeout > 0; timeout--) {
> > +   const u32 lane_mask = (1 << priv->lanes) - 1;
> > +
> > +   if ((rcar_csi2_read(priv, PHCLM_REG) & 1) == 1 &&
> 
> Nitpicking:
>   if ((rcar_csi2_read(priv, PHCLM_REG) & 0x01) &&
> 
> Don't you prefer to provide defines also for bit fields instead of
> using magic values? In this case something like
> PHCLM_REG_STOPSTATE_CLK would do.

Thanks addressed per your and Kieran's suggestion.

> 
> Also, from tables 25.[17-20] it seems to me that for H3 and V3 you
> have to set INSTATE to an hardcoded value after having validated PHDLM.
> Maybe it is not necessary, just pointing it out.

I assume you mean Figures 25.[17-20] and not Tables as the last table in 
chapter 25 is Table 25.15 and the register in question is INTSTATE :-) 
And to clarify this is documented for H3 which this driver supports and 
V3H and M3-N which this driver dose not yet support. And the constant 
you are to set it to is ULPS_START | UPLS_END.

This is a good catch as this was introduced in a later version of the 
datasheet and the current code where the ULPS_START | UPLS_END is set 
before confirming LP-11 have kept on working. Check the 
priv->info->clear_ulps usage in rcar_csi2_start(). I do think it's 
better to follow the flow-chart in the new datasheet so I will move this 
to the end of rcar_csi2_start() to reflect that (provided that the end 
result still works :-) Thanks for pointing this out!

[snip]

> > +static int rcar_csi2_start(struct rcar_csi2 *priv)
> > +{
> > +   const struct rcar_csi2_format *format;
> > +   u32 phycnt, phypll, vcdt = 0, vcdt2 = 0;
> > +   unsigned int i;
> > +   int ret;
> > +
> > +   dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
> > +   priv->mf.width, priv->mf.height,
> > +   priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i');
> > +
> > +   /* Code is validated in set_fmt */
> > +   format = rcar_csi2_code_to_fmt(priv->mf.code);
> > +
> > +   /*
> > +* Enable all Virtual Channels
> > +*
> > +* NOTE: It's not possible to get individual datatype for each
> > +*   source virtual channel. Once this is possible in V4L2
> > +*   it should be used here.
> > +*/
> > +   for (i = 0; i < 4; i++) {
> > +   u32 vcdt_part;
> > +
> > +   vcdt_part = VCDT_SEL_VC(i) | VCDT_VCDTN_EN | VCDT_SEL_DTN_ON |
> > +   VCDT_SEL_DT(format->datatype);
> > +
> > +   /* Store in correct reg and offset */
> > +   if (i < 2)
> > +   vcdt |= vcdt_part << ((i % 2) * 16);
> > +   else
> > +   vcdt2 |= vcdt_part << ((i % 2) * 16);
> > +   }
> > +
> > +   switch (priv->lanes) {
> > +   case 1:
> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0;
> > +   break;
> > +   case 2:
> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0;
> > +   break;
> > +   case 4:
> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2 |
> > +   PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0;
> > +   break;
> 
> Even simpler this could be written as
> 
> phycnt = PHYCNT_ENABLECLK | (1 << priv->lanes) - 1;

Fixed per your and Geert's suggestion.

> 
> > +   default:
> > +   return -EINVAL;
> 
> Can this happen? You have validated priv->lanes already when parsing
> DT

This can't happen but I like to have a catch all in any case, but since 
I took yours and Geert's suggestion above this issue goes away :-)

> 
> > +   }
> > +
> > +   ret = rcar_csi2_calc_phypll(priv, format->bpp, );
> > +   if (ret)
> > +   return ret;
> > +
> > +   /* Clear Ultra Low Power interrupt */
> > +   if (priv->info->clear_ulps)
> > +   rcar_csi2_write(priv, INTSTATE_REG,
> > +   INTSTATE_INT_ULPS_START |
> > +   INTSTATE_INT_ULPS_END);
> > +
> > +   /* Init */
> > +   rcar_csi2_write(priv, TREF_REG, TREF_TREF);
> > +   rcar_csi2_reset(priv);
> > +   rcar_csi2_write(priv, PHTC_REG, 0);
> > +
> > +   /* Configure */
> > +   rcar_csi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 |
> > +   FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN);
> 
> On the FLD_FLD_NUM(2) mask. Why 2?
> I read on the datasheet "the register must not be changed from default
> value" and I read defaul to be 0x

This is based on feedback from Renesas. The register is not properly 
documented. I'm working on improving it but for now I would like to keep 
it as FLD_FLD_NUM(2) and make a neater and documented fix in a follow up 
commit. In short it 

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

I have addressed all your comment's but one for the next version. Please 
indicate if you are fine with this and I can still keep your review tag 
:-)

On 2018-04-04 18:15:16 +0300, Laurent Pinchart wrote:

[snip]

> > +static int rcar_csi2_start(struct rcar_csi2 *priv)
> > +{
> > +   const struct rcar_csi2_format *format;
> > +   u32 phycnt, phypll, vcdt = 0, vcdt2 = 0;
> > +   unsigned int i;
> > +   int ret;
> > +
> > +   dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
> > +   priv->mf.width, priv->mf.height,
> > +   priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i');
> > +
> > +   /* Code is validated in set_fmt */
> > +   format = rcar_csi2_code_to_fmt(priv->mf.code);
> 
> You could store the format pointer iin the rcar_csi2 structure to avoid 
> looking it up here.

I could do that, but then I would duplicate the storage of the code 
between the two cached values priv->mf and priv->. I could find that acceptable but if you don't strongly 
disagree I would prefer to keep the current way of looking it up here 
:-)

[snip]

> > +static int rcar_csi2_probe_resources(struct rcar_csi2 *priv,
> > +struct platform_device *pdev)
> > +{
> > +   struct resource *res;
> > +   int irq;
> > +
> > +   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +   priv->base = devm_ioremap_resource(>dev, res);
> > +   if (IS_ERR(priv->base))
> > +   return PTR_ERR(priv->base);
> > +
> > +   irq = platform_get_irq(pdev, 0);
> > +   if (irq < 0)
> > +   return irq;
> 
> You don't seem to use the IRQ. Is this meant to catch invalid DT that don't 
> specify an IRQ, to make sure we'll always have one available when we'll need 
> to later ?

Yes, as you deducted this is currently only to catch invalid DT. In the 
DT documentation I list it as a mandatory property. I think there might 
be potential use-case to at some point add interrupt support of for the 
some of the error interrupts which can be enabled, specially now when we 
seen similar patches for VIN floating around.

> > +
> > +   return 0;
> > +
> > +error:
> > +   v4l2_async_notifier_unregister(>notifier);
> > +   v4l2_async_notifier_cleanup(>notifier);
> > +
> > +   return ret;
> > +}
> 
> [snip]
> 
> With these small issues fixed and Kieran's and Maxime's comments addressed as 
> you see fit,
> 
> Reviewed-by: Laurent Pinchart 

Thanks, I will hold of adding it until you indicate if you are OK with 
the one comment I'm not fully agreeing with you on.

-- 
Regards,
Niklas Söderlund

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Niklas Söderlund]

Hi Sakari,

Thanks for your feedback.

On 2018-04-04 23:13:57 +0300, Sakari Ailus wrote:

[snip]

> > > + pm_runtime_enable(>dev);
> > 
> > Is CONFIG_PM mandatory on Renesas SoCs? If not, you end up with the
> > device uninitialised at probe, and pm_runtime_get_sync will not
> > initialise it either if CONFIG_PM is not enabled. I guess you could
> > call your runtime_resume function unconditionally, and mark the device
> > as active in runtime_pm using pm_runtime_set_active.
> 
> There doesn't seem to be any runtime_resume function. Was there supposed
> to be one?

No there is not suppose to be one.

> 
> Assuming runtime PM would actually do something here, you might add
> pm_runtime_idle() to power the device off after probing.
> 
> I guess pm_runtime_set_active() should precede pm_runtime_enable().

The CSI-2 is in the always on power domain so the calls to 
pm_runtime_get_sync() and pm_runtime_put() are there in the s_stream() 
callback to enable and disable the module clock. I'm no expert on PM but 
in my testing the pm_ calls in this driver seems to be correct.

1. In probe I call pm_runtime_enable(). And rudimentary tests shows the 
   clock is off (but I might miss something) as I wish it to be until 
   stream on time.
2. In s_stream() I call pm_runtime_get_sync() before writing any 
   register when starting a stream. And likewise I call pm_runtime_put() 
   when stopping and I no longer need to write to a register.
3. In remove() I call pm_runtime_disable().

Am I missing something obvious here?

-- 
Regards,
Niklas Söderlund

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Niklas Söderlund]

Hi Geert and Laurent,

Thanks for the feedback.

On 2018-04-05 11:26:45 +0300, Laurent Pinchart wrote:

[snip]

> > Alternatively, you could check for a valid number of lanes, and use
> > knowledge about the internal lane bits:
> > 
> > phycnt = PHYCNT_ENABLECLK;
> > phycnt |= (1 << priv->lanes) - 1;
> 
> If Niklas is fine with that, I like it too.

I'm fine what that thanks for the suggestion Geert!

-- 
Regards,
Niklas Söderlund

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Niklas Söderlund]

Hi Maxime,

Thanks for your feedback.

On 2018-03-29 13:30:39 +0200, Maxime Ripard wrote:
> Hi Niklas,
> 
> On Tue, Feb 13, 2018 at 12:01:32AM +0100, Niklas Söderlund wrote:
> > +   switch (priv->lanes) {
> > +   case 1:
> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0;
> > +   break;
> > +   case 2:
> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0;
> > +   break;
> > +   case 4:
> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2 |
> > +   PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0;
> > +   break;
> > +   default:
> > +   return -EINVAL;
> > +   }
> 
> I guess you could have a simpler construct here using this:
> 
> phycnt = PHYCNT_ENABLECLK;
> 
> switch (priv->lanes) {
> case 4:
>   phycnt |= PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2;
> case 2:
>   phycnt |= PHYCNT_ENABLE_1;
> case 1:
>   phycnt |= PHYCNT_ENABLE_0;
>   break;
> 
> default:
>   return -EINVAL;
> }
> 
> But that's really up to you.

Thanks for the suggestion and sparking of the discussion, I think I will 
go with Geert at.al approach of:

phycnt = PHYCNT_ENABLECLK;
phycnt |= (1 << priv->lanes) - 1;

> 
> > +static int rcar_csi2_probe(struct platform_device *pdev)
> > +{
> > +   const struct soc_device_attribute *attr;
> > +   struct rcar_csi2 *priv;
> > +   unsigned int i;
> > +   int ret;
> > +
> > +   priv = devm_kzalloc(>dev, sizeof(*priv), GFP_KERNEL);
> > +   if (!priv)
> > +   return -ENOMEM;
> > +
> > +   priv->info = of_device_get_match_data(>dev);
> > +
> > +   /* r8a7795 ES1.x behaves different then ES2.0+ but no own compat */
> > +   attr = soc_device_match(r8a7795es1);
> > +   if (attr)
> > +   priv->info = attr->data;
> > +
> > +   priv->dev = >dev;
> > +
> > +   mutex_init(>lock);
> > +   priv->stream_count = 0;
> > +
> > +   ret = rcar_csi2_probe_resources(priv, pdev);
> > +   if (ret) {
> > +   dev_err(priv->dev, "Failed to get resources\n");
> > +   return ret;
> > +   }
> > +
> > +   platform_set_drvdata(pdev, priv);
> > +
> > +   ret = rcar_csi2_parse_dt(priv);
> > +   if (ret)
> > +   return ret;
> > +
> > +   priv->subdev.owner = THIS_MODULE;
> > +   priv->subdev.dev = >dev;
> > +   v4l2_subdev_init(>subdev, _csi2_subdev_ops);
> > +   v4l2_set_subdevdata(>subdev, >dev);
> > +   snprintf(priv->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s %s",
> > +KBUILD_MODNAME, dev_name(>dev));
> > +   priv->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> > +
> > +   priv->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> > +   priv->subdev.entity.ops = _csi2_entity_ops;
> > +
> > +   priv->pads[RCAR_CSI2_SINK].flags = MEDIA_PAD_FL_SINK;
> > +   for (i = RCAR_CSI2_SOURCE_VC0; i < NR_OF_RCAR_CSI2_PAD; i++)
> > +   priv->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> > +
> > +   ret = media_entity_pads_init(>subdev.entity, NR_OF_RCAR_CSI2_PAD,
> > +priv->pads);
> > +   if (ret)
> > +   goto error;
> > +
> > +   pm_runtime_enable(>dev);
> 
> Is CONFIG_PM mandatory on Renesas SoCs? If not, you end up with the
> device uninitialised at probe, and pm_runtime_get_sync will not
> initialise it either if CONFIG_PM is not enabled. I guess you could
> call your runtime_resume function unconditionally, and mark the device
> as active in runtime_pm using pm_runtime_set_active.

Yes CONFIG_PM is selected by ARCH_RENESAS. Thanks for letting me know 
about this in any case I was not aware of this behavior in the case 
CONFIG_PM where not enabled.

> 
> Looks good otherwise, once fixed (and if relevant):
> Reviewed-by: Maxime Ripard 

Thanks!

-- 
Regards,
Niklas Söderlund

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2018-04-04 18:25:23 +0300, Laurent Pinchart wrote:

[snip]

> > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > b/drivers/media/platform/rcar-vin/rcar-csi2.c new file mode 100644
> > > index ..c0c2a763151bc928
> > > --- /dev/null
> > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > @@ -0,0 +1,884 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> 
> Do you intend making it GPL-2.0+ or did you mean GPL-2.0 ?

Wops I intended to make it GPL-2.0 thanks for catching this.

-- 
Regards,
Niklas Söderlund

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Niklas Söderlund]

Hi Kieran,

Thanks for your feedback.

On 2018-03-13 23:23:49 +0100, Kieran Bingham wrote:
> Hi Niklas
> 
> Thank you for this patch ...
> I know it has been a lot of work getting this and the VIN together!
> 
> On 13/02/18 00:01, Niklas Söderlund wrote:
> > A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver
> > supports the R-Car Gen3 SoCs where separate CSI-2 hardware blocks are
> > connected between the video sources and the video grabbers (VIN).
> > 
> > Driver is based on a prototype by Koji Matsuoka in the Renesas BSP.
> > 
> > Signed-off-by: Niklas Söderlund 
> > Reviewed-by: Hans Verkuil 
> 
> I don't think there's actually anything major in the below - so with any
> comments addressed, or specifically ignored you can add my:
> 
> Reviewed-by: Kieran Bingham 

Thanks, see bellow for answers to your questions. I have removed the 
questions Laurent already have provided a reply for, thanks for doing 
that!

> 
> tag if you like.
> 
> 
> > ---
> >  drivers/media/platform/rcar-vin/Kconfig |  12 +
> >  drivers/media/platform/rcar-vin/Makefile|   1 +
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 884 
> > 
> >  3 files changed, 897 insertions(+)
> >  create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c
> > 
> > diff --git a/drivers/media/platform/rcar-vin/Kconfig 
> > b/drivers/media/platform/rcar-vin/Kconfig
> > index af4c98b44d2e22cb..6875f30c1ae42631 100644
> > --- a/drivers/media/platform/rcar-vin/Kconfig
> > +++ b/drivers/media/platform/rcar-vin/Kconfig
> > @@ -1,3 +1,15 @@
> > +config VIDEO_RCAR_CSI2
> > +   tristate "R-Car MIPI CSI-2 Receiver"
> > +   depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && OF
> 
> I think I recall recently seeing that depending upon OF is redundant and not
> necessary (though I think that's minor in this instance)

I can't seem to find anything to indicate this, but as you state if 
there is such a transition ongoing we can delete this later IMHO.

$ find . -name Kconfig -exec grep OF {} \; | grep "depends on" | wc -l
622

But I'm happy to be proven wrong and remove it now as well.

[snip]

> > +static const struct rcar_csi2_format 
> > *rcar_csi2_code_to_fmt(unsigned int code)
> > +{
> > +   unsigned int i;
> > +
> > +   for (i = 0; i < ARRAY_SIZE(rcar_csi2_formats); i++)
> > +   if (rcar_csi2_formats[i].code == code)
> > +   return rcar_csi2_formats + i;
> 
> I would have written this as:
>   return _csi2_formats[i];  but I think your way is valid too :)

This seems to be the popular opinion among reviewers so I will adopt 
this style for the next version :-)


> 
> I have a nice 'for_each_entity_in_array' macro I keep meaning to post which
> would be nice here too - but hey - for another day.

Looking forward to it.

[snip]

> > +static int rcar_csi2_wait_phy_start(struct rcar_csi2 *priv)
> > +{
> > +   int timeout;
> > +
> > +   /* Wait for the clock and data lanes to enter LP-11 state. */
> > +   for (timeout = 100; timeout > 0; timeout--) {
> > +   const u32 lane_mask = (1 << priv->lanes) - 1;
> > +
> > +   if ((rcar_csi2_read(priv, PHCLM_REG) & 1) == 1 &&
> 
> The '1' is not very clear here.. Could this be:
> 
>   if ((rcar_csi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATE) &&

Great catch I have clearly missed this define. I will call it 
PHCLM_STOPSTATECLK since that it's what the latest datasheet I have 
calls it. Thanks for pointing this out.

[snip]

> > +static int rcar_csi2_s_stream(struct v4l2_subdev *sd, int enable)
> > +{
> > +   struct rcar_csi2 *priv = sd_to_csi2(sd);
> > +   struct v4l2_subdev *nextsd;
> > +   int ret = 0;
> > +
> > +   mutex_lock(>lock);
> > +
> > +   if (!priv->remote) {
> > +   ret = -ENODEV;
> > +   goto out;
> > +   }
> > +
> > +   nextsd = priv->remote;
> > +
> > +   if (enable && priv->stream_count == 0) {
> > +   pm_runtime_get_sync(priv->dev);
> > +
> > +   ret = rcar_csi2_start(priv);
> > +   if (ret) {
> > +   pm_runtime_put(priv->dev);
> > +   goto out;
> > +   }
> > +
> > +   ret = v4l2_subdev_call(nextsd, video, s_stream, 1);
> 
> Would it be nicer to pass 'enable' through instead of the '1'? (of course
> enable==1 here)
> 
> > +   if (ret) {
> > +   rcar_csi2_stop(priv);
> > +   pm_runtime_put(priv->dev);
> > +   goto out;
> > +   }
> > +   } else if (!enable && priv->stream_count == 1) {
> > +   rcar_csi2_stop(priv);
> > +   v4l2_subdev_call(nextsd, video, s_stream, 0);
> 
> likewise here...
> 

I agree with Laurent's comment here that it's much clearer to use 1/0 
instead of 'enable' when it comes to read the code and will keep it as 
such :-)

> > +   pm_runtime_put(priv->dev);
> > +   }
> 
> What's the 'nextsd' in this 

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[jacopo mondi]

A few corrections,

On Thu, Apr 05, 2018 at 11:10:01AM +0200, jacopo mondi wrote:
> Hi Niklas,
> thanks for the VIN and CSI-2 effort!
>
> On Tue, Feb 13, 2018 at 12:01:32AM +0100, Niklas Söderlund wrote:
> > A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver
> > supports the R-Car Gen3 SoCs where separate CSI-2 hardware blocks are
> > connected between the video sources and the video grabbers (VIN).
> >
> > Driver is based on a prototype by Koji Matsuoka in the Renesas BSP.
> >
> > Signed-off-by: Niklas Söderlund 
> > Reviewed-by: Hans Verkuil 
> > ---
> >  drivers/media/platform/rcar-vin/Kconfig |  12 +
> >  drivers/media/platform/rcar-vin/Makefile|   1 +
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 884 
> > 
> >  3 files changed, 897 insertions(+)
> >  create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c
> >
>
> [snip]
>
> > +
> > +static const struct rcar_csi2_format rcar_csi2_formats[] = {
> > +   { .code = MEDIA_BUS_FMT_RGB888_1X24,.datatype = 0x24, .bpp = 24 },
> > +   { .code = MEDIA_BUS_FMT_UYVY8_1X16, .datatype = 0x1e, .bpp = 16 },
> > +   { .code = MEDIA_BUS_FMT_UYVY8_2X8,  .datatype = 0x1e, .bpp = 16 },
> > +   { .code = MEDIA_BUS_FMT_YUYV10_2X10,.datatype = 0x1e, .bpp = 16 },
>
> Shouldn't YUYV10_2X10 format have 20 bits per pixel?
>
> > +};
> > +
> > +static const struct rcar_csi2_format *rcar_csi2_code_to_fmt(unsigned int 
> > code)
> > +{
> > +   unsigned int i;
> > +
> > +   for (i = 0; i < ARRAY_SIZE(rcar_csi2_formats); i++)
> > +   if (rcar_csi2_formats[i].code == code)
> > +   return rcar_csi2_formats + i;
> > +
> > +   return NULL;
> > +}
> > +
> > +enum rcar_csi2_pads {
> > +   RCAR_CSI2_SINK,
> > +   RCAR_CSI2_SOURCE_VC0,
> > +   RCAR_CSI2_SOURCE_VC1,
> > +   RCAR_CSI2_SOURCE_VC2,
> > +   RCAR_CSI2_SOURCE_VC3,
> > +   NR_OF_RCAR_CSI2_PAD,
> > +};
> > +
> > +struct rcar_csi2_info {
> > +   const struct phypll_hsfreqrange *hsfreqrange;
> > +   unsigned int csi0clkfreqrange;
> > +   bool clear_ulps;
> > +   bool init_phtw;
> > +};
> > +
> > +struct rcar_csi2 {
> > +   struct device *dev;
> > +   void __iomem *base;
> > +   const struct rcar_csi2_info *info;
> > +
> > +   struct v4l2_subdev subdev;
> > +   struct media_pad pads[NR_OF_RCAR_CSI2_PAD];
> > +
> > +   struct v4l2_async_notifier notifier;
> > +   struct v4l2_async_subdev asd;
> > +   struct v4l2_subdev *remote;
> > +
> > +   struct v4l2_mbus_framefmt mf;
> > +
> > +   struct mutex lock;
> > +   int stream_count;
> > +
> > +   unsigned short lanes;
> > +   unsigned char lane_swap[4];
> > +};
> > +
> > +static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
> > +{
> > +   return container_of(sd, struct rcar_csi2, subdev);
> > +}
> > +
> > +static inline struct rcar_csi2 *notifier_to_csi2(struct 
> > v4l2_async_notifier *n)
> > +{
> > +   return container_of(n, struct rcar_csi2, notifier);
> > +}
> > +
> > +static u32 rcar_csi2_read(struct rcar_csi2 *priv, unsigned int reg)
> > +{
> > +   return ioread32(priv->base + reg);
> > +}
> > +
> > +static void rcar_csi2_write(struct rcar_csi2 *priv, unsigned int reg, u32 
> > data)
> > +{
> > +   iowrite32(data, priv->base + reg);
> > +}
> > +
> > +static void rcar_csi2_reset(struct rcar_csi2 *priv)
> > +{
> > +   rcar_csi2_write(priv, SRST_REG, SRST_SRST);
> > +   usleep_range(100, 150);
> > +   rcar_csi2_write(priv, SRST_REG, 0);
> > +}
> > +
> > +static int rcar_csi2_wait_phy_start(struct rcar_csi2 *priv)
> > +{
> > +   int timeout;
> > +
> > +   /* Wait for the clock and data lanes to enter LP-11 state. */
> > +   for (timeout = 100; timeout > 0; timeout--) {
> > +   const u32 lane_mask = (1 << priv->lanes) - 1;
> > +
> > +   if ((rcar_csi2_read(priv, PHCLM_REG) & 1) == 1 &&
>
> Nitpicking:
>   if ((rcar_csi2_read(priv, PHCLM_REG) & 0x01) &&
>
> Don't you prefer to provide defines also for bit fields instead of
> using magic values? In this case something like
> PHCLM_REG_STOPSTATE_CLK would do.
>
> Also, from tables 25.[17-20] it seems to me that for H3 and V3 you
> have to set INSTATE to an hardcoded value after having validated PHDLM.
> Maybe it is not necessary, just pointing it out.
>
> > +   (rcar_csi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask)
> > +   return 0;
> > +
> > +   msleep(20);
> > +   }
> > +
> > +   dev_err(priv->dev, "Timeout waiting for LP-11 state\n");
> > +
> > +   return -ETIMEDOUT;
> > +}
> > +
> > +static int rcar_csi2_calc_phypll(struct rcar_csi2 *priv, unsigned int bpp,
> > +u32 *phypll)
> > +{
> > +   const struct phypll_hsfreqrange *hsfreq;
> > +   struct v4l2_subdev *source;
> > +   struct v4l2_ctrl *ctrl;
> > +   u64 mbps;
> > +
> > +   if (!priv->remote)
> > +   return -ENODEV;
> > +
> > +   source = priv->remote;
> > +
> > +   /* Read the pixel rate control from 

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[jacopo mondi]

Hi Niklas,
thanks for the VIN and CSI-2 effort!

On Tue, Feb 13, 2018 at 12:01:32AM +0100, Niklas Söderlund wrote:
> A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver
> supports the R-Car Gen3 SoCs where separate CSI-2 hardware blocks are
> connected between the video sources and the video grabbers (VIN).
>
> Driver is based on a prototype by Koji Matsuoka in the Renesas BSP.
>
> Signed-off-by: Niklas Söderlund 
> Reviewed-by: Hans Verkuil 
> ---
>  drivers/media/platform/rcar-vin/Kconfig |  12 +
>  drivers/media/platform/rcar-vin/Makefile|   1 +
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 884 
> 
>  3 files changed, 897 insertions(+)
>  create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c
>

[snip]

> +
> +static const struct rcar_csi2_format rcar_csi2_formats[] = {
> + { .code = MEDIA_BUS_FMT_RGB888_1X24,.datatype = 0x24, .bpp = 24 },
> + { .code = MEDIA_BUS_FMT_UYVY8_1X16, .datatype = 0x1e, .bpp = 16 },
> + { .code = MEDIA_BUS_FMT_UYVY8_2X8,  .datatype = 0x1e, .bpp = 16 },
> + { .code = MEDIA_BUS_FMT_YUYV10_2X10,.datatype = 0x1e, .bpp = 16 },

Shouldn't YUYV10_2X10 format have 20 bits per pixel?

> +};
> +
> +static const struct rcar_csi2_format *rcar_csi2_code_to_fmt(unsigned int 
> code)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(rcar_csi2_formats); i++)
> + if (rcar_csi2_formats[i].code == code)
> + return rcar_csi2_formats + i;
> +
> + return NULL;
> +}
> +
> +enum rcar_csi2_pads {
> + RCAR_CSI2_SINK,
> + RCAR_CSI2_SOURCE_VC0,
> + RCAR_CSI2_SOURCE_VC1,
> + RCAR_CSI2_SOURCE_VC2,
> + RCAR_CSI2_SOURCE_VC3,
> + NR_OF_RCAR_CSI2_PAD,
> +};
> +
> +struct rcar_csi2_info {
> + const struct phypll_hsfreqrange *hsfreqrange;
> + unsigned int csi0clkfreqrange;
> + bool clear_ulps;
> + bool init_phtw;
> +};
> +
> +struct rcar_csi2 {
> + struct device *dev;
> + void __iomem *base;
> + const struct rcar_csi2_info *info;
> +
> + struct v4l2_subdev subdev;
> + struct media_pad pads[NR_OF_RCAR_CSI2_PAD];
> +
> + struct v4l2_async_notifier notifier;
> + struct v4l2_async_subdev asd;
> + struct v4l2_subdev *remote;
> +
> + struct v4l2_mbus_framefmt mf;
> +
> + struct mutex lock;
> + int stream_count;
> +
> + unsigned short lanes;
> + unsigned char lane_swap[4];
> +};
> +
> +static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
> +{
> + return container_of(sd, struct rcar_csi2, subdev);
> +}
> +
> +static inline struct rcar_csi2 *notifier_to_csi2(struct v4l2_async_notifier 
> *n)
> +{
> + return container_of(n, struct rcar_csi2, notifier);
> +}
> +
> +static u32 rcar_csi2_read(struct rcar_csi2 *priv, unsigned int reg)
> +{
> + return ioread32(priv->base + reg);
> +}
> +
> +static void rcar_csi2_write(struct rcar_csi2 *priv, unsigned int reg, u32 
> data)
> +{
> + iowrite32(data, priv->base + reg);
> +}
> +
> +static void rcar_csi2_reset(struct rcar_csi2 *priv)
> +{
> + rcar_csi2_write(priv, SRST_REG, SRST_SRST);
> + usleep_range(100, 150);
> + rcar_csi2_write(priv, SRST_REG, 0);
> +}
> +
> +static int rcar_csi2_wait_phy_start(struct rcar_csi2 *priv)
> +{
> + int timeout;
> +
> + /* Wait for the clock and data lanes to enter LP-11 state. */
> + for (timeout = 100; timeout > 0; timeout--) {
> + const u32 lane_mask = (1 << priv->lanes) - 1;
> +
> + if ((rcar_csi2_read(priv, PHCLM_REG) & 1) == 1 &&

Nitpicking:
if ((rcar_csi2_read(priv, PHCLM_REG) & 0x01) &&

Don't you prefer to provide defines also for bit fields instead of
using magic values? In this case something like
PHCLM_REG_STOPSTATE_CLK would do.

Also, from tables 25.[17-20] it seems to me that for H3 and V3 you
have to set INSTATE to an hardcoded value after having validated PHDLM.
Maybe it is not necessary, just pointing it out.

> + (rcar_csi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask)
> + return 0;
> +
> + msleep(20);
> + }
> +
> + dev_err(priv->dev, "Timeout waiting for LP-11 state\n");
> +
> + return -ETIMEDOUT;
> +}
> +
> +static int rcar_csi2_calc_phypll(struct rcar_csi2 *priv, unsigned int bpp,
> +  u32 *phypll)
> +{
> + const struct phypll_hsfreqrange *hsfreq;
> + struct v4l2_subdev *source;
> + struct v4l2_ctrl *ctrl;
> + u64 mbps;
> +
> + if (!priv->remote)
> + return -ENODEV;
> +
> + source = priv->remote;
> +
> + /* Read the pixel rate control from remote */
> + ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE);
> + if (!ctrl) {
> + dev_err(priv->dev, "no pixel rate control in subdev %s\n",
> + source->name);
> + return -EINVAL;
> + }
> +
> +  

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Laurent Pinchart]

Hi Geert,

On Thursday, 5 April 2018 10:33:55 EEST Geert Uytterhoeven wrote:
> On Wed, Apr 4, 2018 at 5:26 PM, Laurent Pinchart wrote:
> > On Thursday, 29 March 2018 14:30:39 EEST Maxime Ripard wrote:
> >> On Tue, Feb 13, 2018 at 12:01:32AM +0100, Niklas Söderlund wrote:
> >> > +   switch (priv->lanes) {
> >> > +   case 1:
> >> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0;
> >> > +   break;
> >> > +   case 2:
> >> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 |
> >> > PHYCNT_ENABLE_0;
> >> > +   break;
> >> > +   case 4:
> >> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 |
> >> > PHYCNT_ENABLE_2 |
> >> > +   PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0;
> >> > +   break;
> >> > +   default:
> >> > +   return -EINVAL;
> >> > +   }
> >> 
> >> I guess you could have a simpler construct here using this:
> >> 
> >> phycnt = PHYCNT_ENABLECLK;
> >> 
> >> switch (priv->lanes) {
> >> 
> >> case 4:
> >>   phycnt |= PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2;
> >> 
> >> case 2:
> >>   phycnt |= PHYCNT_ENABLE_1;
> >> 
> >> case 1:
> >>   phycnt |= PHYCNT_ENABLE_0;
> >>   break;
> >> 
> >> default:
> >>   return -EINVAL;
> >> 
> >> }
> >> 
> >> But that's really up to you.
> > 
> > Wouldn't Niklas' version generate simpler code as it uses direct
> > assignments ?
> Alternatively, you could check for a valid number of lanes, and use
> knowledge about the internal lane bits:
> 
> phycnt = PHYCNT_ENABLECLK;
> phycnt |= (1 << priv->lanes) - 1;

If Niklas is fine with that, I like it too.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Geert Uytterhoeven]

On Wed, Apr 4, 2018 at 5:26 PM, Laurent Pinchart
 wrote:
> On Thursday, 29 March 2018 14:30:39 EEST Maxime Ripard wrote:
>> On Tue, Feb 13, 2018 at 12:01:32AM +0100, Niklas Söderlund wrote:
>> > +   switch (priv->lanes) {
>> > +   case 1:
>> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0;
>> > +   break;
>> > +   case 2:
>> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0;
>> > +   break;
>> > +   case 4:
>> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2 |
>> > +   PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0;
>> > +   break;
>> > +   default:
>> > +   return -EINVAL;
>> > +   }
>>
>> I guess you could have a simpler construct here using this:
>>
>> phycnt = PHYCNT_ENABLECLK;
>>
>> switch (priv->lanes) {
>> case 4:
>>   phycnt |= PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2;
>> case 2:
>>   phycnt |= PHYCNT_ENABLE_1;
>> case 1:
>>   phycnt |= PHYCNT_ENABLE_0;
>>   break;
>>
>> default:
>>   return -EINVAL;
>> }
>>
>> But that's really up to you.
>
> Wouldn't Niklas' version generate simpler code as it uses direct assignments ?

Alternatively, you could check for a valid number of lanes, and use knowledge
about the internal lane bits:

phycnt = PHYCNT_ENABLECLK;
phycnt |= (1 << priv->lanes) - 1;

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Sakari Ailus]

Hi Maxime and Niklas,

On Thu, Mar 29, 2018 at 01:30:39PM +0200, Maxime Ripard wrote:
> Hi Niklas,
> 
> On Tue, Feb 13, 2018 at 12:01:32AM +0100, Niklas Söderlund wrote:
> > +   switch (priv->lanes) {
> > +   case 1:
> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0;
> > +   break;
> > +   case 2:
> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0;
> > +   break;
> > +   case 4:
> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2 |
> > +   PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0;
> > +   break;
> > +   default:
> > +   return -EINVAL;
> > +   }
> 
> I guess you could have a simpler construct here using this:
> 
> phycnt = PHYCNT_ENABLECLK;
> 
> switch (priv->lanes) {
> case 4:
>   phycnt |= PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2;
> case 2:
>   phycnt |= PHYCNT_ENABLE_1;
> case 1:
>   phycnt |= PHYCNT_ENABLE_0;
>   break;
> 
> default:
>   return -EINVAL;
> }
> 
> But that's really up to you.
> 
> > +static int rcar_csi2_probe(struct platform_device *pdev)
> > +{
> > +   const struct soc_device_attribute *attr;
> > +   struct rcar_csi2 *priv;
> > +   unsigned int i;
> > +   int ret;
> > +
> > +   priv = devm_kzalloc(>dev, sizeof(*priv), GFP_KERNEL);
> > +   if (!priv)
> > +   return -ENOMEM;
> > +
> > +   priv->info = of_device_get_match_data(>dev);
> > +
> > +   /* r8a7795 ES1.x behaves different then ES2.0+ but no own compat */
> > +   attr = soc_device_match(r8a7795es1);
> > +   if (attr)
> > +   priv->info = attr->data;
> > +
> > +   priv->dev = >dev;
> > +
> > +   mutex_init(>lock);
> > +   priv->stream_count = 0;
> > +
> > +   ret = rcar_csi2_probe_resources(priv, pdev);
> > +   if (ret) {
> > +   dev_err(priv->dev, "Failed to get resources\n");
> > +   return ret;
> > +   }
> > +
> > +   platform_set_drvdata(pdev, priv);
> > +
> > +   ret = rcar_csi2_parse_dt(priv);
> > +   if (ret)
> > +   return ret;
> > +
> > +   priv->subdev.owner = THIS_MODULE;
> > +   priv->subdev.dev = >dev;
> > +   v4l2_subdev_init(>subdev, _csi2_subdev_ops);
> > +   v4l2_set_subdevdata(>subdev, >dev);
> > +   snprintf(priv->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s %s",
> > +KBUILD_MODNAME, dev_name(>dev));
> > +   priv->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> > +
> > +   priv->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> > +   priv->subdev.entity.ops = _csi2_entity_ops;
> > +
> > +   priv->pads[RCAR_CSI2_SINK].flags = MEDIA_PAD_FL_SINK;
> > +   for (i = RCAR_CSI2_SOURCE_VC0; i < NR_OF_RCAR_CSI2_PAD; i++)
> > +   priv->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> > +
> > +   ret = media_entity_pads_init(>subdev.entity, NR_OF_RCAR_CSI2_PAD,
> > +priv->pads);
> > +   if (ret)
> > +   goto error;
> > +
> > +   pm_runtime_enable(>dev);
> 
> Is CONFIG_PM mandatory on Renesas SoCs? If not, you end up with the
> device uninitialised at probe, and pm_runtime_get_sync will not
> initialise it either if CONFIG_PM is not enabled. I guess you could
> call your runtime_resume function unconditionally, and mark the device
> as active in runtime_pm using pm_runtime_set_active.

There doesn't seem to be any runtime_resume function. Was there supposed
to be one?

Assuming runtime PM would actually do something here, you might add
pm_runtime_idle() to power the device off after probing.

I guess pm_runtime_set_active() should precede pm_runtime_enable().

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Laurent Pinchart]

Hi Maxime,

On Thursday, 29 March 2018 14:30:39 EEST Maxime Ripard wrote:
> On Tue, Feb 13, 2018 at 12:01:32AM +0100, Niklas Söderlund wrote:
> > +   switch (priv->lanes) {
> > +   case 1:
> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0;
> > +   break;
> > +   case 2:
> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0;
> > +   break;
> > +   case 4:
> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2 |
> > +   PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0;
> > +   break;
> > +   default:
> > +   return -EINVAL;
> > +   }
> 
> I guess you could have a simpler construct here using this:
> 
> phycnt = PHYCNT_ENABLECLK;
> 
> switch (priv->lanes) {
> case 4:
>   phycnt |= PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2;
> case 2:
>   phycnt |= PHYCNT_ENABLE_1;
> case 1:
>   phycnt |= PHYCNT_ENABLE_0;
>   break;
> 
> default:
>   return -EINVAL;
> }
> 
> But that's really up to you.

Wouldn't Niklas' version generate simpler code as it uses direct assignments ?

> > +static int rcar_csi2_probe(struct platform_device *pdev)
> > +{
> > +   const struct soc_device_attribute *attr;
> > +   struct rcar_csi2 *priv;
> > +   unsigned int i;
> > +   int ret;
> > +
> > +   priv = devm_kzalloc(>dev, sizeof(*priv), GFP_KERNEL);
> > +   if (!priv)
> > +   return -ENOMEM;
> > +
> > +   priv->info = of_device_get_match_data(>dev);
> > +
> > +   /* r8a7795 ES1.x behaves different then ES2.0+ but no own compat */
> > +   attr = soc_device_match(r8a7795es1);
> > +   if (attr)
> > +   priv->info = attr->data;
> > +
> > +   priv->dev = >dev;
> > +
> > +   mutex_init(>lock);
> > +   priv->stream_count = 0;
> > +
> > +   ret = rcar_csi2_probe_resources(priv, pdev);
> > +   if (ret) {
> > +   dev_err(priv->dev, "Failed to get resources\n");
> > +   return ret;
> > +   }
> > +
> > +   platform_set_drvdata(pdev, priv);
> > +
> > +   ret = rcar_csi2_parse_dt(priv);
> > +   if (ret)
> > +   return ret;
> > +
> > +   priv->subdev.owner = THIS_MODULE;
> > +   priv->subdev.dev = >dev;
> > +   v4l2_subdev_init(>subdev, _csi2_subdev_ops);
> > +   v4l2_set_subdevdata(>subdev, >dev);
> > +   snprintf(priv->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s %s",
> > +KBUILD_MODNAME, dev_name(>dev));
> > +   priv->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> > +
> > +   priv->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> > +   priv->subdev.entity.ops = _csi2_entity_ops;
> > +
> > +   priv->pads[RCAR_CSI2_SINK].flags = MEDIA_PAD_FL_SINK;
> > +   for (i = RCAR_CSI2_SOURCE_VC0; i < NR_OF_RCAR_CSI2_PAD; i++)
> > +   priv->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> > +
> > +   ret = media_entity_pads_init(>subdev.entity, NR_OF_RCAR_CSI2_PAD,
> > +priv->pads);
> > +   if (ret)
> > +   goto error;
> > +
> > +   pm_runtime_enable(>dev);
> 
> Is CONFIG_PM mandatory on Renesas SoCs? If not, you end up with the
> device uninitialised at probe, and pm_runtime_get_sync will not
> initialise it either if CONFIG_PM is not enabled. I guess you could
> call your runtime_resume function unconditionally, and mark the device
> as active in runtime_pm using pm_runtime_set_active.
> 
> Looks good otherwise, once fixed (and if relevant):
> Reviewed-by: Maxime Ripard 

-- 
Regards,

Laurent Pinchart

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Laurent Pinchart]

Hello,

On Wednesday, 14 March 2018 00:23:49 EEST Kieran Bingham wrote:
> Hi Niklas
> 
> Thank you for this patch ...
> I know it has been a lot of work getting this and the VIN together!
> 
> On 13/02/18 00:01, Niklas Söderlund wrote:
> > A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver
> > supports the R-Car Gen3 SoCs where separate CSI-2 hardware blocks are
> > connected between the video sources and the video grabbers (VIN).
> > 
> > Driver is based on a prototype by Koji Matsuoka in the Renesas BSP.
> > 
> > Signed-off-by: Niklas Söderlund 
> > Reviewed-by: Hans Verkuil 
> 
> I don't think there's actually anything major in the below - so with any
> comments addressed, or specifically ignored you can add my:
> 
> Reviewed-by: Kieran Bingham 
> 
> tag if you like.
> 
> > ---
> > 
> >  drivers/media/platform/rcar-vin/Kconfig |  12 +
> >  drivers/media/platform/rcar-vin/Makefile|   1 +
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 884 +++
> >  3 files changed, 897 insertions(+)
> >  create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c

[snip]

> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > b/drivers/media/platform/rcar-vin/rcar-csi2.c new file mode 100644
> > index ..c0c2a763151bc928
> > --- /dev/null
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -0,0 +1,884 @@
> > +// SPDX-License-Identifier: GPL-2.0+

Do you intend making it GPL-2.0+ or did you mean GPL-2.0 ?

> > +/*
> > + * Driver for Renesas R-Car MIPI CSI-2 Receiver
> > + *
> > + * Copyright (C) 2018 Renesas Electronics Corp.
> > + */

[snip]

> > +static const struct rcar_csi2_format *rcar_csi2_code_to_fmt(unsigned int
> > code)
> > +{
> > +   unsigned int i;
> > +
> > +   for (i = 0; i < ARRAY_SIZE(rcar_csi2_formats); i++)
> > +   if (rcar_csi2_formats[i].code == code)
> > +   return rcar_csi2_formats + i;
> 
> I would have written this as:
>   return _csi2_formats[i];  but I think your way is valid too :)

I would have done the same.

> I have a nice 'for_each_entity_in_array' macro I keep meaning to post which
> would be nice here too - but hey - for another day.
> 
> > +
> > +   return NULL;
> > +}

[snip]

> > +static int rcar_csi2_wait_phy_start(struct rcar_csi2 *priv)
> > +{
> > +   int timeout;
> > +
> > +   /* Wait for the clock and data lanes to enter LP-11 state. */
> > +   for (timeout = 100; timeout > 0; timeout--) {
> > +   const u32 lane_mask = (1 << priv->lanes) - 1;
> > +
> > +   if ((rcar_csi2_read(priv, PHCLM_REG) & 1) == 1 &&
> 
> The '1' is not very clear here.. Could this be:
> 
>   if ((rcar_csi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATE) &&

That reads a bit better, yes.

> > +   (rcar_csi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask)
> > +   return 0;
> > +
> > +   msleep(20);
> > +   }
> > +
> > +   dev_err(priv->dev, "Timeout waiting for LP-11 state\n");
> 
> Does LP == ULP ? I presume these mean 'low power' / 'ultra low power' ?
> Although the PHCLM lane mask refers to the STOPSTATE, so I guess this is
> also waiting for the lanes to be idle.

LP-11 refers to the low-power single-ended state where both the + and - lanes 
are at high level. It is the bus idle state for D-PHY. The ultra-low power 
state is ULPS and is different (if I remember correctly the lines go to the 
LP-00 state).

> > +
> > +   return -ETIMEDOUT;
> > +}

[snip]

> > +static int rcar_csi2_start(struct rcar_csi2 *priv)
> > +{
> > +   const struct rcar_csi2_format *format;
> > +   u32 phycnt, phypll, vcdt = 0, vcdt2 = 0;
> > +   unsigned int i;
> > +   int ret;
> > +
> > +   dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
> > +   priv->mf.width, priv->mf.height,
> > +   priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i');
> > +
> > +   /* Code is validated in set_fmt */
> > +   format = rcar_csi2_code_to_fmt(priv->mf.code);
> > +
> > +   /*
> > +* Enable all Virtual Channels
> > +*
> > +* NOTE: It's not possible to get individual datatype for each
> > +*   source virtual channel. Once this is possible in V4L2
> > +*   it should be used here.
> > +*/
> > +   for (i = 0; i < 4; i++) {
> 
> Does 4 represent the maximum number of lanes? or can we have 4 VCs on a
> single lane ?

Virtual channels and data lanes are independent. Of course the more virtual 
channels you need to carry the more bandwidth you will likely need, so you 
might need more data lanes, but there's no direct correlation.

> > +   u32 vcdt_part;
> > +
> > +   vcdt_part = VCDT_SEL_VC(i) | VCDT_VCDTN_EN | VCDT_SEL_DTN_ON |
> > +   VCDT_SEL_DT(format->datatype);
> > +
> > +   /* Store in correct reg and offset */
> > +   if (i < 2)
> > +   vcdt |= vcdt_part << ((i % 2) * 16);
> > +   

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday, 13 February 2018 01:01:32 EEST Niklas Söderlund wrote:
> A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver
> supports the R-Car Gen3 SoCs where separate CSI-2 hardware blocks are
> connected between the video sources and the video grabbers (VIN).
> 
> Driver is based on a prototype by Koji Matsuoka in the Renesas BSP.
> 
> Signed-off-by: Niklas Söderlund 
> Reviewed-by: Hans Verkuil 
> ---
>  drivers/media/platform/rcar-vin/Kconfig |  12 +
>  drivers/media/platform/rcar-vin/Makefile|   1 +
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 884 +
>  3 files changed, 897 insertions(+)
>  create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c

[snip]

> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> b/drivers/media/platform/rcar-vin/rcar-csi2.c new file mode 100644
> index ..c0c2a763151bc928
> --- /dev/null
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c

[snip]

> +static const struct phypll_hsfreqrange hsfreqrange_m3w_h3es1[] = {
> + { .mbps =   80, .reg = 0x00 },
> + { .mbps =   90, .reg = 0x10 },
> + { .mbps =  100, .reg = 0x20 },
> + { .mbps =  110, .reg = 0x30 },
> + { .mbps =  120, .reg = 0x01 },
> + { .mbps =  130, .reg = 0x11 },
> + { .mbps =  140, .reg = 0x21 },
> + { .mbps =  150, .reg = 0x31 },
> + { .mbps =  160, .reg = 0x02 },
> + { .mbps =  170, .reg = 0x12 },
> + { .mbps =  180, .reg = 0x22 },
> + { .mbps =  190, .reg = 0x32 },
> + { .mbps =  205, .reg = 0x03 },
> + { .mbps =  220, .reg = 0x13 },
> + { .mbps =  235, .reg = 0x23 },
> + { .mbps =  250, .reg = 0x33 },
> + { .mbps =  275, .reg = 0x04 },
> + { .mbps =  300, .reg = 0x14 },
> + { .mbps =  325, .reg = 0x05 },
> + { .mbps =  350, .reg = 0x15 },
> + { .mbps =  400, .reg = 0x25 },
> + { .mbps =  450, .reg = 0x06 },
> + { .mbps =  500, .reg = 0x16 },
> + { .mbps =  550, .reg = 0x07 },
> + { .mbps =  600, .reg = 0x17 },
> + { .mbps =  650, .reg = 0x08 },
> + { .mbps =  700, .reg = 0x18 },
> + { .mbps =  750, .reg = 0x09 },
> + { .mbps =  800, .reg = 0x19 },
> + { .mbps =  850, .reg = 0x29 },
> + { .mbps =  900, .reg = 0x39 },
> + { .mbps =  950, .reg = 0x0A },
> + { .mbps = 1000, .reg = 0x1A },
> + { .mbps = 1050, .reg = 0x2A },
> + { .mbps = 1100, .reg = 0x3A },
> + { .mbps = 1150, .reg = 0x0B },
> + { .mbps = 1200, .reg = 0x1B },
> + { .mbps = 1250, .reg = 0x2B },
> + { .mbps = 1300, .reg = 0x3B },
> + { .mbps = 1350, .reg = 0x0C },
> + { .mbps = 1400, .reg = 0x1C },
> + { .mbps = 1450, .reg = 0x2C },
> + { .mbps = 1500, .reg = 0x3C },

All the other hex values in the file are lowercase, I'd do the same here.

> + /* guard */
> + { .mbps =   0,  .reg = 0x00 },
> +};

[snip]

> +static int rcar_csi2_start(struct rcar_csi2 *priv)
> +{
> + const struct rcar_csi2_format *format;
> + u32 phycnt, phypll, vcdt = 0, vcdt2 = 0;
> + unsigned int i;
> + int ret;
> +
> + dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
> + priv->mf.width, priv->mf.height,
> + priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i');
> +
> + /* Code is validated in set_fmt */
> + format = rcar_csi2_code_to_fmt(priv->mf.code);

You could store the format pointer iin the rcar_csi2 structure to avoid 
looking it up here.

> + /*
> +  * Enable all Virtual Channels
> +  *
> +  * NOTE: It's not possible to get individual datatype for each
> +  *   source virtual channel. Once this is possible in V4L2
> +  *   it should be used here.
> +  */
> + for (i = 0; i < 4; i++) {
> + u32 vcdt_part;
> +
> + vcdt_part = VCDT_SEL_VC(i) | VCDT_VCDTN_EN | VCDT_SEL_DTN_ON |
> + VCDT_SEL_DT(format->datatype);
> +
> + /* Store in correct reg and offset */
> + if (i < 2)
> + vcdt |= vcdt_part << ((i % 2) * 16);
> + else
> + vcdt2 |= vcdt_part << ((i % 2) * 16);
> + }
> +
> + switch (priv->lanes) {
> + case 1:
> + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0;
> + break;
> + case 2:
> + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0;
> + break;
> + case 4:
> + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2 |
> + PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + ret = rcar_csi2_calc_phypll(priv, format->bpp, );
> + if (ret)
> + return ret;
> +
> + /* Clear Ultra Low Power interrupt */
> + if (priv->info->clear_ulps)
> + rcar_csi2_write(priv, INTSTATE_REG,
> + 

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Maxime Ripard]

Hi Niklas,

On Tue, Feb 13, 2018 at 12:01:32AM +0100, Niklas Söderlund wrote:
> + switch (priv->lanes) {
> + case 1:
> + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0;
> + break;
> + case 2:
> + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0;
> + break;
> + case 4:
> + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2 |
> + PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0;
> + break;
> + default:
> + return -EINVAL;
> + }

I guess you could have a simpler construct here using this:

phycnt = PHYCNT_ENABLECLK;

switch (priv->lanes) {
case 4:
phycnt |= PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2;
case 2:
phycnt |= PHYCNT_ENABLE_1;
case 1:
phycnt |= PHYCNT_ENABLE_0;
break;

default:
return -EINVAL;
}

But that's really up to you.

> +static int rcar_csi2_probe(struct platform_device *pdev)
> +{
> + const struct soc_device_attribute *attr;
> + struct rcar_csi2 *priv;
> + unsigned int i;
> + int ret;
> +
> + priv = devm_kzalloc(>dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->info = of_device_get_match_data(>dev);
> +
> + /* r8a7795 ES1.x behaves different then ES2.0+ but no own compat */
> + attr = soc_device_match(r8a7795es1);
> + if (attr)
> + priv->info = attr->data;
> +
> + priv->dev = >dev;
> +
> + mutex_init(>lock);
> + priv->stream_count = 0;
> +
> + ret = rcar_csi2_probe_resources(priv, pdev);
> + if (ret) {
> + dev_err(priv->dev, "Failed to get resources\n");
> + return ret;
> + }
> +
> + platform_set_drvdata(pdev, priv);
> +
> + ret = rcar_csi2_parse_dt(priv);
> + if (ret)
> + return ret;
> +
> + priv->subdev.owner = THIS_MODULE;
> + priv->subdev.dev = >dev;
> + v4l2_subdev_init(>subdev, _csi2_subdev_ops);
> + v4l2_set_subdevdata(>subdev, >dev);
> + snprintf(priv->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s %s",
> +  KBUILD_MODNAME, dev_name(>dev));
> + priv->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> +
> + priv->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> + priv->subdev.entity.ops = _csi2_entity_ops;
> +
> + priv->pads[RCAR_CSI2_SINK].flags = MEDIA_PAD_FL_SINK;
> + for (i = RCAR_CSI2_SOURCE_VC0; i < NR_OF_RCAR_CSI2_PAD; i++)
> + priv->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> +
> + ret = media_entity_pads_init(>subdev.entity, NR_OF_RCAR_CSI2_PAD,
> +  priv->pads);
> + if (ret)
> + goto error;
> +
> + pm_runtime_enable(>dev);

Is CONFIG_PM mandatory on Renesas SoCs? If not, you end up with the
device uninitialised at probe, and pm_runtime_get_sync will not
initialise it either if CONFIG_PM is not enabled. I guess you could
call your runtime_resume function unconditionally, and mark the device
as active in runtime_pm using pm_runtime_set_active.

Looks good otherwise, once fixed (and if relevant):
Reviewed-by: Maxime Ripard 

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Kieran Bingham]

Hi Niklas

Thank you for this patch ...
I know it has been a lot of work getting this and the VIN together!

On 13/02/18 00:01, Niklas Söderlund wrote:
> A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver
> supports the R-Car Gen3 SoCs where separate CSI-2 hardware blocks are
> connected between the video sources and the video grabbers (VIN).
> 
> Driver is based on a prototype by Koji Matsuoka in the Renesas BSP.
> 
> Signed-off-by: Niklas Söderlund 
> Reviewed-by: Hans Verkuil 

I don't think there's actually anything major in the below - so with any
comments addressed, or specifically ignored you can add my:

Reviewed-by: Kieran Bingham 

tag if you like.


> ---
>  drivers/media/platform/rcar-vin/Kconfig |  12 +
>  drivers/media/platform/rcar-vin/Makefile|   1 +
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 884 
> 
>  3 files changed, 897 insertions(+)
>  create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c
> 
> diff --git a/drivers/media/platform/rcar-vin/Kconfig 
> b/drivers/media/platform/rcar-vin/Kconfig
> index af4c98b44d2e22cb..6875f30c1ae42631 100644
> --- a/drivers/media/platform/rcar-vin/Kconfig
> +++ b/drivers/media/platform/rcar-vin/Kconfig
> @@ -1,3 +1,15 @@
> +config VIDEO_RCAR_CSI2
> + tristate "R-Car MIPI CSI-2 Receiver"
> + depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && OF

I think I recall recently seeing that depending upon OF is redundant and not
necessary (though I think that's minor in this instance)


> + depends on ARCH_RENESAS || COMPILE_TEST
> + select V4L2_FWNODE
> + ---help---
> +   Support for Renesas R-Car MIPI CSI-2 receiver.
> +   Supports R-Car Gen3 SoCs.
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called rcar-csi2.
> +
>  config VIDEO_RCAR_VIN
>   tristate "R-Car Video Input (VIN) Driver"
>   depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && OF && HAS_DMA && 
> MEDIA_CONTROLLER
> diff --git a/drivers/media/platform/rcar-vin/Makefile 
> b/drivers/media/platform/rcar-vin/Makefile
> index 48c5632c21dc060b..5ab803d3e7c1aa57 100644
> --- a/drivers/media/platform/rcar-vin/Makefile
> +++ b/drivers/media/platform/rcar-vin/Makefile
> @@ -1,3 +1,4 @@
>  rcar-vin-objs = rcar-core.o rcar-dma.o rcar-v4l2.o
>  
> +obj-$(CONFIG_VIDEO_RCAR_CSI2) += rcar-csi2.o
>  obj-$(CONFIG_VIDEO_RCAR_VIN) += rcar-vin.o
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c 
> b/drivers/media/platform/rcar-vin/rcar-csi2.c
> new file mode 100644
> index ..c0c2a763151bc928
> --- /dev/null
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -0,0 +1,884 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Driver for Renesas R-Car MIPI CSI-2 Receiver
> + *
> + * Copyright (C) 2018 Renesas Electronics Corp.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/* Register offsets and bits */
> +
> +/* Control Timing Select */
> +#define TREF_REG 0x00
> +#define TREF_TREFBIT(0)
> +
> +/* Software Reset */
> +#define SRST_REG 0x04
> +#define SRST_SRSTBIT(0)
> +
> +/* PHY Operation Control */
> +#define PHYCNT_REG   0x08
> +#define PHYCNT_SHUTDOWNZ BIT(17)
> +#define PHYCNT_RSTZ  BIT(16)
> +#define PHYCNT_ENABLECLK BIT(4)
> +#define PHYCNT_ENABLE_3  BIT(3)
> +#define PHYCNT_ENABLE_2  BIT(2)
> +#define PHYCNT_ENABLE_1  BIT(1)
> +#define PHYCNT_ENABLE_0  BIT(0)
> +
> +/* Checksum Control */
> +#define CHKSUM_REG   0x0c
> +#define CHKSUM_ECC_ENBIT(1)
> +#define CHKSUM_CRC_ENBIT(0)
> +
> +/*
> + * Channel Data Type Select
> + * VCDT[0-15]:  Channel 1 VCDT[16-31]:  Channel 2
> + * VCDT2[0-15]: Channel 3 VCDT2[16-31]: Channel 4
> + */
> +#define VCDT_REG 0x10
> +#define VCDT2_REG0x14
> +#define VCDT_VCDTN_ENBIT(15)
> +#define VCDT_SEL_VC(n)   (((n) & 0x3) << 8)
> +#define VCDT_SEL_DTN_ON  BIT(6)
> +#define VCDT_SEL_DT(n)   (((n) & 0x3f) << 0)
> +
> +/* Frame Data Type Select */
> +#define FRDT_REG 0x18
> +
> +/* Field Detection Control */
> +#define FLD_REG  0x1c
> +#define FLD_FLD_NUM(n)   (((n) & 0xff) << 16)
> +#define FLD_FLD_EN4  BIT(3)
> +#define FLD_FLD_EN3  BIT(2)
> +#define FLD_FLD_EN2  BIT(1)
> +#define FLD_FLD_EN   BIT(0)
> +
> +/* Automatic 

[PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Niklas Söderlund]

A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver
supports the R-Car Gen3 SoCs where separate CSI-2 hardware blocks are
connected between the video sources and the video grabbers (VIN).

Driver is based on a prototype by Koji Matsuoka in the Renesas BSP.

Signed-off-by: Niklas Söderlund 
Reviewed-by: Hans Verkuil 
---
 drivers/media/platform/rcar-vin/Kconfig |  12 +
 drivers/media/platform/rcar-vin/Makefile|   1 +
 drivers/media/platform/rcar-vin/rcar-csi2.c | 884 
 3 files changed, 897 insertions(+)
 create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c

diff --git a/drivers/media/platform/rcar-vin/Kconfig 
b/drivers/media/platform/rcar-vin/Kconfig
index af4c98b44d2e22cb..6875f30c1ae42631 100644
--- a/drivers/media/platform/rcar-vin/Kconfig
+++ b/drivers/media/platform/rcar-vin/Kconfig
@@ -1,3 +1,15 @@
+config VIDEO_RCAR_CSI2
+   tristate "R-Car MIPI CSI-2 Receiver"
+   depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && OF
+   depends on ARCH_RENESAS || COMPILE_TEST
+   select V4L2_FWNODE
+   ---help---
+ Support for Renesas R-Car MIPI CSI-2 receiver.
+ Supports R-Car Gen3 SoCs.
+
+ To compile this driver as a module, choose M here: the
+ module will be called rcar-csi2.
+
 config VIDEO_RCAR_VIN
tristate "R-Car Video Input (VIN) Driver"
depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && OF && HAS_DMA && 
MEDIA_CONTROLLER
diff --git a/drivers/media/platform/rcar-vin/Makefile 
b/drivers/media/platform/rcar-vin/Makefile
index 48c5632c21dc060b..5ab803d3e7c1aa57 100644
--- a/drivers/media/platform/rcar-vin/Makefile
+++ b/drivers/media/platform/rcar-vin/Makefile
@@ -1,3 +1,4 @@
 rcar-vin-objs = rcar-core.o rcar-dma.o rcar-v4l2.o
 
+obj-$(CONFIG_VIDEO_RCAR_CSI2) += rcar-csi2.o
 obj-$(CONFIG_VIDEO_RCAR_VIN) += rcar-vin.o
diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c 
b/drivers/media/platform/rcar-vin/rcar-csi2.c
new file mode 100644
index ..c0c2a763151bc928
--- /dev/null
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -0,0 +1,884 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Driver for Renesas R-Car MIPI CSI-2 Receiver
+ *
+ * Copyright (C) 2018 Renesas Electronics Corp.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* Register offsets and bits */
+
+/* Control Timing Select */
+#define TREF_REG   0x00
+#define TREF_TREF  BIT(0)
+
+/* Software Reset */
+#define SRST_REG   0x04
+#define SRST_SRST  BIT(0)
+
+/* PHY Operation Control */
+#define PHYCNT_REG 0x08
+#define PHYCNT_SHUTDOWNZ   BIT(17)
+#define PHYCNT_RSTZBIT(16)
+#define PHYCNT_ENABLECLK   BIT(4)
+#define PHYCNT_ENABLE_3BIT(3)
+#define PHYCNT_ENABLE_2BIT(2)
+#define PHYCNT_ENABLE_1BIT(1)
+#define PHYCNT_ENABLE_0BIT(0)
+
+/* Checksum Control */
+#define CHKSUM_REG 0x0c
+#define CHKSUM_ECC_EN  BIT(1)
+#define CHKSUM_CRC_EN  BIT(0)
+
+/*
+ * Channel Data Type Select
+ * VCDT[0-15]:  Channel 1 VCDT[16-31]:  Channel 2
+ * VCDT2[0-15]: Channel 3 VCDT2[16-31]: Channel 4
+ */
+#define VCDT_REG   0x10
+#define VCDT2_REG  0x14
+#define VCDT_VCDTN_EN  BIT(15)
+#define VCDT_SEL_VC(n) (((n) & 0x3) << 8)
+#define VCDT_SEL_DTN_ONBIT(6)
+#define VCDT_SEL_DT(n) (((n) & 0x3f) << 0)
+
+/* Frame Data Type Select */
+#define FRDT_REG   0x18
+
+/* Field Detection Control */
+#define FLD_REG0x1c
+#define FLD_FLD_NUM(n) (((n) & 0xff) << 16)
+#define FLD_FLD_EN4BIT(3)
+#define FLD_FLD_EN3BIT(2)
+#define FLD_FLD_EN2BIT(1)
+#define FLD_FLD_EN BIT(0)
+
+/* Automatic Standby Control */
+#define ASTBY_REG  0x20
+
+/* Long Data Type Setting 0 */
+#define LNGDT0_REG 0x28
+
+/* Long Data Type Setting 1 */
+#define LNGDT1_REG 0x2c
+
+/* Interrupt Enable */
+#define INTEN_REG  0x30
+
+/* Interrupt Source Mask */
+#define INTCLOSE_REG   0x34
+
+/* Interrupt Status Monitor */
+#define INTSTATE_REG   0x38
+#define INTSTATE_INT_ULPS_STARTBIT(7)
+#define INTSTATE_INT_ULPS_END  BIT(6)
+
+/* Interrupt Error Status Monitor */
+#define INTERRSTATE_REG0x3c
+
+/* Short Packet Data */
+#define SHPDAT_REG 0x40
+
+/* Short Packet Count