| Message ID | 20231123175425.496956-3-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: ti-sn65dsi86: Some updates | expand |
On 23/11/2023 18:54, Uwe Kleine-König wrote: > This aligns the function's parameters to regmap_{read,write} and > simplifies the next change that takes pwm driver data out of struct > ti_sn65dsi86. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index 5b8e1dfc458d..d6e3b1280e38 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -221,13 +221,13 @@ static const struct regmap_config ti_sn65dsi86_regmap_config = { > .max_register = 0xFF, > }; > > -static int __maybe_unused ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata, > +static int __maybe_unused ti_sn65dsi86_read_u16(struct regmap *regmap, > unsigned int reg, u16 *val) > { > u8 buf[2]; > int ret; > > - ret = regmap_bulk_read(pdata->regmap, reg, buf, ARRAY_SIZE(buf)); > + ret = regmap_bulk_read(regmap, reg, buf, ARRAY_SIZE(buf)); > if (ret) > return ret; > > @@ -236,12 +236,12 @@ static int __maybe_unused ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata, > return 0; > } > > -static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata, > +static void ti_sn65dsi86_write_u16(struct regmap *regmap, > unsigned int reg, u16 val) > { > u8 buf[2] = { val & 0xff, val >> 8 }; > > - regmap_bulk_write(pdata->regmap, reg, buf, ARRAY_SIZE(buf)); > + regmap_bulk_write(regmap, reg, buf, ARRAY_SIZE(buf)); > } > > static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *pdata) > @@ -968,9 +968,9 @@ static void ti_sn_bridge_set_video_timings(struct ti_sn65dsi86 *pdata) > if (mode->flags & DRM_MODE_FLAG_NVSYNC) > vsync_polarity = CHA_VSYNC_POLARITY; > > - ti_sn65dsi86_write_u16(pdata, SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG, > + ti_sn65dsi86_write_u16(pdata->regmap, SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG, > mode->hdisplay); > - ti_sn65dsi86_write_u16(pdata, SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG, > + ti_sn65dsi86_write_u16(pdata->regmap, SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG, > mode->vdisplay); > regmap_write(pdata->regmap, SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG, > (mode->hsync_end - mode->hsync_start) & 0xFF); > @@ -1509,8 +1509,8 @@ static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > goto out; > } > > - ti_sn65dsi86_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, scale); > - ti_sn65dsi86_write_u16(pdata, SN_BACKLIGHT_REG, backlight); > + ti_sn65dsi86_write_u16(pdata->regmap, SN_BACKLIGHT_SCALE_REG, scale); > + ti_sn65dsi86_write_u16(pdata->regmap, SN_BACKLIGHT_REG, backlight); > } > > pwm_en_inv = FIELD_PREP(SN_PWM_EN_MASK, state->enabled) | > @@ -1544,11 +1544,11 @@ static int ti_sn_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > if (ret) > return ret; > > - ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_SCALE_REG, &scale); > + ret = ti_sn65dsi86_read_u16(pdata->regmap, SN_BACKLIGHT_SCALE_REG, &scale); > if (ret) > return ret; > > - ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_REG, &backlight); > + ret = ti_sn65dsi86_read_u16(pdata->regmap, SN_BACKLIGHT_REG, &backlight); > if (ret) > return ret; > Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
Hi, On Thu, Nov 23, 2023 at 9:54 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > This aligns the function's parameters to regmap_{read,write} and > simplifies the next change that takes pwm driver data out of struct > ti_sn65dsi86. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) I'm on the fence for this one. It almost feels like if this takes a "regmap" as the first field then it should be part of the regmap API. Adding a concept like this to the regmap API might be interesting if there were more than one user, but that's a pretty big yak to shave. I'd tend to agree with your statement in the cover letter that this patch really makes more sense if we were to take patch #3, and (as per my response there) I'm not convinced. That being said, similar to patch #3 if everything else thinks this is great then I won't stand in the way. -Doug
On Tue, Nov 28, 2023 at 04:34:30PM -0800, Doug Anderson wrote: > On Thu, Nov 23, 2023 at 9:54 AM Uwe Kleine-König wrote: > > > > This aligns the function's parameters to regmap_{read,write} and > > simplifies the next change that takes pwm driver data out of struct > > ti_sn65dsi86. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > --- > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 ++++++++++---------- > > 1 file changed, 10 insertions(+), 10 deletions(-) > > I'm on the fence for this one. It almost feels like if this takes a > "regmap" as the first field then it should be part of the regmap API. > Adding a concept like this to the regmap API might be interesting if > there were more than one user, but that's a pretty big yak to shave. See include/media/v4l2-cci.h and drivers/media/v4l2-core/v4l2-cci.c. We could discuss moving it to regmap. > I'd tend to agree with your statement in the cover letter that this > patch really makes more sense if we were to take patch #3, and (as per > my response there) I'm not convinced. Likewise :-) 1/3 is good, but without 3/3, I'm not conviced by 2/3. > That being said, similar to patch #3 if everything else thinks this is > great then I won't stand in the way.
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 5b8e1dfc458d..d6e3b1280e38 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -221,13 +221,13 @@ static const struct regmap_config ti_sn65dsi86_regmap_config = { .max_register = 0xFF, }; -static int __maybe_unused ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata, +static int __maybe_unused ti_sn65dsi86_read_u16(struct regmap *regmap, unsigned int reg, u16 *val) { u8 buf[2]; int ret; - ret = regmap_bulk_read(pdata->regmap, reg, buf, ARRAY_SIZE(buf)); + ret = regmap_bulk_read(regmap, reg, buf, ARRAY_SIZE(buf)); if (ret) return ret; @@ -236,12 +236,12 @@ static int __maybe_unused ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata, return 0; } -static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata, +static void ti_sn65dsi86_write_u16(struct regmap *regmap, unsigned int reg, u16 val) { u8 buf[2] = { val & 0xff, val >> 8 }; - regmap_bulk_write(pdata->regmap, reg, buf, ARRAY_SIZE(buf)); + regmap_bulk_write(regmap, reg, buf, ARRAY_SIZE(buf)); } static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *pdata) @@ -968,9 +968,9 @@ static void ti_sn_bridge_set_video_timings(struct ti_sn65dsi86 *pdata) if (mode->flags & DRM_MODE_FLAG_NVSYNC) vsync_polarity = CHA_VSYNC_POLARITY; - ti_sn65dsi86_write_u16(pdata, SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG, + ti_sn65dsi86_write_u16(pdata->regmap, SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG, mode->hdisplay); - ti_sn65dsi86_write_u16(pdata, SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG, + ti_sn65dsi86_write_u16(pdata->regmap, SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG, mode->vdisplay); regmap_write(pdata->regmap, SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG, (mode->hsync_end - mode->hsync_start) & 0xFF); @@ -1509,8 +1509,8 @@ static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, goto out; } - ti_sn65dsi86_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, scale); - ti_sn65dsi86_write_u16(pdata, SN_BACKLIGHT_REG, backlight); + ti_sn65dsi86_write_u16(pdata->regmap, SN_BACKLIGHT_SCALE_REG, scale); + ti_sn65dsi86_write_u16(pdata->regmap, SN_BACKLIGHT_REG, backlight); } pwm_en_inv = FIELD_PREP(SN_PWM_EN_MASK, state->enabled) | @@ -1544,11 +1544,11 @@ static int ti_sn_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, if (ret) return ret; - ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_SCALE_REG, &scale); + ret = ti_sn65dsi86_read_u16(pdata->regmap, SN_BACKLIGHT_SCALE_REG, &scale); if (ret) return ret; - ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_REG, &backlight); + ret = ti_sn65dsi86_read_u16(pdata->regmap, SN_BACKLIGHT_REG, &backlight); if (ret) return ret;
This aligns the function's parameters to regmap_{read,write} and simplifies the next change that takes pwm driver data out of struct ti_sn65dsi86. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)