Re: [PATCH v5 038/111] staging: greybus: pwm: Make use of pwmchip_parent() macro

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 1/25/24 6:09 AM, Uwe Kleine-König wrote:
struct pwm_chip::dev is about to change. To not have to touch this
driver in the same commit as struct pwm_chip::dev, use the macro
provided for exactly this purpose.

Acked-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>

I think I'd rather see the footprint of your change be much
smaller than it is.  Please see below.

					-Alex

---
  drivers/staging/greybus/pwm.c | 55 +++++++++++++++++------------------
  1 file changed, 26 insertions(+), 29 deletions(-)

diff --git a/drivers/staging/greybus/pwm.c b/drivers/staging/greybus/pwm.c
index a3cb68cfa0f9..75e0518791d8 100644
--- a/drivers/staging/greybus/pwm.c
+++ b/drivers/staging/greybus/pwm.c
@@ -17,7 +17,6 @@
  struct gb_pwm_chip {
  	struct gb_connection	*connection;
  	u8			pwm_max;	/* max pwm number */
-
  	struct pwm_chip		chip;
  };
@@ -39,9 +38,9 @@ static int gb_pwm_count_operation(struct gb_pwm_chip *pwmc)
  	return 0;
  }
-static int gb_pwm_activate_operation(struct gb_pwm_chip *pwmc,
-				     u8 which)
+static int gb_pwm_activate_operation(struct pwm_chip *chip, u8 which)

