All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] mfd: Add support for Merrifield Basin Cove PMIC
@ 2019-06-12 10:19 Andy Shevchenko
  2019-06-24 16:13 ` Lee Jones
  0 siblings, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2019-06-12 10:19 UTC (permalink / raw)
  To: Lee Jones, linux-kernel; +Cc: Andy Shevchenko

Add an MFD driver for Intel Merrifield Basin Cove PMIC.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
- updated copyright year to be 2019
- rebased on top of latest vanilla rc

 drivers/mfd/Kconfig                      |  11 ++
 drivers/mfd/Makefile                     |   1 +
 drivers/mfd/intel_soc_pmic_mrfld.c       | 157 +++++++++++++++++++++++
 include/linux/mfd/intel_soc_pmic_mrfld.h |  81 ++++++++++++
 4 files changed, 250 insertions(+)
 create mode 100644 drivers/mfd/intel_soc_pmic_mrfld.c
 create mode 100644 include/linux/mfd/intel_soc_pmic_mrfld.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index a17d275bf1d4..466dfede2e63 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -583,6 +583,17 @@ config INTEL_SOC_PMIC_CHTDC_TI
 	  Select this option for supporting Dollar Cove (TI version) PMIC
 	  device that is found on some Intel Cherry Trail systems.
 
+config INTEL_SOC_PMIC_MRFLD
+	tristate "Support for Intel Merrifield Basin Cove PMIC"
+	depends on GPIOLIB
+	depends on ACPI
+	depends on INTEL_SCU_IPC
+	select MFD_CORE
+	select REGMAP_IRQ
+	help
+	  Select this option for supporting Basin Cove PMIC device
+	  that is found on Intel Merrifield systems.
+
 config MFD_INTEL_LPSS
 	tristate
 	select COMMON_CLK
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 52b1a90ff515..2f4927819a14 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -235,6 +235,7 @@ obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
 obj-$(CONFIG_INTEL_SOC_PMIC_BXTWC)	+= intel_soc_pmic_bxtwc.o
 obj-$(CONFIG_INTEL_SOC_PMIC_CHTWC)	+= intel_soc_pmic_chtwc.o
 obj-$(CONFIG_INTEL_SOC_PMIC_CHTDC_TI)	+= intel_soc_pmic_chtdc_ti.o
+obj-$(CONFIG_INTEL_SOC_PMIC_MRFLD)	+= intel_soc_pmic_mrfld.o
 obj-$(CONFIG_MFD_MT6397)	+= mt6397-core.o
 
 obj-$(CONFIG_MFD_ALTERA_A10SR)	+= altera-a10sr.o
diff --git a/drivers/mfd/intel_soc_pmic_mrfld.c b/drivers/mfd/intel_soc_pmic_mrfld.c
new file mode 100644
index 000000000000..26a1551c5faf
--- /dev/null
+++ b/drivers/mfd/intel_soc_pmic_mrfld.c
@@ -0,0 +1,157 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device access for Basin Cove PMIC
+ *
+ * Copyright (c) 2019, Intel Corporation.
+ * Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
+ */
+
+#include <linux/acpi.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/intel_soc_pmic.h>
+#include <linux/mfd/intel_soc_pmic_mrfld.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include <asm/intel_scu_ipc.h>
+
+/*
+ * Level 2 IRQs
+ *
+ * Firmware on the systems with Basin Cove PMIC services Level 1 IRQs
+ * without an assistance. Thus, each of the Level 1 IRQ is represented
+ * as a separate RTE in IOAPIC.
+ */
+static struct resource irq_level2_resources[] = {
+	DEFINE_RES_IRQ(0), /* power button */
+	DEFINE_RES_IRQ(0), /* TMU */
+	DEFINE_RES_IRQ(0), /* thermal */
+	DEFINE_RES_IRQ(0), /* BCU */
+	DEFINE_RES_IRQ(0), /* ADC */
+	DEFINE_RES_IRQ(0), /* charger */
+	DEFINE_RES_IRQ(0), /* GPIO */
+};
+
+static const struct mfd_cell bcove_dev[] = {
+	{
+		.name = "mrfld_bcove_pwrbtn",
+		.num_resources = 1,
+		.resources = &irq_level2_resources[0],
+	}, {
+		.name = "mrfld_bcove_tmu",
+		.num_resources = 1,
+		.resources = &irq_level2_resources[1],
+	}, {
+		.name = "mrfld_bcove_thermal",
+		.num_resources = 1,
+		.resources = &irq_level2_resources[2],
+	}, {
+		.name = "mrfld_bcove_bcu",
+		.num_resources = 1,
+		.resources = &irq_level2_resources[3],
+	}, {
+		.name = "mrfld_bcove_adc",
+		.num_resources = 1,
+		.resources = &irq_level2_resources[4],
+	}, {
+		.name = "mrfld_bcove_charger",
+		.num_resources = 1,
+		.resources = &irq_level2_resources[5],
+	}, {
+		.name = "mrfld_bcove_pwrsrc",
+		.num_resources = 1,
+		.resources = &irq_level2_resources[5],
+	}, {
+		.name = "mrfld_bcove_gpio",
+		.num_resources = 1,
+		.resources = &irq_level2_resources[6],
+	},
+	{	.name = "mrfld_bcove_region", },
+};
+
+static int bcove_ipc_byte_reg_read(void *context, unsigned int reg,
+				    unsigned int *val)
+{
+	u8 ipc_out;
+	int ret;
+
+	ret = intel_scu_ipc_ioread8(reg, &ipc_out);
+	if (ret)
+		return ret;
+
+	*val = ipc_out;
+	return 0;
+}
+
+static int bcove_ipc_byte_reg_write(void *context, unsigned int reg,
+				     unsigned int val)
+{
+	u8 ipc_in = val;
+	int ret;
+
+	ret = intel_scu_ipc_iowrite8(reg, ipc_in);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static const struct regmap_config bcove_regmap_config = {
+	.reg_bits = 16,
+	.val_bits = 8,
+	.max_register = 0xff,
+	.reg_write = bcove_ipc_byte_reg_write,
+	.reg_read = bcove_ipc_byte_reg_read,
+};
+
+static int bcove_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct intel_soc_pmic *pmic;
+	unsigned int i;
+	int ret;
+
+	pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
+	if (!pmic)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, pmic);
+	pmic->dev = &pdev->dev;
+
+	pmic->regmap = devm_regmap_init(dev, NULL, pmic, &bcove_regmap_config);
+	if (IS_ERR(pmic->regmap))
+		return PTR_ERR(pmic->regmap);
+
+	for (i = 0; i < ARRAY_SIZE(irq_level2_resources); i++) {
+		ret = platform_get_irq(pdev, i);
+		if (ret < 0)
+			return ret;
+
+		irq_level2_resources[i].start = ret;
+		irq_level2_resources[i].end = ret;
+	}
+
+	return devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
+				    bcove_dev, ARRAY_SIZE(bcove_dev),
+				    NULL, 0, NULL);
+}
+
+static const struct acpi_device_id bcove_acpi_ids[] = {
+	{ "INTC100E" },
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, bcove_acpi_ids);
+
+static struct platform_driver bcove_driver = {
+	.driver = {
+		.name = "intel_soc_pmic_mrfld",
+		.acpi_match_table = bcove_acpi_ids,
+	},
+	.probe = bcove_probe,
+};
+module_platform_driver(bcove_driver);
+
+MODULE_DESCRIPTION("IPC driver for Intel SoC Basin Cove PMIC");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/intel_soc_pmic_mrfld.h b/include/linux/mfd/intel_soc_pmic_mrfld.h
new file mode 100644
index 000000000000..4daecd682275
--- /dev/null
+++ b/include/linux/mfd/intel_soc_pmic_mrfld.h
@@ -0,0 +1,81 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Header file for Intel Merrifield Basin Cove PMIC
+ *
+ * Copyright (C) 2019 Intel Corporation. All rights reserved.
+ */
+
+#ifndef __INTEL_SOC_PMIC_MRFLD_H__
+#define __INTEL_SOC_PMIC_MRFLD_H__
+
+#include <linux/bits.h>
+
+#define BCOVE_ID		0x00
+
+#define BCOVE_ID_MINREV0	GENMASK(2, 0)
+#define BCOVE_ID_MAJREV0	GENMASK(5, 3)
+#define BCOVE_ID_VENDID0	GENMASK(7, 6)
+
+#define BCOVE_MINOR(x)		(unsigned int)(((x) & BCOVE_ID_MINREV0) >> 0)
+#define BCOVE_MAJOR(x)		(unsigned int)(((x) & BCOVE_ID_MAJREV0) >> 3)
+#define BCOVE_VENDOR(x)		(unsigned int)(((x) & BCOVE_ID_VENDID0) >> 6)
+
+#define BCOVE_IRQLVL1		0x01
+
+#define BCOVE_PBIRQ		0x02
+#define BCOVE_TMUIRQ		0x03
+#define BCOVE_THRMIRQ		0x04
+#define BCOVE_BCUIRQ		0x05
+#define BCOVE_ADCIRQ		0x06
+#define BCOVE_CHGRIRQ0		0x07
+#define BCOVE_CHGRIRQ1		0x08
+#define BCOVE_GPIOIRQ		0x09
+#define BCOVE_CRITIRQ		0x0B
+
+#define BCOVE_MIRQLVL1		0x0C
+
+#define BCOVE_MPBIRQ		0x0D
+#define BCOVE_MTMUIRQ		0x0E
+#define BCOVE_MTHRMIRQ		0x0F
+#define BCOVE_MBCUIRQ		0x10
+#define BCOVE_MADCIRQ		0x11
+#define BCOVE_MCHGRIRQ0		0x12
+#define BCOVE_MCHGRIRQ1		0x13
+#define BCOVE_MGPIOIRQ		0x14
+#define BCOVE_MCRITIRQ		0x16
+
+#define BCOVE_SCHGRIRQ0		0x4E
+#define BCOVE_SCHGRIRQ1		0x4F
+
+/* Level 1 IRQs */
+#define BCOVE_LVL1_PWRBTN	BIT(0)	/* power button */
+#define BCOVE_LVL1_TMU		BIT(1)	/* time management unit */
+#define BCOVE_LVL1_THRM		BIT(2)	/* thermal */
+#define BCOVE_LVL1_BCU		BIT(3)	/* burst control unit */
+#define BCOVE_LVL1_ADC		BIT(4)	/* ADC */
+#define BCOVE_LVL1_CHGR		BIT(5)	/* charger */
+#define BCOVE_LVL1_GPIO		BIT(6)	/* GPIO */
+#define BCOVE_LVL1_CRIT		BIT(7)	/* critical event */
+
+/* Level 2 IRQs: power button */
+#define BCOVE_PBIRQ_PBTN	BIT(0)
+#define BCOVE_PBIRQ_UBTN	BIT(1)
+
+/* Level 2 IRQs: ADC */
+#define BCOVE_ADCIRQ_BATTEMP	BIT(2)
+#define BCOVE_ADCIRQ_SYSTEMP	BIT(3)
+#define BCOVE_ADCIRQ_BATTID	BIT(4)
+#define BCOVE_ADCIRQ_VIBATT	BIT(5)
+#define BCOVE_ADCIRQ_CCTICK	BIT(7)
+
+/* Level 2 IRQs: charger */
+#define BCOVE_CHGRIRQ_BAT0ALRT	BIT(4)
+#define BCOVE_CHGRIRQ_BAT1ALRT	BIT(5)
+#define BCOVE_CHGRIRQ_BATCRIT	BIT(6)
+
+#define BCOVE_CHGRIRQ_VBUSDET	BIT(0)
+#define BCOVE_CHGRIRQ_DCDET	BIT(1)
+#define BCOVE_CHGRIRQ_BATTDET	BIT(2)
+#define BCOVE_CHGRIRQ_USBIDDET	BIT(3)
+
+#endif	/* __INTEL_SOC_PMIC_MRFLD_H__ */
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] mfd: Add support for Merrifield Basin Cove PMIC
  2019-06-12 10:19 [PATCH v3] mfd: Add support for Merrifield Basin Cove PMIC Andy Shevchenko
@ 2019-06-24 16:13 ` Lee Jones
  2019-06-26  9:26   ` Andy Shevchenko
  0 siblings, 1 reply; 7+ messages in thread
From: Lee Jones @ 2019-06-24 16:13 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-kernel

On Wed, 12 Jun 2019, Andy Shevchenko wrote:

> Add an MFD driver for Intel Merrifield Basin Cove PMIC.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> - updated copyright year to be 2019
> - rebased on top of latest vanilla rc
> 
>  drivers/mfd/Kconfig                      |  11 ++
>  drivers/mfd/Makefile                     |   1 +
>  drivers/mfd/intel_soc_pmic_mrfld.c       | 157 +++++++++++++++++++++++
>  include/linux/mfd/intel_soc_pmic_mrfld.h |  81 ++++++++++++
>  4 files changed, 250 insertions(+)
>  create mode 100644 drivers/mfd/intel_soc_pmic_mrfld.c
>  create mode 100644 include/linux/mfd/intel_soc_pmic_mrfld.h

[...]

> +static int bcove_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct intel_soc_pmic *pmic;
> +	unsigned int i;
> +	int ret;
> +
> +	pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
> +	if (!pmic)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, pmic);
> +	pmic->dev = &pdev->dev;
> +
> +	pmic->regmap = devm_regmap_init(dev, NULL, pmic, &bcove_regmap_config);
> +	if (IS_ERR(pmic->regmap))
> +		return PTR_ERR(pmic->regmap);
> +
> +	for (i = 0; i < ARRAY_SIZE(irq_level2_resources); i++) {
> +		ret = platform_get_irq(pdev, i);

If you already know the order, define the children's device IDs in the
parent's shared header ('intel_soc_pmic_mrfld.h'?) and retreive them
like:

  platform_get_irq(pdev->parent, <SUITABLE_DEFINED_ID>);

Then you can skip all of this platform device -> platform device hoop
jumping.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] mfd: Add support for Merrifield Basin Cove PMIC
  2019-06-24 16:13 ` Lee Jones
