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

[<Manikandan.M@microchip.com>]

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