Message-ID: <MN2PR12MB3710E54A1E4BA4FD3AD77B87CBD30@MN2PR12MB3710.namprd12.prod.outlook.com>
Date:   Mon, 12 Aug 2019 09:45:15 +0000
From:   Luis de Oliveira <Luis.Oliveira@...opsys.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>,
        Sakari Ailus <sakari.ailus@....fi>
CC:     Luis Oliveira <Luis.Oliveira@...opsys.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Rob Herring <robh@...nel.org>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        "paulmck@...ux.ibm.com" <paulmck@...ux.ibm.com>,
        Mark Rutland <mark.rutland@....com>,
        "Kishon Vijay Abraham I" <kishon@...com>,
        devicetree <devicetree@...r.kernel.org>,
        "Linux Media Mailing List" <linux-media@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Joao Pinto <Joao.Pinto@...opsys.com>
Subject: RE: [v4 2/6] media: platform: dwc: Add MIPI CSI-2 controller driver

Hi Sakari, Andy,

From: Andy Shevchenko <andy.shevchenko@...il.com>
Date: Sat, Aug 10, 2019 at 14:09:21

> On Fri, Aug 9, 2019 at 5:38 PM Sakari Ailus <sakari.ailus@....fi> wrote:
> > On Tue, Jun 11, 2019 at 09:20:51PM +0200, Luis Oliveira wrote:
> > > 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.
> 
> > > +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.

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

> > > +             return cfg ? v4l2_subdev_get_try_format(&dev->sd,
> > > +                                                     cfg,
> > > +                                                     0) : NULL;
> 
> This indentation looks ugly.
> I would rather put this on one line.
> 
> > > +     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.

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

> >
> > > +     return &dev->format;
> > > +}
> 
> > > +     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.

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

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

> > > +     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.
 
> > > +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> 
> > > +     if (!res)
> > > +             return -ENXIO;
> 
> Redundant. Below does the check for you.
> 

Yep, thanks.

> > > +
> > > +     csi->base_address = devm_ioremap_resource(dev, res);
> > > +     if (IS_ERR(csi->base_address)) {
> 
> > > +             dev_err(dev, "Base address not set.\n");
> 
> Redundant. Above does print an error message for you.
> 

Ok.

> > > +             return PTR_ERR(csi->base_address);
> > > +     }
> 
> Moreover, use devm_platform_ioremap_resource() instead of both.
> 

Nice, thanks.

> > > +     csi->ctrl_irq_number = platform_get_irq(pdev, 0);
> > > +     if (csi->ctrl_irq_number < 0) {
> 
> > > +             dev_err(dev, "irq number %d not set.\n", csi->ctrl_irq_number);
> 
> Redundant since this cycle (v5.4).
> 

Ok,

> > > +             ret = csi->ctrl_irq_number;
> 
> Better to do the opposite
> 
> ret = platform_get_irq();
> if (ret)
>  goto end;
> ... = ret;
> 
> > > +             goto end;
> > > +     }
> 
> > > +     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? 

> > > +static const struct of_device_id dw_mipi_csi_of_match[] = {
> > > +     { .compatible = "snps,dw-csi" },
> 
> > > +     {},
> 
> Better without comma. Terminator may terminate even at compile time.
> 

Ok.
> > > +};
> 
> > > +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);
> 
> > > +
> > > +     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.
> 
> > > +}
> 
> > > +static ssize_t n_lanes_store(struct device *dev, struct device_attribute *attr,
> > > +                          const char *buf, size_t count)
> > > +{
> > > +     int ret;
> > > +     unsigned long lanes;
> 
> > > +
> 
> More blank lines! We need them!
> 

Ok.

> > > +     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?
> 
> > > +     dev_info(dev, "Lanes %lu\n", lanes);
> 
> Noise.
> The user gets it, why to spam kernel log???
> 
Ok.

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

Powered by blists - more mailing lists