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 <jdelv...@suse.de>
--
Jean Delvare
SUSE L3 Support
