On Sun, 28 Jan 2024 at 07:31, Paloma Arellano <quic_parellan@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> >> ---
> >> 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
