RE: [v4 2/6] media: platform: dwc: Add MIPI CSI-2 controller driver

Hi Sakari, Andy,

From: Andy Shevchenko <redacted>
Date: Sat, Aug 10, 2019 at 14:09:21

On Fri, Aug 9, 2019 at 5:38 PM Sakari Ailus [off-list ref] wrote:

quoted

On Tue, Jun 11, 2019 at 09:20:51PM +0200, Luis Oliveira wrote:

quoted

Add the Synopsys MIPI CSI-2 controller driver. This
controller driver is divided in platform functions and core functions.
This way it serves as platform for future DesignWare drivers.

quoted

quoted

+const struct mipi_dt csi_dt[] = {

Make this static or use a common prefix that somehow resembles the name
name of the driver.

I will do it.

quoted

quoted

+     {
+             .hex = CSI_2_YUV420_8,
+             .name = "YUV420_8bits",
+     }, {
+             .hex = CSI_2_YUV420_10,
+             .name = "YUV420_10bits",
+     }, {
+             .hex = CSI_2_YUV420_8_LEG,
+             .name = "YUV420_8bits_LEGACY",
+     }, {
+             .hex = CSI_2_YUV420_8_SHIFT,
+             .name = "YUV420_8bits_SHIFT",
+     }, {
+             .hex = CSI_2_YUV420_10_SHIFT,
+             .name = "YUV420_10bits_SHIFT",
+     }, {
+             .hex = CSI_2_YUV422_8,
+             .name = "YUV442_8bits",
+     }, {
+             .hex = CSI_2_YUV422_10,
+             .name = "YUV442_10bits",
+     }, {
+             .hex = CSI_2_RGB444,
+             .name = "RGB444",
+     }, {
+             .hex = CSI_2_RGB555,
+             .name = "RGB555",
+     }, {
+             .hex = CSI_2_RGB565,
+             .name = "RGB565",
+     }, {
+             .hex = CSI_2_RGB666,
+             .name = "RGB666",
+     }, {
+             .hex = CSI_2_RGB888,
+             .name = "RGB888",
+     }, {
+             .hex = CSI_2_RAW6,
+             .name = "RAW6",
+     }, {
+             .hex = CSI_2_RAW7,
+             .name = "RAW7",
+     }, {
+             .hex = CSI_2_RAW8,
+             .name = "RAW8",
+     }, {
+             .hex = CSI_2_RAW10,
+             .name = "RAW10",
+     }, {
+             .hex = CSI_2_RAW12,
+             .name = "RAW12",
+     }, {
+             .hex = CSI_2_RAW14,
+             .name = "RAW14",
+     }, {
+             .hex = CSI_2_RAW16,
+             .name = "RAW16",
+     },
+};

One may utilize __stringify() macro and do somelike

#define CSI_FMT_DESC(fmt) \
 { .hex = CSI_2_##fmt, .name = __stringify(fmt), }

And do

 CSI_FMT_DESC(RAW16),

etc.

Great, thanks! 

quoted

quoted

+             return cfg ? v4l2_subdev_get_try_format(&dev->sd,
+                                                     cfg,
+                                                     0) : NULL;

This indentation looks ugly.
I would rather put this on one line.

quoted

quoted

+     dev_dbg(dev->dev,
+             "%s got v4l2_mbus_pixelcode. 0x%x\n", __func__,
+             dev->format.code);
+     dev_dbg(dev->dev,
+             "%s got width. 0x%x\n", __func__,
+             dev->format.width);
+     dev_dbg(dev->dev,
+             "%s got height. 0x%x\n", __func__,
+             dev->format.height);

__func__ is usually redundant (if Dynamic Debug in use it can be
switched at run-time).

That's true, I don't need it.

quoted

I'd just omit these debug prints in a driver. But adding them to the
framework might make sense. We don't have a lot of debug prints dealing
with user parameters in there. OTOH the common test programs largely do the
same already.

I would rather see tracepoints instead of debug prints if we are
talking about generic solution for entire framework.

I will check that.

quoted

quoted

+     return &dev->format;
+}

quoted

quoted

+     struct mipi_fmt *dev_fmt;

This is simple bad name. We have dev_fmt() macro. I would rather avoid
potential collisions.

True, I will change the name.

quoted

quoted

