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

[Geert Uytterhoeven <geert@linux-m68k.org>]

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!

> --- /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)

> +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?

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

unsigned int

> +
> +       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.

> +};
> +
> +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.

> +static int rcar_csi2_start(struct rcar_csi2 *priv)
> +{

> +       dev_dbg(priv->dev, "Input size (%dx%d%c)\n", mf->width,

%u for __u32

> +               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.

> +               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.

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