@ 2019-06-26  9:26   ` Andy Shevchenko
  2019-06-26 10:17     ` Lee Jones
  0 siblings, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2019-06-26  9:26 UTC (permalink / raw)
  To: Lee Jones; +Cc: linux-kernel

On Mon, Jun 24, 2019 at 05:13:48PM +0100, Lee Jones wrote:
> On Wed, 12 Jun 2019, Andy Shevchenko wrote:
> 
> > Add an MFD driver for Intel Merrifield Basin Cove PMIC.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> > - updated copyright year to be 2019
> > - rebased on top of latest vanilla rc
> > 
> >  drivers/mfd/Kconfig                      |  11 ++
> >  drivers/mfd/Makefile                     |   1 +
> >  drivers/mfd/intel_soc_pmic_mrfld.c       | 157 +++++++++++++++++++++++
> >  include/linux/mfd/intel_soc_pmic_mrfld.h |  81 ++++++++++++
> >  4 files changed, 250 insertions(+)
> >  create mode 100644 drivers/mfd/intel_soc_pmic_mrfld.c
> >  create mode 100644 include/linux/mfd/intel_soc_pmic_mrfld.h
> 
> [...]
> 
> > +static int bcove_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct intel_soc_pmic *pmic;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
> > +	if (!pmic)
> > +		return -ENOMEM;
> > +
> > +	platform_set_drvdata(pdev, pmic);
> > +	pmic->dev = &pdev->dev;
> > +
> > +	pmic->regmap = devm_regmap_init(dev, NULL, pmic, &bcove_regmap_config);
> > +	if (IS_ERR(pmic->regmap))
> > +		return PTR_ERR(pmic->regmap);
> > +
> > +	for (i = 0; i < ARRAY_SIZE(irq_level2_resources); i++) {
> > +		ret = platform_get_irq(pdev, i);
> 
> If you already know the order, define the children's device IDs in the
> parent's shared header ('intel_soc_pmic_mrfld.h'?) and retreive them
> like:
> 
>   platform_get_irq(pdev->parent, <SUITABLE_DEFINED_ID>);
> 
> Then you can skip all of this platform device -> platform device hoop
> jumping.

The idea of MFD is to get children to be parent agnostic
(at least to some extent). What you are proposing here
seems like disadvantage from MFD philosophy. No?


-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] mfd: Add support for Merrifield Basin Cove PMIC
  2019-06-26  9:26   ` Andy Shevchenko
@ 2019-06-26 10:17     ` Lee Jones
  2019-06-26 11:10       ` Andy Shevchenko
  0 siblings, 1 reply; 7+ messages in thread
From: Lee Jones @ 2019-06-26 10:17 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-kernel

On Wed, 26 Jun 2019, Andy Shevchenko wrote:

> On Mon, Jun 24, 2019 at 05:13:48PM +0100, Lee Jones wrote:
> > On Wed, 12 Jun 2019, Andy Shevchenko wrote:
> > 
> > > Add an MFD driver for Intel Merrifield Basin Cove PMIC.
> > > 
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > ---
> > > - updated copyright year to be 2019
> > > - rebased on top of latest vanilla rc
> > > 
> > >  drivers/mfd/Kconfig                      |  11 ++
> > >  drivers/mfd/Makefile                     |   1 +
> > >  drivers/mfd/intel_soc_pmic_mrfld.c       | 157 +++++++++++++++++++++++
> > >  include/linux/mfd/intel_soc_pmic_mrfld.h |  81 ++++++++++++
> > >  4 files changed, 250 insertions(+)
> > >  create mode 100644 drivers/mfd/intel_soc_pmic_mrfld.c
> > >  create mode 100644 include/linux/mfd/intel_soc_pmic_mrfld.h
> > 
> > [...]
> > 
> > > +static int bcove_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	struct intel_soc_pmic *pmic;
> > > +	unsigned int i;
> > > +	int ret;
> > > +
> > > +	pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
> > > +	if (!pmic)
> > > +		return -ENOMEM;
> > > +
> > > +	platform_set_drvdata(pdev, pmic);
> > > +	pmic->dev = &pdev->dev;
> > > +
> > > +	pmic->regmap = devm_regmap_init(dev, NULL, pmic, &bcove_regmap_config);
> > > +	if (IS_ERR(pmic->regmap))
> > > +		return PTR_ERR(pmic->regmap);
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(irq_level2_resources); i++) {
> > > +		ret = platform_get_irq(pdev, i);
> > 
> > If you already know the order, define the children's device IDs in the
> > parent's shared header ('intel_soc_pmic_mrfld.h'?) and retreive them
> > like:
> > 
> >   platform_get_irq(pdev->parent, <SUITABLE_DEFINED_ID>);
> > 
> > Then you can skip all of this platform device -> platform device hoop
> > jumping.
> 
> The idea of MFD is to get children to be parent agnostic
> (at least to some extent). What you are proposing here
> seems like disadvantage from MFD philosophy. No?

Not at all.  The idea of MFD is to split up support for monolithic h/w
such that they can be handled properly by their appropriate
subsystems, and by extension, maintained by the associated subject
matter experts.

Children are often aware of their parents (some siblings are even
aware of each other!), and many expect and depend on the data-sets
provided by their parents.

For instance (this example may come to bite me in the behind, but),
taken from this very patch, where is this consumed?

 platform_set_drvdata(pdev, pmic);

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] mfd: Add support for Merrifield Basin Cove PMIC
  2019-06-26 10:17     ` Lee Jones
