[PATCH 1/2] dt-bindings: spi: Add Spreadtrum ADI controller documentation

[PATCH 1/2] dt-bindings: spi: Add Spreadtrum ADI controller documentation

[Baolin Wang]

This patch adds the binding documentation for Spreadtrum ADI
controller device.

Signed-off-by: Baolin Wang <baolin.wang-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>
---
 .../devicetree/bindings/spi/spi-sprd-adi.txt       |   44 ++++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-sprd-adi.txt

diff --git a/Documentation/devicetree/bindings/spi/spi-sprd-adi.txt b/Documentation/devicetree/bindings/spi/spi-sprd-adi.txt
new file mode 100644
index 0000000..11c5259
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-sprd-adi.txt
@@ -0,0 +1,44 @@
+Spreadtrum ADI controller based on SPI framework
+
+ADI is the abbreviation of Anolog-Digital interface, which is used to access
+analog chip (such as PMIC) from digital chip. ADI controller follows the SPI
+framework for its hardware implementation is alike to SPI bus and its timing
+is compatile to SPI timing.
+
+ADI controller has 50 channels including 2 software read/write channels and
+48 hardware channels to access analog chip. For hardware channels, we introduce
+one property named "sprd,hw-channels" to configure hardware channels, the first
+value specifies the channel id, and the second value specifies the address which
+is mapped into analog chip space.
+
+Since we have multi-subsystems will use ADI to access analog chip, then we need
+one hardware spinlock to synchronize between the multiple subsystems.
+
+Required properties:
+- compatible: Should be "sprd,sc9860-adi".
+- reg: Offset and length of ADI-SPI controller register space.
+- hwlocks: Reference to a phandle of a hwlock provider node.
+- hwlock-names: Reference to hwlock name strings defined in the same order
+	as the hwlocks.
+- #address-cells: Number of cells required to define a chip select address
+	on the ADI-SPI bus. Should be set to 1.
+- #size-cells: Size of cells required to define a chip select address size
+	on the ADI-SPI bus. Should be set to 0.
+
+Optional properties:
+- sprd,hw-channels: Specify the hardware channel number and mapped address
+	for hardware channel accessing.
+
+SPI slave nodes must be children of the SPI controller node and can contain
+properties described in Documentation/devicetree/bindings/spi/spi-bus.txt.
+
+Example:
+	adi_bus: spi@40030000 {
+		compatible = "sprd,sc9860-adi";
+		reg = <0 0x40030000 0 0x10000>;
+		hwlocks = <&hwlock1 0>;
+		hwlock-names = "adi";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		sprd,hw-channels = <30 0x8c20>;
+	};
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] spi: Add ADI driver for Spreadtrum platform

[Baolin Wang]

This patch adds ADI driver based on SPI framework for
Spreadtrum SC9860 platform.

Signed-off-by: Baolin Wang <baolin.wang@spreadtrum.com>
---
 drivers/spi/Kconfig        |    6 +
 drivers/spi/Makefile       |    1 +
 drivers/spi/spi-sprd-adi.c |  431 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 438 insertions(+)
 create mode 100644 drivers/spi/spi-sprd-adi.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 9b31351..a00b9a9 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -627,6 +627,12 @@ config SPI_SIRF
 	help
 	  SPI driver for CSR SiRFprimaII SoCs
 
+config SPI_SPRD_ADI
+	tristate "Spreadtrum ADI controller"
+	depends on ARCH_SPRD
+	help
+	  ADI driver based on SPI for Spreadtrum SoCs.
+
 config SPI_STM32
 	tristate "STMicroelectronics STM32 SPI controller"
 	depends on ARCH_STM32 || COMPILE_TEST
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index a3ae2b7..de5ae2a 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -90,6 +90,7 @@ obj-$(CONFIG_SPI_SH_HSPI)		+= spi-sh-hspi.o
 obj-$(CONFIG_SPI_SH_MSIOF)		+= spi-sh-msiof.o
 obj-$(CONFIG_SPI_SH_SCI)		+= spi-sh-sci.o
 obj-$(CONFIG_SPI_SIRF)		+= spi-sirf.o
