On Mon, Feb 27, 2017 at 12:18 PM, Hans de Goede <hdego...@redhat.com> 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. > +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. > +/* PMIC device registers */ > +#define REG_ADDR_MASK 0xff00 GENMASK() > +#define REG_ADDR_SHIFT 8 > +#define REG_OFFSET_MASK 0xff Ditto. > + > +/* 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. Why not to define DEVICE1_ADDR as 0x6e00, for example? > + > + 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? > + 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? > + 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(...); > +} > +++ 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. -- With Best Regards, Andy Shevchenko