| Message ID | 1606202906-14485-1-git-send-email-cang@codeaurora.org |
|---|---|
| Headers | show |
| Series | Refector ufshcd_setup_clocks() to remove param skip_ref_clk | expand |
On Mon, 2020-11-23 at 23:28 -0800, Can Guo wrote: > +++ b/drivers/scsi/ufs/ufshcd.h > @@ -229,6 +229,8 @@ struct ufs_dev_cmd { > * @max_freq: maximum frequency supported by the clock > * @min_freq: min frequency that can be used for clock scaling > * @curr_freq: indicates the current frequency that it is set to > + * @always_on_while_link_active: indicate that the clk should not be > disabled if > + link is still active > * @enabled: variable to check against multiple enable/disable > */ > struct ufs_clk_info { > @@ -238,6 +240,7 @@ struct ufs_clk_info { > u32 max_freq; > u32 min_freq; > u32 curr_freq; > + bool always_on_while_link_active; Can, using a sentence as a parameter name looks a little bit clumsy to me. The meaning has been explained in the comments section. How about simplify it and in line with other parameters in the structure? Thanks, Bean > bool enabled; > }; >
On 2020-11-25 05:09, Bean Huo wrote: > On Mon, 2020-11-23 at 23:28 -0800, Can Guo wrote: >> +++ b/drivers/scsi/ufs/ufshcd.h >> @@ -229,6 +229,8 @@ struct ufs_dev_cmd { >> * @max_freq: maximum frequency supported by the clock >> * @min_freq: min frequency that can be used for clock scaling >> * @curr_freq: indicates the current frequency that it is set to >> + * @always_on_while_link_active: indicate that the clk should not be >> disabled if >> + link is still active >> * @enabled: variable to check against multiple enable/disable >> */ >> struct ufs_clk_info { >> @@ -238,6 +240,7 @@ struct ufs_clk_info { >> u32 max_freq; >> u32 min_freq; >> u32 curr_freq; >> + bool always_on_while_link_active; > > Can, > using a sentence as a parameter name looks a little bit clumsy to me. > The meaning has been explained in the comments section. How about > simplify it and in line with other parameters in the structure? > Do you have a better name in mind? Thanks, Can Guo. > Thanks, > Bean > >> bool enabled; >> }; >>
On 2020-11-25 08:53, Can Guo wrote: > On 2020-11-25 05:09, Bean Huo wrote: >> On Mon, 2020-11-23 at 23:28 -0800, Can Guo wrote: >>> +++ b/drivers/scsi/ufs/ufshcd.h >>> @@ -229,6 +229,8 @@ struct ufs_dev_cmd { >>> * @max_freq: maximum frequency supported by the clock >>> * @min_freq: min frequency that can be used for clock scaling >>> * @curr_freq: indicates the current frequency that it is set to >>> + * @always_on_while_link_active: indicate that the clk should not be >>> disabled if >>> + link is still active >>> * @enabled: variable to check against multiple enable/disable >>> */ >>> struct ufs_clk_info { >>> @@ -238,6 +240,7 @@ struct ufs_clk_info { >>> u32 max_freq; >>> u32 min_freq; >>> u32 curr_freq; >>> + bool always_on_while_link_active; >> >> Can, >> using a sentence as a parameter name looks a little bit clumsy to me. >> The meaning has been explained in the comments section. How about >> simplify it and in line with other parameters in the structure? >> > > Do you have a better name in mind? > > Thanks, > > Can Guo. > >> Thanks, >> Bean >> >>> bool enabled; >>> }; >>> Looks good to me. The variable name is not a problem. Reviewed-by: Hongwu Su<hongwus@codeaurora.org>
On Wed, 2020-11-25 at 08:53 +0800, Can Guo wrote: > > > + bool always_on_while_link_active; > > > > Can, > > using a sentence as a parameter name looks a little bit clumsy to > > me. > > The meaning has been explained in the comments section. How about > > simplify it and in line with other parameters in the structure? > > > > Do you have a better name in mind? > no specail input in mind, maybe just "bool eternal_on" > Thanks, > > Can Guo.
On 2020-11-25 19:54, Bean Huo wrote: > On Wed, 2020-11-25 at 08:53 +0800, Can Guo wrote: >> > > + bool always_on_while_link_active; >> > >> > Can, >> > using a sentence as a parameter name looks a little bit clumsy to >> > me. >> > The meaning has been explained in the comments section. How about >> > simplify it and in line with other parameters in the structure? >> > >> >> Do you have a better name in mind? >> > no specail input in mind, maybe just "bool eternal_on" It is like plain "always_on", but it cannot tell the whole story. If it is not something crutial, let's just let it go first so long as it does not break the original functionality. What do you say? Thanks, Can Guo. > >> Thanks, >> >> Can Guo.
On Wed, 2020-11-25 at 20:28 +0800, Can Guo wrote: > > On Wed, 2020-11-25 at 08:53 +0800, Can Guo wrote: > > > > > + bool always_on_while_link_active; > > > > > > > > Can, > > > > using a sentence as a parameter name looks a little bit clumsy > > > > to > > > > me. > > > > The meaning has been explained in the comments section. How > > > > about > > > > simplify it and in line with other parameters in the structure? > > > > > > > > > > Do you have a better name in mind? > > > > > > > no specail input in mind, maybe just "bool eternal_on" > > It is like plain "always_on", but it cannot tell the whole story. > If it is not something crutial, let's just let it go first so long > as it does not break the original functionality. What do you say? > > Thanks, > > Can Guo. Can, yes, it is not functional change, but always_on_while_link_active is too fat, and not non-productive way. anyway, Reviewed-by: Bean Huo <beanhuo@micron.com>
On 2020-11-26 03:02, Bean Huo wrote: > On Wed, 2020-11-25 at 20:28 +0800, Can Guo wrote: >> > On Wed, 2020-11-25 at 08:53 +0800, Can Guo wrote: >> > > > > + bool always_on_while_link_active; >> > > > >> > > > Can, >> > > > using a sentence as a parameter name looks a little bit clumsy >> > > > to >> > > > me. >> > > > The meaning has been explained in the comments section. How >> > > > about >> > > > simplify it and in line with other parameters in the structure? >> > > > >> > > >> > > Do you have a better name in mind? >> > > >> > >> > no specail input in mind, maybe just "bool eternal_on" >> >> It is like plain "always_on", but it cannot tell the whole story. >> If it is not something crutial, let's just let it go first so long >> as it does not break the original functionality. What do you say? >> >> Thanks, >> >> Can Guo. > > Can, > > yes, it is not functional change, but always_on_while_link_active is > too fat, and not non-productive way. > anyway, I will change it to keep_link_active in next version. Thank you sir. Regards, Can Guo. > > Reviewed-by: Bean Huo <beanhuo@micron.com>