+obj-$(CONFIG_SPI_SPRD_ADI)		+= spi-sprd-adi.o
 obj-$(CONFIG_SPI_STM32) 		+= spi-stm32.o
 obj-$(CONFIG_SPI_ST_SSC4)		+= spi-st-ssc4.o
 obj-$(CONFIG_SPI_SUN4I)			+= spi-sun4i.o
diff --git a/drivers/spi/spi-sprd-adi.c b/drivers/spi/spi-sprd-adi.c
new file mode 100644
index 0000000..6dd014f
--- /dev/null
+++ b/drivers/spi/spi-sprd-adi.c
@@ -0,0 +1,431 @@
+/*
+ * Copyright (C) 2017 Spreadtrum Communications Inc.
+ *
+ * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+ */
+
+#include <linux/hwspinlock.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/spi/spi.h>
+#include <linux/sizes.h>
+
+/* Registers definitions for ADI controller */
+#define REG_ADI_CTRL0			0x4
+#define REG_ADI_CHN_PRIL		0x8
+#define REG_ADI_CHN_PRIH		0xc
+#define REG_ADI_INT_EN			0x10
+#define REG_ADI_INT_RAW			0x14
+#define REG_ADI_INT_MASK		0x18
+#define REG_ADI_INT_CLR			0x1c
+#define REG_ADI_GSSI_CFG0		0x20
+#define REG_ADI_GSSI_CFG1		0x24
+#define REG_ADI_RD_CMD			0x28
+#define REG_ADI_RD_DATA			0x2c
+#define REG_ADI_ARM_FIFO_STS		0x30
+#define REG_ADI_STS			0x34
+#define REG_ADI_EVT_FIFO_STS		0x38
+#define REG_ADI_ARM_CMD_STS		0x3c
+#define REG_ADI_CHN_EN			0x40
+#define REG_ADI_CHN_ADDR(id)		(0x44 + (id - 2) * 4)
+#define REG_ADI_CHN_EN1			0x20c
+
+/* Bits definitions for register REG_ADI_GSSI_CFG0 */
+#define BIT_CLK_ALL_ON			BIT(30)
+
+/* Bits definitions for register REG_ADI_RD_DATA */
+#define BIT_RD_CMD_BUSY			BIT(31)
+#define RD_ADDR_SHIFT			16
+#define RD_VALUE_MASK			GENMASK(15, 0)
+#define RD_ADDR_MASK			GENMASK(30, 16)
+
+/* Bits definitions for register REG_ADI_ARM_FIFO_STS */
+#define BIT_FIFO_FULL			BIT(11)
+#define BIT_FIFO_EMPTY			BIT(10)
+
+/*
+ * ADI slave devices include RTC, ADC, regulator, charger, thermal and so on.
+ * The slave devices address offset is always 0x8000 and size is 4K.
+ */
+#define ADI_SLAVE_ADDR_SIZE		SZ_4K
+#define ADI_SLAVE_OFFSET		0x8000
+
+/* Timeout (ms) for the trylock of hardware spinlocks */
+#define ADI_HWSPINLOCK_TIMEOUT		5000
+/*
+ * ADI controller has 50 channels including 2 software channels
+ * and 48 hardware channels.
+ */
+#define ADI_HW_CHNS			50
+
+#define ADI_FIFO_DRAIN_TIMEOUT		1000
+#define ADI_READ_TIMEOUT		2000
+#define REG_ADDR_LOW_MASK		GENMASK(11, 0)
+
+struct sprd_adi {
+	struct spi_controller	*ctlr;
+	struct device		*dev;
+	void __iomem		*base;
+	struct hwspinlock	*hwlock;
+	unsigned long		slave_vbase;
+	unsigned long		slave_pbase;
+};
+
+static int sprd_adi_check_paddr(struct sprd_adi *sadi, u32 paddr)
+{
+	if (paddr < sadi->slave_pbase || paddr >
+	    (sadi->slave_pbase + ADI_SLAVE_ADDR_SIZE)) {
+		dev_err(sadi->dev,
+			"slave physical address is incorrect, addr = 0x%x\n",
+			paddr);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static unsigned long sprd_adi_to_vaddr(struct sprd_adi *sadi, u32 paddr)
+{
+	return (paddr - sadi->slave_pbase + sadi->slave_vbase);
+}
+
+static int sprd_adi_drain_fifo(struct sprd_adi *sadi)
+{
+	u32 timeout = ADI_FIFO_DRAIN_TIMEOUT;
+	u32 sts;
+
+	do {
+		sts = readl_relaxed(sadi->base + REG_ADI_ARM_FIFO_STS);
+		if (sts & BIT_FIFO_EMPTY)
+			break;
+
+		cpu_relax();
+	} while (--timeout);
+
+	if (timeout == 0) {
+		dev_err(sadi->dev, "drain write fifo timeout\n");
+		return -EBUSY;
+	}
+
+	return 0;
+}
+
+static int sprd_adi_fifo_is_full(struct sprd_adi *sadi)
+{
+	return readl_relaxed(sadi->base + REG_ADI_ARM_FIFO_STS) & BIT_FIFO_FULL;
+}
+
+static int sprd_adi_read(struct sprd_adi *sadi, u32 reg_paddr, u32 *read_val)
+{
+	int read_timeout = ADI_READ_TIMEOUT;
+	u32 val, rd_addr;
+
+	/*
+	 * Set the physical register address need to read into RD_CMD register,
+	 * then ADI controller will start to transfer automatically.
+	 */
+	writel_relaxed(reg_paddr, sadi->base + REG_ADI_RD_CMD);
+
+	/*
+	 * Wait read operation complete, the BIT_RD_CMD_BUSY will be set
+	 * simultaneously when writing read command to register, and the
+	 * BIT_RD_CMD_BUSY will be cleared after the read operation is
+	 * completed.
+	 */
+	do {
+		val = readl_relaxed(sadi->base + REG_ADI_RD_DATA);
+		if (!(val & BIT_RD_CMD_BUSY))
+			break;
+
+		cpu_relax();
+	} while (--read_timeout);
+
+	if (read_timeout == 0) {
+		dev_err(sadi->dev, "ADI read timeout\n");
+		return -EBUSY;
+	}
+
+	/*
+	 * The return value includes data and read register address, from bit 0
+	 * to bit 15 are data, and from bit 16 to bit 30 are read register
+	 * address. Then we can check the returned register address to validate
+	 * data.
+	 */
+	rd_addr = (val & RD_ADDR_MASK ) >> RD_ADDR_SHIFT;
+
+	if (rd_addr != (reg_paddr & REG_ADDR_LOW_MASK)) {
+		dev_err(sadi->dev, "read error, reg addr = 0x%x, val = 0x%x\n",
+			reg_paddr, val);
+		return -EIO;
+	}
+
+	*read_val = val & RD_VALUE_MASK;
+	return 0;
+}
+
+static int sprd_adi_write(struct sprd_adi *sadi, unsigned long reg, u32 val)
+{
+	u32 timeout = ADI_FIFO_DRAIN_TIMEOUT;
+	int ret;
+
+	ret = sprd_adi_drain_fifo(sadi);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * we should wait for write fifo is empty before writing data to PMIC
+	 * registers.
+	 */
+	do {
+		if (!sprd_adi_fifo_is_full(sadi)) {
+			writel_relaxed(val, (void __iomem *)reg);
+			break;
+		}
+
+		cpu_relax();
+	} while (--timeout);
+
+	if (timeout == 0) {
+		dev_err(sadi->dev, "write fifo is full\n");
+		return -EBUSY;
+	}
+
+	return 0;
+}
+
+static int sprd_adi_transfer_one(struct spi_controller *ctlr,
+				 struct spi_device *spi_dev,
+				 struct spi_transfer *t)
+{
+	struct sprd_adi *sadi = spi_controller_get_devdata(ctlr);
+	unsigned long flags, virt_reg;
+	u32 phy_reg, val;
+	int ret;
+
+	if (t->rx_buf) {
+		phy_reg = *(u32 *)t->rx_buf + sadi->slave_pbase;
+
+		ret = sprd_adi_check_paddr(sadi, phy_reg);
+		if (ret)
+			return ret;
+
+		ret = hwspin_lock_timeout_irqsave(sadi->hwlock,
+						  ADI_HWSPINLOCK_TIMEOUT,
+						  &flags);
+		if (ret) {
+			dev_err(sadi->dev, "get the hw lock failed\n");
+			return ret;
+		}
+
+		ret = sprd_adi_read(sadi, phy_reg, &val);
+		hwspin_unlock_irqrestore(sadi->hwlock, &flags);
+		if (ret)
+			return ret;
+
+		*(u32 *)t->rx_buf = val;
+	} else if (t->tx_buf) {
+		u32 *p = (u32 *)t->tx_buf;
+
+		/*
+		 * Get the physical register address need to write and convert
+		 * the physical address to virtual address. Since we need
+		 * virtual register address to write.
+		 */
+		phy_reg = *p++ + sadi->slave_pbase;
+		ret = sprd_adi_check_paddr(sadi, phy_reg);
+		if (ret)
+			return ret;
+
+		virt_reg = sprd_adi_to_vaddr(sadi, phy_reg);
+		val = *p;
+
+		ret = hwspin_lock_timeout_irqsave(sadi->hwlock,
+						  ADI_HWSPINLOCK_TIMEOUT,
+						  &flags);
+		if (ret) {
+			dev_err(sadi->dev, "get the hw lock failed\n");
+			return ret;
+		}
+
+		ret = sprd_adi_write(sadi, virt_reg, val);
+		hwspin_unlock_irqrestore(sadi->hwlock, &flags);
+		if (ret)
+			return ret;
+	} else {
+		dev_err(sadi->dev, "no buffer for transfer\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void sprd_adi_hw_init(struct sprd_adi *sadi)
+{
+	struct device_node *np = sadi->dev->of_node;
+	int i, size, chn_cnt;
+	const __be32 *list;
+	u32 tmp;
+
+	/* Address bits select default 12 bits */
+	writel_relaxed(0, sadi->base + REG_ADI_CTRL0);
+
+	/* Set all channels as default priority */
+	writel_relaxed(0, sadi->base + REG_ADI_CHN_PRIL);
+	writel_relaxed(0, sadi->base + REG_ADI_CHN_PRIH);
+
+	/* Set clock auto gate mode */
+	tmp = readl_relaxed(sadi->base + REG_ADI_GSSI_CFG0);
+	tmp &= ~BIT_CLK_ALL_ON;
+	writel_relaxed(tmp, sadi->base + REG_ADI_GSSI_CFG0);
+
+	/* Set hardware channels setting */
+	list = of_get_property(np, "sprd,hw-channels", &size);
+	if (!size || !list) {
+		dev_info(sadi->dev, "no hw channels setting in node\n");
+		return;
+	}
+
+	chn_cnt = size / 8;
+	for (i = 0; i < chn_cnt; i++) {
+		u32 value;
+		u32 chn_id = be32_to_cpu(*list++);
+		u32 chn_config = be32_to_cpu(*list++);
+
+		/* Channel 0 and 1 are software channels */
+		if (chn_id < 2)
+			continue;
+
+		writel_relaxed(chn_config, sadi->base +
+			       REG_ADI_CHN_ADDR(chn_id));
+
+		if (chn_id < 31) {
+			value = readl_relaxed(sadi->base + REG_ADI_CHN_EN);
+			value |= BIT(chn_id);
+			writel_relaxed(value, sadi->base + REG_ADI_CHN_EN);
+		} else if (chn_id < ADI_HW_CHNS) {
+			value = readl_relaxed(sadi->base + REG_ADI_CHN_EN1);
+			value |= BIT(chn_id - 32);
+			writel_relaxed(value, sadi->base + REG_ADI_CHN_EN1);
+		}
+	}
+}
+
+static int sprd_adi_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct spi_controller *ctlr;
+	struct sprd_adi *sadi;
+	struct resource *res;
+	u32 num_chipselect;
+	int ret;
+
+	if (!np) {
+		dev_err(&pdev->dev, "can not find the adi bus node\n");
+		return -ENODEV;
+	}
+
+	pdev->id = of_alias_get_id(np, "spi");
+	num_chipselect = of_get_child_count(np);
+
+	ctlr = spi_alloc_master(&pdev->dev, sizeof(struct sprd_adi));
+	if (!ctlr)
+		return -ENOMEM;
+
+	dev_set_drvdata(&pdev->dev, ctlr);
+	sadi = spi_controller_get_devdata(ctlr);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	sadi->base = devm_ioremap_resource(&pdev->dev, res);
+	if (!sadi->base) {
+		ret = -ENOMEM;
+		goto put_ctlr;
+	}
+
+	sadi->slave_vbase = (unsigned long)sadi->base + ADI_SLAVE_OFFSET;
+	sadi->slave_pbase = res->start + ADI_SLAVE_OFFSET;
+	sadi->ctlr = ctlr;
+	sadi->dev = &pdev->dev;
+	ret = of_hwspin_lock_get_id(np, 0);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "can not get the hardware spinlock\n");
+		goto put_ctlr;
+	}
+
+	sadi->hwlock = hwspin_lock_request_specific(ret);
+	if (!sadi->hwlock) {
+		ret = -ENXIO;
+		goto put_ctlr;
+	}
+
+	sprd_adi_hw_init(sadi);
+
+	ctlr->dev.of_node = pdev->dev.of_node;
+	ctlr->bus_num = pdev->id;
+	ctlr->num_chipselect = num_chipselect;
+	ctlr->flags = SPI_MASTER_HALF_DUPLEX;
+	ctlr->bits_per_word_mask = 0;
+	ctlr->transfer_one = sprd_adi_transfer_one;
+
+	ret = devm_spi_register_controller(&pdev->dev, ctlr);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register SPI controller\n");
+		goto free_hwlock;
+	}
+
+	return 0;
+
+free_hwlock:
+	hwspin_lock_free(sadi->hwlock);
+put_ctlr:
+	spi_controller_put(ctlr);
+	return ret;
+}
+
+static int sprd_adi_remove(struct platform_device *pdev)
+{
+	struct spi_controller *ctlr = dev_get_drvdata(&pdev->dev);
+	struct sprd_adi *sadi = spi_controller_get_devdata(ctlr);
+
+	hwspin_lock_free(sadi->hwlock);
+	spi_controller_put(ctlr);
+	return 0;
+}
+
+static const struct of_device_id sprd_adi_of_match[] = {
+	{
+		.compatible = "sprd,sc9860-adi",
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, sprd_adi_of_match);
+
+static struct platform_driver sprd_adi_driver = {
+	.driver = {
+		.name = "sprd-adi",
+		.owner = THIS_MODULE,
+		.of_match_table = sprd_adi_of_match,
+	},
+	.probe = sprd_adi_probe,
+	.remove = sprd_adi_remove,
+};
+
+static int __init sprd_adi_init(void)
+{
+	return platform_driver_register(&sprd_adi_driver);
+}
+subsys_initcall(sprd_adi_init);
+
+static void __exit sprd_adi_exit(void)
+{
+	platform_driver_unregister(&sprd_adi_driver);
+}
+module_exit(sprd_adi_exit);
+
+MODULE_DESCRIPTION("Spreadtrum ADI Controller Driver");
+MODULE_AUTHOR("Baolin Wang <Baolin.Wang@spreadtrum.com>");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH 2/2] spi: Add ADI driver for Spreadtrum platform

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 877 bytes --]

