Re: [PATCH 6/8] dmi: Move memdev_dmi_entry definition to dmi.h (v2)

[Jean Delvare <jdelvare@suse.de>]

Hi Matt,

On Tue,  8 Mar 2016 10:32:37 -0800, Matt Roper wrote:
> A couple of the EDAC drivers have a nice memdev_dmi_entry structure for
> decoding DMI memory device entries.  Move the structure definition to
> dmi.h so that it can be shared between those drivers and also other
> parts of the kernel; the i915 graphics driver is going to need to use
> this structure soon as well.  As part of this move we rename the
> structure s/memdev_dmi_entry/dmi_entry_memdev/ to ensure it has a proper
> 'dmi' prefix.
> 
> v2:
>  - Rename structure to dmi_entry_memdev.  (Jean)
>  - Use __packed instead of __attribute__((__packed__)) for consistency
>    with the rest of the dmi.h header.  (Jean)

Looks better. One more comment inline below:

> (...)
> diff --git a/drivers/edac/i7core_edac.c b/drivers/edac/i7core_edac.c
> index 01087a3..fbfb06f 100644
> --- a/drivers/edac/i7core_edac.c
> +++ b/drivers/edac/i7core_edac.c
> (...)
> @@ -1946,28 +1921,28 @@ static void decode_dclk(const struct dmi_header *dh, void *_dclk_freq)
>  		return;
>  
>  	if (dh->type == DMI_ENTRY_MEM_DEVICE) {
> -		struct memdev_dmi_entry *memdev_dmi_entry =
> -			(struct memdev_dmi_entry *)dh;
> +		struct dmi_entry_memdev *dmi_entry_memdev =
> +			(struct dmi_entry_memdev *)dh;
>  		unsigned long conf_mem_clk_speed_offset =
> -			(unsigned long)&memdev_dmi_entry->conf_mem_clk_speed -
> -			(unsigned long)&memdev_dmi_entry->type;
> +			(unsigned long)&dmi_entry_memdev->conf_mem_clk_speed -
> +			(unsigned long)&dmi_entry_memdev->type;
>  		unsigned long speed_offset =
> -			(unsigned long)&memdev_dmi_entry->speed -
> -			(unsigned long)&memdev_dmi_entry->type;
> +			(unsigned long)&dmi_entry_memdev->speed -
> +			(unsigned long)&dmi_entry_memdev->type;
>  
>  		/* Check that a DIMM is present */
> -		if (memdev_dmi_entry->size == 0)
> +		if (dmi_entry_memdev->size == 0)
>  			return;
>  
>  		/*
>  		 * Pick the configured speed if it's available, otherwise
>  		 * pick the DIMM speed, or we don't have a speed.
>  		 */
> -		if (memdev_dmi_entry->length > conf_mem_clk_speed_offset) {
> +		if (dmi_entry_memdev->length > conf_mem_clk_speed_offset) {
>  			dmi_mem_clk_speed =
> -				memdev_dmi_entry->conf_mem_clk_speed;
> -		} else if (memdev_dmi_entry->length > speed_offset) {
> -			dmi_mem_clk_speed = memdev_dmi_entry->speed;
> +				dmi_entry_memdev->conf_mem_clk_speed;
> +		} else if (dmi_entry_memdev->length > speed_offset) {
> +			dmi_mem_clk_speed = dmi_entry_memdev->speed;
>  		} else {
>  			*dclk_freq = -1;
>  			return;

You do not need all of these changes. memdev_dmi_entry was both the
structure name and the variable name in the original code (something I
usually avoid, but C allows it.) Just because you renamed the structure
doesn't mean you have to rename the variable.

Other than that, looks good to me.

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support