Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1701920 > unrolled thread

Re: [PATCH v2 3/7] pwm: rockchip: Remove the dumplicate rockchip_pwm_ops ops

Started byBoris Brezillon <boris.brezillon@free-electrons.com>
First post2017-08-02 11:00 +0200
Last post2017-08-08 17:50 +0200
Articles 6 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 3/7] pwm: rockchip: Remove the dumplicate  rockchip_pwm_ops ops Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-08-02 11:00 +0200
    Re: [PATCH v2 3/7] pwm: rockchip: Remove the dumplicate  rockchip_pwm_ops ops "David.Wu" <david.wu@rock-chips.com> - 2017-08-02 13:40 +0200
      Re: [PATCH v2 3/7] pwm: rockchip: Remove the dumplicate  rockchip_pwm_ops ops Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-08-02 13:50 +0200
        Re: [PATCH v2 3/7] pwm: rockchip: Remove the dumplicate  rockchip_pwm_ops ops "David.Wu" <david.wu@rock-chips.com> - 2017-08-04 04:40 +0200
          Re: [PATCH v2 3/7] pwm: rockchip: Remove the dumplicate  rockchip_pwm_ops ops Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-08-04 09:10 +0200
            Re: [PATCH v2 3/7] pwm: rockchip: Remove the dumplicate  rockchip_pwm_ops ops "David.Wu" <david.wu@rock-chips.com> - 2017-08-08 17:50 +0200

#1701920 — Re: [PATCH v2 3/7] pwm: rockchip: Remove the dumplicate rockchip_pwm_ops ops

FromBoris Brezillon <boris.brezillon@free-electrons.com>
Date2017-08-02 11:00 +0200
SubjectRe: [PATCH v2 3/7] pwm: rockchip: Remove the dumplicate rockchip_pwm_ops ops
Message-ID<u9XFw-7As-13@gated-at.bofh.it>
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.

[toc] | [next] | [standalone]

#1702028

From"David.Wu" <david.wu@rock-chips.com>
Date2017-08-02 13:40 +0200
Message-ID<ua0am-Og-17@gated-at.bofh.it>
In reply to#1701920
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...?

[toc] | [prev] | [next] | [standalone]

#1702032

FromBoris Brezillon <boris.brezillon@free-electrons.com>
Date2017-08-02 13:50 +0200
Message-ID<ua0k1-RL-3@gated-at.bofh.it>
In reply to#1702028
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);

[toc] | [prev] | [next] | [standalone]

#1703608

From"David.Wu" <david.wu@rock-chips.com>
Date2017-08-04 04:40 +0200
Message-ID<uaAGS-Pk-21@gated-at.bofh.it>
In reply to#1702032
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.

[toc] | [prev] | [next] | [standalone]

#1703731

FromBoris Brezillon <boris.brezillon@free-electrons.com>
Date2017-08-04 09:10 +0200
Message-ID<uaEUa-3Ro-9@gated-at.bofh.it>
In reply to#1703608
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.

[toc] | [prev] | [next] | [standalone]

#1706689

From"David.Wu" <david.wu@rock-chips.com>
Date2017-08-08 17:50 +0200
Message-ID<uceVB-21I-43@gated-at.bofh.it>
In reply to#1703731
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.

> 
> 
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web