@ 2019-06-26 11:10       ` Andy Shevchenko
  2019-06-27 13:44         ` Lee Jones
  0 siblings, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2019-06-26 11:10 UTC (permalink / raw)
  To: Lee Jones; +Cc: linux-kernel

On Wed, Jun 26, 2019 at 11:17:27AM +0100, Lee Jones wrote:
> On Wed, 26 Jun 2019, Andy Shevchenko wrote:
> > On Mon, Jun 24, 2019 at 05:13:48PM +0100, Lee Jones wrote:
> > > On Wed, 12 Jun 2019, Andy Shevchenko wrote:

> > > > Add an MFD driver for Intel Merrifield Basin Cove PMIC.

> > > > +	for (i = 0; i < ARRAY_SIZE(irq_level2_resources); i++) {
> > > > +		ret = platform_get_irq(pdev, i);
> > > 
> > > If you already know the order, define the children's device IDs in the
> > > parent's shared header ('intel_soc_pmic_mrfld.h'?) and retreive them
> > > like:
> > > 
> > >   platform_get_irq(pdev->parent, <SUITABLE_DEFINED_ID>);
> > > 
> > > Then you can skip all of this platform device -> platform device hoop
> > > jumping.
> > 
> > The idea of MFD is to get children to be parent agnostic
> > (at least to some extent). What you are proposing here
> > seems like disadvantage from MFD philosophy. No?
> 
> Not at all.  The idea of MFD is to split up support for monolithic h/w
> such that they can be handled properly by their appropriate
> subsystems, and by extension, maintained by the associated subject
> matter experts.
> 
> Children are often aware of their parents (some siblings are even
> aware of each other!), and many expect and depend on the data-sets
> provided by their parents.

Yes, that's true and that's why I put wording "to some extent" above.

> For instance (this example may come to bite me in the behind, but),
> taken from this very patch, where is this consumed?
> 
>  platform_set_drvdata(pdev, pmic);

Yes. It's used in children. BUT. This structure covers several PMIC chips and
the children driver doesn't know which generation / version of PMIC is serving.

What you are proposing with the change is to strictly link the children driver
to PMIC gen X ver Y, while above example doesn't do that.

So, I'm not convinced it's a good change to have.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] mfd: Add support for Merrifield Basin Cove PMIC
  2019-06-26 11:10       ` Andy Shevchenko
@ 2019-06-27 13:44         ` Lee Jones
  2019-07-04 13:54           ` Andy Shevchenko
  0 siblings, 1 reply; 7+ messages in thread
From: Lee Jones @ 2019-06-27 13:44 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-kernel

On Wed, 26 Jun 2019, Andy Shevchenko wrote:

> On Wed, Jun 26, 2019 at 11:17:27AM +0100, Lee Jones wrote:
> > On Wed, 26 Jun 2019, Andy Shevchenko wrote:
> > > On Mon, Jun 24, 2019 at 05:13:48PM +0100, Lee Jones wrote:
> > > > On Wed, 12 Jun 2019, Andy Shevchenko wrote:
> 
> > > > > Add an MFD driver for Intel Merrifield Basin Cove PMIC.
> 
> > > > > +	for (i = 0; i < ARRAY_SIZE(irq_level2_resources); i++) {
> > > > > +		ret = platform_get_irq(pdev, i);
> > > > 
> > > > If you already know the order, define the children's device IDs in the
> > > > parent's shared header ('intel_soc_pmic_mrfld.h'?) and retreive them
> > > > like:
> > > > 
> > > >   platform_get_irq(pdev->parent, <SUITABLE_DEFINED_ID>);
> > > > 
> > > > Then you can skip all of this platform device -> platform device hoop
> > > > jumping.
> > > 
> > > The idea of MFD is to get children to be parent agnostic
> > > (at least to some extent). What you are proposing here
> > > seems like disadvantage from MFD philosophy. No?
> > 
> > Not at all.  The idea of MFD is to split up support for monolithic h/w
> > such that they can be handled properly by their appropriate
> > subsystems, and by extension, maintained by the associated subject
> > matter experts.
> > 
> > Children are often aware of their parents (some siblings are even
> > aware of each other!), and many expect and depend on the data-sets
> > provided by their parents.
> 
> Yes, that's true and that's why I put wording "to some extent" above.
> 
> > For instance (this example may come to bite me in the behind, but),
> > taken from this very patch, where is this consumed?
> > 
> >  platform_set_drvdata(pdev, pmic);
> 
> Yes. It's used in children. BUT. This structure covers several PMIC chips and
> the children driver doesn't know which generation / version of PMIC is serving.
> 
> What you are proposing with the change is to strictly link the children driver
> to PMIC gen X ver Y, while above example doesn't do that.

Well that is a different argument. :)

I still don't really like the idea of sucking one set of platform data
just to place in another.  The implementation even looks hacky.

What if you were to provide the child driver with its IRQ index?
Perhaps via platform data?

> So, I'm not convinced it's a good change to have.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] mfd: Add support for Merrifield Basin Cove PMIC
  2019-06-27 13:44         ` Lee Jones
