From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-path: Received: from mga04.intel.com ([192.55.52.120]:58233 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753784AbeAKUtT (ORCPT ); Thu, 11 Jan 2018 15:49:19 -0500 Subject: Re: [PATCH linux dev-4.10 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon To: Arnd Bergmann Cc: Joel Stanley , Andrew Jeffery , gregkh , Jean Delvare , Guenter Roeck , Linux Kernel Mailing List , linux-doc@vger.kernel.org, DTML , linux-hwmon@vger.kernel.org, Linux ARM , OpenBMC Maillist References: <20180109223126.13093-1-jae.hyun.yoo@linux.intel.com> <20180109223126.13093-7-jae.hyun.yoo@linux.intel.com> From: Jae Hyun Yoo Message-ID: <58fbf0b8-4e70-e7f3-a929-87fb7e9ac5ba@linux.intel.com> Date: Thu, 11 Jan 2018 12:49:18 -0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-hwmon-owner@vger.kernel.org List-Id: linux-hwmon@vger.kernel.org On 1/11/2018 5:22 AM, Arnd Bergmann wrote: > On Thu, Jan 11, 2018 at 12:45 AM, Jae Hyun Yoo > wrote: >> On 1/10/2018 4:29 AM, Arnd Bergmann wrote: >>> >>> On Tue, Jan 9, 2018 at 11:31 PM, Jae Hyun Yoo >>> wrote: >>>> >>>> This commit adds driver implementation for a generic PECI hwmon. >>>> >>>> Signed-off-by: Jae Hyun Yoo >>> >>> >>>> +static int xfer_peci_msg(int cmd, void *pmsg) >>>> +{ >>>> + int rc; >>>> + >>>> + mutex_lock(&peci_hwmon_lock); >>>> + rc = peci_ioctl(NULL, cmd, (unsigned long)pmsg); >>>> + mutex_unlock(&peci_hwmon_lock); >>>> + >>>> + return rc; >>>> +} >>> >>> >>> I said earlier that peci_ioctl() looked unused, that was obviously >>> wrong, but what you have here >>> is not a proper way to abstract a bus. >>> >>> Maybe this can be done more like an i2c bus: make the peci controller >>> a bus device >>> and register all known target/index pairs as devices with the peci bus >>> type, and have >>> them probed from DT. The driver can then bind to each of those >>> individually. >>> Not sure if that is getting to granular at that point, I'd have to >>> understand better >>> how it is expected to get used, and what the variances are between >>> implementations. >>> >> >> Thanks for sharing your opinion. In fact, this was also suggested by openbmc >> community so I should consider of redesigning it. I'm currently thinking >> about adding a new PECI device class as an abstract layer and any BMC >> chipset specific driver could be attached to the PECI class driver. Then, >> each CPU client could be registered as an individual device as you >> suggested. Will consider your suggestion. > > Another idea might be to pretend that PECI was I2C. We already have a few > drivers for hardware that is not I2C but whose software interface looks > similar enough that it just works. No idea if that is the case for PECI, but > xfer_peci_msg might be close enough to i2c_xfer to make it work. If you > are able to do that, then the PECI controller would just register itself > as an i2c controller and it can be accessed using /dev/i2c from user space > or a high-level i2c_driver. > > Arnd > Thanks for the good idea. It looks like one of possible options. I'll check this idea as well. :) Thanks, Jae From mboxrd@z Thu Jan 1 00:00:00 1970 From: jae.hyun.yoo@linux.intel.com (Jae Hyun Yoo) Date: Thu, 11 Jan 2018 12:49:18 -0800 Subject: [PATCH linux dev-4.10 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon In-Reply-To: References: <20180109223126.13093-1-jae.hyun.yoo@linux.intel.com> <20180109223126.13093-7-jae.hyun.yoo@linux.intel.com> Message-ID: <58fbf0b8-4e70-e7f3-a929-87fb7e9ac5ba@linux.intel.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 1/11/2018 5:22 AM, Arnd Bergmann wrote: > On Thu, Jan 11, 2018 at 12:45 AM, Jae Hyun Yoo > wrote: >> On 1/10/2018 4:29 AM, Arnd Bergmann wrote: >>> >>> On Tue, Jan 9, 2018 at 11:31 PM, Jae Hyun Yoo >>> wrote: >>>> >>>> This commit adds driver implementation for a generic PECI hwmon. >>>> >>>> Signed-off-by: Jae Hyun Yoo >>> >>> >>>> +static int xfer_peci_msg(int cmd, void *pmsg) >>>> +{ >>>> + int rc; >>>> + >>>> + mutex_lock(&peci_hwmon_lock); >>>> + rc = peci_ioctl(NULL, cmd, (unsigned long)pmsg); >>>> + mutex_unlock(&peci_hwmon_lock); >>>> + >>>> + return rc; >>>> +} >>> >>> >>> I said earlier that peci_ioctl() looked unused, that was obviously >>> wrong, but what you have here >>> is not a proper way to abstract a bus. >>> >>> Maybe this can be done more like an i2c bus: make the peci controller >>> a bus device >>> and register all known target/index pairs as devices with the peci bus >>> type, and have >>> them probed from DT. The driver can then bind to each of those >>> individually. >>> Not sure if that is getting to granular at that point, I'd have to >>> understand better >>> how it is expected to get used, and what the variances are between >>> implementations. >>> >> >> Thanks for sharing your opinion. In fact, this was also suggested by openbmc >> community so I should consider of redesigning it. I'm currently thinking >> about adding a new PECI device class as an abstract layer and any BMC >> chipset specific driver could be attached to the PECI class driver. Then, >> each CPU client could be registered as an individual device as you >> suggested. Will consider your suggestion. > > Another idea might be to pretend that PECI was I2C. We already have a few > drivers for hardware that is not I2C but whose software interface looks > similar enough that it just works. No idea if that is the case for PECI, but > xfer_peci_msg might be close enough to i2c_xfer to make it work. If you > are able to do that, then the PECI controller would just register itself > as an i2c controller and it can be accessed using /dev/i2c from user space > or a high-level i2c_driver. > > Arnd > Thanks for the good idea. It looks like one of possible options. I'll check this idea as well. :) Thanks, Jae