linux-kernel - Re: [PATCH v4 2/4] drm/bridge: add Microchip DSI controller support for sam9x7 SoC series

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAA8EJppZNW7-qwkqPke-8vbf=PGhf0MU-h47EJHuVgKmdV9+8Q@mail.gmail.com>
Date: Tue, 24 Sep 2024 14:33:53 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Manikandan.M@...rochip.com
Cc: andrzej.hajda@...el.com, neil.armstrong@...aro.org, rfoss@...nel.org, 
	Laurent.pinchart@...asonboard.com, jonas@...boo.se, jernej.skrabec@...il.com, 
	airlied@...il.com, simona@...ll.ch, maarten.lankhorst@...ux.intel.com, 
	mripard@...nel.org, tzimmermann@...e.de, robh@...nel.org, krzk+dt@...nel.org, 
	conor+dt@...nel.org, linux@...linux.org.uk, Nicolas.Ferre@...rochip.com, 
	alexandre.belloni@...tlin.com, claudiu.beznea@...on.dev, 
	Dharma.B@...rochip.com, arnd@...db.de, Hari.PrasathGE@...rochip.com, 
	dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v4 2/4] drm/bridge: add Microchip DSI controller support
 for sam9x7 SoC series

On Tue, 24 Sept 2024 at 12:17, <Manikandan.M@...rochip.com> wrote:
>
> Hi Dmitry,
>
> On 20/09/24 9:45 pm, Dmitry Baryshkov wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > On Wed, Sep 18, 2024 at 04:01:17PM GMT, Manikandan Muralidharan wrote:
> >> Add the Microchip's DSI controller wrapper driver that uses
> >> the Synopsys DesignWare MIPI DSI host controller bridge.
> >>
> >> Signed-off-by: Manikandan Muralidharan <manikandan.m@...rochip.com>
> >> ---
> >> changes in v4:
> >> - Fixed issues reported by kernel test robot
> >> - replaced syscon_regmap_lookup_by_phandle with
> >> syscon_regmap_lookup_by_compatible
> >>
> >> changes in v3:
> >> - make remove callback return void
> >>
> >> changes in v2:
> >> - use static const with global variables
> >> - replace dev_err with dev_err_probe
> >> - move clk_prepare_enable to simplify the error path
> >> - use FIELD_PREP calls
> >> - remove unused macros and unused functions
> >> - rename function name with mchp_ suffix
> >> - add suspend and resume pm calls to handle pllref_clk
> >> ---
> >>   drivers/gpu/drm/bridge/Kconfig            |   8 +
> >>   drivers/gpu/drm/bridge/Makefile           |   1 +
> >>   drivers/gpu/drm/bridge/dw-mipi-dsi-mchp.c | 545 ++++++++++++++++++++++
> >>   3 files changed, 554 insertions(+)
> >>   create mode 100644 drivers/gpu/drm/bridge/dw-mipi-dsi-mchp.c
> >>
> >> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> >> index 683cb33805b2..317246509601 100644
> >> --- a/drivers/gpu/drm/bridge/Kconfig
> >> +++ b/drivers/gpu/drm/bridge/Kconfig
> >> @@ -196,6 +196,14 @@ config DRM_MICROCHIP_LVDS_SERIALIZER
> >>        help
> >>          Support for Microchip's LVDS serializer.
> >>
> >> +config DRM_MICROCHIP_DW_MIPI_DSI
> >> +     tristate "Microchip specific extensions for Synopsys DW MIPI DSI"
> >> +     depends on DRM_ATMEL_HLCDC
> >> +     select DRM_DW_MIPI_DSI
> >> +     help
> >> +       This selects support for Microchip's SoC specific extensions
> >> +       for the Synopsys DesignWare dsi driver.
> >> +
> >>   config DRM_NWL_MIPI_DSI
> >>        tristate "Northwest Logic MIPI DSI Host controller"
> >>        depends on DRM
> >> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> >> index 3daf803ce80b..32e4233e6b5e 100644
> >> --- a/drivers/gpu/drm/bridge/Makefile
> >> +++ b/drivers/gpu/drm/bridge/Makefile
> >> @@ -14,6 +14,7 @@ obj-$(CONFIG_DRM_LONTIUM_LT9611UXC) += lontium-lt9611uxc.o
> >>   obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o
> >>   obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
> >>   obj-$(CONFIG_DRM_MICROCHIP_LVDS_SERIALIZER) += microchip-lvds.o
> >> +obj-$(CONFIG_DRM_MICROCHIP_DW_MIPI_DSI) += dw-mipi-dsi-mchp.o
> >>   obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
> >>   obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
> >>   obj-$(CONFIG_DRM_PARADE_PS8640) += parade-ps8640.o
> >> diff --git a/drivers/gpu/drm/bridge/dw-mipi-dsi-mchp.c b/drivers/gpu/drm/bridge/dw-mipi-dsi-mchp.c
> >> new file mode 100644
> >> index 000000000000..35cfca1ff000
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/bridge/dw-mipi-dsi-mchp.c
> >> @@ -0,0 +1,545 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + * Copyright (C) 2024 Microchip Technology Inc. and its subsidiaries
> >> + *
> >> + * Author: Manikandan Muralidharan <manikandan.m@...rochip.com>
> >> + *
> >> + */
> >> +
> >
> > [...]
> >
> >> +
> >> +static int dw_mipi_dsi_mchp_get_lane_mbps(void *priv_data,
> >> +                                       const struct drm_display_mode *mode,
> >> +                                       unsigned long mode_flags, u32 lanes,
> >> +                                       u32 format, unsigned int *lane_mbps)
> >> +{
> >> +     struct dw_mipi_dsi_mchp *dsi = priv_data;
> >> +     unsigned long best_freq, fvco_min, fvco_max, fin, fout;
> >> +     unsigned long min_delta = ULONG_MAX, delta;
> >> +     unsigned int target_mbps = 0, mpclk, desired_mbps;
> >> +     unsigned int max_mbps = dppa_map[ARRAY_SIZE(dppa_map) - 1].max_mbps;
> >> +     unsigned int min_prediv, max_prediv;
> >> +     unsigned int _fbdiv, best_fbdiv, _prediv, best_prediv;
> >> +     int bpp;
> >> +     u64 freq_factor;
> >
> > This function is significantly overlapping with dw_mipi_dsi_get_lane_mbps().
> > Please extract a common helper and add it to dw-mipi-dsi.c. Other than
> > that, LGTM.
> >
> I find it difficult to understand your concern.
> The dw_mipi_dsi_mchp_get_lane_mbps() is the .get_lane_mbps phy_op
> specific to Microchip DSI wrapper. The dw_mipi_dsi_get_lane_mbps
> functionalities across different SoC wrapper is not the same to extract
> a common helper.

Ok, somebody didn't do a proper prefix naming. I was looking at
dw_mipi_dsi_get_lane_mbps() from dw-mipi-dsi-rockchip.c. It looks
pretty close to your code.

> >> +
> >> +     dsi->format = format;
> >> +     bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> >> +     if (bpp < 0) {
> >> +             dev_err(dsi->dev,
> >> +                     "failed to get bpp for pixel format %d\n",
> >> +                     dsi->format);
> >> +             return bpp;
> >> +     }
> >> +
> > --
> > With best wishes
> > Dmitry
>
> --
> Thanks and Regards,
> Manikandan M.
>


-- 
With best wishes
Dmitry