[RFC PATCH v2 0/8] ACPI/IORT: Support for IORT RMR node
by Shameer Kolothum
RFC v1 --> v2:
- Added a generic interface for IOMMU drivers to retrieve all the
RMR info associated with a given IOMMU.
- SMMUv3 driver gets the RMR list during probe() and installs
bypass STEs for all the SIDs in the RMR list. This is to keep
the ongoing traffic alive(if any) during SMMUv3 reset. This is
based on the suggestions received for v1 to take care of the
EFI framebuffer use case. Only sanity tested for now.
- During the probe/attach device, SMMUv3 driver reserves any
RMR region associated with the device such that there is a unity
mapping for them in SMMU.
---
The series adds support to IORT RMR nodes specified in IORT
Revision E -ARM DEN 0049E[0]. RMR nodes are used to describe memory
ranges that are used by endpoints and require a unity mapping
in SMMU.
We have faced issues with 3408iMR RAID controller cards which
fail to boot when SMMU is enabled. This is because these controllers
make use of host memory for various caching related purposes and when
SMMU is enabled the iMR firmware fails to access these memory regions
as there is no mapping for them. IORT RMR provides a way for UEFI to
describe and report these memory regions so that the kernel can make
a unity mapping for these in SMMU.
RFC because, Patch #1 is to update the actbl2.h and should be done
through acpica update. I have send out a pull request[1] for that.
Tests:
With a UEFI, that reports the RMR for the dev,
....
[16F0h 5872 1] Type : 06
[16F1h 5873 2] Length : 007C
[16F3h 5875 1] Revision : 00
[1038h 0056 2] Reserved : 00000000
[1038h 0056 2] Identifier : 00000000
[16F8h 5880 4] Mapping Count : 00000001
[16FCh 5884 4] Mapping Offset : 00000040
[1700h 5888 4] Number of RMR Descriptors : 00000002
[1704h 5892 4] RMR Descriptor Offset : 00000018
[1708h 5896 8] Base Address of RMR : 0000E6400000
[1710h 5904 8] Length of RMR : 000000100000
[1718h 5912 4] Reserved : 00000000
[171Ch 5916 8] Base Address of RMR : 0000000027B00000
[1724h 5924 8] Length of RMR : 0000000000C00000
[172Ch 5932 4] Reserved : 00000000
[1730h 5936 4] Input base : 00000000
[1734h 5940 4] ID Count : 00000001
[1738h 5944 4] Output Base : 00000003
[173Ch 5948 4] Output Reference : 00000064
[1740h 5952 4] Flags (decoded below) : 00000001
Single Mapping : 1
...
Without the series the RAID controller initialization fails as
below,
...
[ 12.631117] megaraid_sas 0000:03:00.0: FW supports sync cache : Yes
[ 12.637360] megaraid_sas 0000:03:00.0: megasas_disable_intr_fusion is called outbound_intr_mask:0x40000009
[ 18.776377] megaraid_sas 0000:03:00.0: Init cmd return status FAILED for SCSI host 0
[ 23.019383] megaraid_sas 0000:03:00.0: Waiting for FW to come to ready state
[ 106.684281] megaraid_sas 0000:03:00.0: FW in FAULT state, Fault code:0x10000 subcode:0x0 func:megasas_transition_to_ready
[ 106.695186] megaraid_sas 0000:03:00.0: System Register set:
[ 106.889787] megaraid_sas 0000:03:00.0: Failed to transition controller to ready for scsi0.
[ 106.910475] megaraid_sas 0000:03:00.0: Failed from megasas_init_fw 6407
estuary:/$
With the series, now the kernel has direct mapping for the dev as
below,
estuary:/$ cat /sys/kernel/iommu_groups/0/reserved_regions
0x0000000008000000 0x00000000080fffff msi
0x0000000027b00000 0x00000000286fffff direct
0x00000000e6400000 0x00000000e64fffff direct
estuary:/$
....
[ 12.254318] megaraid_sas 0000:03:00.0: megasas_disable_intr_fusion is called outbound_intr_mask:0x40000009
[ 12.739089] megaraid_sas 0000:03:00.0: FW provided supportMaxExtLDs: 0 max_lds: 32
[ 12.746628] megaraid_sas 0000:03:00.0: controller type : iMR(0MB)
[ 12.752694] megaraid_sas 0000:03:00.0: Online Controller Reset(OCR) : Enabled
[ 12.759798] megaraid_sas 0000:03:00.0: Secure JBOD support : Yes
[ 12.765778] megaraid_sas 0000:03:00.0: NVMe passthru support : Yes
[ 12.771931] megaraid_sas 0000:03:00.0: FW provided TM TaskAbort/Reset timeou: 6 secs/60 secs
[ 12.780503] megaraid_sas 0000:03:00.0: JBOD sequence map support : Yes
[ 12.787000] megaraid_sas 0000:03:00.0: PCI Lane Margining support : No
[ 12.819179] megaraid_sas 0000:03:00.0: NVME page size : (4096)
[ 12.825672] megaraid_sas 0000:03:00.0: megasas_enable_intr_fusion is called outbound_intr_mask:0x40000000
[ 12.835199] megaraid_sas 0000:03:00.0: INIT adapter done
[ 12.873932] megaraid_sas 0000:03:00.0: pci id : (0x1000)/(0x0017)/(0x19e5)/(0xd213)
[ 12.881644] megaraid_sas 0000:03:00.0: unevenspan support : no
[ 12.887451] megaraid_sas 0000:03:00.0: firmware crash dump : no
[ 12.893344] megaraid_sas 0000:03:00.0: JBOD sequence map : enabled
RAID controller init is now success and can detect the drives
attached as well.
Thanks,
Shameer
[0]. https://developer.arm.com/documentation/den0049/latest/
[1]. https://github.com/acpica/acpica/pull/638
Shameer Kolothum (8):
ACPICA: IORT: Update for revision E
ACPI/IORT: Add support for RMR node parsing
iommu/dma: Introduce generic helper to retrieve RMR info
ACPI/IORT: Add RMR memory regions reservation helper
iommu/arm-smmu-v3: Introduce strtab init helper
iommu/arm-smmu-v3: Add bypass flag to arm_smmu_write_strtab_ent()
iommu/arm-smmu-v3: Get associated RMR info and install bypass STE
iommu/arm-smmu-v3: Reserve any RMR regions associated with a dev
drivers/acpi/arm64/iort.c | 182 +++++++++++++++++++-
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 112 ++++++++++--
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 2 +
drivers/iommu/dma-iommu.c | 39 +++++
include/acpi/actbl2.h | 25 ++-
include/linux/acpi_iort.h | 6 +
include/linux/dma-iommu.h | 7 +
include/linux/iommu.h | 16 ++
8 files changed, 367 insertions(+), 22 deletions(-)
--
2.17.1
1 year, 5 months
Re: [PATCH] [ACPICA] IORT: Fix HTTU Override mask for the SMMUv3 subtable
by Moore, Robert
Yes, you are correct. ACPI_DMT_FLAGS1 Will pick up two bits at bit [2:1]
Sorry for the noise,
Bob
-----Original Message-----
From: Zenghui Yu <yuzenghui(a)huawei.com>
Sent: Thursday, February 25, 2021 5:59 PM
To: Moore, Robert <robert.moore(a)intel.com>; devel(a)acpica.org; linux-acpi(a)vger.kernel.org
Cc: Kaneda, Erik <erik.kaneda(a)intel.com>; robin.murphy(a)arm.com; Wysocki, Rafael J <rafael.j.wysocki(a)intel.com>; guohanjun(a)huawei.com; wanghaibin.wang(a)huawei.com; Kunkun Jiang <jiangkunkun(a)huawei.com>
Subject: Re: [PATCH] [ACPICA] IORT: Fix HTTU Override mask for the SMMUv3 subtable
On 2021/2/26 4:45, Moore, Robert wrote:
> If the field is two bits, I think the ACPI_DMT_* symbol should be ACPI_DMT_FLAGS2, not ACPI_DMT_FLAGS1
The SMMUv3 flags is decoded as:
- bit[0] "COHACC Override"
- bit[2:1] "HTTU Override"
- bit[3] "Proximity Domain Valid"
- bit[31:4] "Reserved"
whilst ACPI_DMT_FLAGS2 will extract bit[3:2] for us, right?
> -----Original Message-----
> From: Zenghui Yu <yuzenghui(a)huawei.com>
> Sent: Thursday, February 25, 2021 3:26 AM
> To: devel(a)acpica.org; linux-acpi(a)vger.kernel.org
> Cc: Moore, Robert <robert.moore(a)intel.com>; Kaneda, Erik <erik.kaneda(a)intel.com>; robin.murphy(a)arm.com; Wysocki, Rafael J <rafael.j.wysocki(a)intel.com>; guohanjun(a)huawei.com; wanghaibin.wang(a)huawei.com; Zenghui Yu <yuzenghui(a)huawei.com>; Kunkun Jiang <jiangkunkun(a)huawei.com>
> Subject: [PATCH] [ACPICA] IORT: Fix HTTU Override mask for the SMMUv3 subtable
>
> As per the IORT spec, this field overrides the value in SMMU_IRD0.HTTU which should always have been 2 bits.
>
> Fixes: 9f7c3e148f44 ("IORT: Add in support for the SMMUv3 subtable")
> Reported-by: Kunkun Jiang <jiangkunkun(a)huawei.com>
> Signed-off-by: Zenghui Yu <yuzenghui(a)huawei.com>
> ---
> source/common/dmtbinfo2.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/source/common/dmtbinfo2.c b/source/common/dmtbinfo2.c index 17a80ba21..321f106fa 100644
> --- a/source/common/dmtbinfo2.c
> +++ b/source/common/dmtbinfo2.c
> @@ -343,7 +343,7 @@ ACPI_DMTABLE_INFO AcpiDmTableInfoIort4[] =
> {ACPI_DMT_UINT64, ACPI_IORT4_OFFSET (BaseAddress), "Base Address", 0},
> {ACPI_DMT_UINT32, ACPI_IORT4_OFFSET (Flags), "Flags (decoded below)", 0},
> {ACPI_DMT_FLAG0, ACPI_IORT4_FLAG_OFFSET (Flags, 0), "COHACC Override", 0},
> - {ACPI_DMT_FLAG1, ACPI_IORT4_FLAG_OFFSET (Flags, 0), "HTTU Override", 0},
> + {ACPI_DMT_FLAGS1, ACPI_IORT4_FLAG_OFFSET (Flags, 0), "HTTU Override", 0},
> {ACPI_DMT_FLAG3, ACPI_IORT4_FLAG_OFFSET (Flags, 0), "Proximity Domain Valid", 0},
> {ACPI_DMT_UINT32, ACPI_IORT4_OFFSET (Reserved), "Reserved", 0},
> {ACPI_DMT_UINT64, ACPI_IORT4_OFFSET (VatosAddress), "VATOS Address", 0},
> --
> 2.19.1
>
> .
>
1 year, 5 months
Re: [PATCH] [ACPICA] IORT: Fix HTTU Override mask for the SMMUv3 subtable
by Moore, Robert
If the field is two bits, I think the ACPI_DMT_* symbol should be ACPI_DMT_FLAGS2, not ACPI_DMT_FLAGS1
-----Original Message-----
From: Zenghui Yu <yuzenghui(a)huawei.com>
Sent: Thursday, February 25, 2021 3:26 AM
To: devel(a)acpica.org; linux-acpi(a)vger.kernel.org
Cc: Moore, Robert <robert.moore(a)intel.com>; Kaneda, Erik <erik.kaneda(a)intel.com>; robin.murphy(a)arm.com; Wysocki, Rafael J <rafael.j.wysocki(a)intel.com>; guohanjun(a)huawei.com; wanghaibin.wang(a)huawei.com; Zenghui Yu <yuzenghui(a)huawei.com>; Kunkun Jiang <jiangkunkun(a)huawei.com>
Subject: [PATCH] [ACPICA] IORT: Fix HTTU Override mask for the SMMUv3 subtable
As per the IORT spec, this field overrides the value in SMMU_IRD0.HTTU which should always have been 2 bits.
Fixes: 9f7c3e148f44 ("IORT: Add in support for the SMMUv3 subtable")
Reported-by: Kunkun Jiang <jiangkunkun(a)huawei.com>
Signed-off-by: Zenghui Yu <yuzenghui(a)huawei.com>
---
source/common/dmtbinfo2.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/source/common/dmtbinfo2.c b/source/common/dmtbinfo2.c index 17a80ba21..321f106fa 100644
--- a/source/common/dmtbinfo2.c
+++ b/source/common/dmtbinfo2.c
@@ -343,7 +343,7 @@ ACPI_DMTABLE_INFO AcpiDmTableInfoIort4[] =
{ACPI_DMT_UINT64, ACPI_IORT4_OFFSET (BaseAddress), "Base Address", 0},
{ACPI_DMT_UINT32, ACPI_IORT4_OFFSET (Flags), "Flags (decoded below)", 0},
{ACPI_DMT_FLAG0, ACPI_IORT4_FLAG_OFFSET (Flags, 0), "COHACC Override", 0},
- {ACPI_DMT_FLAG1, ACPI_IORT4_FLAG_OFFSET (Flags, 0), "HTTU Override", 0},
+ {ACPI_DMT_FLAGS1, ACPI_IORT4_FLAG_OFFSET (Flags, 0), "HTTU Override", 0},
{ACPI_DMT_FLAG3, ACPI_IORT4_FLAG_OFFSET (Flags, 0), "Proximity Domain Valid", 0},
{ACPI_DMT_UINT32, ACPI_IORT4_OFFSET (Reserved), "Reserved", 0},
{ACPI_DMT_UINT64, ACPI_IORT4_OFFSET (VatosAddress), "VATOS Address", 0},
--
2.19.1
1 year, 5 months
[pm:bleeding-edge] BUILD SUCCESS aa22459146987c639004f1fe5a30fe95743c1a1c
by kernel test robot
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git bleeding-edge
branch HEAD: aa22459146987c639004f1fe5a30fe95743c1a1c Merge branches 'acpi-platform' and 'acpi-tables' into linux-next
elapsed time: 728m
configs tested: 134
configs skipped: 2
The following configs have been built successfully.
More configs may be tested in the coming days.
gcc tested configs:
arm defconfig
arm64 allyesconfig
arm64 defconfig
arm allyesconfig
arm allmodconfig
powerpc tqm5200_defconfig
mips db1xxx_defconfig
arm pcm027_defconfig
ia64 zx1_defconfig
sh migor_defconfig
powerpc ppa8548_defconfig
xtensa generic_kc705_defconfig
sh urquell_defconfig
arm s3c6400_defconfig
arm magician_defconfig
mips bcm47xx_defconfig
sh shmin_defconfig
arm clps711x_defconfig
arm moxart_defconfig
powerpc skiroot_defconfig
arm jornada720_defconfig
nios2 allyesconfig
arc nsim_700_defconfig
riscv allnoconfig
riscv defconfig
powerpc stx_gp3_defconfig
um kunit_defconfig
x86_64 allyesconfig
powerpc cm5200_defconfig
arm s3c2410_defconfig
arc nsimosci_defconfig
powerpc adder875_defconfig
sh titan_defconfig
sparc sparc64_defconfig
arm ezx_defconfig
sh rsk7201_defconfig
arm corgi_defconfig
mips ip27_defconfig
arm u8500_defconfig
mips jmr3927_defconfig
powerpc acadia_defconfig
mips decstation_64_defconfig
sh se7750_defconfig
m68k m5475evb_defconfig
arm sama5_defconfig
nios2 defconfig
mips ar7_defconfig
arm multi_v7_defconfig
sh se7780_defconfig
sh allmodconfig
parisc generic-64bit_defconfig
arm tegra_defconfig
xtensa defconfig
arm cm_x300_defconfig
powerpc ge_imp3a_defconfig
arm mxs_defconfig
arm mmp2_defconfig
arm lpd270_defconfig
mips ci20_defconfig
arm tct_hammer_defconfig
c6x defconfig
powerpc taishan_defconfig
powerpc eiger_defconfig
powerpc mpc83xx_defconfig
mips ip28_defconfig
arc vdk_hs38_defconfig
mips tb0219_defconfig
arm realview_defconfig
mips decstation_r4k_defconfig
ia64 allmodconfig
ia64 defconfig
ia64 allyesconfig
m68k allmodconfig
m68k defconfig
m68k allyesconfig
arc allyesconfig
nds32 allnoconfig
c6x allyesconfig
nds32 defconfig
csky defconfig
alpha defconfig
alpha allyesconfig
xtensa allyesconfig
h8300 allyesconfig
arc defconfig
parisc defconfig
s390 allyesconfig
s390 allmodconfig
parisc allyesconfig
s390 defconfig
i386 allyesconfig
sparc allyesconfig
sparc defconfig
i386 tinyconfig
i386 defconfig
mips allyesconfig
mips allmodconfig
powerpc allyesconfig
powerpc allmodconfig
powerpc allnoconfig
i386 randconfig-a005-20210223
i386 randconfig-a006-20210223
i386 randconfig-a004-20210223
i386 randconfig-a003-20210223
i386 randconfig-a001-20210223
i386 randconfig-a002-20210223
x86_64 randconfig-a015-20210223
x86_64 randconfig-a011-20210223
x86_64 randconfig-a012-20210223
x86_64 randconfig-a016-20210223
x86_64 randconfig-a014-20210223
x86_64 randconfig-a013-20210223
i386 randconfig-a013-20210223
i386 randconfig-a012-20210223
i386 randconfig-a011-20210223
i386 randconfig-a014-20210223
i386 randconfig-a016-20210223
i386 randconfig-a015-20210223
riscv nommu_k210_defconfig
riscv allyesconfig
riscv nommu_virt_defconfig
riscv rv32_defconfig
riscv allmodconfig
x86_64 rhel-7.6-kselftests
x86_64 defconfig
x86_64 rhel-8.3
x86_64 rhel-8.3-kbuiltin
x86_64 kexec
clang tested configs:
x86_64 randconfig-a001-20210223
x86_64 randconfig-a002-20210223
x86_64 randconfig-a003-20210223
x86_64 randconfig-a005-20210223
x86_64 randconfig-a006-20210223
x86_64 randconfig-a004-20210223
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
1 year, 5 months
Re: [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver
by Andy Shevchenko
On Wed, Feb 24, 2021 at 12:16 PM Laurent Pinchart
<laurent.pinchart(a)ideasonboard.com> wrote:
> On Tue, Feb 23, 2021 at 10:36:18PM +0000, Daniel Scally wrote:
> > On 23/02/2021 20:04, Laurent Pinchart wrote:
...
> > >> + get_device(&int3472->sensor->dev);
> > >
> > > I see no corresponding put_device(), am I missing something ? I'm also
> > > not sure why this is needed.
> >
> > The put is acpi_dev_put() in skl_int3472_discrete_remove(); there seems
> > to be no acpi_dev_get() for some reason. We use the sensor acpi_device
> > to get the clock frequency, and to fetch the sensor module string, so I
> > thought it ought to hold a reference on those grounds.
>
> Shouldn't acpi_dev_get_dependent_dev() increase the reference count
> then, instead of doing it manually here ?
That's what I expected as well.
We have plenty of acpi_dev_get_*() and they do increase the reference
counter one way or the other.
--
With Best Regards,
Andy Shevchenko
1 year, 5 months
Re: [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver
by Andy Shevchenko
On Wednesday, February 24, 2021, Daniel Scally <djrscally(a)gmail.com> wrote:
> Hi Laurent
>
> On 23/02/2021 20:04, Laurent Pinchart wrote:
> > +
> > +/*
> > + * Here follows platform specific mapping information that we can pass
> to
> > + * the functions mapping resources to the sensors. Where the sensors
> have
> > + * a power enable pin defined in DSDT we need to provide a supply name
> so
> > + * the sensor drivers can find the regulator. The device name will be
> derived
> > + * from the sensor's ACPI device within the code. Optionally, we can
> provide a
> > + * NULL terminated array of function name mappings to deal with any
> platform
> > + * specific deviations from the documented behaviour of GPIOs.
> > + *
> > + * Map a GPIO function name to NULL to prevent the driver from mapping
> that
> > + * GPIO at all.
> > + */
> > +
> > +static const struct int3472_gpio_function_remap
> ov2680_gpio_function_remaps[] = {
> > + { "reset", NULL },
> > + { "powerdown", "reset" },
> > + { }
> > +};
> > +
> > +static struct int3472_sensor_config int3472_sensor_configs[] = {
> > This should be static const (and there will be some fallout due to that,
> > as skl_int3472_register_regulator() modifies the supply_map, so I think
> > you'll have a copy of supply_map in int3472_discrete_device).
>
>
> Ack to all of the constness; you mentioned that last time too - not sure
> how I missed doing those! I think I can just having a local struct
> regulator_consumer_supply in skl_int3472_register_regulator and fill it
> from int3472->sensor_config.supply_map
>
> >> +static unsigned int skl_int3472_get_clk_frequency(struct
> int3472_discrete_device *int3472)
> >> +{
> >> + union acpi_object *obj;
> >> + unsigned int ret = 0;
> >> +
> >> + obj = skl_int3472_get_acpi_buffer(int3472->sensor, "SSDB");
> >> + if (IS_ERR(obj))
> >> + return 0; /* report rate as 0 on error */
> >> +
> >> + if (obj->buffer.length < CIO2_SENSOR_SSDB_MCLKSPEED_OFFSET +
> sizeof(u32)) {
> > Should we define an ssdb structure instead of peeking into the buffer
> > with an offset ?
>
>
> I thought about that, but in the end decided it didn't seem worth
> defining the whole SSDB structure just to use one field. Particularly
> since we use it in cio2-bridge already, so if we're going to do that it
> really ought to just live in a header that's included in both - and that
> seemed even less worthwhile.
>
>
> I don't have a strong feeling though, so if you think it's better to
> define the struct I'm happy to.
>
>
> >> +static unsigned long skl_int3472_clk_recalc_rate(struct clk_hw *hw,
> >> + unsigned long parent_rate)
> >> +{
> >> + struct int3472_gpio_clock *clk = to_int3472_clk(hw);
> >> + struct int3472_discrete_device *int3472 = to_int3472_device(clk);
> >> +
> >> + return int3472->clock.frequency;
> > Maybe just
> >
> > struct int3472_gpio_clock *clk = to_int3472_clk(hw);
> >
> > return clk->frequency;
>
>
> Oops, of course.
>
> >> +static int skl_int3472_register_regulator(struct
> int3472_discrete_device *int3472,
> >> + struct acpi_resource *ares)
> >> +{
> >> + char *path = ares->data.gpio.resource_source.string_ptr;
> >> + struct int3472_sensor_config *sensor_config;
> >> + struct regulator_init_data init_data = { };
> >> + struct regulator_config cfg = { };
> >> + int ret;
> >> +
> >> + sensor_config = int3472->sensor_config;
> >> + if (IS_ERR_OR_NULL(sensor_config)) {
> >> + dev_err(int3472->dev, "No sensor module config\n");
> >> + return PTR_ERR(sensor_config);
> >> + }
> >> +
> >> + if (!sensor_config->supply_map.supply) {
> >> + dev_err(int3472->dev, "No supply name defined\n");
> >> + return -ENODEV;
> >> + }
> >> +
> >> + init_data.constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
> >> + init_data.num_consumer_supplies = 1;
> >> + sensor_config->supply_map.dev_name = int3472->sensor_name;
> >> + init_data.consumer_supplies = &sensor_config->supply_map;
> >> +
> >> + snprintf(int3472->regulator.regulator_name,
> >> + sizeof(int3472->regulator.regulator_name),
> "%s-regulator",
> >> + acpi_dev_name(int3472->adev));
> >> + snprintf(int3472->regulator.supply_name,
> >> + GPIO_REGULATOR_SUPPLY_NAME_LENGTH, "supply-0");
> >> +
> >> + int3472->regulator.rdesc = INT3472_REGULATOR(
> >> +
> int3472->regulator.regulator_name,
> >> + int3472->regulator.supply_
> name,
> >> +
> &int3472_gpio_regulator_ops);
> >> +
> >> + int3472->regulator.gpio = acpi_get_gpiod(path,
> >> +
> ares->data.gpio.pin_table[0],
> >> + "int3472,regulator");
> >> + if (IS_ERR(int3472->regulator.gpio)) {
> >> + dev_err(int3472->dev, "Failed to get regulator GPIO
> lines\n");
> > s/lines/line/ (sorry, it was a typo in my review of v2)
>
>
> No problem!
>
> >> +static int skl_int3472_parse_crs(struct int3472_discrete_device
> *int3472)
> >> +{
> >> + struct list_head resource_list;
> >> + int ret;
> >> +
> >> + INIT_LIST_HEAD(&resource_list);
> >> +
> >> + int3472->sensor_config = skl_int3472_get_sensor_module_
> config(int3472);
> > I have forgotten some of the context I'm afraid :-/ Are there valid use
> > cases for not checking for an error here, or should we do so and drop
> > the error checks in other functions above ?
>
>
> Not all platforms need a sensor_config; only those which have either a
> regulator pin or need a GPIO function to be remapped; the rest will do
> without it.
>
> So, we need to not check for an error here because the absence of a
> sensor_config isn't necessarily an error, we won't know till later.
>
> > +int skl_int3472_discrete_probe(struct platform_device *pdev)
> > +{
> > + struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> > + struct int3472_discrete_device *int3472;
> > + struct int3472_cldb cldb;
> > + int ret;
> > +
> > + ret = skl_int3472_fill_cldb(adev, &cldb);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Couldn't fill CLDB structure\n");
> > + return ret;
> > + }
> > +
> > + if (cldb.control_logic_type != 1) {
> > + dev_err(&pdev->dev, "Unsupported control logic type %u\n",
> > + cldb.control_logic_type);
> > + return -EINVAL;
> > + }
> > +
> > + /* Max num GPIOs we've seen plus a terminator */
> > + int3472 = kzalloc(struct_size(int3472, gpios.table,
> > + INT3472_MAX_SENSOR_GPIOS + 1), GFP_KERNEL);
> > + if (!int3472)
> > + return -ENOMEM;
> > +
> > + int3472->adev = adev;
> > + int3472->dev = &pdev->dev;
> > + platform_set_drvdata(pdev, int3472);
> > +
> > + int3472->sensor = acpi_dev_get_dependent_dev(adev);
> > + if (IS_ERR_OR_NULL(int3472->sensor)) {
> > + dev_err(&pdev->dev,
> > + "INT3472 seems to have no dependents.\n");
> > + ret = -ENODEV;
> > + goto err_free_int3472;
> > + }
> > + get_device(&int3472->sensor->dev);
> > I see no corresponding put_device(), am I missing something ? I'm also
> > not sure why this is needed.
> >
>
> The put is acpi_dev_put() in skl_int3472_discrete_remove(); there seems
> to be no acpi_dev_get() for some reason. We use the sensor acpi_device
> to get the clock frequency, and to fetch the sensor module string, so I
> thought it ought to hold a reference on those grounds.
>
>
> >> diff --git a/drivers/platform/x86/intel-int3472/intel_skl_int3472_tps68470.c
> b/drivers/platform/x86/intel-int3472/intel_skl_int3472_tps68470.c
> >> new file mode 100644
> >> index 000000000000..d0d2391e263f
> >> --- /dev/null
> >> +++ b/drivers/platform/x86/intel-int3472/intel_skl_int3472_tps68470.c
> >> @@ -0,0 +1,113 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/* Author: Dan Scally <djrscally(a)gmail.com> */
> >> +
> >> +#include <linux/i2c.h>
> >> +#include <linux/mfd/core.h>
> >> +#include <linux/mfd/tps68470.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/regmap.h>
> >> +
> >> +#include "intel_skl_int3472_common.h"
> >> +
> >> +static const struct mfd_cell tps68470_c[] = {
> >> + { .name = "tps68470-gpio" },
> >> + { .name = "tps68470_pmic_opregion" },
> >> +};
> >> +
> >> +static const struct mfd_cell tps68470_w[] = {
> > Maybe more explicit names than _c and _w could be nice ?
>
>
> _chrome and _windows was in my mind - sound ok?
In kernel “cros” used for Chrome OS, you may take it, for the Windows I
have no idea, b/c “win” maybe still ambiguous.
--
With Best Regards,
Andy Shevchenko
1 year, 5 months
Re: [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver
by Andy Shevchenko
On Mon, Feb 22, 2021 at 10:35:44PM +0000, Daniel Scally wrote:
> On 22/02/2021 14:58, Andy Shevchenko wrote:
> > On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally <djrscally(a)gmail.com> wrote:
...
> >> + if (obj->buffer.length > sizeof(*cldb)) {
> >> + dev_err(&adev->dev, "The CLDB buffer is too large\n");
> >> + ret = -EINVAL;
> > ENOSPC? ENOMEM?
>
> I still think EINVAL actually, as in this case the problem isn't that
> space couldn't be allocated but that the buffer in the SSDB is larger
> than I expect it to be, which means the definition of it has changed /
> this device isn't actually supported.
OK!
...
> >> + if (!IS_ERR_OR_NULL(sensor_config) && sensor_config->function_maps) {
> > Hmm...
> >
> > Would
> >
> > if (IS_ERR_OR_NULL(sensor_config))
> > return 0;
> >
> > if (!_maps)
> > return 0;
> >
> > with respective comments working here?
>
> No, because the absence of either sensor_config or
> sensor_config->function_maps is not a failure mode. We only need to
> provide sensor_configs for some platforms, and function_maps for even
> fewer. So if that check is false, the rest of the function should still
> execute.
I see, thanks for elaboration.
...
> >> + if (ares->type != ACPI_RESOURCE_TYPE_GPIO ||
> >> + ares->data.gpio.connection_type != ACPI_RESOURCE_GPIO_TYPE_IO)
> >> + return 1; /* Deliberately positive so parsing continues */
> > I don't like to lose control over ACPI_RESOURCE_TYPE_GPIO, i.e.
> > spreading it over kernel code (yes, I know about one existing TS
> > case).
> > Consider to provide a helper in analogue to acpi_gpio_get_irq_resource().
>
> Sure, but I probably name it acpi_gpio_is_io_resource() - a function
> named "get" which returns a bool seems a bit funny to me.
But don't you need the resource itself?
You may extract and check resource at the same time as
acpi_gpio_get_irq_resource() does.
...
> >> + struct int3472_discrete_device *int3472 = platform_get_drvdata(pdev);
> >> + if (int3472->gpios.dev_id)
> >> + gpiod_remove_lookup_table(&int3472->gpios);
> > gpiod_remove_lookup_table() is now NULL-aware.
> > But in any case I guess you don't need the above check.
>
> Sorry; forgot to call out that I didn't follow that suggestion;
> int3472->gpios is a _struct_ rather than a pointer, so &int3472->gpios
> won't be NULL, even if I haven't filled anything in to there yet because
> it failed before it got to that point. So, not sure that it quite works
> there.
I think if you initialize the ->list member you can remove without check.
--
With Best Regards,
Andy Shevchenko
1 year, 5 months
Re: [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver
by Andy Shevchenko
On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally <djrscally(a)gmail.com> wrote:
>
> ACPI devices with _HID INT3472 are currently matched to the tps68470
> driver, however this does not cover all situations in which that _HID
> occurs. We've encountered three possibilities:
>
> 1. On Chrome OS devices, an ACPI device with _HID INT3472 (representing
> a physical TPS68470 device) that requires a GPIO and OpRegion driver
> 2. On devices designed for Windows, an ACPI device with _HID INT3472
> (again representing a physical TPS68470 device) which requires GPIO,
> Clock and Regulator drivers.
> 3. On other devices designed for Windows, an ACPI device with _HID
> INT3472 which does **not** represent a physical TPS68470, and is instead
> used as a dummy device to group some system GPIO lines which are meant
> to be consumed by the sensor that is dependent on this entry.
>
> This commit adds a new module, registering a platform driver to deal
> with the 3rd scenario plus an i2c driver to deal with #1 and #2, by
> querying the CLDB buffer found against INT3472 entries to determine
> which is most appropriate.
Can you split CLK parts (and maybe regulators as well) to something
like intel_skl_int3472_clk.c?
...
> +#include <linux/acpi.h>
> +#include <linux/i2c.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> + dev_err(&adev->dev, "%s object is not an ACPI buffer\n", id);
Perhaps acpi_handle_err() et al. instead of dev_*(&adev->dev, ...)
where it's applicable?
...
> + if (obj->buffer.length > sizeof(*cldb)) {
> + dev_err(&adev->dev, "The CLDB buffer is too large\n");
> + ret = -EINVAL;
ENOSPC? ENOMEM?
> + goto out_free_obj;
> + }
...
> +static int skl_int3472_init(void)
> +{
> + int ret = 0;
Redundant assignment.
> + ret = platform_driver_register(&int3472_discrete);
> + if (ret)
> + return ret;
> +
> + ret = i2c_register_driver(THIS_MODULE, &int3472_tps68470);
> + if (ret)
> + platform_driver_unregister(&int3472_discrete);
Not a fan of the above, but let's see what others will say...
> + return ret;
> +}
> +module_init(skl_int3472_init);
...
> +#include <linux/clk-provider.h>
This is definitely not for *.h. (Not all C files needed this)
> +#include <linux/gpio/machine.h>
Ditto.
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>
Ditto.
...
> +/*
> + * 79234640-9e10-4fea-a5c1-b5aa8b19756f
> + * This _DSM GUID returns information about the GPIO lines mapped to a
> + * discrete INT3472 device. Function number 1 returns a count of the GPIO
> + * lines that are mapped. Subsequent functions return 32 bit ints encoding
> + * information about the GPIO line, including its purpose.
> + */
> +static const guid_t int3472_gpio_guid =
> + GUID_INIT(0x79234640, 0x9e10, 0x4fea,
> + 0xa5, 0xc1, 0xb5, 0xaa, 0x8b, 0x19, 0x75, 0x6f);
uuid.h ?
...
> +/*
> + * The regulators have to have .ops to be valid, but the only ops we actually
> + * support are .enable and .disable which are handled via .ena_gpiod. Pass an
> + * empty struct to clear the check without lying about capabilities.
> + */
> +static const struct regulator_ops int3472_gpio_regulator_ops = { 0 };
{ 0 } is implied by the static keyword and C standard.
...
> +static int skl_int3472_clk_prepare(struct clk_hw *hw)
> +{
> + struct int3472_gpio_clock *clk = to_int3472_clk(hw);
> +
> + gpiod_set_value(clk->ena_gpio, 1);
> + if (clk->led_gpio)
Make it optional and drop this check. Same for other places of use of this GPIO.
> + gpiod_set_value(clk->led_gpio, 1);
> +
> + return 0;
> +}
...
> +static int skl_int3472_clk_enable(struct clk_hw *hw)
> +{
> + /*
> + * We're just turning a GPIO on to enable, which operation has the
> + * potential to sleep. Given enable cannot sleep, but prepare can,
> + * we toggle the GPIO in prepare instead. Thus, nothing to do here.
> + */
Missed . and / or () in some words? (Describing callbacks, personally
I use the form "->callback()" in such cases)
> + return 0;
> +}
...
> +static unsigned int skl_int3472_get_clk_frequency(struct int3472_discrete_device *int3472)
> +{
> + union acpi_object *obj;
> + unsigned int ret = 0;
unsigned for ret is unusual. Looking into the code, first of all it
doesn't need this assignment; second, it probably can gain a better
name: "frequency"?
> + obj = skl_int3472_get_acpi_buffer(int3472->sensor, "SSDB");
> + if (IS_ERR(obj))
> + return 0; /* report rate as 0 on error */
> +
> + if (obj->buffer.length < CIO2_SENSOR_SSDB_MCLKSPEED_OFFSET + sizeof(u32)) {
> + dev_err(int3472->dev, "The buffer is too small\n");
> + goto out_free_buff;
> + }
> +
> + ret = *(u32 *)(obj->buffer.pointer + CIO2_SENSOR_SSDB_MCLKSPEED_OFFSET);
> +
> +out_free_buff:
> + kfree(obj);
> + return ret;
> +}
...
> + sensor_config = int3472->sensor_config;
> + if (!IS_ERR_OR_NULL(sensor_config) && sensor_config->function_maps) {
Hmm...
Would
if (IS_ERR_OR_NULL(sensor_config))
return 0;
if (!_maps)
return 0;
with respective comments working here?
> + const struct int3472_gpio_function_remap *remap =
> + sensor_config->function_maps;
Split assignment so we can see what is the initial for-loop iterator value.
> + for (; remap->documented; ++remap)
remap++
> + if (!strcmp(func, remap->documented)) {
> + func = remap->actual;
> + break;
> + }
> + }
> +
> + /* Functions mapped to NULL should not be mapped to the sensor */
> + if (!func)
> + return 0;
...
> +static int skl_int3472_register_clock(struct int3472_discrete_device *int3472)
> +{
> + struct clk_init_data init = {
> + .ops = &skl_int3472_clock_ops,
> + .flags = CLK_GET_RATE_NOCACHE,
> + };
> + int ret = 0;
> +
> + init.name = kasprintf(GFP_KERNEL, "%s-clk",
> + acpi_dev_name(int3472->adev));
devm_*() ? Or is the lifetime different?
> + if (!init.name)
> + return -ENOMEM;
> +
> + int3472->clock.frequency = skl_int3472_get_clk_frequency(int3472);
> +
> + int3472->clock.clk_hw.init = &init;
> + int3472->clock.clk = clk_register(&int3472->adev->dev,
> + &int3472->clock.clk_hw);
> + if (IS_ERR(int3472->clock.clk)) {
> + ret = PTR_ERR(int3472->clock.clk);
> + goto out_free_init_name;
> + }
> +
> + int3472->clock.cl = clkdev_create(int3472->clock.clk, NULL,
> + int3472->sensor_name);
> + if (!int3472->clock.cl) {
> + ret = -ENOMEM;
> + goto err_unregister_clk;
> + }
> +
> + goto out_free_init_name;
> +
> +err_unregister_clk:
> + clk_unregister(int3472->clock.clk);
> +out_free_init_name:
> + kfree(init.name);
> +
> + return ret;
> +}
...
> + sensor_config = int3472->sensor_config;
> + if (IS_ERR_OR_NULL(sensor_config)) {
> + dev_err(int3472->dev, "No sensor module config\n");
> + return PTR_ERR(sensor_config);
NULL -> 0. Is it okay?
> + }
...
> + int ret = 0;
Seems redundant assignment.
...
> + if (ares->type != ACPI_RESOURCE_TYPE_GPIO ||
> + ares->data.gpio.connection_type != ACPI_RESOURCE_GPIO_TYPE_IO)
> + return 1; /* Deliberately positive so parsing continues */
I don't like to lose control over ACPI_RESOURCE_TYPE_GPIO, i.e.
spreading it over kernel code (yes, I know about one existing TS
case).
Consider to provide a helper in analogue to acpi_gpio_get_irq_resource().
...
> + if (ret < 0 && ret != -EPROBE_DEFER)
> + dev_err(int3472->dev, err_msg);
dev_err_probe() will make the above conditional go away. And you may even do...
> + int3472->n_gpios++;
> + ACPI_FREE(obj);
> + return ret;
...here
return dev_err_probe(...);
...
> + struct list_head resource_list;
> + INIT_LIST_HEAD(&resource_list);
LIST_HEAD(resource_list);
will do two in one.
...
> + if (int3472->clock.ena_gpio) {
Not sure you need this here.
> + ret = skl_int3472_register_clock(int3472);
> + if (ret)
> + goto out_free_res_list;
> + } else {
> + if (int3472->clock.led_gpio)
Ditto.
> + dev_warn(int3472->dev,
> + "No clk GPIO. The privacy LED won't work\n");
> + }
...
> + /* Max num GPIOs we've seen plus a terminator */
> + int3472 = kzalloc(struct_size(int3472, gpios.table,
> + INT3472_MAX_SENSOR_GPIOS + 1), GFP_KERNEL);
Wonder of you can use devm_*() APIs in this function.
> + if (!int3472)
> + return -ENOMEM;
...
> + int3472->sensor = acpi_dev_get_dependent_dev(adev);
> + if (IS_ERR_OR_NULL(int3472->sensor)) {
> + dev_err(&pdev->dev,
> + "INT3472 seems to have no dependents.\n");
> + ret = -ENODEV;
Don't shadow error code when you got IS_ERR() case.
> + goto err_free_int3472;
> + }
...
> +int skl_int3472_discrete_remove(struct platform_device *pdev)
> +{
> + struct int3472_discrete_device *int3472 = platform_get_drvdata(pdev);
> + if (int3472->gpios.dev_id)
> + gpiod_remove_lookup_table(&int3472->gpios);
gpiod_remove_lookup_table() is now NULL-aware.
But in any case I guess you don't need the above check.
> + if (!IS_ERR(int3472->regulator.rdev))
> + regulator_unregister(int3472->regulator.rdev);
Shouldn't it be the pointer to the regulator itself?
> + if (!IS_ERR(int3472->clock.clk))
If you get it optional, you won't need this additional check.
> + clk_unregister(int3472->clock.clk);
> +
> + if (int3472->clock.cl)
> + clkdev_drop(int3472->clock.cl);
> +
> + gpiod_put(int3472->regulator.gpio);
> + gpiod_put(int3472->clock.ena_gpio);
> + gpiod_put(int3472->clock.led_gpio);
> +
> + acpi_dev_put(int3472->sensor);
> +
> + kfree(int3472->sensor_name);
> + kfree(int3472);
> +
> + return 0;
> +}
...
> + ret = skl_int3472_fill_cldb(adev, &cldb);
> + if (!ret && cldb.control_logic_type != 2) {
> + dev_err(&client->dev, "Unsupported control logic type %u\n",
> + cldb.control_logic_type);
> + return -EINVAL;
> + }
> +
> + if (ret)
> + cldb_present = false;
if (ret)
...
else if (...) {
...
return ...;
}
--
With Best Regards,
Andy Shevchenko
1 year, 5 months
Re: [PATCH v3 0/6] Introduce intel_skl_int3472 module
by Andy Shevchenko
On Mon, Feb 22, 2021 at 3:11 PM Daniel Scally <djrscally(a)gmail.com> wrote:
>
> v1 for this series was originally 14-18 of this series:
> https://lore.kernel.org/linux-media/20201130133129.1024662-1-djrscally@gm...
>
> v2 was here:
> https://lore.kernel.org/platform-driver-x86/20210118003428.568892-1-djrsc...
>
> Series level changelog:
>
> - Dropped the patch moving acpi_lpss_dep() to utils since it's not used
> in acpi_dev_get_dependent_dev() anymore.
> - Replaced it with a patch extending acpi_walk_dep_device_list() to be
> able to apply a given callback against each device in acpi_dep_list
> - Dropped the patch creating acpi_i2c_dev_name() and simply open coded
> that functionality.
>
> This has been tested on a number of devices, but currently **not** on a device
> designed for ChromeOS, which we ideally need to do to ensure no regression
> caused by replacing the tps68470 MFD driver. Sakari / Tomasz, is there any way
> you could help with that? Unfortunately, I don't have a device to test it on
> myself.
+Cc: Dmitry and Guenter. Guys, do you know by a chance who can help
with the above request from Daniel?
> Original cover letter:
>
> At the moment in the kernel the ACPI _HID INT3472 is taken by the tps68470
> MFD driver, but that driver can only handle some of the cases of that _HID
> that we see. There are at least these three possibilities:
>
> 1. INT3472 devices that provide GPIOs through the usual framework and run
> power and clocks through an operation region; this is the situation that
> the current module handles and is seen on ChromeOS devices
> 2. INT3472 devices that provide GPIOs, plus clocks and regulators that are
> meant to be driven through the usual frameworks, usually seen on devices
> designed to run Windows
> 3. INT3472 devices that don't actually represent a physical tps68470, but
> are being used as a convenient way of grouping a bunch of system GPIO
> lines that are intended to enable power and clocks for sensors which
> are called out as dependent on them. Also seen on devices designed to
> run Windows.
>
> This series introduces a new module which registers:
>
> 1. An i2c driver that determines which scenario (#1 or #2) applies to the
> machine and registers platform devices to be bound to GPIO, OpRegion,
> clock and regulator drivers as appropriate.
> 2. A platform driver that binds to the dummy INT3472 devices described in
> #3
>
> The platform driver for the dummy device registers the GPIO lines that
> enable the clocks and regulators to the sensors via those frameworks so
> that sensor drivers can consume them in the usual fashion. The existing
> GPIO and OpRegion tps68470 drivers will work with the i2c driver that's
> registered. Clock and regulator drivers are available but have not so far been
> tested, so aren't part of this series.
>
> The existing mfd/tps68470.c driver being thus superseded, it is removed.
>
> Thanks
> Dan
>
> Daniel Scally (6):
> ACPI: scan: Extend acpi_walk_dep_device_list()
> ACPI: scan: Add function to fetch dependent of acpi device
> i2c: core: Add a format macro for I2C device names
> gpiolib: acpi: Export acpi_get_gpiod()
> platform/x86: Add intel_skl_int3472 driver
> mfd: tps68470: Remove tps68470 MFD driver
>
> MAINTAINERS | 5 +
> drivers/acpi/ec.c | 2 +-
> drivers/acpi/pmic/Kconfig | 2 +-
> drivers/acpi/pmic/intel_pmic_chtdc_ti.c | 2 +-
> drivers/acpi/scan.c | 92 ++-
> drivers/gpio/Kconfig | 2 +-
> drivers/gpio/gpiolib-acpi.c | 38 +-
> drivers/i2c/i2c-core-acpi.c | 2 +-
> drivers/i2c/i2c-core-base.c | 4 +-
> drivers/mfd/Kconfig | 18 -
> drivers/mfd/Makefile | 1 -
> drivers/mfd/tps68470.c | 97 ---
> drivers/platform/surface/surface3_power.c | 2 +-
> drivers/platform/x86/Kconfig | 2 +
> drivers/platform/x86/Makefile | 1 +
> drivers/platform/x86/intel-int3472/Kconfig | 31 +
> drivers/platform/x86/intel-int3472/Makefile | 4 +
> .../intel-int3472/intel_skl_int3472_common.c | 106 ++++
> .../intel-int3472/intel_skl_int3472_common.h | 110 ++++
> .../intel_skl_int3472_discrete.c | 592 ++++++++++++++++++
> .../intel_skl_int3472_tps68470.c | 113 ++++
> include/acpi/acpi_bus.h | 8 +
> include/linux/acpi.h | 4 +-
> include/linux/gpio/consumer.h | 7 +
> include/linux/i2c.h | 3 +
> 25 files changed, 1100 insertions(+), 148 deletions(-)
> delete mode 100644 drivers/mfd/tps68470.c
> create mode 100644 drivers/platform/x86/intel-int3472/Kconfig
> create mode 100644 drivers/platform/x86/intel-int3472/Makefile
> create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_common.c
> create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_common.h
> create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c
> create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_tps68470.c
>
> --
> 2.25.1
>
--
With Best Regards,
Andy Shevchenko
1 year, 5 months
Re: [PATCH v3 6/6] mfd: tps68470: Remove tps68470 MFD driver
by Andy Shevchenko
On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally <djrscally(a)gmail.com> wrote:
>
> This driver only covered one scenario in which ACPI devices with _HID
> INT3472 are found, and its functionality has been taken over by the
> intel-skl-int3472 module, so remove it.
As long as patch 5 accepted
Acked-by: Andy Shevchenko <andy.shevchenko(a)gmail.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart(a)ideasonboard.com>
> Signed-off-by: Daniel Scally <djrscally(a)gmail.com>
> ---
> Changes in v3:
> - Replaced Kconfig dependencies with INTEL_SKL_INT3472 for the tps68470
> OpRegion and GPIO drivers.
>
> drivers/acpi/pmic/Kconfig | 2 +-
> drivers/gpio/Kconfig | 2 +-
> drivers/mfd/Kconfig | 18 --------
> drivers/mfd/Makefile | 1 -
> drivers/mfd/tps68470.c | 97 ---------------------------------------
> 5 files changed, 2 insertions(+), 118 deletions(-)
> delete mode 100644 drivers/mfd/tps68470.c
>
> diff --git a/drivers/acpi/pmic/Kconfig b/drivers/acpi/pmic/Kconfig
> index 56bbcb2ce61b..f84b8f6038dc 100644
> --- a/drivers/acpi/pmic/Kconfig
> +++ b/drivers/acpi/pmic/Kconfig
> @@ -52,7 +52,7 @@ endif # PMIC_OPREGION
>
> config TPS68470_PMIC_OPREGION
> bool "ACPI operation region support for TPS68470 PMIC"
> - depends on MFD_TPS68470
> + depends on INTEL_SKL_INT3472
> help
> This config adds ACPI operation region support for TI TPS68470 PMIC.
> TPS68470 device is an advanced power management unit that powers
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index c70f46e80a3b..998898c72af8 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1343,7 +1343,7 @@ config GPIO_TPS65912
>
> config GPIO_TPS68470
> bool "TPS68470 GPIO"
> - depends on MFD_TPS68470
> + depends on INTEL_SKL_INT3472
> help
> Select this option to enable GPIO driver for the TPS68470
> chip family.
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index bdfce7b15621..9a1f648efde0 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1520,24 +1520,6 @@ config MFD_TPS65217
> This driver can also be built as a module. If so, the module
> will be called tps65217.
>
> -config MFD_TPS68470
> - bool "TI TPS68470 Power Management / LED chips"
> - depends on ACPI && PCI && I2C=y
> - depends on I2C_DESIGNWARE_PLATFORM=y
> - select MFD_CORE
> - select REGMAP_I2C
> - help
> - If you say yes here you get support for the TPS68470 series of
> - Power Management / LED chips.
> -
> - These include voltage regulators, LEDs and other features
> - that are often used in portable devices.
> -
> - This option is a bool as it provides an ACPI operation
> - region, which must be available before any of the devices
> - using this are probed. This option also configures the
> - designware-i2c driver to be built-in, for the same reason.
> -
> config MFD_TI_LP873X
> tristate "TI LP873X Power Management IC"
> depends on I2C
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 14fdb188af02..5994e812f479 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -105,7 +105,6 @@ obj-$(CONFIG_MFD_TPS65910) += tps65910.o
> obj-$(CONFIG_MFD_TPS65912) += tps65912-core.o
> obj-$(CONFIG_MFD_TPS65912_I2C) += tps65912-i2c.o
> obj-$(CONFIG_MFD_TPS65912_SPI) += tps65912-spi.o
> -obj-$(CONFIG_MFD_TPS68470) += tps68470.o
> obj-$(CONFIG_MFD_TPS80031) += tps80031.o
> obj-$(CONFIG_MENELAUS) += menelaus.o
>
> diff --git a/drivers/mfd/tps68470.c b/drivers/mfd/tps68470.c
> deleted file mode 100644
> index 4a4df4ffd18c..000000000000
> --- a/drivers/mfd/tps68470.c
> +++ /dev/null
> @@ -1,97 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -/*
> - * TPS68470 chip Parent driver
> - *
> - * Copyright (C) 2017 Intel Corporation
> - *
> - * Authors:
> - * Rajmohan Mani <rajmohan.mani(a)intel.com>
> - * Tianshu Qiu <tian.shu.qiu(a)intel.com>
> - * Jian Xu Zheng <jian.xu.zheng(a)intel.com>
> - * Yuning Pu <yuning.pu(a)intel.com>
> - */
> -
> -#include <linux/acpi.h>
> -#include <linux/delay.h>
> -#include <linux/i2c.h>
> -#include <linux/init.h>
> -#include <linux/mfd/core.h>
> -#include <linux/mfd/tps68470.h>
> -#include <linux/regmap.h>
> -
> -static const struct mfd_cell tps68470s[] = {
> - { .name = "tps68470-gpio" },
> - { .name = "tps68470_pmic_opregion" },
> -};
> -
> -static const struct regmap_config tps68470_regmap_config = {
> - .reg_bits = 8,
> - .val_bits = 8,
> - .max_register = TPS68470_REG_MAX,
> -};
> -
> -static int tps68470_chip_init(struct device *dev, struct regmap *regmap)
> -{
> - unsigned int version;
> - int ret;
> -
> - /* Force software reset */
> - ret = regmap_write(regmap, TPS68470_REG_RESET, TPS68470_REG_RESET_MASK);
> - if (ret)
> - return ret;
> -
> - ret = regmap_read(regmap, TPS68470_REG_REVID, &version);
> - if (ret) {
> - dev_err(dev, "Failed to read revision register: %d\n", ret);
> - return ret;
> - }
> -
> - dev_info(dev, "TPS68470 REVID: 0x%x\n", version);
> -
> - return 0;
> -}
> -
> -static int tps68470_probe(struct i2c_client *client)
> -{
> - struct device *dev = &client->dev;
> - struct regmap *regmap;
> - int ret;
> -
> - regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config);
> - if (IS_ERR(regmap)) {
> - dev_err(dev, "devm_regmap_init_i2c Error %ld\n",
> - PTR_ERR(regmap));
> - return PTR_ERR(regmap);
> - }
> -
> - i2c_set_clientdata(client, regmap);
> -
> - ret = tps68470_chip_init(dev, regmap);
> - if (ret < 0) {
> - dev_err(dev, "TPS68470 Init Error %d\n", ret);
> - return ret;
> - }
> -
> - ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, tps68470s,
> - ARRAY_SIZE(tps68470s), NULL, 0, NULL);
> - if (ret < 0) {
> - dev_err(dev, "devm_mfd_add_devices failed: %d\n", ret);
> - return ret;
> - }
> -
> - return 0;
> -}
> -
> -static const struct acpi_device_id tps68470_acpi_ids[] = {
> - {"INT3472"},
> - {},
> -};
> -
> -static struct i2c_driver tps68470_driver = {
> - .driver = {
> - .name = "tps68470",
> - .acpi_match_table = tps68470_acpi_ids,
> - },
> - .probe_new = tps68470_probe,
> -};
> -builtin_i2c_driver(tps68470_driver);
> --
> 2.25.1
>
--
With Best Regards,
Andy Shevchenko
1 year, 5 months