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

2017-11-10 Thread Niklas Söderlund 
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
>  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 
> 
> 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 intmbps;
> > +   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(>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(>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 */
> > +   if (priv->info == _csi2_info_r8a7795 &&
> > +   soc_device_match(r8a7795es1))
> > +   priv->info = _csi2_info_r8a7795es1;
> 
> Please store _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 -- 
> 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

-- 
Regards,
Niklas Söderlund

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

2017-11-10 Thread Geert Uytterhoeven 
Hi Niklas,

On Fri, Nov 10, 2017 at 12:43 AM, Niklas Söderlund
 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 

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 intmbps;
> +   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(>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(>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 */
> +   if (priv->info == _csi2_info_r8a7795 &&
> +   soc_device_match(r8a7795es1))
> +   priv->info = _csi2_info_r8a7795es1;

Please store _csi2_info_r8a7795es1 in r8a7795es1[0].data instead.

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

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

2017-11-09 Thread Niklas Söderlund 
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 
---
 drivers/media/platform/rcar-vin/Kconfig |  12 +
 drivers/media/platform/rcar-vin/Makefile|   1 +
 drivers/media/platform/rcar-vin/rcar-csi2.c | 933 
 3 files changed, 946 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 ..f3c17545c12dc057
--- /dev/null
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -0,0 +1,933 @@
+/*
+ * Driver for Renesas R-Car MIPI CSI-2 Receiver
+ *
+ * Copyright (C) 2017 Renesas Electronics Corp.
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ */
+
+#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  (1 << 0)
+
+/* Software Reset */
+#define SRST_REG   0x04
+#define SRST_SRST  (1 << 0)
+
+/* PHY Operation Control */
+#define PHYCNT_REG 0x08
+#define PHYCNT_SHUTDOWNZ   (1 << 17)
+#define PHYCNT_RSTZ(1 << 16)
+#define PHYCNT_ENABLECLK   (1 << 4)
+#define PHYCNT_ENABLE_3(1 << 3)
+#define PHYCNT_ENABLE_2(1 << 2)
+#define PHYCNT_ENABLE_1(1 << 1)
+#define PHYCNT_ENABLE_0(1 << 0)
+
+/* Checksum Control */
+#define CHKSUM_REG 0x0c
+#define CHKSUM_ECC_EN  (1 << 1)
+#define CHKSUM_CRC_EN  (1 << 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  (1 << 15)
+#define VCDT_SEL_VC(n) (((n) & 0x3) << 8)
+#define VCDT_SEL_DTN_ON(1 << 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_EN4(1 << 3)
+#define FLD_FLD_EN3(1 << 2)
+#define FLD_FLD_EN2(1 << 1)
+#define FLD_FLD_EN (1 << 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_START(1 << 7)