Why change the type of the argument here?

  {
+	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
  	struct gb_pwm_activate_request request;
  	struct gbphy_device *gbphy_dev;
  	int ret;
@@ -51,7 +50,7 @@ static int gb_pwm_activate_operation(struct gb_pwm_chip *pwmc,
request.which = which; - gbphy_dev = to_gbphy_dev(pwmc->chip.dev);
+	gbphy_dev = to_gbphy_dev(pwmchip_parent(chip));

Just make this line look like this:

	gbphy_dev = to_gbphy_dev(pwmchip_parent(&pwmc->chip));

  	ret = gbphy_runtime_get_sync(gbphy_dev);
  	if (ret)
  		return ret;
@@ -64,9 +63,10 @@ static int gb_pwm_activate_operation(struct gb_pwm_chip *pwmc,
  	return ret;
  }
-static int gb_pwm_deactivate_operation(struct gb_pwm_chip *pwmc,
+static int gb_pwm_deactivate_operation(struct pwm_chip *chip,
  				       u8 which)

Same question here.

  {
+	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
  	struct gb_pwm_deactivate_request request;
  	struct gbphy_device *gbphy_dev;
  	int ret;
@@ -76,7 +76,7 @@ static int gb_pwm_deactivate_operation(struct gb_pwm_chip *pwmc,
request.which = which; - gbphy_dev = to_gbphy_dev(pwmc->chip.dev);
+	gbphy_dev = to_gbphy_dev(pwmchip_parent(chip));

	gbphy_dev = to_gbphy_dev(pwmchip_parent(&pwmc->chip));

  	ret = gbphy_runtime_get_sync(gbphy_dev);
  	if (ret)
  		return ret;
@@ -89,9 +89,10 @@ static int gb_pwm_deactivate_operation(struct gb_pwm_chip *pwmc,
  	return ret;
  }
-static int gb_pwm_config_operation(struct gb_pwm_chip *pwmc,
+static int gb_pwm_config_operation(struct pwm_chip *chip,
  				   u8 which, u32 duty, u32 period)

And here.

  {
+	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
  	struct gb_pwm_config_request request;
  	struct gbphy_device *gbphy_dev;
  	int ret;
@@ -103,7 +104,7 @@ static int gb_pwm_config_operation(struct gb_pwm_chip *pwmc,
  	request.duty = cpu_to_le32(duty);
  	request.period = cpu_to_le32(period);
- gbphy_dev = to_gbphy_dev(pwmc->chip.dev);
+	gbphy_dev = to_gbphy_dev(pwmchip_parent(chip));

	gbphy_dev = to_gbphy_dev(pwmchip_parent(&pwmc->chip));

  	ret = gbphy_runtime_get_sync(gbphy_dev);
  	if (ret)
  		return ret;
@@ -116,9 +117,10 @@ static int gb_pwm_config_operation(struct gb_pwm_chip *pwmc,
  	return ret;
  }
-static int gb_pwm_set_polarity_operation(struct gb_pwm_chip *pwmc,
+static int gb_pwm_set_polarity_operation(struct pwm_chip *chip,
  					 u8 which, u8 polarity)

And here.

  {
+	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
  	struct gb_pwm_polarity_request request;
  	struct gbphy_device *gbphy_dev;
  	int ret;
@@ -129,7 +131,7 @@ static int gb_pwm_set_polarity_operation(struct gb_pwm_chip *pwmc,
  	request.which = which;
  	request.polarity = polarity;
- gbphy_dev = to_gbphy_dev(pwmc->chip.dev);
+	gbphy_dev = to_gbphy_dev(pwmchip_parent(chip));

	gbphy_dev = to_gbphy_dev(pwmchip_parent(&pwmc->chip));

  	ret = gbphy_runtime_get_sync(gbphy_dev);
  	if (ret)
  		return ret;
@@ -142,9 +144,9 @@ static int gb_pwm_set_polarity_operation(struct gb_pwm_chip *pwmc,
  	return ret;
  }
-static int gb_pwm_enable_operation(struct gb_pwm_chip *pwmc,
-				   u8 which)
+static int gb_pwm_enable_operation(struct pwm_chip *chip, u8 which)

And on and on.

  {
+	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
  	struct gb_pwm_enable_request request;
  	struct gbphy_device *gbphy_dev;
  	int ret;
@@ -154,7 +156,7 @@ static int gb_pwm_enable_operation(struct gb_pwm_chip *pwmc,
request.which = which; - gbphy_dev = to_gbphy_dev(pwmc->chip.dev);
+	gbphy_dev = to_gbphy_dev(pwmchip_parent(chip));
  	ret = gbphy_runtime_get_sync(gbphy_dev);
  	if (ret)
  		return ret;
@@ -167,9 +169,9 @@ static int gb_pwm_enable_operation(struct gb_pwm_chip *pwmc,
  	return ret;
  }
-static int gb_pwm_disable_operation(struct gb_pwm_chip *pwmc,
-				    u8 which)
+static int gb_pwm_disable_operation(struct pwm_chip *chip, u8 which)
  {
+	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
  	struct gb_pwm_disable_request request;
  	struct gbphy_device *gbphy_dev;
  	int ret;
@@ -182,7 +184,7 @@ static int gb_pwm_disable_operation(struct gb_pwm_chip *pwmc,
  	ret = gb_operation_sync(pwmc->connection, GB_PWM_TYPE_DISABLE,
  				&request, sizeof(request), NULL, 0);
- gbphy_dev = to_gbphy_dev(pwmc->chip.dev);
+	gbphy_dev = to_gbphy_dev(pwmchip_parent(chip));
  	gbphy_runtime_put_autosuspend(gbphy_dev);
return ret;
@@ -190,19 +192,15 @@ static int gb_pwm_disable_operation(struct gb_pwm_chip *pwmc,
static int gb_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
  {

I guess my question now is, why don't this function and the
next one take a gb_pwm_chip pointer as argument like the rest...
(Not your problem--don't "fix" this in this series.)

-	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
-
-	return gb_pwm_activate_operation(pwmc, pwm->hwpwm);
+	return gb_pwm_activate_operation(chip, pwm->hwpwm);
  };
static void gb_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
  {
-	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
-
  	if (pwm_is_enabled(pwm))
-		dev_warn(chip->dev, "freeing PWM device without disabling\n");
+		dev_warn(pwmchip_parent(chip), "freeing PWM device without disabling\n");
- gb_pwm_deactivate_operation(pwmc, pwm->hwpwm);
+	gb_pwm_deactivate_operation(chip, pwm->hwpwm);
  }
static int gb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -212,22 +210,21 @@ static int gb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
  	bool enabled = pwm->state.enabled;
  	u64 period = state->period;
  	u64 duty_cycle = state->duty_cycle;
-	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
/* Set polarity */
  	if (state->polarity != pwm->state.polarity) {
  		if (enabled) {
-			gb_pwm_disable_operation(pwmc, pwm->hwpwm);
+			gb_pwm_disable_operation(chip, pwm->hwpwm);
  			enabled = false;
  		}
-		err = gb_pwm_set_polarity_operation(pwmc, pwm->hwpwm, state->polarity);
+		err = gb_pwm_set_polarity_operation(chip, pwm->hwpwm, state->polarity);
  		if (err)
  			return err;
  	}
if (!state->enabled) {
  		if (enabled)
-			gb_pwm_disable_operation(pwmc, pwm->hwpwm);
+			gb_pwm_disable_operation(chip, pwm->hwpwm);
  		return 0;
  	}
@@ -243,13 +240,13 @@ static int gb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
  	if (duty_cycle > period)
  		duty_cycle = period;
- err = gb_pwm_config_operation(pwmc, pwm->hwpwm, duty_cycle, period);
+	err = gb_pwm_config_operation(chip, pwm->hwpwm, duty_cycle, period);
  	if (err)
  		return err;
/* enable/disable */
  	if (!enabled)
-		return gb_pwm_enable_operation(pwmc, pwm->hwpwm);
+		return gb_pwm_enable_operation(chip, pwm->hwpwm);
return 0;
  }





[Index of Archives]     [Linux Driver Development]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux