diff mbox series

[3/3] drm/bridge: ti-sn65dsi86: Loosen coupling of PWM to ti-sn65dsi86 core

Message ID 20231123175425.496956-4-u.kleine-koenig@pengutronix.de (mailing list archive)
State New, archived
Headers show
Series drm/bridge: ti-sn65dsi86: Some updates | expand

Commit Message

Uwe Kleine-König Nov. 23, 2023, 5:54 p.m. UTC
Introduce a dedicated private data structure for the pwm aux driver
provided by the sn65dsi86 driver. This way data needed for PWM operation
is (to a certain degree) nicely separated and doesn't occupy memory in
the ti_sn65dsi86 core's private data if the PWM isn't used.

The eventual goal is to decouple the PWM driver completely from the
ti-sn65dsi86 core and maybe even move it to a dedicated driver below
drivers/pwm. There are a few obstacles to that quest though:

 - The busy pin check (implemented in ti_sn_pwm_pin_request() and
   ti_sn_pwm_pin_release()) would need to be available unconditionally.

 - The refclk should probably abstracted by a struct clk such that the
   pwm_refclk_freq member that currently still lives in ti-sn65dsi86
   core driver data can be dropped.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 130 +++++++++++++++-----------
 1 file changed, 76 insertions(+), 54 deletions(-)

Comments

Doug Anderson Nov. 29, 2023, 12:32 a.m. UTC | #1
Hi,

On Thu, Nov 23, 2023 at 9:54 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Introduce a dedicated private data structure for the pwm aux driver
> provided by the sn65dsi86 driver. This way data needed for PWM operation
> is (to a certain degree) nicely separated and doesn't occupy memory in
> the ti_sn65dsi86 core's private data if the PWM isn't used.

I suspect we still end up at a loss memory-wise. All of the extra code
+ the overhead of another kmalloc seems like it would take up more
space than the tiny bit of data in the structure.


> The eventual goal is to decouple the PWM driver completely from the
> ti-sn65dsi86 core and maybe even move it to a dedicated driver below
> drivers/pwm. There are a few obstacles to that quest though:
>
>  - The busy pin check (implemented in ti_sn_pwm_pin_request() and
>    ti_sn_pwm_pin_release()) would need to be available unconditionally.
>
>  - The refclk should probably abstracted by a struct clk such that the
>    pwm_refclk_freq member that currently still lives in ti-sn65dsi86
>    core driver data can be dropped.

Right that the above could be done with more abstraction layers. I
guess the question I have is: how much do we gain with that?

Personally I'm not really sold on the idea. If others think this is a
great change then I won't stand in the way, but IMO without a
compelling reason this is just extra abstraction / complexity without
any gain...

