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

[Jean Delvare <jdelvare@suse.de>]

Hi Matt,

On Mon,  7 Mar 2016 17:05:44 -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.

Looks good in principle, a few suggestions inline below:

> Cc: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> Cc: Jean Delvare <jdelvare@suse.com>
> Cc: linux-edac@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/edac/ghes_edac.c   | 26 --------------------------
>  drivers/edac/i7core_edac.c | 25 -------------------------
>  include/linux/dmi.h        | 25 +++++++++++++++++++++++++
>  3 files changed, 25 insertions(+), 51 deletions(-)
> (...)
> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
> index 5e9c74c..1eb2136 100644
> --- a/include/linux/dmi.h
> +++ b/include/linux/dmi.h
> @@ -78,6 +78,31 @@ struct dmi_header {
>  	u16 handle;
>  } __packed;
>  
> +struct memdev_dmi_entry {

As a member of the API of the dmi subsystem, this symbol should start
with dmi_. What about dmi_entry_memdev?

> +	u8 type;
> +	u8 length;
> +	u16 handle;
> +	u16 phys_mem_array_handle;
> +	u16 mem_err_info_handle;
> +	u16 total_width;
> +	u16 data_width;
> +	u16 size;
> +	u8 form;
> +	u8 device_set;
> +	u8 device_locator;
> +	u8 bank_locator;
> +	u8 memory_type;
> +	u16 type_detail;
> +	u16 speed;
> +	u8 manufacturer;
> +	u8 serial_number;
> +	u8 asset_tag;
> +	u8 part_number;
> +	u8 attributes;
> +	u32 extended_size;
> +	u16 conf_mem_clk_speed;
> +} __attribute__((__packed__));

dmi_header is declared with __packed instead. I would appreciate
consistency within this header file, so please use __packed too.

> +
>  struct dmi_device {
>  	struct list_head list;
>  	int type;

Thanks,
-- 
Jean Delvare
SUSE L3 Support