From: Dan Carpenter <dan.carpenter@oracle.com>
To: micky_ching@realsil.com.cn
Cc: cjb@laptop.org, sameo@linux.intel.com,
gregkh@linuxfoundation.org, linux-mmc@vger.kernel.org,
linux-kernel@vger.kernel.org, wei_wang@realsil.com.cn,
devel@linuxdriverproject.org
Subject: Re: [PATCH 2/2] mmc: rtsx: modify phase searching method for tunning
Date: Wed, 20 Nov 2013 11:17:58 +0300 [thread overview]
Message-ID: <20131120081758.GX5302@mwanda> (raw)
In-Reply-To: <38cd1d9d7f10f6a65b4dc608f310ac3818da827f.1384858553.git.micky_ching@realsil.com.cn>
On Tue, Nov 19, 2013 at 07:10:35PM +0800, micky_ching@realsil.com.cn wrote:
> -static u8 sd_search_final_phase(struct realtek_pci_sdmmc *host, u32 phase_map)
> +static inline u32 sd_get_phase_point(u32 phase_map, unsigned int idx)
> {
> - struct timing_phase_path path[MAX_PHASE + 1];
> - int i, j, cont_path_cnt;
> - int new_block, max_len, final_path_idx;
> - u8 final_phase = 0xFF;
> + idx &= MAX_PHASE;
> + return phase_map & (1 << idx);
> +}
> +
This function is confusing....
MAX_PHASE is a terrible name. It's in a global header but it doesn't
have a prefix. It should be #define RTS_MAX_PHASE. It's weird that we
are using it both as a number and as bit mask. It's weird that we
always add a "+ 1" to MAX_PHASE. It means the name is chosen poorly.
Maybe it should be:
#define RTS_MAX_PHASE 32
#define RTS_PHASE_MASK 0x1f
So this function takes a phase_map and an index and it wraps the index
by masking away the top bits then it tests to see if the wrapped index
bit is set? The wrapping is very strange.
The name of the function is sd_get_phase_point() but we only test to
see if it returns zero. The name of the function is not helpful.
"sd" is a generic prefix.
"get" is misleading because we don't "get" anything it only returns zero
on success?
"phase_point" might be helpful but I don't know what a phase_point is.
Add a comment about that maybe.
regards,
dan carpenter
WARNING: multiple messages have this Message-ID (diff)
From: Dan Carpenter <dan.carpenter@oracle.com>
To: micky_ching@realsil.com.cn
Cc: sameo@linux.intel.com, gregkh@linuxfoundation.org,
linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org,
wei_wang@realsil.com.cn, devel@linuxdriverproject.org,
cjb@laptop.org
Subject: Re: [PATCH 2/2] mmc: rtsx: modify phase searching method for tunning
Date: Wed, 20 Nov 2013 11:17:58 +0300 [thread overview]
Message-ID: <20131120081758.GX5302@mwanda> (raw)
In-Reply-To: <38cd1d9d7f10f6a65b4dc608f310ac3818da827f.1384858553.git.micky_ching@realsil.com.cn>
On Tue, Nov 19, 2013 at 07:10:35PM +0800, micky_ching@realsil.com.cn wrote:
> -static u8 sd_search_final_phase(struct realtek_pci_sdmmc *host, u32 phase_map)
> +static inline u32 sd_get_phase_point(u32 phase_map, unsigned int idx)
> {
> - struct timing_phase_path path[MAX_PHASE + 1];
> - int i, j, cont_path_cnt;
> - int new_block, max_len, final_path_idx;
> - u8 final_phase = 0xFF;
> + idx &= MAX_PHASE;
> + return phase_map & (1 << idx);
> +}
> +
This function is confusing....
MAX_PHASE is a terrible name. It's in a global header but it doesn't
have a prefix. It should be #define RTS_MAX_PHASE. It's weird that we
are using it both as a number and as bit mask. It's weird that we
always add a "+ 1" to MAX_PHASE. It means the name is chosen poorly.
Maybe it should be:
#define RTS_MAX_PHASE 32
#define RTS_PHASE_MASK 0x1f
So this function takes a phase_map and an index and it wraps the index
by masking away the top bits then it tests to see if the wrapped index
bit is set? The wrapping is very strange.
The name of the function is sd_get_phase_point() but we only test to
see if it returns zero. The name of the function is not helpful.
"sd" is a generic prefix.
"get" is misleading because we don't "get" anything it only returns zero
on success?
"phase_point" might be helpful but I don't know what a phase_point is.
Add a comment about that maybe.
regards,
dan carpenter
next prev parent reply other threads:[~2013-11-20 8:21 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-11-19 11:10 [PATCH 0/2] fix sd power bug and modify phase-searching method micky_ching
2013-11-19 11:10 ` micky_ching
2013-11-19 11:10 ` [PATCH 1/2] mmc: rtsx: fix card poweroff bug micky_ching
2013-11-19 11:10 ` micky_ching
2013-11-19 11:10 ` [PATCH 2/2] mmc: rtsx: modify phase searching method for tunning micky_ching
2013-11-19 11:10 ` micky_ching
2013-11-20 8:17 ` Dan Carpenter [this message]
2013-11-20 8:17 ` Dan Carpenter
2013-11-20 9:13 ` micky
2013-11-20 9:13 ` micky
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20131120081758.GX5302@mwanda \
--to=dan.carpenter@oracle.com \
--cc=cjb@laptop.org \
--cc=devel@linuxdriverproject.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mmc@vger.kernel.org \
--cc=micky_ching@realsil.com.cn \
--cc=sameo@linux.intel.com \
--cc=wei_wang@realsil.com.cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.