On Wed, Sep 06, 2017 at 02:10:44PM +0800, Baolin Wang wrote:

This looks like a nice, clean driver.  A few fairly small issues though:

> +config SPI_SPRD_ADI
> +	tristate "Spreadtrum ADI controller"
> +	depends on ARCH_SPRD
> +	help
> +	  ADI driver based on SPI for Spreadtrum SoCs.
> +

I can't see any hard dependencies on the architecture - can you add an
|| COMPILE_TEST for more coverage?

> +	ret = devm_spi_register_controller(&pdev->dev, ctlr);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register SPI controller\n");
> +		goto free_hwlock;
> +	}

> +	spi_controller_put(ctlr);

You used devm_ to register the controller, no need to free it
explicitly.

> +static int __init sprd_adi_init(void)
> +{
> +	return platform_driver_register(&sprd_adi_driver);
> +}
> +subsys_initcall(sprd_adi_init);

Why is this subsys_initcall() and not module_platform_driver()?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] spi: Add ADI driver for Spreadtrum platform

[Baolin Wang]

Hi Mark,

On 6 September 2017 at 23:04, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Wed, Sep 06, 2017 at 02:10:44PM +0800, Baolin Wang wrote:
>
> This looks like a nice, clean driver.  A few fairly small issues though:
>
>> +config SPI_SPRD_ADI
>> +     tristate "Spreadtrum ADI controller"
>> +     depends on ARCH_SPRD
>> +     help
>> +       ADI driver based on SPI for Spreadtrum SoCs.
>> +
>
> I can't see any hard dependencies on the architecture - can you add an
> || COMPILE_TEST for more coverage?