> +/*
> + * struct ti_sn65dsi86_pwm_ddata - Platform data for ti-sn65dsi86 pwm driver.

Why "ddata" exactly? It seems like this is just the pwm "data" ?


> + * @chip:         pwm_chip if the PWM is exposed.
> + * @pwm_enabled:  Used to track if the PWM signal is currently enabled.
> + * @regmap:       Regmap for accessing i2c.
> + * @pdata:       platform data of the parent device

"pdata" isn't a member of the struct, but "pwm_refclk_freq" is.


-Doug
Laurent Pinchart Nov. 29, 2023, 12:37 a.m. UTC | #2
On Tue, Nov 28, 2023 at 04:32:10PM -0800, Doug Anderson wrote:
> On Thu, Nov 23, 2023 at 9:54 AM Uwe Kleine-König wrote:
> >
> > Introduce a dedicated private data structure for the pwm aux driver
> > provided by the sn65dsi86 driver. This way data needed for PWM operation
> > is (to a certain degree) nicely separated and doesn't occupy memory in
> > the ti_sn65dsi86 core's private data if the PWM isn't used.
> 
> I suspect we still end up at a loss memory-wise. All of the extra code
> + the overhead of another kmalloc seems like it would take up more
> space than the tiny bit of data in the structure.
> 
> 
> > The eventual goal is to decouple the PWM driver completely from the
> > ti-sn65dsi86 core and maybe even move it to a dedicated driver below
> > drivers/pwm. There are a few obstacles to that quest though:
> >
> >  - The busy pin check (implemented in ti_sn_pwm_pin_request() and
> >    ti_sn_pwm_pin_release()) would need to be available unconditionally.
> >
> >  - The refclk should probably abstracted by a struct clk such that the
> >    pwm_refclk_freq member that currently still lives in ti-sn65dsi86
> >    core driver data can be dropped.
> 
> Right that the above could be done with more abstraction layers. I
> guess the question I have is: how much do we gain with that?
> 
> Personally I'm not really sold on the idea. If others think this is a
> great change then I won't stand in the way, but IMO without a
> compelling reason this is just extra abstraction / complexity without
> any gain...

I'm not convinced either, especially on moving to a separate driver, but
even when it comes to dynamically allocating a new structure. Splitting
the PWM fields to a new ti_sn65dsi86_pwm would be fine (and I think
would increase readibility), but we can then simply embed it in
ti_sn65dsi86.

> > +/*
> > + * struct ti_sn65dsi86_pwm_ddata - Platform data for ti-sn65dsi86 pwm driver.
> 
> Why "ddata" exactly? It seems like this is just the pwm "data" ?
> 
> 
> > + * @chip:         pwm_chip if the PWM is exposed.
> > + * @pwm_enabled:  Used to track if the PWM signal is currently enabled.
> > + * @regmap:       Regmap for accessing i2c.
> > + * @pdata:       platform data of the parent device
> 
> "pdata" isn't a member of the struct, but "pwm_refclk_freq" is.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index d6e3b1280e38..2f030998cb09 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -163,9 +163,6 @@ 
  *                bitmap so we can do atomic ops on it without an extra
  *                lock so concurrent users of our 4 GPIOs don't stomp on
  *                each other's read-modify-write.
- *
- * @pchip:        pwm_chip if the PWM is exposed.
- * @pwm_enabled:  Used to track if the PWM signal is currently enabled.
  * @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM.
  * @pwm_refclk_freq: Cache for the reference clock input to the PWM.
  */
@@ -197,13 +194,25 @@  struct ti_sn65dsi86 {
 	DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
 #endif
 #if defined(CONFIG_PWM)
-	struct pwm_chip			pchip;
-	bool				pwm_enabled;
 	atomic_t			pwm_pin_busy;
 #endif
 	unsigned int			pwm_refclk_freq;
 };
 
+/*
+ * struct ti_sn65dsi86_pwm_ddata - Platform data for ti-sn65dsi86 pwm driver.
+ * @chip:         pwm_chip if the PWM is exposed.
+ * @pwm_enabled:  Used to track if the PWM signal is currently enabled.
+ * @regmap:       Regmap for accessing i2c.
+ * @pdata:	  platform data of the parent device
+ */
+struct ti_sn65dsi86_pwm_ddata {
+	struct pwm_chip			chip;
+	bool				pwm_enabled;
+	struct regmap			*regmap;
+	unsigned int			*pwm_refclk_freq;
+};
+
 static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = {
 	{ .range_min = 0, .range_max = 0xFF },
 };
@@ -1360,33 +1369,37 @@  static struct auxiliary_driver ti_sn_bridge_driver = {
  * PWM Controller
  */
 #if defined(CONFIG_PWM)
-static int ti_sn_pwm_pin_request(struct ti_sn65dsi86 *pdata)
+static int ti_sn_pwm_pin_request(struct device *dev)
 {
+	struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev);
+
 	return atomic_xchg(&pdata->pwm_pin_busy, 1) ? -EBUSY : 0;
 }
 
-static void ti_sn_pwm_pin_release(struct ti_sn65dsi86 *pdata)
+static void ti_sn_pwm_pin_release(struct device *dev)
 {
+	struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev);
+
 	atomic_set(&pdata->pwm_pin_busy, 0);
 }
 
