| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 28 Feb 2017 14:23:02 +0100
From: Hans de Goede <hdegoede@...hat.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: Lee Jones <lee.jones@...aro.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Bin Gao <bin.gao@...el.com>,
Felipe Balbi <felipe.balbi@...ux.intel.com>
Subject: Re: [PATCH] mfd: Add Cherrytrail WhiskeyCove PMIC driver
Hi,
On 27-02-17 23:04, Andy Shevchenko wrote:
> On Mon, Feb 27, 2017 at 12:18 PM, Hans de Goede <hdegoede@...hat.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.
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
Powered by blists - more mailing lists