Re: [PATCH v5 00/18] platform/x86: Rework intel_scu_ipc and intel_pmc_ipc drivers

From: Andy Shevchenko
Date: Tue Feb 11 2020 - 11:10:37 EST

On Tue, Feb 11, 2020 at 04:25:45PM +0300, Mika Westerberg wrote:
> Hi,
>
> Currently both intel_scu_ipc.c and intel_pmc_ipc.c implement the same SCU
> IPC communications with minor differences. This duplication does not make
> much sense so this series reworks the two drivers so that there is only a
> single implementation of the SCU IPC. In addition to that the API will be
> updated to take SCU instance pointer as an argument, and most of the
> callers will be converted to this new API. The old API is left there but
> the plan is to get rid the callers and then the old API as well (this is
> something we are working with Andy Shevchenko).
>
> The intel_pmc_ipc.c is then moved under MFD which suits better for this
> kind of a driver that pretty much sets up the SCU IPC and then creates a
> bunch of platform devices for the things sitting behind the PMC. The driver
> is renamed to intel_pmc_bxt.c which should follow the existing conventions
> under drivers/mfd (and it is only meant for Intel Broxton derivatives).
>
> This is on top of platform-driver-x86.git/for-next branch because there is
> already some cleanup work queued that re-organizes Kconfig and Makefile
> entries.
>
> I have tested this on Intel Joule (Broxton-M) board.

Thank you!
I have bunch of nit picks, but overall it's good to me.
Please, add my Rb tag wherever it applies.