-static struct ti_sn65dsi86 *pwm_chip_to_ti_sn_bridge(struct pwm_chip *chip)
+static struct ti_sn65dsi86_pwm_ddata *pwm_chip_to_ddata(struct pwm_chip *chip)
 {
-	return container_of(chip, struct ti_sn65dsi86, pchip);
+	return container_of(chip, struct ti_sn65dsi86_pwm_ddata, chip);
 }
 
 static int ti_sn_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 {
-	struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
+	struct ti_sn65dsi86_pwm_ddata *ddata = pwm_chip_to_ddata(chip);
 
-	return ti_sn_pwm_pin_request(pdata);
+	return ti_sn_pwm_pin_request(ddata->chip.dev);
 }
 
 static void ti_sn_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 {
-	struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
+	struct ti_sn65dsi86_pwm_ddata *ddata = pwm_chip_to_ddata(chip);
 
-	ti_sn_pwm_pin_release(pdata);
+	ti_sn_pwm_pin_release(ddata->chip.dev);
 }
 
 /*
@@ -1403,7 +1416,7 @@  static void ti_sn_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			   const struct pwm_state *state)
 {
-	struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
+	struct ti_sn65dsi86_pwm_ddata *ddata = pwm_chip_to_ddata(chip);
 	unsigned int pwm_en_inv;
 	unsigned int backlight;
 	unsigned int pre_div;
@@ -1412,24 +1425,24 @@  static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	u64 period;
 	int ret;
 
-	if (!pdata->pwm_enabled) {
-		ret = pm_runtime_resume_and_get(pdata->dev);
+	if (!ddata->pwm_enabled) {
+		ret = pm_runtime_resume_and_get(ddata->chip.dev);
 		if (ret < 0)
 			return ret;
 	}
 
 	if (state->enabled) {
-		if (!pdata->pwm_enabled) {
+		if (!ddata->pwm_enabled) {
 			/*
 			 * The chip might have been powered down while we
 			 * didn't hold a PM runtime reference, so mux in the
 			 * PWM function on the GPIO pin again.
 			 */
-			ret = regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG,
+			ret = regmap_update_bits(ddata->regmap, SN_GPIO_CTRL_REG,
 						 SN_GPIO_MUX_MASK << (2 * SN_PWM_GPIO_IDX),
 						 SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO_IDX));
 			if (ret) {
-				dev_err(pdata->dev, "failed to mux in PWM function\n");
+				dev_err(ddata->chip.dev, "failed to mux in PWM function\n");
 				goto out;
 			}
 		}
@@ -1470,7 +1483,7 @@  static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		 */
 
 		/* Minimum T_pwm is 1 / REFCLK_FREQ */
-		if (state->period <= NSEC_PER_SEC / pdata->pwm_refclk_freq) {
+		if (state->period <= NSEC_PER_SEC / *ddata->pwm_refclk_freq) {
 			ret = -EINVAL;
 			goto out;
 		}
@@ -1480,12 +1493,12 @@  static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		 * Limit period to this to avoid overflows
 		 */
 		period_max = div_u64((u64)NSEC_PER_SEC * 255 * (65535 + 1),
-				     pdata->pwm_refclk_freq);
+				     *ddata->pwm_refclk_freq);
 		period = min(state->period, period_max);
 
