Re: [PATCH] mfd: Add Cherrytrail WhiskeyCove PMIC driver

[Andy Shevchenko]

On Mon, Feb 27, 2017 at 12:18 PM, Hans de Goede <hdegoede@xxxxxxxxxx> 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