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

["Niklas Söderlund" <niklas.soderlund@ragnatech.se>]

Hi Geert,

On 2017-11-10 10:30:24 +0100, Geert Uytterhoeven wrote:
> Hi Niklas,
> 
> On Fri, Nov 10, 2017 at 12:43 AM, Niklas Söderlund
> <niklas.soderlund+renesas@ragnatech.se> wrote:
> > A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver
> > supports the rcar-vin driver on 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 <niklas.soderlund+renesas@ragnatech.se>
> 
> Thanks for your patch!

Thanks for your feedback, much appreciated!

> 
> > --- /dev/null
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -0,0 +1,933 @@
> 
> > +/* Control Timing Select */
> > +#define TREF_REG                       0x00
> > +#define TREF_TREF                      (1 << 0)
> 
> BIT(0)? (many more)

Good idea.

> 
> > +struct phypll_hsfreqrange {
> > +       unsigned int    mbps;
> > +       unsigned char   reg;
> 
> The "unsigned char" doesn't buy you much, due to alignment rules.
> What about making both u16 instead?

Yes that would work, thanks.

> 
> > +static const struct rcar_csi2_format *rcar_csi2_code_to_fmt(unsigned int code)
> > +{
> > +       int i;
> 
> unsigned int

Thanks.

> 
> > +
> > +       for (i = 0; i < ARRAY_SIZE(rcar_csi2_formats); i++)
> > +               if (rcar_csi2_formats[i].code == code)
> > +                       return rcar_csi2_formats + i;
> > +       return NULL;
> > +}
> 
> > +struct rcar_csi2_info {
> > +       const struct phypll_hsfreqrange *hsfreqrange;
> > +       bool clear_ulps;
> > +       bool have_phtw;
> > +       unsigned int csi0clkfreqrange;
> 
> I'd sort by decreasing size/alignment, i.e. the bools last.

I had not consider packing of the struct, thanks for pointing this out.

> 
> > +};
> > +
> > +struct rcar_csi2 {
> > +       struct device *dev;
> > +       void __iomem *base;
> > +       const struct rcar_csi2_info *info;
> > +
> > +       unsigned short lanes;
> > +       unsigned char lane_swap[4];
> > +
> > +       struct v4l2_subdev subdev;
> > +       struct media_pad pads[NR_OF_RCAR_CSI2_PAD];
> > +
> > +       struct v4l2_mbus_framefmt mf;
> > +
> > +       struct mutex lock;
> > +       int stream_count;
> > +
> > +       struct v4l2_async_notifier notifier;
> > +       struct v4l2_async_subdev remote;
> 
> Likewise.

Thanks for this, I learnt something new.

> 
> > +static int rcar_csi2_start(struct rcar_csi2 *priv)
> > +{
> 
> > +       dev_dbg(priv->dev, "Input size (%dx%d%c)\n", mf->width,
> 
> %u for __u32

Good catch.

> 
> > +               mf->height, mf->field == V4L2_FIELD_NONE ? 'p' : 'i');
> 
> > +static int rcar_csi2_probe_resources(struct rcar_csi2 *priv,
> > +                                    struct platform_device *pdev)
> > +{
> > +       struct resource *mem;
> > +       int irq;
> > +
> > +       mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       if (!mem)
> 
> No need to check mem, platform_get_resource() and devm_ioremap_resource()
> are designed to be pipelined.

Did not know that, thanks.

> 
> > +               return -ENODEV;
> > +
> > +       priv->base = devm_ioremap_resource(&pdev->dev, mem);
> 
> 
> > +static const struct soc_device_attribute r8a7795es1[] = {
> > +       { .soc_id = "r8a7795", .revision = "ES1.*" },
> > +       { /* sentinel */ }
> > +};
> > +
> > +static int rcar_csi2_probe(struct platform_device *pdev)
> > +{
> > +       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 */
> > +       if (priv->info == &rcar_csi2_info_r8a7795 &&
> > +           soc_device_match(r8a7795es1))
> > +               priv->info = &rcar_csi2_info_r8a7795es1;
> 
> Please store &rcar_csi2_info_r8a7795es1 in r8a7795es1[0].data instead.

Good idea.

I will fix this and resend, thanks again for taking the time to review 
this patch.

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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

-- 
Regards,
Niklas Söderlund