-		pre_div = DIV64_U64_ROUND_UP(period * pdata->pwm_refclk_freq,
+		pre_div = DIV64_U64_ROUND_UP(period * *ddata->pwm_refclk_freq,
 					     (u64)NSEC_PER_SEC * (BACKLIGHT_SCALE_MAX + 1));
-		scale = div64_u64(period * pdata->pwm_refclk_freq, (u64)NSEC_PER_SEC * pre_div) - 1;
+		scale = div64_u64(period * *ddata->pwm_refclk_freq, (u64)NSEC_PER_SEC * pre_div) - 1;
 
 		/*
 		 * The documentation has the duty ratio given as:
@@ -1498,34 +1511,34 @@  static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		 * to definition above and adjusting for nanosecond
 		 * representation of duty cycle gives us:
 		 */
-		backlight = div64_u64(state->duty_cycle * pdata->pwm_refclk_freq,
+		backlight = div64_u64(state->duty_cycle * *ddata->pwm_refclk_freq,
 				      (u64)NSEC_PER_SEC * pre_div);
 		if (backlight > scale)
 			backlight = scale;
 
-		ret = regmap_write(pdata->regmap, SN_PWM_PRE_DIV_REG, pre_div);
+		ret = regmap_write(ddata->regmap, SN_PWM_PRE_DIV_REG, pre_div);
 		if (ret) {
-			dev_err(pdata->dev, "failed to update PWM_PRE_DIV\n");
+			dev_err(ddata->chip.dev, "failed to update PWM_PRE_DIV\n");
 			goto out;
 		}
 
-		ti_sn65dsi86_write_u16(pdata->regmap, SN_BACKLIGHT_SCALE_REG, scale);
-		ti_sn65dsi86_write_u16(pdata->regmap, SN_BACKLIGHT_REG, backlight);
+		ti_sn65dsi86_write_u16(ddata->regmap, SN_BACKLIGHT_SCALE_REG, scale);
+		ti_sn65dsi86_write_u16(ddata->regmap, SN_BACKLIGHT_REG, backlight);
 	}
 
 	pwm_en_inv = FIELD_PREP(SN_PWM_EN_MASK, state->enabled) |
 		     FIELD_PREP(SN_PWM_INV_MASK, state->polarity == PWM_POLARITY_INVERSED);
-	ret = regmap_write(pdata->regmap, SN_PWM_EN_INV_REG, pwm_en_inv);
+	ret = regmap_write(ddata->regmap, SN_PWM_EN_INV_REG, pwm_en_inv);
 	if (ret) {
-		dev_err(pdata->dev, "failed to update PWM_EN/PWM_INV\n");
+		dev_err(ddata->chip.dev, "failed to update PWM_EN/PWM_INV\n");
 		goto out;
 	}
 
-	pdata->pwm_enabled = state->enabled;
+	ddata->pwm_enabled = state->enabled;
 out:
 
-	if (!pdata->pwm_enabled)
-		pm_runtime_put_sync(pdata->dev);
+	if (!ddata->pwm_enabled)
+		pm_runtime_put_sync(ddata->chip.dev);
 
 	return ret;
 }
@@ -1533,26 +1546,26 @@  static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 static int ti_sn_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 			       struct pwm_state *state)
 {
-	struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
+	struct ti_sn65dsi86_pwm_ddata *ddata = pwm_chip_to_ddata(chip);
 	unsigned int pwm_en_inv;
 	unsigned int pre_div;
 	u16 backlight;
 	u16 scale;
 	int ret;
 
-	ret = regmap_read(pdata->regmap, SN_PWM_EN_INV_REG, &pwm_en_inv);
+	ret = regmap_read(ddata->regmap, SN_PWM_EN_INV_REG, &pwm_en_inv);
 	if (ret)
 		return ret;
 
-	ret = ti_sn65dsi86_read_u16(pdata->regmap, SN_BACKLIGHT_SCALE_REG, &scale);
+	ret = ti_sn65dsi86_read_u16(ddata->regmap, SN_BACKLIGHT_SCALE_REG, &scale);
 	if (ret)
 		return ret;
 
-	ret = ti_sn65dsi86_read_u16(pdata->regmap, SN_BACKLIGHT_REG, &backlight);
+	ret = ti_sn65dsi86_read_u16(ddata->regmap, SN_BACKLIGHT_REG, &backlight);
 	if (ret)
 		return ret;
 
-	ret = regmap_read(pdata->regmap, SN_PWM_PRE_DIV_REG, &pre_div);
+	ret = regmap_read(ddata->regmap, SN_PWM_PRE_DIV_REG, &pre_div);
 	if (ret)
 		return ret;
 
@@ -1563,9 +1576,9 @@  static int ti_sn_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 		state->polarity = PWM_POLARITY_NORMAL;
 
 	state->period = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC * pre_div * (scale + 1),
-					 pdata->pwm_refclk_freq);
+					 *ddata->pwm_refclk_freq);
 	state->duty_cycle = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC * pre_div * backlight,
