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(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->info = of_device_get_match_data(&pdev->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 = &pdev->dev;
> > +
> > + mutex_init(&priv->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 = &pdev->dev;
> > + v4l2_subdev_init(&priv->subdev, &rcar_csi2_subdev_ops);
> > + v4l2_set_subdevdata(&priv->subdev, &pdev->dev);
> > + snprintf(priv->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s %s",
> > + KBUILD_MODNAME, dev_name(&pdev->dev));
> > + priv->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> > +
> > + priv->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> > + priv->subdev.entity.ops = &rcar_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(&priv->subdev.entity, NR_OF_RCAR_CSI2_PAD,
> > + priv->pads);
> > + if (ret)
> > + goto error;
> > +
> > + pm_runtime_enable(&pdev->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 <maxime.ripard@xxxxxxxxxxx>
Thanks!
--
Regards,
Niklas Söderlund