Yes.

>
>> +     ret = devm_spi_register_controller(&pdev->dev, ctlr);
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "failed to register SPI controller\n");
>> +             goto free_hwlock;
>> +     }
>
>> +     spi_controller_put(ctlr);
>
> You used devm_ to register the controller, no need to free it
> explicitly.

Indeed.

>
>> +static int __init sprd_adi_init(void)
>> +{
>> +     return platform_driver_register(&sprd_adi_driver);
>> +}
>> +subsys_initcall(sprd_adi_init);
>
> Why is this subsys_initcall() and not module_platform_driver()?

Since ADI is one very fundamental driver for our SoC, many drivers
such as regulator need depend on ADI, and regulator need to regulate
core voltage as earlier as possible.
Very appreciated for your comments.

-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] spi: Add ADI driver for Spreadtrum platform

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 934 bytes --]

On Thu, Sep 07, 2017 at 11:13:00AM +0800, Baolin Wang wrote:

> >> +static int __init sprd_adi_init(void)
> >> +{
> >> +     return platform_driver_register(&sprd_adi_driver);
> >> +}
> >> +subsys_initcall(sprd_adi_init);

> > Why is this subsys_initcall() and not module_platform_driver()?

> Since ADI is one very fundamental driver for our SoC, many drivers
> such as regulator need depend on ADI, and regulator need to regulate
> core voltage as earlier as possible.

