On Tue, 03 Mar 2020, Mika Westerberg wrote:
> This driver only creates a bunch of platform devices sharing resources
> belonging to the PMC device. This is pretty much what MFD subsystem is
> for so move the driver there, renaming it to intel_pmc_bxt.c which
> should be more clear what it is.
>
> MFD subsystem provides nice helper APIs for subdevice creation so
> convert the driver to use those. Unfortunately the ACPI device includes
> separate resources for most of the subdevices so we cannot simply call
> mfd_add_devices() to create all of them but instead we need to call it
> separately for each device.
>
> The new MFD driver continues to expose two sysfs attributes that allow
> userspace to send IPC commands to the PMC/SCU to avoid breaking any
> existing applications that may use these. Generally this is bad idea so
> document this in the ABI documentation.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
> .../ABI/obsolete/sysfs-driver-intel_pmc_bxt | 22 +
> arch/x86/include/asm/intel_pmc_ipc.h | 47 --
> arch/x86/include/asm/intel_telemetry.h | 1 +
> drivers/mfd/Kconfig | 16 +-
> drivers/mfd/Makefile | 1 +
> drivers/mfd/intel_pmc_bxt.c | 504 ++++++++++++++
> drivers/platform/x86/Kconfig | 16 +-
> drivers/platform/x86/Makefile | 1 -
> drivers/platform/x86/intel_pmc_ipc.c | 645 ------------------
> .../platform/x86/intel_telemetry_debugfs.c | 12 +-
> drivers/platform/x86/intel_telemetry_pltdrv.c | 2 +
> drivers/usb/typec/tcpm/Kconfig | 2 +-
> include/linux/mfd/intel_pmc_bxt.h | 43 ++
> 13 files changed, 602 insertions(+), 710 deletions(-)
> create mode 100644 Documentation/ABI/obsolete/sysfs-driver-intel_pmc_bxt
> delete mode 100644 arch/x86/include/asm/intel_pmc_ipc.h
> create mode 100644 drivers/mfd/intel_pmc_bxt.c
> delete mode 100644 drivers/platform/x86/intel_pmc_ipc.c
> create mode 100644 include/linux/mfd/intel_pmc_bxt.h
[...]
> +/*
> + * We use the below templates to construct MFD cells. The struct
> + * intel_pmc_dev instance holds the real MFD cells where we first copy
> + * these and then fill the dynamic parts based on the extracted resources.
> + */
> +
> +static const struct mfd_cell punit = {
> + .name = "intel_punit_ipc",
> +};
> +
> +static int update_no_reboot_bit(void *priv, bool set)
> +{
> + struct intel_pmc_dev *pmc = priv;
> + u32 bits = PMC_CFG_NO_REBOOT_EN;
> + u32 value = set ? bits : 0;
> +
> + return intel_pmc_gcr_update(pmc, PMC_GCR_PMC_CFG_REG, bits, value);
> +}
> +
> +static const struct itco_wdt_platform_data tco_pdata = {
> + .name = "Apollo Lake SoC",
> + .version = 5,
> + .update_no_reboot_bit = update_no_reboot_bit,
> +};
> +
> +static const struct mfd_cell tco = {
> + .name = "iTCO_wdt",
> + .ignore_resource_conflicts = true,
> +};
> +
> +static const struct resource telem_res[] = {
> + DEFINE_RES_MEM(TELEM_PUNIT_SSRAM_OFFSET, TELEM_SSRAM_SIZE),
> + DEFINE_RES_MEM(TELEM_PMC_SSRAM_OFFSET, TELEM_SSRAM_SIZE),
> +};
> +
> +static const struct mfd_cell telem = {
> + .name = "intel_telemetry",
> + .resources = telem_res,
> + .num_resources = ARRAY_SIZE(telem_res),
> +};
> +
> +static int intel_pmc_get_tco_resources(struct platform_device *pdev,
> + struct intel_pmc_dev *pmc)
> +{
> + struct itco_wdt_platform_data *pdata;
> + struct resource *res, *tco_res;
> +
> + if (acpi_has_watchdog())
> + return 0;
> +
> + res = platform_get_resource(pdev, IORESOURCE_IO,
> + PLAT_RESOURCE_ACPI_IO_INDEX);
> + if (!res) {
> + dev_err(&pdev->dev, "Failed to get IO resource\n");
> + return -EINVAL;
> + }
> +
> + tco_res = devm_kcalloc(&pdev->dev, 2, sizeof(*tco_res), GFP_KERNEL);
> + if (!tco_res)
> + return -ENOMEM;
> +
> + tco_res[0].flags = IORESOURCE_IO;
> + tco_res[0].start = res->start + TCO_BASE_OFFSET;
> + tco_res[0].end = tco_res[0].start + TCO_REGS_SIZE - 1;
> + tco_res[1].flags = IORESOURCE_IO;
> + tco_res[1].start = res->start + SMI_EN_OFFSET;
> + tco_res[1].end = tco_res[1].start + SMI_EN_SIZE - 1;
> +
> + pmc->cells[PMC_TCO].resources = tco_res;
> + pmc->cells[PMC_TCO].num_resources = 2;
> +
> + pdata = devm_kmemdup(&pdev->dev, &tco_pdata, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + return -ENOMEM;
Why do you need to take a copy?
This can be referenced directly in 'mfd_cell tco', no?
> + pdata->no_reboot_priv = pmc;
You're putting device data inside platform data?
This doesn't sit right with me at all.
You already saved it using platform_set_drvdata(), why do you need it
twice? Why can't you export update_no_reboot_bit() and make it take
'struct intel_pmc_dev' or better yet 'pdev' as an argument?
> + pmc->cells[PMC_TCO].platform_data = pdata;
> + pmc->cells[PMC_TCO].pdata_size = sizeof(*pdata);
> +
> + return 0;
> +}
> +
> +static int intel_pmc_get_resources(struct platform_device *pdev,
> + struct intel_pmc_dev *pmc,
> + struct intel_scu_ipc_data *scu_data)
> +{
> + struct resource *res, *punit_res;
> + struct resource gcr_res;
> + size_t npunit_res = 0;
> + int ret;
> +
> + scu_data->irq = platform_get_irq_optional(pdev, 0);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM,
> + PLAT_RESOURCE_IPC_INDEX);
> + if (!res) {
> + dev_err(&pdev->dev, "Failed to get IPC resource\n");
> + return -EINVAL;
> + }
> +
> + /* IPC registers */
> + scu_data->mem.flags = res->flags;
> + scu_data->mem.start = res->start;
> + scu_data->mem.end = res->start + PLAT_RESOURCE_IPC_SIZE - 1;
> +
> + /* GCR registers */
> + gcr_res.flags = res->flags;
> + gcr_res.start = res->start + PLAT_RESOURCE_GCR_OFFSET;
> + gcr_res.end = gcr_res.start + PLAT_RESOURCE_GCR_SIZE - 1;
> +
> + pmc->gcr_mem_base = devm_ioremap_resource(&pdev->dev, &gcr_res);
> + if (IS_ERR(pmc->gcr_mem_base))
> + return PTR_ERR(pmc->gcr_mem_base);
> +
> + pmc->cells[PMC_TCO] = tco;
> + pmc->cells[PMC_PUNIT] = punit;
> + pmc->cells[PMC_TELEM] = telem;
Why are you still saving these to device data?
What's stopping you operating on the structures directly?
[...]
Remainder looks half sane.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
