From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752830AbdB1NeO (ORCPT ); Tue, 28 Feb 2017 08:34:14 -0500 Received: from mx1.redhat.com ([209.132.183.28]:50722 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752469AbdB1NeJ (ORCPT ); Tue, 28 Feb 2017 08:34:09 -0500 Subject: Re: [PATCH] mfd: Add Cherrytrail WhiskeyCove PMIC driver To: Andy Shevchenko References: <20170227101809.10177-1-hdegoede@redhat.com> Cc: Lee Jones , "linux-kernel@vger.kernel.org" , Bin Gao , Felipe Balbi From: Hans de Goede Message-ID: Date: Tue, 28 Feb 2017 14:23:02 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Tue, 28 Feb 2017 13:23:05 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 27-02-17 23:04, Andy Shevchenko wrote: > On Mon, Feb 27, 2017 at 12:18 PM, Hans de Goede wrote: >> Add mfd driver for Intel CHT WhiskeyCove PMIC, based on various non >> upstreamed CHT WhiskeyCove PMIC patches. For now this just adds a minimal >> version which implements just enough to get ACPI PMIC opregion support to >> work, so that suspend/resume will work on machines with this PMIC. > > Whiskey Cove in Subject and commit message. Will fix for v3. >> +intel-soc-pmic-objs := intel_soc_pmic_core.o intel_soc_pmic_crc.o intel_soc_pmic_chtwc.o >> intel-soc-pmic-$(CONFIG_INTEL_PMC_IPC) += intel_soc_pmic_bxtwc.o >> obj-$(CONFIG_INTEL_SOC_PMIC) += intel-soc-pmic.o > > Can we use one module per driver? I already pointed out to my patch > (unfortunately needs to be updated) that tries to fix it for BXT WC. Will fix for v3. >> +/* PMIC device registers */ > >> +#define REG_ADDR_MASK 0xff00 > > GENMASK() > >> +#define REG_ADDR_SHIFT 8 >> +#define REG_OFFSET_MASK 0xff > > Ditto. Both fixed for v3. >> + >> +/* Whiskey Cove PMIC share same ACPI ID between different platforms */ >> +#define CHT_WC_HRV 3 >> + >> +static struct mfd_cell cht_wc_dev[] = { >> + { >> + .name = "cht_wcove_region", >> + }, >> +}; >> + >> +/* >> + * The CHT Whiskey Cove covers multiple i2c addresses, with a 1 byte >> + * register address space per i2c address, so we use 16 bit register >> + * addresses where the high 8 bits contain the i2c client address. >> + */ >> +static int cht_wc_byte_reg_read(void *context, unsigned int reg, >> + unsigned int *val) >> +{ >> + struct i2c_client *client = context; > >> + int ret, orig_addr = client->addr; >> + >> + if (reg & REG_ADDR_MASK) >> + client->addr = (reg & REG_ADDR_MASK) >> REG_ADDR_SHIFT; >> + else >> + client->addr = CHT_WC_DEVICE1_ADDR; >> + ret = i2c_smbus_read_byte_data(client, reg & REG_OFFSET_MASK); >> + client->addr = orig_addr; > > Looks a bit hackish to me. Agreed. > Why not to define DEVICE1_ADDR as 0x6e00, for example? This is intended for callers who specify a register offset only without specifying which Whiskey Cove sub-device (i2c address) they want. This comes from the out of tree code I based this on, where this is also actually (ab)used by some of the mfd child device drivers. I plan for all the mainlined child device drivers to properly use a fill 16 bit address, which only leaves the ACPI REGS opregion accesses as potentially leaving the upper 8 bits 0. All the DSDTs I've access to do always properly use a 16 bit address. So I will just drop this hack for v3 replacing it with: if (!(reg & REG_ADDR_MASK)) { dev_err(&client->dev, "i2c device address not specified\n"); return -EINVAL; } Which will catch and invalid REGS opregion accesses. > >> + >> + if (ret < 0) >> + return ret; >> + >> + *val = ret; >> + return 0; >> +} >> + >> +static int cht_wc_byte_reg_write(void *context, unsigned int reg, >> + unsigned int val) >> +{ >> + struct i2c_client *client = context; >> + int ret, orig_addr = client->addr; >> + >> + if (reg & REG_ADDR_MASK) >> + client->addr = (reg & REG_ADDR_MASK) >> REG_ADDR_SHIFT; >> + else >> + client->addr = CHT_WC_DEVICE1_ADDR; >> + ret = i2c_smbus_write_byte_data(client, reg & REG_OFFSET_MASK, val); >> + client->addr = orig_addr; > > Ditto. > >> + >> + return ret; >> +} > >> +static int cht_wc_probe(struct i2c_client *client, >> + const struct i2c_device_id *i2c_id) >> +{ >> + struct device *dev = &client->dev; >> + struct intel_soc_pmic *pmic; >> + acpi_handle handle; >> + acpi_status status; >> + unsigned long long hrv; >> + int ret; >> + > >> + handle = ACPI_HANDLE(dev); > > Useless temporary variable? Will fix for v3. > >> + status = acpi_evaluate_integer(handle, "_HRV", NULL, &hrv); >> + if (ACPI_FAILURE(status)) { >> + dev_err(dev, "Failed to get PMIC hardware revision\n"); >> + return -ENODEV; >> + } >> + if (hrv != CHT_WC_HRV) { >> + dev_err(dev, "Invalid PMIC hardware revision: %llu\n", hrv); >> + return -ENODEV; >> + } >> + if (client->irq < 0) { >> + dev_err(dev, "Invalid IRQ\n"); >> + return -ENODEV; >> + } >> + > >> + pmic->regmap = devm_regmap_init(dev, NULL, client, &cht_wc_regmap_cfg); >> + if (IS_ERR(pmic->regmap)) { >> + ret = PTR_ERR(pmic->regmap); > >> + dev_err(dev, "Failed to initialise regmap: %d\n", ret); > > Is it anyhow useful? Nope, I will remove this for v3. > >> + return ret; >> + } >> + > >> + ret = mfd_add_devices(dev, -1, cht_wc_dev, ARRAY_SIZE(cht_wc_dev), >> + NULL, 0, NULL); >> + if (ret) { >> + dev_err(dev, "Failed to add devices: %d\n", ret); >> + return ret; >> + } >> + >> + return 0; > > return devm_mfd_add_devices(...); Will fix for v3. >> +} > >> +++ b/include/linux/mfd/intel_chtwc.h >> @@ -0,0 +1,75 @@ >> +/* > >> + * intel_chtwc.h - Header file for Intel Cherrytrail Whiskey Cove PMIC > > Remove file name. Will fix for v3. Regards, Hans