That applies to huge numbers of systems - you should still just use
regular init ordering in mainline, there are efforts to make things
better there (look at Viresh's dependency stuff) so hopefully things
will improve in the future and in the meantime the cost of probe
deferral isn't *that* great and it's less fiddly than tweaking ordering.  
Practically speaking init ordering stuff can always be added in vendor
kernels in the meantime.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] spi: Add ADI driver for Spreadtrum platform

[Baolin Wang]

On 7 September 2017 at 18:10, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Sep 07, 2017 at 11:13:00AM +0800, Baolin Wang wrote:
>
>> >> +static int __init sprd_adi_init(void)
>> >> +{
>> >> +     return platform_driver_register(&sprd_adi_driver);
>> >> +}
>> >> +subsys_initcall(sprd_adi_init);
>
>> > Why is this subsys_initcall() and not module_platform_driver()?
>
>> Since ADI is one very fundamental driver for our SoC, many drivers
>> such as regulator need depend on ADI, and regulator need to regulate
>> core voltage as earlier as possible.
>
> That applies to huge numbers of systems - you should still just use
> regular init ordering in mainline, there are efforts to make things
> better there (look at Viresh's dependency stuff) so hopefully things
> will improve in the future and in the meantime the cost of probe
> deferral isn't *that* great and it's less fiddly than tweaking ordering.
> Practically speaking init ordering stuff can always be added in vendor
> kernels in the meantime.

Make sense. So I change back to module_platform_driver().

-- 
Baolin.wang
Best Regards

Re: [PATCH 1/2] dt-bindings: spi: Add Spreadtrum ADI controller documentation

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 513 bytes --]

