From mboxrd@z Thu Jan 1 00:00:00 1970 From: Masahiro Yamada Date: Wed, 05 Nov 2014 14:06:09 +0900 Subject: [U-Boot] [PATCH 6/6] linux/kernel.h: sync min, max, min3, max3 macros with Linux In-Reply-To: <20141104195013.GF13607@amd> References: <1415100418-29016-7-git-send-email-yamada.m@jp.panasonic.com> <20141104195013.GF13607@amd> Message-ID: <20141105140608.CB29.AA925319@jp.panasonic.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Pavel, Thanks for your close checking. On Tue, 4 Nov 2014 20:50:13 +0100 Pavel Machek wrote: > On Tue 2014-11-04 20:26:26, Masahiro Yamada wrote: > > U-Boot has never cared about the type when we get max/min of two > > values, but Linux Kernel does. This commit gets min, max, min3, max3 > > macros synced with the kernel introduing type checks. > > "introducing" I will fix this. > > Many of references of those macros must be fixed to suppress warnings. > > We have two options: > > - Use min, max, min3, max3 only when the arguments have the same type > > (or add casts to the arguments) > > - Use min_t/max_t instead with the appropriate type for the first > > argument > > > > Signed-off-by: Masahiro Yamada > > Acked-by: Pavel Machek > > [I guess the conversion is okay, as is, still, there are places where > code could be cleaned up afterwards...] Fully agreed. In some places, I think we should change the variable types as you suggested but it is beyond the motivation of this series. > > - for (postdiv = 1; postdiv <= min(div, MAX_POSTDIV); postdiv++) { > > + for (postdiv = 1; postdiv <= min(div, (unsigned long)MAX_POSTDIV); > > + postdiv++) { > > It might be cleaner to change MAX_POSTDIV definition to include UL? Sounds good to me. > > > @@ -1838,12 +1838,18 @@ static void program_tr(unsigned long *dimm_populated, > > else > > sdram_ddr1 = false; > > > > - t_rcd_ns = max(t_rcd_ns, spd_read(iic0_dimm_addr[dimm_num], 29) >> 2); > > - t_rrd_ns = max(t_rrd_ns, spd_read(iic0_dimm_addr[dimm_num], 28) >> 2); > > - t_rp_ns = max(t_rp_ns, spd_read(iic0_dimm_addr[dimm_num], 27) >> 2); > > - t_ras_ns = max(t_ras_ns, spd_read(iic0_dimm_addr[dimm_num], 30)); > > - t_rc_ns = max(t_rc_ns, spd_read(iic0_dimm_addr[dimm_num], 41)); > > - t_rfc_ns = max(t_rfc_ns, spd_read(iic0_dimm_addr[dimm_num], 42)); > > + t_rcd_ns = max(t_rcd_ns, > > + (unsigned long)spd_read(iic0_dimm_addr[dimm_num], 29) >> 2); > > + t_rrd_ns = max(t_rrd_ns, > > + (unsigned long)spd_read(iic0_dimm_addr[dimm_num], 28) >> 2); > > + t_rp_ns = max(t_rp_ns, > > + (unsigned long)spd_read(iic0_dimm_addr[dimm_num], 27) >> 2); > > + t_ras_ns = max(t_ras_ns, > > + (unsigned long)spd_read(iic0_dimm_addr[dimm_num], 30)); > > + t_rc_ns = max(t_rc_ns, > > + (unsigned long)spd_read(iic0_dimm_addr[dimm_num], 41)); > > + t_rfc_ns = max(t_rfc_ns, > > + (unsigned long)spd_read(iic0_dimm_addr[dimm_num], 42)); > > } > > } > > Would it be feasible to make spd_read return unsigned long? I am not familiar enough with this code to judge this. > > diff --git a/arch/sandbox/cpu/start.c b/arch/sandbox/cpu/start.c > > index b3d7051..b7e12ab 100644 > > --- a/arch/sandbox/cpu/start.c > > +++ b/arch/sandbox/cpu/start.c > > @@ -38,7 +38,7 @@ int sandbox_early_getopt_check(void) > > > > max_arg_len = 0; > > for (i = 0; i < num_options; ++i) > > - max_arg_len = max(strlen(sb_opt[i]->flag), max_arg_len); > > + max_arg_len = max((int)strlen(sb_opt[i]->flag), max_arg_len); > > max_noarg_len = max_arg_len + 7; > > > > for (i = 0; i < num_options; ++i) { > > make max_arg_len size_t? > > > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c > > index 623f749..677c89f 100644 > > --- a/drivers/net/netconsole.c > > +++ b/drivers/net/netconsole.c > > @@ -256,7 +256,7 @@ static void nc_puts(struct stdio_dev *dev, const char *s) > > > > len = strlen(s); > > while (len) { > > - int send_len = min(len, sizeof(input_buffer)); > > + int send_len = min(len, (int)sizeof(input_buffer)); > > nc_send_packet(s, send_len); > > len -= send_len; > > s += send_len; > > Looks like len/send_len wants to be size_t here. > > Actually, I'd argue that anytime you need to explicitly cast one > argument would be good time to use _t variant... but that would mean > redoing rather big patch. > > min((int) a, b) -> min_t(int, a, b). Agreed but I think we should do this after we understand the code well. Best Regards Masahiro Yamada