@ 2019-07-04 13:54           ` Andy Shevchenko
  0 siblings, 0 replies; 7+ messages in thread
From: Andy Shevchenko @ 2019-07-04 13:54 UTC (permalink / raw)
  To: Lee Jones; +Cc: linux-kernel

On Thu, Jun 27, 2019 at 02:44:46PM +0100, Lee Jones wrote:
> On Wed, 26 Jun 2019, Andy Shevchenko wrote:
> 
> > On Wed, Jun 26, 2019 at 11:17:27AM +0100, Lee Jones wrote:
> > > On Wed, 26 Jun 2019, Andy Shevchenko wrote:
> > > > On Mon, Jun 24, 2019 at 05:13:48PM +0100, Lee Jones wrote:
> > > > > On Wed, 12 Jun 2019, Andy Shevchenko wrote:
> > 
> > > > > > Add an MFD driver for Intel Merrifield Basin Cove PMIC.
> > 
> > > > > > +	for (i = 0; i < ARRAY_SIZE(irq_level2_resources); i++) {
> > > > > > +		ret = platform_get_irq(pdev, i);
> > > > > 
> > > > > If you already know the order, define the children's device IDs in the
> > > > > parent's shared header ('intel_soc_pmic_mrfld.h'?) and retreive them
> > > > > like:
> > > > > 
> > > > >   platform_get_irq(pdev->parent, <SUITABLE_DEFINED_ID>);
> > > > > 
> > > > > Then you can skip all of this platform device -> platform device hoop
> > > > > jumping.
> > > > 
> > > > The idea of MFD is to get children to be parent agnostic
> > > > (at least to some extent). What you are proposing here
> > > > seems like disadvantage from MFD philosophy. No?
> > > 
> > > Not at all.  The idea of MFD is to split up support for monolithic h/w
> > > such that they can be handled properly by their appropriate
> > > subsystems, and by extension, maintained by the associated subject
> > > matter experts.
> > > 
> > > Children are often aware of their parents (some siblings are even
> > > aware of each other!), and many expect and depend on the data-sets
> > > provided by their parents.
> > 
> > Yes, that's true and that's why I put wording "to some extent" above.
> > 
> > > For instance (this example may come to bite me in the behind, but),
> > > taken from this very patch, where is this consumed?
> > > 
> > >  platform_set_drvdata(pdev, pmic);
> > 
> > Yes. It's used in children. BUT. This structure covers several PMIC chips and
> > the children driver doesn't know which generation / version of PMIC is serving.
> > 
> > What you are proposing with the change is to strictly link the children driver
> > to PMIC gen X ver Y, while above example doesn't do that.
> 
> Well that is a different argument. :)

> I still don't really like the idea of sucking one set of platform data
> just to place in another.  The implementation even looks hacky.

To me it looks straight forward. We get a description of MFD and split it
accordingly. Look at the rest of Intel PMIC drivers, they are built on the
same principles.

"Thanks" to firmware here, that does more, than on other PMICs, that's why we
have several IRQ resources, but from hardware point of view it's still an MFD.

> What if you were to provide the child driver with its IRQ index?
> Perhaps via platform data?

It seems some people would like to bring up Shady Cove PMIC support in the
future, which seems to behave in a similar way, while the (most of the) drivers
can be re-used.

So, for me it will look like a hack in each of PMIC driver to have additional
platform data for whatever can be passed using well-established facilities,
like device resources.

> > So, I'm not convinced it's a good change to have.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-07-04 13:54 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-12 10:19 [PATCH v3] mfd: Add support for Merrifield Basin Cove PMIC Andy Shevchenko
2019-06-24 16:13 ` Lee Jones
2019-06-26  9:26   ` Andy Shevchenko
2019-06-26 10:17     ` Lee Jones
2019-06-26 11:10       ` Andy Shevchenko
2019-06-27 13:44         ` Lee Jones
2019-07-04 13:54           ` Andy Shevchenko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.