Hi Niklas,
On Fri, Nov 10, 2017 at 12:43 AM, Niklas Söderlund
<niklas.soderlund+renesas@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
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@xxxxxxxxxxxxxx
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