+     struct v4l2_mbus_framefmt *mf;
+
+     mf = dw_mipi_csi_get_format(dev, cfg, fmt->which);
+     if (!mf)
+             return -EINVAL;

Can't you rather return an error pointer in this and similar cases?

Yes, ofc.

quoted

quoted

+     dev_vdbg(dev->dev, "%s: on=%d\n", __func__, on);

This is noise. If you would like to debug Function Tracer is a good start.

Ok.

quoted

quoted

+     of_id = of_match_node(dw_mipi_csi_of_match, dev->of_node);
+     if (!of_id)
+             return -EINVAL;

Is it possible to have this asserted?

I will remove it.
 

quoted

quoted

+     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

quoted

quoted

+     if (!res)
+             return -ENXIO;

Redundant. Below does the check for you.

Yep, thanks.

quoted

quoted

+
+     csi->base_address = devm_ioremap_resource(dev, res);
+     if (IS_ERR(csi->base_address)) {

quoted

quoted

+             dev_err(dev, "Base address not set.\n");

Redundant. Above does print an error message for you.

Ok.

quoted

quoted

+             return PTR_ERR(csi->base_address);
+     }

Moreover, use devm_platform_ioremap_resource() instead of both.

Nice, thanks.

quoted

quoted

+     csi->ctrl_irq_number = platform_get_irq(pdev, 0);
+     if (csi->ctrl_irq_number < 0) {

quoted

quoted

+             dev_err(dev, "irq number %d not set.\n", csi->ctrl_irq_number);

Redundant since this cycle (v5.4).

Ok,

quoted

quoted

+             ret = csi->ctrl_irq_number;

Better to do the opposite

ret = platform_get_irq();
if (ret)
 goto end;
... = ret;

quoted

quoted

+             goto end;
+     }

quoted

quoted

+     ret = devm_request_irq(dev, csi->ctrl_irq_number,
+                            dw_mipi_csi_irq1, IRQF_SHARED,
+                            dev_name(dev), csi);
+     if (ret) {
+             dev_err(dev, "irq csi %s failed\n", of_id->name);
+
+             goto end;
+     }

devm_*irq() might be a bad idea. Is it race free in your driver?

I never thought about it like that. Should I use request_irq and 
free_irq? 

quoted

quoted

+static const struct of_device_id dw_mipi_csi_of_match[] = {
+     { .compatible = "snps,dw-csi" },

quoted

quoted

+     {},

Better without comma. Terminator may terminate even at compile time.

Ok.

quoted

quoted

+};

quoted

quoted

+static ssize_t core_version_show(struct device *dev,
+                              struct device_attribute *attr,
+                              char *buf)
+{
+     struct platform_device *pdev = to_platform_device(dev);
+     struct v4l2_subdev *sd = platform_get_drvdata(pdev);
+     struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);

quoted

quoted

+
+     char buffer[10];
+
+     snprintf(buffer, 10, "v.%d.%d*\n", csi_dev->hw_version_major,
+              csi_dev->hw_version_minor);
+
+     return strlcpy(buf, buffer, PAGE_SIZE);

Oh, can't you simple without any temprorary useless buffers?
 sprintf(buf, ...)?
(Yes, note _absence_ of *n* there)

You are right.

quoted

quoted

+}

quoted

quoted

+static ssize_t n_lanes_store(struct device *dev, struct device_attribute *attr,
+                          const char *buf, size_t count)
+{
+     int ret;
+     unsigned long lanes;

quoted

quoted

+

More blank lines! We need them!

Ok.

quoted

quoted

+     struct platform_device *pdev = to_platform_device(dev);
+     struct v4l2_subdev *sd = platform_get_drvdata(pdev);
+     struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
+
+     ret = kstrtoul(buf, 10, &lanes);
+     if (ret < 0)
+             return ret;

Can it return positive number?

quoted

quoted

+     dev_info(dev, "Lanes %lu\n", lanes);

Noise.
The user gets it, why to spam kernel log???

Ok.

quoted

quoted

+     csi_dev->hw.num_lanes = lanes;
+
+     return count;
+}

I told once, can repeat again. Synopsys perhaps needs better reviews
inside company. Each time I see the code, it repeats same mistakes
over and over. Have you, guys, do something about it?

We are working on it. It will get better, sorry.

-- 
With Best Regards,
Andy Shevchenko

Thanks,
Luis