[PATCH] drm/msm/dp: use ARRAY_SIZE for calculating num_descs

[Dmitry Baryshkov]

On 28/06/2022 00:01, Kuogee Hsieh wrote:
> 
> On 6/27/2022 1:05 PM, Dmitry Baryshkov wrote:
>> On Mon, 27 Jun 2022 at 22:26, Kuogee Hsieh <quic_khsieh at quicinc.com> 
>> wrote:
>>>
>>> On 6/27/2022 9:54 AM, Dmitry Baryshkov wrote:
>>>> If for some reason the msm_dp_config::descs array starts from non-zero
>>>> index or contains the hole, setting the msm_dp_config::num_descs might
>>>> be not that obvious and error-prone. Use ARRAY_SIZE to set this field
>>>> rather than encoding the value manually.
>>>>
>>>> Reported-by: Kuogee Hsieh <quic_khsieh at quicinc.com>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>>> ---
>>>>    drivers/gpu/drm/msm/dp/dp_display.c | 46 
>>>> +++++++++++++++++------------
>>>>    1 file changed, 27 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
>>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> index f87fa3ba1e25..6fed738a9467 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> @@ -131,35 +131,43 @@ struct msm_dp_config {
>>>>        size_t num_descs;
>>>>    };
>>>>
>>>> +static const struct msm_dp_desc sc7180_dp_descs[] = {
>>>> +     [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, 
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>> +};
>>>> +
>>>>    static const struct msm_dp_config sc7180_dp_cfg = {
>>>> -     .descs = (const struct msm_dp_desc[]) {
>>>> -             [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, 
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>> -     },
>>>> -     .num_descs = 1,
>>>> +     .descs = sc7180_dp_descs,
>>>> +     .num_descs = ARRAY_SIZE(sc7180_dp_descs),
>>>> +};
>>>> +
>>> why you want to do that?
>>>
>>> It is very clear only one entry, why you want to make it 2 entry here?
>>>
>>> can you just embedded MSM_DP_COTROLLER_x into struct msm_dp_config?
>> Because we had enough stories of using a manually set 'number of
>> something' field. So I'd prefer to have it done automatically.
>> Also using the indexed array spares us from 'look for the DP
>> controller number N' functions. You can just get it.
> 
> static const struct msm_dp_config sc7280_dp_cfg = {
>           .descs = (const struct msm_dp_desc[]) {
>                   [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, 
> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>           },
>           .num_descs = ARRAY_SIZE(sc7280_dp_descs),

This will not work.

> };
> 
> At above example table, it just waste one entry. is it ok?

I'd assume this is an interim development state. Then it's fine. In the 
final version one would fill all existing DP controllers starting from 0.

> 
> can you elaborate  more on 'look for the DP controller number N' 
> functions, where is it?

Ignore this.

> 
> 
>>>> +static const struct msm_dp_desc sc7280_dp_descs[] = {
>>>> +     [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, 
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = 
>>>> true },
>>>> +     [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, 
>>>> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>>>>    };
>>>>
>>>>    static const struct msm_dp_config sc7280_dp_cfg = {
>>>> -     .descs = (const struct msm_dp_desc[]) {
>>>> -             [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, 
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = 
>>>> true },
>>>> -             [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, 
>>>> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>>>> -     },
>>>> -     .num_descs = 2,
>>>> +     .descs = sc7280_dp_descs,
>>>> +     .num_descs = ARRAY_SIZE(sc7280_dp_descs),
>>>> +};
>>>> +
>>>> +static const struct msm_dp_desc sc8180x_dp_descs[] = {
>>>> +     [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, 
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>> +     [MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000, 
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>> +     [MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000, 
>>>> .connector_type = DRM_MODE_CONNECTOR_eDP },
>>>>    };
>>>>
>>>>    static const struct msm_dp_config sc8180x_dp_cfg = {
>>>> -     .descs = (const struct msm_dp_desc[]) {
>>>> -             [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, 
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>> -             [MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000, 
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>> -             [MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000, 
>>>> .connector_type = DRM_MODE_CONNECTOR_eDP },
>>>> -     },
>>>> -     .num_descs = 3,
>>>> +     .descs = sc8180x_dp_descs,
>>>> +     .num_descs = ARRAY_SIZE(sc8180x_dp_descs),
>>>> +};
>>>> +
>>>> +static const struct msm_dp_desc sm8350_dp_descs[] = {
>>>> +     [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, 
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>>    };
>>>>
>>>>    static const struct msm_dp_config sm8350_dp_cfg = {
>>>> -     .descs = (const struct msm_dp_desc[]) {
>>>> -             [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, 
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>> -     },
>>>> -     .num_descs = 1,
>>>> +     .descs = sm8350_dp_descs,
>>>> +     .num_descs = ARRAY_SIZE(sm8350_dp_descs),
>>>>    };
>>>>
>>>>    static const struct of_device_id dp_dt_match[] = {
>>
>>


-- 
With best wishes
Dmitry