Linux-Hwmon Archive on lore.kernel.org
 help / Atom feed
From: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
To: Lee Jones <lee.jones@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>,
	Jean Delvare <jdelvare@suse.com>,
	Guenter Roeck <linux@roeck-us.net>,
	Mark Rutland <mark.rutland@arm.com>,
	Joel Stanley <joel@jms.id.au>, Andrew Jeffery <andrew@aj.id.au>,
	Jonathan Corbet <corbet@lwn.net>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
	Kishon Vijay Abraham I <kishon@ti.com>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	"Darrick J . Wong" <darrick.wong@oracle.com>,
	Eric Sandeen <sandeen@redhat.com>, Arnd Bergmann <arnd@arndb.de>,
	Wu Hao <hao.wu@intel.com>,
	Tomohiro Kusumi <kusumi.tomohiro@gmail.com>,
	"Bryant G . Ly" <bryantly@linux.vnet.ibm.com>,
	Frederic Barrat <fbarrat@linux.vnet.ibm.com>,
	"David S . Miller" <davem@davemloft.net>,
	Mauro Carvalho Chehab <mchehab+samsung@kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Randy Dunlap <rdunlap@infradead.org>,
	Philippe Ombredanne <pombredanne@nexb.com>,
	Vinod Koul <vkoul@kernel.org>,
	Stephen Boyd <sboyd@codeaurora.org>,
	David Kershner <david.kershner@unisys.com>,
	Uwe Kleine-Konig <u.kleine-koenig@pengutronix.de>,
	Sagar Dharia <sdharia@codeaurora.org>,
	Johan Hovold <johan@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Juergen Gross <jgross@suse.com>,
	Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>,
	linux-hwmon@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
	openbmc@lists.ozlabs.org,
	James Feist <james.feist@linux.intel.com>,
	Jason M Biils <jason.m.bills@linux.intel.com>,
	Vernon Mauery <vernon.mauery@linux.intel.com>
Subject: Re: [PATCH v9 08/12] mfd: intel-peci-client: Add PECI client MFD driver
Date: Wed, 2 Jan 2019 10:22:18 -0800
Message-ID: <8e4bcf7d-aa43-6745-2ade-9123c8d182ef@linux.intel.com> (raw)
In-Reply-To: <20181221144603.GR13248@dell>

Hi Lee,

On 12/21/2018 6:46 AM, Lee Jones wrote:
> On Tue, 18 Dec 2018, Jae Hyun Yoo wrote:
> 
>> This commit adds PECI client MFD driver.
>>

<snip>

>>   
>> +config MFD_INTEL_PECI_CLIENT
>> +	bool "Intel PECI client"
>> +	depends on (PECI || COMPILE_TEST)
>> +	select MFD_CORE
>> +	help
>> +	  If you say yes to this option, support will be included for the
>> +	  Intel PECI (Platform Environment Control Interface) client. PECI is a
>> +	  one-wire bus interface that provides a communication channel from PECI
>> +	  clients in Intel processors and chipset components to external
>> +	  monitoring or control devices.
> 
> This driver doesn't appear to actually do anything that can't be done
> in a header file i.e. match some static data with a CPU ID.  The child
> devices can be registered by whatever registers this device.
> 
> It seems superfluous.  Why do you need it?
>

