| Message ID | 1499486629-9659-4-git-send-email-david.wu@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 8 Jul 2017 12:03:45 +0800 David Wu <david.wu@rock-chips.com> wrote: > The rockchip_pwm_ops_v1 and rockchip_pwm_ops_v2 ops are the same > struct members, remove one of them. > > Signed-off-by: David Wu <david.wu@rock-chips.com> > --- > drivers/pwm/pwm-rockchip.c | 14 ++++---------- > 1 file changed, 4 insertions(+), 10 deletions(-) > > diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c > index cd45f17..85f9515 100644 > --- a/drivers/pwm/pwm-rockchip.c > +++ b/drivers/pwm/pwm-rockchip.c > @@ -255,13 +255,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > return ret; > } > > -static const struct pwm_ops rockchip_pwm_ops_v1 = { > - .get_state = rockchip_pwm_get_state, > - .apply = rockchip_pwm_apply, > - .owner = THIS_MODULE, > -}; > - > -static const struct pwm_ops rockchip_pwm_ops_v2 = { > +static const struct pwm_ops rockchip_pwm_ops = { > .get_state = rockchip_pwm_get_state, > .apply = rockchip_pwm_apply, > .owner = THIS_MODULE, > @@ -275,7 +269,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > .ctrl = 0x0c, > }, > .prescaler = 2, > - .ops = &rockchip_pwm_ops_v1, > + .ops = &rockchip_pwm_ops, > .set_enable = rockchip_pwm_set_enable_v1, > .get_state = rockchip_pwm_get_state_v1, > }; > @@ -289,7 +283,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > }, > .prescaler = 1, > .supports_polarity = true, > - .ops = &rockchip_pwm_ops_v2, > + .ops = &rockchip_pwm_ops, > .set_enable = rockchip_pwm_set_enable_v2, > .get_state = rockchip_pwm_get_state_v2, > }; > @@ -303,7 +297,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > }, > .prescaler = 1, > .supports_polarity = true, > - .ops = &rockchip_pwm_ops_v2, > + .ops = &rockchip_pwm_ops, > .set_enable = rockchip_pwm_set_enable_v2, > .get_state = rockchip_pwm_get_state_v2, > }; Actually, when I suggested to just implement ->apply_state() and be done with all other fields I was thinking that you could get rid of this rockchip_pwm_data struct entirely and just have 3 different pwm_ops. You seem to take the other direction here: you're removing rockchip_pwm_ops_v1 and renaming rockchip_pwm_ops_v2 into rockchip_pwm_ops.
Hi Boris, 在 2017/8/2 16:59, Boris Brezillon 写道: > Actually, when I suggested to just implement ->apply_state() and be > done with all other fields I was thinking that you could get rid of > this rockchip_pwm_data struct entirely and just have 3 different > pwm_ops. You seem to take the other direction here: you're removing > rockchip_pwm_ops_v1 and renaming rockchip_pwm_ops_v2 into > rockchip_pwm_ops. Yes, i really didn't understand exactly what you mean. Your mean is that remove the set_enable, get_state and other hooks, then use the pwm_ops instead of them, which has 3 different version, and implement the pwm_ops's functions like apply(), enable(), get_state() and others...?
On Wed, 2 Aug 2017 19:31:57 +0800 "David.Wu" <david.wu@rock-chips.com> wrote: > Hi Boris, > > 在 2017/8/2 16:59, Boris Brezillon 写道: > > Actually, when I suggested to just implement ->apply_state() and be > > done with all other fields I was thinking that you could get rid of > > this rockchip_pwm_data struct entirely and just have 3 different > > pwm_ops. You seem to take the other direction here: you're removing > > rockchip_pwm_ops_v1 and renaming rockchip_pwm_ops_v2 into > > rockchip_pwm_ops. > > Yes, i really didn't understand exactly what you mean. > Your mean is that remove the set_enable, get_state and other hooks, > then use the pwm_ops instead of them, which has 3 different version, and > implement the pwm_ops's functions like apply(), enable(), get_state() > and others...? > Yep, just define 3 different pwm_ops (one for each IP), each of them implementing ->apply() and ->get_state() and that's all. Something like: static const struct pwm_ops rockchip_pwm_ops_v1 = { .get_state = rockchip_pwm_v1_get_state, .apply = rockchip_pwm_v1_apply, .owner = THIS_MODULE, }; static const struct pwm_ops rockchip_pwm_ops_v2 = { .get_state = rockchip_pwm_v2_get_state, .apply = rockchip_pwm_v2_apply, .owner = THIS_MODULE, }; static const struct pwm_ops rockchip_pwm_ops_vop = { .get_state = rockchip_pwm_vop_get_state, .apply = rockchip_pwm_vop_apply, .owner = THIS_MODULE, }; static const struct of_device_id rockchip_pwm_dt_ids[] = { { .compatible = "rockchip,rk2928-pwm", .data = &rockchip_pwm_ops_v1 }, { .compatible = "rockchip,rk3288-pwm", .data = &rockchip_pwm_ops_v2 }, { .compatible = "rockchip,vop-pwm", .data = &rockchip_pwm_ops_vop }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, rockchip_pwm_dt_ids);
Hi Boris， 在 2017/8/2 19:40, Boris Brezillon 写道: > Yep, just define 3 different pwm_ops (one for each IP), each of them > implementing ->apply() and ->get_state() and that's all. > > Something like: > > static const struct pwm_ops rockchip_pwm_ops_v1 = { > .get_state = rockchip_pwm_v1_get_state, > .apply = rockchip_pwm_v1_apply, > .owner = THIS_MODULE, > }; > > static const struct pwm_ops rockchip_pwm_ops_v2 = { > .get_state = rockchip_pwm_v2_get_state, > .apply = rockchip_pwm_v2_apply, > .owner = THIS_MODULE, > }; > > static const struct pwm_ops rockchip_pwm_ops_vop = { > .get_state = rockchip_pwm_vop_get_state, > .apply = rockchip_pwm_vop_apply, > .owner = THIS_MODULE, > }; > > static const struct of_device_id rockchip_pwm_dt_ids[] = { > { .compatible = "rockchip,rk2928-pwm", .data = &rockchip_pwm_ops_v1 }, > { .compatible = "rockchip,rk3288-pwm", .data = &rockchip_pwm_ops_v2 }, > { .compatible = "rockchip,vop-pwm", .data = &rockchip_pwm_ops_vop }, > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(of, rockchip_pwm_dt_ids); I think we should keep the data members in the rockchip_pwm_data，like supports_polarity and regs... The supports_polarity is needed for of_pwm_n_cells when pwm registered. And the other data members is helpful for us to use common code. It's okay for just define 3 different pwm_ops (one for each IP), but they are with other data members in the struct of rockchip_pwm_data.
On Fri, 4 Aug 2017 10:38:26 +0800 "David.Wu" <david.wu@rock-chips.com> wrote: > Hi Boris， > > 在 2017/8/2 19:40, Boris Brezillon 写道: > > Yep, just define 3 different pwm_ops (one for each IP), each of them > > implementing ->apply() and ->get_state() and that's all. > > > > Something like: > > > > static const struct pwm_ops rockchip_pwm_ops_v1 = { > > .get_state = rockchip_pwm_v1_get_state, > > .apply = rockchip_pwm_v1_apply, > > .owner = THIS_MODULE, > > }; > > > > static const struct pwm_ops rockchip_pwm_ops_v2 = { > > .get_state = rockchip_pwm_v2_get_state, > > .apply = rockchip_pwm_v2_apply, > > .owner = THIS_MODULE, > > }; > > > > static const struct pwm_ops rockchip_pwm_ops_vop = { > > .get_state = rockchip_pwm_vop_get_state, > > .apply = rockchip_pwm_vop_apply, > > .owner = THIS_MODULE, > > }; > > > > static const struct of_device_id rockchip_pwm_dt_ids[] = { > > { .compatible = "rockchip,rk2928-pwm", .data = &rockchip_pwm_ops_v1 }, > > { .compatible = "rockchip,rk3288-pwm", .data = &rockchip_pwm_ops_v2 }, > > { .compatible = "rockchip,vop-pwm", .data = &rockchip_pwm_ops_vop }, > > { /* sentinel */ } > > }; > > MODULE_DEVICE_TABLE(of, rockchip_pwm_dt_ids); > > I think we should keep the data members in the rockchip_pwm_data，like > supports_polarity and regs... > > The supports_polarity is needed for of_pwm_n_cells when pwm registered. > And the other data members is helpful for us to use common code. > > It's okay for just define 3 different pwm_ops (one for each IP), but > they are with other data members in the struct of rockchip_pwm_data. > I think we could even get rid of the other fields in rockchip_pwm_data, but ok, let's do that.
Hi Boris, 在 2017/8/4 15:09, Boris Brezillon 写道: > On Fri, 4 Aug 2017 10:38:26 +0800 > "David.Wu" <david.wu@rock-chips.com> wrote: > >> Hi Boris， >> >> 在 2017/8/2 19:40, Boris Brezillon 写道: >>> Yep, just define 3 different pwm_ops (one for each IP), each of them >>> implementing ->apply() and ->get_state() and that's all. >>> >>> Something like: >>> >>> static const struct pwm_ops rockchip_pwm_ops_v1 = { >>> .get_state = rockchip_pwm_v1_get_state, >>> .apply = rockchip_pwm_v1_apply, >>> .owner = THIS_MODULE, >>> }; >>> >>> static const struct pwm_ops rockchip_pwm_ops_v2 = { >>> .get_state = rockchip_pwm_v2_get_state, >>> .apply = rockchip_pwm_v2_apply, >>> .owner = THIS_MODULE, >>> }; >>> >>> static const struct pwm_ops rockchip_pwm_ops_vop = { >>> .get_state = rockchip_pwm_vop_get_state, >>> .apply = rockchip_pwm_vop_apply, >>> .owner = THIS_MODULE, >>> }; >>> >>> static const struct of_device_id rockchip_pwm_dt_ids[] = { >>> { .compatible = "rockchip,rk2928-pwm", .data = &rockchip_pwm_ops_v1 }, >>> { .compatible = "rockchip,rk3288-pwm", .data = &rockchip_pwm_ops_v2 }, >>> { .compatible = "rockchip,vop-pwm", .data = &rockchip_pwm_ops_vop }, >>> { /* sentinel */ } >>> }; >>> MODULE_DEVICE_TABLE(of, rockchip_pwm_dt_ids); >> >> I think we should keep the data members in the rockchip_pwm_data，like >> supports_polarity and regs... >> >> The supports_polarity is needed for of_pwm_n_cells when pwm registered. >> And the other data members is helpful for us to use common code. >> >> It's okay for just define 3 different pwm_ops (one for each IP), but >> they are with other data members in the struct of rockchip_pwm_data. >> > > I think we could even get rid of the other fields in rockchip_pwm_data, > but ok, let's do that. I use the same pwm ops for each IP at V3's patch, but defined 3 different rockchip_pwm_data for use. I think this might look more clean. > > >
diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c index cd45f17..85f9515 100644 --- a/drivers/pwm/pwm-rockchip.c +++ b/drivers/pwm/pwm-rockchip.c @@ -255,13 +255,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, return ret; } -static const struct pwm_ops rockchip_pwm_ops_v1 = { - .get_state = rockchip_pwm_get_state, - .apply = rockchip_pwm_apply, - .owner = THIS_MODULE, -}; - -static const struct pwm_ops rockchip_pwm_ops_v2 = { +static const struct pwm_ops rockchip_pwm_ops = { .get_state = rockchip_pwm_get_state, .apply = rockchip_pwm_apply, .owner = THIS_MODULE, @@ -275,7 +269,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, .ctrl = 0x0c, }, .prescaler = 2, - .ops = &rockchip_pwm_ops_v1, + .ops = &rockchip_pwm_ops, .set_enable = rockchip_pwm_set_enable_v1, .get_state = rockchip_pwm_get_state_v1, }; @@ -289,7 +283,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, }, .prescaler = 1, .supports_polarity = true, - .ops = &rockchip_pwm_ops_v2, + .ops = &rockchip_pwm_ops, .set_enable = rockchip_pwm_set_enable_v2, .get_state = rockchip_pwm_get_state_v2, }; @@ -303,7 +297,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, }, .prescaler = 1, .supports_polarity = true, - .ops = &rockchip_pwm_ops_v2, + .ops = &rockchip_pwm_ops, .set_enable = rockchip_pwm_set_enable_v2, .get_state = rockchip_pwm_get_state_v2, };
The rockchip_pwm_ops_v1 and rockchip_pwm_ops_v2 ops are the same struct members, remove one of them. Signed-off-by: David Wu <david.wu@rock-chips.com> --- drivers/pwm/pwm-rockchip.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)