On Wed, Sep 06, 2017 at 02:10:43PM +0800, Baolin Wang wrote:

> +- hwlocks: Reference to a phandle of a hwlock provider node.
> +- hwlock-names: Reference to hwlock name strings defined in the same order
> +	as the hwlocks.

What are these hwlocks protecting, and what names are expected?

> +Optional properties:
> +- sprd,hw-channels: Specify the hardware channel number and mapped address
> +	for hardware channel accessing.

What do these mean and how are the numbers and how will the binding be
interpreted?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] dt-bindings: spi: Add Spreadtrum ADI controller documentation

[Baolin Wang]

Hi Mark,

On 6 September 2017 at 22:59, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Wed, Sep 06, 2017 at 02:10:43PM +0800, Baolin Wang wrote:
>
>> +- hwlocks: Reference to a phandle of a hwlock provider node.
>> +- hwlock-names: Reference to hwlock name strings defined in the same order
>> +     as the hwlocks.
>
> What are these hwlocks protecting, and what names are expected?

I made one explanation in above sentence, I assume it is not clear.
Since we have multi-subsystems will use ADI to access analog chip,
when one system is reading/writing data by ADI, which should be under
one hardware spinlock protection to prevent other systems from
reading/writing data by ADI at the same time, or two parallel routine
of setting ADI registers will get incorrect results.