-					     pdata->pwm_refclk_freq);
+					     *ddata->pwm_refclk_freq);
 
 	if (state->duty_cycle > state->period)
 		state->duty_cycle = state->period;
@@ -1583,25 +1596,34 @@  static const struct pwm_ops ti_sn_pwm_ops = {
 static int ti_sn_pwm_probe(struct auxiliary_device *adev,
 			   const struct auxiliary_device_id *id)
 {
+	struct ti_sn65dsi86_pwm_ddata *ddata;
 	struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
 
-	pdata->pchip.dev = pdata->dev;
-	pdata->pchip.ops = &ti_sn_pwm_ops;
-	pdata->pchip.npwm = 1;
-	pdata->pchip.of_xlate = of_pwm_single_xlate;
-	pdata->pchip.of_pwm_n_cells = 1;
+	ddata = devm_kzalloc(pdata->dev, sizeof(*ddata), GFP_KERNEL);
+	if (!ddata)
+		return -ENOMEM;
 
-	return pwmchip_add(&pdata->pchip);
+	ddata->chip.dev = pdata->dev;
+	ddata->chip.ops = &ti_sn_pwm_ops;
+	ddata->chip.npwm = 1;
+	ddata->chip.of_xlate = of_pwm_single_xlate;
+	ddata->chip.of_pwm_n_cells = 1;
+	ddata->regmap = pdata->regmap;
+	ddata->pwm_refclk_freq = &pdata->pwm_refclk_freq;
+
+	auxiliary_set_drvdata(adev, ddata);
+
+	return pwmchip_add(&ddata->chip);
 }
 
 static void ti_sn_pwm_remove(struct auxiliary_device *adev)
 {
-	struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
+	struct ti_sn65dsi86_pwm_ddata *ddata = auxiliary_get_drvdata(adev);
 
-	pwmchip_remove(&pdata->pchip);
+	pwmchip_remove(&ddata->chip);
 
-	if (pdata->pwm_enabled)
-		pm_runtime_put_sync(pdata->dev);
+	if (ddata->pwm_enabled)
+		pm_runtime_put_sync(ddata->chip.dev);
 }
 
 static const struct auxiliary_device_id ti_sn_pwm_id_table[] = {
@@ -1627,8 +1649,8 @@  static void ti_sn_pwm_unregister(void)
 }
 
 #else
-static inline int ti_sn_pwm_pin_request(struct ti_sn65dsi86 *pdata) { return 0; }
-static inline void ti_sn_pwm_pin_release(struct ti_sn65dsi86 *pdata) {}
+static inline int ti_sn_pwm_pin_request(struct device *dev) { return 0; }
+static inline void ti_sn_pwm_pin_release(struct device *dev) {}
 
 static inline int ti_sn_pwm_register(void) { return 0; }
 static inline void ti_sn_pwm_unregister(void) {}
@@ -1774,7 +1796,7 @@  static int ti_sn_bridge_gpio_request(struct gpio_chip *chip, unsigned int offset
 	struct ti_sn65dsi86 *pdata = gpiochip_get_data(chip);
 
 	if (offset == SN_PWM_GPIO_IDX)
-		return ti_sn_pwm_pin_request(pdata);
+		return ti_sn_pwm_pin_request(pdata->dev);
 
 	return 0;
 }
@@ -1787,7 +1809,7 @@  static void ti_sn_bridge_gpio_free(struct gpio_chip *chip, unsigned int offset)
 	ti_sn_bridge_gpio_direction_input(chip, offset);
 
 	if (offset == SN_PWM_GPIO_IDX)
-		ti_sn_pwm_pin_release(pdata);
+		ti_sn_pwm_pin_release(pdata->dev);
 }
 
 static const char * const ti_sn_bridge_gpio_names[SN_NUM_GPIOS] = {