>
> Previous version of the series:
>
> v4: https://www.spinics.net/lists/platform-driver-x86/msg20658.html
> v3: https://www.spinics.net/lists/platform-driver-x86/msg20583.html
> v2: https://www.spinics.net/lists/platform-driver-x86/msg20446.html
> v1: https://www.spinics.net/lists/platform-driver-x86/msg20359.html
>
> Changes from v4:
>
> * Cleanups already merged in v5.6-rc1 reducing this series to 18 patches.
> * Make SCU IPC a simple class, and now intel_scu_ipc_register() creates
> a new device that belongs to the SCU IPC class under the parent.
> * Handle refcounting using the newly created device.
> * We still call try_module_get() in intel_scu_ipc_dev_get() because we
> need to make sure the SCU IPC provider module is not unloaded but the
> SCU IPC device refcount is now increased and decreased as well.
> * Make SCU IPC code to log an error if there is a failure so that callers
> don't need to do that.
> * Replace telemetry_pltconfig_valid() with telemetry_get_pltdata().
> * Move intel_pmc_gcr_update() closer to intel_pmc_gcr_read64().
> * Use more standard "update" pattern in intel_pmc_gcr_update() and move
> check outside of the lock.
> * Use platform_get_irq_optional() instead.
> * Move iTCO resource extraction into separate helper function
> (intel_pmc_get_tco_resources()).
>
> Changes from v3:
>
> * Rename intel_scu_ipc_probe() to intel_scu_ipc_register() and _remove()
> to _unregister() accordingly.
> * Make intel_scu_ipc_register() to perform handle resource request and
> ioremap itself.
> * Add devm_intel_scu_ipc_register().
> * Improve kernel-docs of struct intel_soc_pmic.
> * Add Documentation/ABI/obsolete/sysfs-driver-intel_pmc_bxt to document the
> two sysfs attributes the driver exposes.
> * Fix typos in the MFD driver
> * Drop gcr_data_readq() wrapper
> * No need to check for !pmcdev.gcr_mem_base in the MFD accessors
> * Allocate PMC instance dynamically and pass this from the callers
> (telemetry) as well
> * Take the lock in intel_pmc_s0ix_counter_read() to be consistent with other
> register accessors (and serialize them).
> * Use kstrtoul() return value directly (new patch)
> * Use static const MFD cell and resources where possible. Take a copy of
> these before they get populated and passed to the MFD code.
> * Use module_platform_driver() in the MFD driver
> * Drop dev_dbg() prints.
> * Return -EINVAL instead of -ENXIO when platform_get_resource() for
> mandatory resources.
> * Clarify "residency" in intel_pmc_s0ix_counter_read().
>
> Changes from v2:
>
> * Added review tags from Andy
> * Patch 12: Put intel_scu_ipc_probe() prototype and implementation in one line
> * Patch 12: Correct wording in Kconfig description.
> * Patch 12: Put devm_request_irq() in one line.
> * Patch 14: Add blank line before intel_scu_ipc_dev_update() in header.
> * Patch 14: intel_scu_ipc_dev_update() move 'u8 data' to the next line in header.
> * Patch 14: Drop outlen check in intel_scu_ipc_dev_command_with_size().
> * Patch 16: Added Guenter's ack.
> * Patch 25: Put intel_scu_ipc_dev_command() call in one line.
> * Patch 25: Put intel_scu_ipc_dev_simple_command() call in one line.
> * Patch 32: Drop X86_INTEL_MID dependency from INTEL_SCU_PCI.
> * Patch 34: Split MFD_INTEL_PMC_BXT dependencies one per line.
> * Patch 35: Reorder to happen before patch 34.
> * Patch 35: Drop comma from terminating line.
>
> Changes from v1:
>
> * Update changelog of patch 16 according to what the patch actually does.
> * Add kernel-doc for struct intel_soc_pmic.
> * Move octal permission patch to be before MFD conversion.
> * Convert the intel_pmc_bxt.c to MFD APIs whilst it is being moved under
> drivers/mfd.
>
> Mika Westerberg (18):
> platform/x86: intel_scu_ipc: Split out SCU IPC functionality from the SCU driver
> platform/x86: intel_scu_ipc: Log more information if SCU IPC command fails
> platform/x86: intel_scu_ipc: Introduce new SCU IPC API
> platform/x86: intel_mid_powerbtn: Convert to use new SCU IPC API
> watchdog: intel-mid_wdt: Convert to use new SCU IPC API
> platform/x86: intel_scu_ipcutil: Convert to use new SCU IPC API
> platform/x86: intel_scu_ipc: Add managed function to register SCU IPC
> platform/x86: intel_pmc_ipc: Start using SCU IPC
> mfd: intel_soc_pmic: Add SCU IPC member to struct intel_soc_pmic
> mfd: intel_soc_pmic_bxtwc: Convert to use new SCU IPC API
> mfd: intel_soc_pmic_mrfld: Convert to use new SCU IPC API
> platform/x86: intel_telemetry: Convert to use new SCU IPC API
> platform/x86: intel_pmc_ipc: Drop intel_pmc_ipc_command()
> x86/platform/intel-mid: Add empty stubs for intel_scu_devices_[create|destroy]()
> platform/x86: intel_pmc_ipc: Move PCI IDs to intel_scu_pcidrv.c
> platform/x86: intel_telemetry: Add telemetry_get_pltdata()
> platform/x86: intel_pmc_ipc: Convert to MFD
> MAINTAINERS: Update entry for Intel Broxton PMC driver
>
> .../ABI/obsolete/sysfs-driver-intel_pmc_bxt | 22 +
> MAINTAINERS | 13 +-
> arch/x86/Kconfig | 2 +-
> arch/x86/include/asm/intel-mid.h | 9 +-
> arch/x86/include/asm/intel_pmc_ipc.h | 59 --
> arch/x86/include/asm/intel_scu_ipc.h | 114 ++-
> arch/x86/include/asm/intel_scu_ipc_legacy.h | 84 ++
> arch/x86/include/asm/intel_telemetry.h | 6 +-
> drivers/mfd/Kconfig | 20 +-
> drivers/mfd/Makefile | 1 +
> drivers/mfd/intel_pmc_bxt.c | 491 +++++++++
> drivers/mfd/intel_soc_pmic_bxtwc.c | 34 +-
> drivers/mfd/intel_soc_pmic_mrfld.c | 10 +-
> drivers/platform/x86/Kconfig | 46 +-
> drivers/platform/x86/Makefile | 2 +-
> drivers/platform/x86/intel_mid_powerbtn.c | 15 +-
> drivers/platform/x86/intel_pmc_ipc.c | 949 ------------------
> drivers/platform/x86/intel_scu_ipc.c | 452 +++++++--
> drivers/platform/x86/intel_scu_ipcutil.c | 43 +-
> drivers/platform/x86/intel_scu_pcidrv.c | 68 ++
> drivers/platform/x86/intel_telemetry_core.c | 17 +-
> .../platform/x86/intel_telemetry_debugfs.c | 15 +-
> drivers/platform/x86/intel_telemetry_pltdrv.c | 97 +-
> drivers/usb/typec/tcpm/Kconfig | 2 +-
> drivers/watchdog/intel-mid_wdt.c | 53 +-
> include/linux/mfd/intel_pmc_bxt.h | 21 +
> include/linux/mfd/intel_soc_pmic.h | 15 +
> 27 files changed, 1359 insertions(+), 1301 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 arch/x86/include/asm/intel_scu_ipc_legacy.h
> create mode 100644 drivers/mfd/intel_pmc_bxt.c
> delete mode 100644 drivers/platform/x86/intel_pmc_ipc.c
> create mode 100644 drivers/platform/x86/intel_scu_pcidrv.c
> create mode 100644 include/linux/mfd/intel_pmc_bxt.h
>
> --
> 2.25.0
>

--
With Best Regards,
Andy Shevchenko