The hwspinlock name should be "adi", and I will make it clear in next version.

>
>> +Optional properties:
>> +- sprd,hw-channels: Specify the hardware channel number and mapped address
>> +     for hardware channel accessing.
>
> What do these mean and how are the numbers and how will the binding be
> interpreted?

I also gave one explanation in above sentence, is it not clear? I try again.

ADI controller has 50 channels including 2 software read/write
channels and 48 hardware channels to access analog chip. For 2
software read/write channels, which means we should set ADI registers
to access analog chip. But For hardware channels, we can just mapped
one analog chip address to one hardware channel, then user can access
analog chip by hardware channel without setting ADI registers.

For this "sprd,hw-channels" property, the first value specifies the
channel id, and the second value specifies the address which is mapped
into analog chip space.

-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] dt-bindings: spi: Add Spreadtrum ADI controller documentation

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2172 bytes --]

On Thu, Sep 07, 2017 at 11:29:05AM +0800, Baolin Wang wrote:
> On 6 September 2017 at 22:59, Mark Brown <broonie@kernel.org> wrote:
> > On Wed, Sep 06, 2017 at 02:10:43PM +0800, Baolin Wang wrote:

> >> +- hwlocks: Reference to a phandle of a hwlock provider node.
> >> +- hwlock-names: Reference to hwlock name strings defined in the same order
> >> +     as the hwlocks.

> > What are these hwlocks protecting, and what names are expected?

> I made one explanation in above sentence, I assume it is not clear.
> Since we have multi-subsystems will use ADI to access analog chip,
> when one system is reading/writing data by ADI, which should be under
> one hardware spinlock protection to prevent other systems from
> reading/writing data by ADI at the same time, or two parallel routine
> of setting ADI registers will get incorrect results.

> The hwspinlock name should be "adi", and I will make it clear in next version.

So there's other drivers that might also be accessing this IP block?

> >> +Optional properties:
> >> +- sprd,hw-channels: Specify the hardware channel number and mapped address
> >> +     for hardware channel accessing.

> > What do these mean and how are the numbers and how will the binding be
> > interpreted?

> I also gave one explanation in above sentence, is it not clear? I try again.

It says what they are, it doesn't say for example what a hardware
channel is or how those numbers map onto the actual hardware.

> ADI controller has 50 channels including 2 software read/write
> channels and 48 hardware channels to access analog chip. For 2
> software read/write channels, which means we should set ADI registers
> to access analog chip. But For hardware channels, we can just mapped
> one analog chip address to one hardware channel, then user can access
> analog chip by hardware channel without setting ADI registers.

> For this "sprd,hw-channels" property, the first value specifies the
> channel id, and the second value specifies the address which is mapped
> into analog chip space.

So does this driver control all the channels or are there other drivers
(or hardware components) that control some of the other channels?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] dt-bindings: spi: Add Spreadtrum ADI controller documentation

[Baolin Wang]

On 7 September 2017 at 17:54, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Sep 07, 2017 at 11:29:05AM +0800, Baolin Wang wrote:
>> On 6 September 2017 at 22:59, Mark Brown <broonie@kernel.org> wrote:
>> > On Wed, Sep 06, 2017 at 02:10:43PM +0800, Baolin Wang wrote:
>
>> >> +- hwlocks: Reference to a phandle of a hwlock provider node.
>> >> +- hwlock-names: Reference to hwlock name strings defined in the same order
>> >> +     as the hwlocks.
>
>> > What are these hwlocks protecting, and what names are expected?
>
>> I made one explanation in above sentence, I assume it is not clear.
>> Since we have multi-subsystems will use ADI to access analog chip,
>> when one system is reading/writing data by ADI, which should be under
>> one hardware spinlock protection to prevent other systems from
>> reading/writing data by ADI at the same time, or two parallel routine
>> of setting ADI registers will get incorrect results.
>
>> The hwspinlock name should be "adi", and I will make it clear in next version.
>
> So there's other drivers that might also be accessing this IP block?

