| Message ID | 20230823104444.1954663-12-bryan.odonoghue@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: qcom: camss: Add parameter passing to remove several outstanding bugs | expand |
On 23.08.2023 12:44, Bryan O'Donoghue wrote: > The csiphyX_timer and csiX_phy values need not be hard-coded. We can > functionally decompose the string matching inside of a loop. > > Static string values are brittle, difficult to extend and not required > anyway since the camss->res->csiphy_num value informs us of the number > of CSIPHYs and hence the set of potential clocks for a given CSIPHY. > > In simple terms if we have five CSIPHYs we can have no more and no less > than five csiphy_timer clocks. Similarly csi_phy core clocks have a 1:1 > relationship with the PHY they clock. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > .../media/platform/qcom/camss/camss-csiphy.c | 37 ++++++++++++------- > 1 file changed, 23 insertions(+), 14 deletions(-) > > diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c > index 0e8c2a59ea241..baf78c525fbfc 100644 > --- a/drivers/media/platform/qcom/camss/camss-csiphy.c > +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c > @@ -536,6 +536,15 @@ static int csiphy_init_formats(struct v4l2_subdev *sd, > return csiphy_set_format(sd, fh ? fh->state : NULL, &format); > } > > +static bool csiphy_match_clock_name(const char *clock_name, const char *format, > + int index) > +{ > + char name[CAMSS_RES_MAX]; similar comment to the previous patch about the buf size > + > + snprintf(name, sizeof(name), format, index); > + return !strcmp(clock_name, name); > +} > + > /* > * msm_csiphy_subdev_init - Initialize CSIPHY device structure and resources > * @csiphy: CSIPHY device > @@ -550,7 +559,7 @@ int msm_csiphy_subdev_init(struct camss *camss, > { > struct device *dev = camss->dev; > struct platform_device *pdev = to_platform_device(dev); > - int i, j; > + int i, j, k; > int ret; > > csiphy->camss = camss; > @@ -666,19 +675,19 @@ int msm_csiphy_subdev_init(struct camss *camss, > for (j = 0; j < clock->nfreqs; j++) > clock->freq[j] = res->clock_rate[i][j]; > > - if (!strcmp(clock->name, "csiphy0_timer") || > - !strcmp(clock->name, "csiphy1_timer") || > - !strcmp(clock->name, "csiphy2_timer") || > - !strcmp(clock->name, "csiphy3_timer") || > - !strcmp(clock->name, "csiphy4_timer") || > - !strcmp(clock->name, "csiphy5_timer")) > - csiphy->rate_set[i] = true; > - > - if (camss->res->version == CAMSS_660 && > - (!strcmp(clock->name, "csi0_phy") || > - !strcmp(clock->name, "csi1_phy") || > - !strcmp(clock->name, "csi2_phy"))) > - csiphy->rate_set[i] = true; > + for (k = 0; k < camss->res->csiphy_num; k++) { > + csiphy->rate_set[i] = csiphy_match_clock_name(clock->name, > + "csiphy%d_timer", k); This entire functions is like.. soooo over-engineered adding something like csiphy_timer_clks and cisphy_clks and stuff would make this string comparison mess unnecessary > + if (csiphy->rate_set[i]) > + break; > + > + if (camss->res->version == CAMSS_660) { > + csiphy->rate_set[i] = csiphy_match_clock_name(clock->name, > + "csi%d_phy", k); hm, only ratesetting on 660 sounds very sus Konrad
On 26/08/2023 11:12, Konrad Dybcio wrote: >> - csiphy->rate_set[i] = true; >> + for (k = 0; k < camss->res->csiphy_num; k++) { >> + csiphy->rate_set[i] = csiphy_match_clock_name(clock->name, >> + "csiphy%d_timer", k); > This entire functions is like.. soooo over-engineered I'm going to accept your compliment there. > adding something like csiphy_timer_clks and cisphy_clks and stuff > would make this string comparison mess unnecessary I don't understand your comment. Having a litany of static comparisons is definitely inferior to a generic helper function. I'm not sure what you are asking/arguing for here. --- bod
On 26.08.2023 14:07, Bryan O'Donoghue wrote: > On 26/08/2023 11:12, Konrad Dybcio wrote: >>> - csiphy->rate_set[i] = true; >>> + for (k = 0; k < camss->res->csiphy_num; k++) { >>> + csiphy->rate_set[i] = csiphy_match_clock_name(clock->name, >>> + "csiphy%d_timer", k); >> This entire functions is like.. soooo over-engineered > > I'm going to accept your compliment there. > > >> adding something like csiphy_timer_clks and cisphy_clks and stuff >> would make this string comparison mess unnecessary > > I don't understand your comment. > > Having a litany of static comparisons is definitely inferior to a generic helper function. portray this struct camss_whatever_it_was_called { struct clk_bulk_data *csiphy_clks; struct clk_bulk_data *csiphy_timer_clks; [...] } and then clk_bulk_prepare_enable(csiphy_clks) etc instead of weird looping and matching Konrad
On 26/08/2023 13:11, Konrad Dybcio wrote: >>> adding something like csiphy_timer_clks and cisphy_clks and stuff >>> would make this string comparison mess unnecessary >> I don't understand your comment. >> >> Having a litany of static comparisons is definitely inferior to a generic helper function. > portray this > > struct camss_whatever_it_was_called { > struct clk_bulk_data *csiphy_clks; > struct clk_bulk_data *csiphy_timer_clks; > [...] > } > > and then > > clk_bulk_prepare_enable(csiphy_clks) Ah would be grateful if you had just said "hey could you try using clk_bulk_prepare_enable()" But, OK. Thanks ! --- bod
On 26.08.2023 14:14, Bryan O'Donoghue wrote: > On 26/08/2023 13:11, Konrad Dybcio wrote: >>>> adding something like csiphy_timer_clks and cisphy_clks and stuff >>>> would make this string comparison mess unnecessary >>> I don't understand your comment. >>> >>> Having a litany of static comparisons is definitely inferior to a generic helper function. >> portray this >> >> struct camss_whatever_it_was_called { >> struct clk_bulk_data *csiphy_clks; >> struct clk_bulk_data *csiphy_timer_clks; >> [...] >> } >> >> and then >> >> clk_bulk_prepare_enable(csiphy_clks) > > Ah would be grateful if you had just said "hey could you try using clk_bulk_prepare_enable()" Right I could have been more clear Konrad
Hi Bryan, Thank you for the patch. On Wed, Aug 23, 2023 at 11:44:40AM +0100, Bryan O'Donoghue wrote: > The csiphyX_timer and csiX_phy values need not be hard-coded. We can > functionally decompose the string matching inside of a loop. > > Static string values are brittle, difficult to extend and not required > anyway since the camss->res->csiphy_num value informs us of the number > of CSIPHYs and hence the set of potential clocks for a given CSIPHY. > > In simple terms if we have five CSIPHYs we can have no more and no less > than five csiphy_timer clocks. Similarly csi_phy core clocks have a 1:1 > relationship with the PHY they clock. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > .../media/platform/qcom/camss/camss-csiphy.c | 37 ++++++++++++------- > 1 file changed, 23 insertions(+), 14 deletions(-) > > diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c > index 0e8c2a59ea241..baf78c525fbfc 100644 > --- a/drivers/media/platform/qcom/camss/camss-csiphy.c > +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c > @@ -536,6 +536,15 @@ static int csiphy_init_formats(struct v4l2_subdev *sd, > return csiphy_set_format(sd, fh ? fh->state : NULL, &format); > } > > +static bool csiphy_match_clock_name(const char *clock_name, const char *format, > + int index) > +{ > + char name[CAMSS_RES_MAX]; > + > + snprintf(name, sizeof(name), format, index); > + return !strcmp(clock_name, name); This is very error-prone. You hide the buffer size from the caller, which will make it prone to buffer overflows. > +} > + > /* > * msm_csiphy_subdev_init - Initialize CSIPHY device structure and resources > * @csiphy: CSIPHY device > @@ -550,7 +559,7 @@ int msm_csiphy_subdev_init(struct camss *camss, > { > struct device *dev = camss->dev; > struct platform_device *pdev = to_platform_device(dev); > - int i, j; > + int i, j, k; > int ret; > > csiphy->camss = camss; > @@ -666,19 +675,19 @@ int msm_csiphy_subdev_init(struct camss *camss, > for (j = 0; j < clock->nfreqs; j++) > clock->freq[j] = res->clock_rate[i][j]; > > - if (!strcmp(clock->name, "csiphy0_timer") || > - !strcmp(clock->name, "csiphy1_timer") || > - !strcmp(clock->name, "csiphy2_timer") || > - !strcmp(clock->name, "csiphy3_timer") || > - !strcmp(clock->name, "csiphy4_timer") || > - !strcmp(clock->name, "csiphy5_timer")) > - csiphy->rate_set[i] = true; > - > - if (camss->res->version == CAMSS_660 && > - (!strcmp(clock->name, "csi0_phy") || > - !strcmp(clock->name, "csi1_phy") || > - !strcmp(clock->name, "csi2_phy"))) > - csiphy->rate_set[i] = true; > + for (k = 0; k < camss->res->csiphy_num; k++) { > + csiphy->rate_set[i] = csiphy_match_clock_name(clock->name, > + "csiphy%d_timer", k); > + if (csiphy->rate_set[i]) > + break; > + > + if (camss->res->version == CAMSS_660) { > + csiphy->rate_set[i] = csiphy_match_clock_name(clock->name, > + "csi%d_phy", k); > + if (csiphy->rate_set[i]) > + break; > + } > + } > } > > return 0;
diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c index 0e8c2a59ea241..baf78c525fbfc 100644 --- a/drivers/media/platform/qcom/camss/camss-csiphy.c +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c @@ -536,6 +536,15 @@ static int csiphy_init_formats(struct v4l2_subdev *sd, return csiphy_set_format(sd, fh ? fh->state : NULL, &format); } +static bool csiphy_match_clock_name(const char *clock_name, const char *format, + int index) +{ + char name[CAMSS_RES_MAX]; + + snprintf(name, sizeof(name), format, index); + return !strcmp(clock_name, name); +} + /* * msm_csiphy_subdev_init - Initialize CSIPHY device structure and resources * @csiphy: CSIPHY device @@ -550,7 +559,7 @@ int msm_csiphy_subdev_init(struct camss *camss, { struct device *dev = camss->dev; struct platform_device *pdev = to_platform_device(dev); - int i, j; + int i, j, k; int ret; csiphy->camss = camss; @@ -666,19 +675,19 @@ int msm_csiphy_subdev_init(struct camss *camss, for (j = 0; j < clock->nfreqs; j++) clock->freq[j] = res->clock_rate[i][j]; - if (!strcmp(clock->name, "csiphy0_timer") || - !strcmp(clock->name, "csiphy1_timer") || - !strcmp(clock->name, "csiphy2_timer") || - !strcmp(clock->name, "csiphy3_timer") || - !strcmp(clock->name, "csiphy4_timer") || - !strcmp(clock->name, "csiphy5_timer")) - csiphy->rate_set[i] = true; - - if (camss->res->version == CAMSS_660 && - (!strcmp(clock->name, "csi0_phy") || - !strcmp(clock->name, "csi1_phy") || - !strcmp(clock->name, "csi2_phy"))) - csiphy->rate_set[i] = true; + for (k = 0; k < camss->res->csiphy_num; k++) { + csiphy->rate_set[i] = csiphy_match_clock_name(clock->name, + "csiphy%d_timer", k); + if (csiphy->rate_set[i]) + break; + + if (camss->res->version == CAMSS_660) { + csiphy->rate_set[i] = csiphy_match_clock_name(clock->name, + "csi%d_phy", k); + if (csiphy->rate_set[i]) + break; + } + } } return 0;
The csiphyX_timer and csiX_phy values need not be hard-coded. We can functionally decompose the string matching inside of a loop. Static string values are brittle, difficult to extend and not required anyway since the camss->res->csiphy_num value informs us of the number of CSIPHYs and hence the set of potential clocks for a given CSIPHY. In simple terms if we have five CSIPHYs we can have no more and no less than five csiphy_timer clocks. Similarly csi_phy core clocks have a 1:1 relationship with the PHY they clock. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- .../media/platform/qcom/camss/camss-csiphy.c | 37 ++++++++++++------- 1 file changed, 23 insertions(+), 14 deletions(-)