From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1030945AbcCQOS1 (ORCPT ); Thu, 17 Mar 2016 10:18:27 -0400 Received: from mx2.suse.de ([195.135.220.15]:54354 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1030852AbcCQOSY (ORCPT ); Thu, 17 Mar 2016 10:18:24 -0400 Date: Thu, 17 Mar 2016 15:18:20 +0100 From: Jean Delvare To: Matt Roper Cc: intel-gfx@lists.freedesktop.org, Mauro Carvalho Chehab , linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 6/8] dmi: Move memdev_dmi_entry definition to dmi.h (v2) Message-ID: <20160317151820.28fe70da@endymion> In-Reply-To: <1457461957-23029-1-git-send-email-matthew.d.roper@intel.com> References: <20160308133713.24e0b71a@endymion> <1457461957-23029-1-git-send-email-matthew.d.roper@intel.com> Organization: SUSE Linux X-Mailer: Claws Mail 3.12.0 (GTK+ 2.24.29; x86_64-suse-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 -- Jean Delvare SUSE L3 Support