The main reason I added it is to provide a way for sharing the same unit
address from multiple side-band function drivers that read 'reg'
property setting from the parent node (this driver's node). The 'reg'
property reading code is not in this driver but it will be performed in
peci-core when this driver is registered using
module_peci_driver(peci_client_driver), and then it will provides
client->addr information for all its child node drivers.

<snip>

>> +#include <linux/bitfield.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/mfd/intel-peci-client.h>
>> +#include <linux/module.h>
>> +#include <linux/peci.h>
>> +#include <linux/of_device.h>
> 
> Alphabetical.
> 

Will fix it. Thanks!

<snip>

>> +enum cpu_gens {
>> +	CPU_GEN_HSX = 0, /* Haswell Xeon */
>> +	CPU_GEN_BRX,     /* Broadwell Xeon */
>> +	CPU_GEN_SKX,     /* Skylake Xeon */
>> +};
> 
> This is unused.
> 

This is being used in 8 lines below but actually the static array can be
initialized without using this enum type. Will remove it.

>> +static struct mfd_cell peci_functions[] = {
>> +	{ .name = "peci-cputemp", },
>> +	{ .name = "peci-dimmtemp", },
>> +	/* TODO: Add additional PECI sideband functions into here */
>> +};
>> +
>> +static const struct cpu_gen_info cpu_gen_info_table[] = {
>> +	[CPU_GEN_HSX] = {
>> +		.family        = 6, /* Family code */
>> +		.model         = INTEL_FAM6_HASWELL_X,
>> +		.core_max      = CORE_MAX_ON_HSX,
>> +		.chan_rank_max = CHAN_RANK_MAX_ON_HSX,
>> +		.dimm_idx_max  = DIMM_IDX_MAX_ON_HSX },
>> +	[CPU_GEN_BRX] = {
>> +		.family        = 6, /* Family code */
>> +		.model         = INTEL_FAM6_BROADWELL_X,
>> +		.core_max      = CORE_MAX_ON_BDX,
>> +		.chan_rank_max = CHAN_RANK_MAX_ON_BDX,
>> +		.dimm_idx_max  = DIMM_IDX_MAX_ON_BDX },
>> +	[CPU_GEN_SKX] = {
>> +		.family        = 6, /* Family code */
>> +		.model         = INTEL_FAM6_SKYLAKE_X,
>> +		.core_max      = CORE_MAX_ON_SKX,
>> +		.chan_rank_max = CHAN_RANK_MAX_ON_SKX,
>> +		.dimm_idx_max  = DIMM_IDX_MAX_ON_SKX },
>> +};
>> +
>> +static int peci_client_get_cpu_gen_info(struct peci_client_manager *priv)
>> +{
>> +	u32 cpu_id;
>> +	u16 family;
>> +	u8 model;
>> +	int rc;
> 
> ret is almost ubiquitous in the kernel.  Please use it instead.
> 

Okay. Will change rc to ret.

<snip>

>> +static int peci_client_probe(struct peci_client *client)
>> +{
>> +	struct device *dev = &client->dev;
>> +	struct peci_client_manager *priv;
>> +	uint cpu_no;
>> +	int ret;
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	dev_set_drvdata(dev, priv);
>> +	priv->client = client;
>> +	priv->dev = dev;
> 
> If you have client (Which contains dev, you don't need dev).
> 

Makes sense. Will remove the 'dev' member.

<snip>

  reply index

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-18 21:04 [PATCH v9 00/12] PECI device driver introduction Jae Hyun Yoo
2018-12-18 21:04 ` [PATCH v9 01/12] dt-bindings: Add a document of PECI subsystem Jae Hyun Yoo
2018-12-18 21:04 ` [PATCH v9 02/12] Documentation: ioctl: Add ioctl numbers for " Jae Hyun Yoo
2018-12-18 21:04 ` [PATCH v9 03/12] peci: Add support for PECI bus driver core Jae Hyun Yoo
2018-12-18 21:04 ` [PATCH v9 04/12] dt-bindings: Add a document of PECI adapter driver for ASPEED AST24xx/25xx SoCs Jae Hyun Yoo
2018-12-18 21:04 ` [PATCH v9 05/12] ARM: dts: aspeed: peci: Add PECI node Jae Hyun Yoo
2018-12-18 21:04 ` [PATCH v9 06/12] peci: Add a PECI adapter driver for Aspeed AST24xx/AST25xx Jae Hyun Yoo
2018-12-18 21:04 ` [PATCH v9 07/12] dt-bindings: mfd: Add a document for PECI client MFD Jae Hyun Yoo
2018-12-21 14:47   ` Lee Jones
2019-01-02 18:29     ` Jae Hyun Yoo
2018-12-18 21:04 ` [PATCH v9 08/12] mfd: intel-peci-client: Add PECI client MFD driver Jae Hyun Yoo
2018-12-21 14:46   ` Lee Jones
2019-01-02 18:22     ` Jae Hyun Yoo [this message]
2018-12-18 21:04 ` [PATCH v9 09/12] Documentation: hwmon: Add documents for PECI hwmon client drivers Jae Hyun Yoo
2018-12-18 21:04 ` [PATCH v9 10/12] hwmon: Add PECI cputemp driver Jae Hyun Yoo
2018-12-18 21:04 ` [PATCH v9 11/12] hwmon: Add PECI dimmtemp driver Jae Hyun Yoo
2018-12-18 21:04 ` [PATCH v9 12/12] Add maintainers for the PECI subsystem Jae Hyun Yoo

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8e4bcf7d-aa43-6745-2ade-9123c8d182ef@linux.intel.com \
    --to=jae.hyun.yoo@linux.intel.com \
    --cc=akpm@linux-foundation.org \
    --cc=andrew@aj.id.au \
    --cc=arnd@arndb.de \
    --cc=bryantly@linux.vnet.ibm.com \
    --cc=corbet@lwn.net \
    --cc=cyrille.pitchen@wedev4u.fr \
    --cc=darrick.wong@oracle.com \
    --cc=davem@davemloft.net \
    --cc=david.kershner@unisys.com \
    --cc=devicetree@vger.kernel.org \
    --cc=fbarrat@linux.vnet.ibm.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=gustavo.pimentel@synopsys.com \
    --cc=hao.wu@intel.com \
    --cc=james.feist@linux.intel.com \
    --cc=jason.m.bills@linux.intel.com \
    --cc=jdelvare@suse.com \
    --cc=jgross@suse.com \
    --cc=joel@jms.id.au \
    --cc=johan@kernel.org \
    --cc=kishon@ti.com \
    --cc=kusumi.tomohiro@gmail.com \
    --cc=lee.jones@linaro.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-hwmon@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=mark.rutland@arm.com \
    --cc=mchehab+samsung@kernel.org \
    --cc=openbmc@lists.ozlabs.org \
    --cc=pombredanne@nexb.com \
    --cc=rdunlap@infradead.org \
    --cc=robh+dt@kernel.org \
    --cc=sandeen@redhat.com \
    --cc=sboyd@codeaurora.org \
    --cc=sdharia@codeaurora.org \
    --cc=tglx@linutronix.de \
    --cc=u.kleine-koenig@pengutronix.de \
    --cc=vernon.mauery@linux.intel.com \
    --cc=vkoul@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Hwmon Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hwmon/0 linux-hwmon/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hwmon linux-hwmon/ https://lore.kernel.org/linux-hwmon \
		linux-hwmon@vger.kernel.org linux-hwmon@archiver.kernel.org
	public-inbox-index linux-hwmon


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hwmon


AGPL code for this site: git clone https://public-inbox.org/ public-inbox