Yes. Other drivers (like regulator, RTC or charger ... ) can access
analog chip (like PMIC) by ADI controller. But the hardware spinlock
is used to synchronize between the multiple subsystems, since we only
have one ADI controller.

>
>> >> +Optional properties:
>> >> +- sprd,hw-channels: Specify the hardware channel number and mapped address
>> >> +     for hardware channel accessing.
>
>> > What do these mean and how are the numbers and how will the binding be
>> > interpreted?
>
>> I also gave one explanation in above sentence, is it not clear? I try again.
>
> It says what they are, it doesn't say for example what a hardware
> channel is or how those numbers map onto the actual hardware.

OK. I will add more documentation to explain that.

>
>> ADI controller has 50 channels including 2 software read/write
>> channels and 48 hardware channels to access analog chip. For 2
>> software read/write channels, which means we should set ADI registers
>> to access analog chip. But For hardware channels, we can just mapped
>> one analog chip address to one hardware channel, then user can access
>> analog chip by hardware channel without setting ADI registers.
>
>> For this "sprd,hw-channels" property, the first value specifies the
>> channel id, and the second value specifies the address which is mapped
>> into analog chip space.
>
> So does this driver control all the channels or are there other drivers
> (or hardware components) that control some of the other channels?

The ADI driver only controls 2 software channels (read/write), and
other hardware channels can be controlled by hardware components if we
set the hardware config.

-- 
Baolin.wang
Best Regards

Re: [PATCH 1/2] dt-bindings: spi: Add Spreadtrum ADI controller documentation

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1187 bytes --]

On Thu, Sep 07, 2017 at 07:03:18PM +0800, Baolin Wang wrote:
> On 7 September 2017 at 17:54, Mark Brown <broonie@kernel.org> wrote:
> > On Thu, Sep 07, 2017 at 11:29:05AM +0800, Baolin Wang wrote:

> >> The hwspinlock name should be "adi", and I will make it clear in next version.

> > So there's other drivers that might also be accessing this IP block?

> Yes. Other drivers (like regulator, RTC or charger ... ) can access
> analog chip (like PMIC) by ADI controller. But the hardware spinlock
> is used to synchronize between the multiple subsystems, since we only
> have one ADI controller.

If it were other drivers then the kernel should already be doing that
but...

> > So does this driver control all the channels or are there other drivers
> > (or hardware components) that control some of the other channels?

> The ADI driver only controls 2 software channels (read/write), and
> other hardware channels can be controlled by hardware components if we
> set the hardware config.

...if it can be configured to allow other things to use it independently
then this is fine, just needs a bit more documentation so someone can
understand how the bindings and hardware match up.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] dt-bindings: spi: Add Spreadtrum ADI controller documentation

[Baolin Wang]

On 7 September 2017 at 19:44, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Thu, Sep 07, 2017 at 07:03:18PM +0800, Baolin Wang wrote:
>> On 7 September 2017 at 17:54, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> > On Thu, Sep 07, 2017 at 11:29:05AM +0800, Baolin Wang wrote:
>
>> >> The hwspinlock name should be "adi", and I will make it clear in next version.
>
>> > So there's other drivers that might also be accessing this IP block?
>
>> Yes. Other drivers (like regulator, RTC or charger ... ) can access
>> analog chip (like PMIC) by ADI controller. But the hardware spinlock
>> is used to synchronize between the multiple subsystems, since we only
>> have one ADI controller.
>
> If it were other drivers then the kernel should already be doing that
> but...

Not only kernel drivers, but also other systems' drivers will access
ADI too. So here we need one hardware spinlock to protect, I will add
more documentation.

>
>> > So does this driver control all the channels or are there other drivers
>> > (or hardware components) that control some of the other channels?
>
>> The ADI driver only controls 2 software channels (read/write), and
>> other hardware channels can be controlled by hardware components if we
>> set the hardware config.
>
> ...if it can be configured to allow other things to use it independently
> then this is fine, just needs a bit more documentation so someone can
> understand how the bindings and hardware match up.

OK. Thanks for your comments.

-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html