Re: [PATCH 10/17] drm/msm/dp: modify dp_catalog_hw_revision to show major and minor val

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On Sun, 28 Jan 2024 at 07:31, Paloma Arellano <quic_parellan@quicinc.com> wrote:
>
>
> On 1/25/2024 2:07 PM, Dmitry Baryshkov wrote:
> > On 25/01/2024 21:38, Paloma Arellano wrote:
> >> Modify dp_catalog_hw_revision to make the major and minor version values
> >> known instead of outputting the entire hex value of the hardware version
> >> register in preparation of using it for VSC SDP programming.
> >>
> >> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
> >> ---
> >>   drivers/gpu/drm/msm/dp/dp_catalog.c | 12 +++++++++---
> >>   drivers/gpu/drm/msm/dp/dp_catalog.h |  2 +-
> >>   2 files changed, 10 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> index 5d84c089e520a..c025786170ba5 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> @@ -24,6 +24,9 @@
> >>   #define DP_INTERRUPT_STATUS_ACK_SHIFT    1
> >>   #define DP_INTERRUPT_STATUS_MASK_SHIFT    2
> >>   +#define DP_HW_VERSION_MAJOR(reg)    FIELD_GET(GENMASK(31, 28), reg)
> >> +#define DP_HW_VERSION_MINOR(reg)    FIELD_GET(GENMASK(27, 16), reg)
> >> +
> >>   #define DP_INTF_CONFIG_DATABUS_WIDEN     BIT(4)
> >>     #define DP_INTERRUPT_STATUS1 \
> >> @@ -531,15 +534,18 @@ int
> >> dp_catalog_ctrl_set_pattern_state_bit(struct dp_catalog *dp_catalog,
> >>    *
> >>    * @dp_catalog: DP catalog structure
> >>    *
> >> - * Return: DP controller hw revision
> >> + * Return: void
> >>    *
> >>    */
> >> -u32 dp_catalog_hw_revision(const struct dp_catalog *dp_catalog)
> >> +void dp_catalog_hw_revision(const struct dp_catalog *dp_catalog, u16
> >> *major, u16 *minor)
> >>   {
> >>       const struct dp_catalog_private *catalog =
> >> container_of(dp_catalog,
> >>                   struct dp_catalog_private, dp_catalog);
> >> +    u32 reg_dp_hw_version;
> >>   -    return dp_read_ahb(catalog, REG_DP_HW_VERSION);
> >> +    reg_dp_hw_version = dp_read_ahb(catalog, REG_DP_HW_VERSION);
> >> +    *major = DP_HW_VERSION_MAJOR(reg_dp_hw_version);
> >> +    *minor = DP_HW_VERSION_MINOR(reg_dp_hw_version);
> >
> > After looking at the code, it might be easier to keep
> > dp_catalog_hw_revision as is, add define for hw revision 1.2 and
> > corepare to it directly.
> I thought having a  define value of the version would be harder to
> follow than what's here currently. Since having it compare to the
> version value looks a little difficult to read versus having an explicit
> major and minor value version to compare to. For example having (major
>  >= 1 && minor >= 2) versus having something like (hw_version >=
> DPU_HW_VERSION_1_2)

The problem is that major + minor are harder to follow and harder to
implement. You got them wrong, btw. For example 2.1 is greater or
equal than 1.2, but it doesn't pass your test. So, I think, a single
define is easier and less error prone.

> >
> >>   }
> >>     /**
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h
> >> b/drivers/gpu/drm/msm/dp/dp_catalog.h
> >> index 563903605b3a7..94c377ef90c35 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
> >> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
> >> @@ -170,7 +170,7 @@ void dp_catalog_ctrl_config_misc(struct
> >> dp_catalog *dp_catalog, u32 cc, u32 tb);
> >>   void dp_catalog_ctrl_config_msa(struct dp_catalog *dp_catalog, u32
> >> rate,
> >>                   u32 stream_rate_khz, bool fixed_nvid, bool
> >> is_ycbcr_420);
> >>   int dp_catalog_ctrl_set_pattern_state_bit(struct dp_catalog
> >> *dp_catalog, u32 pattern);
> >> -u32 dp_catalog_hw_revision(const struct dp_catalog *dp_catalog);
> >> +void dp_catalog_hw_revision(const struct dp_catalog *dp_catalog, u16
> >> *major, u16 *minor);
> >>   void dp_catalog_ctrl_reset(struct dp_catalog *dp_catalog);
> >>   bool dp_catalog_ctrl_mainlink_ready(struct dp_catalog *dp_catalog);
> >>   void dp_catalog_ctrl_enable_irq(struct dp_catalog *dp_catalog, bool
> >> enable);
> >



-- 
With best wishes
Dmitry