mfd: mc13xxx: Honor designed SPI mode

PATCH New Alexander Shiyan 4 participants 15 messages Mar 2, 2014 – Mar 5, 2014

Subsystems (5)

Arm (32-bit) Arm sub-architectures Arm i.MX and Layerscape Multifunction devices (MFD) +1 more subsystems

lore

Share & tools

lore

Alexander Shiyan

author

2014-03-02 07:44 UTC #0 lore raw

PATCH 1/4 mfd: mc13xxx: Honor designed SPI mode

Instead of using hard coded SPI mode, use mode described in devicetree or defined for the board.
Additionally, patch updates all users of this PMIC to use correct SPI mode.

Signed-off-by Alexander Shiyan <[email protected]>

---
 arch/arm/mach-imx/mach-mx31lilly.c | 1 +
 arch/arm/mach-imx/mach-mx31lite.c  | 1 +
 drivers/mfd/mc13xxx-spi.c          | 2 --
 3 files changed, 2 insertions(+), 2 deletions(-)

arch/arm/mach-imx/mach-mx31lilly.c +1 -0

index 832b1e2..16c688e 100644
@@ -231,6 +231,7 @@ static struct spi_board_info mc13783_dev __initdata = {
 	.bus_num	= 1,
 	.chip_select	= 0,
 	.platform_data	= &mc13783_pdata,
+	.mode		= SPI_MODE_0 | SPI_CS_HIGH,
 	/* irq number is run-time assigned */
 };

arch/arm/mach-imx/mach-mx31lite.c +1 -0

index bea0729..bf3e61e 100644
@@ -121,6 +121,7 @@ static struct spi_board_info mc13783_spi_dev __initdata = {
 	.bus_num	= 1,
 	.chip_select    = 0,
 	.platform_data  = &mc13783_pdata,
+	.mode		= SPI_MODE_0 | SPI_CS_HIGH,
 	/* irq number is run-time assigned */
 };

drivers/mfd/mc13xxx-spi.c +0 -2

index 38ab678..f88087f 100644
@@ -136,8 +136,6 @@ static int mc13xxx_spi_probe(struct spi_device *spi)
 
 	dev_set_drvdata(&spi->dev, mc13xxx);
 
-	spi->mode = SPI_MODE_0 | SPI_CS_HIGH;
-
 	mc13xxx->irq = spi->irq;
 
 	mc13xxx->regmap = devm_regmap_init(&spi->dev, &regmap_mc13xxx_bus,

Signature · 1 line

1.8.3.2

Alexander Shiyan

author

2014-03-02 07:44 UTC #1 lore raw

PATCH 2/4 mfd: mc13xxx: Limit maximum SPI speed

The patch adds the maximum speed limit in accordance with the PMIC datasheet if other value is not given in the devicetree description or board data.

Signed-off-by Alexander Shiyan <[email protected]>

---
 drivers/mfd/mc13xxx-spi.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/mfd/mc13xxx-spi.c +2 -0

index f88087f..cd27c32 100644
@@ -138,6 +138,8 @@ static int mc13xxx_spi_probe(struct spi_device *spi)
 
 	mc13xxx->irq = spi->irq;
 
+	spi->max_speed_hz = spi->max_speed_hz ? : 26000000;
+
 	mc13xxx->regmap = devm_regmap_init(&spi->dev, &regmap_mc13xxx_bus,
 					   &spi->dev,
 					   &mc13xxx_regmap_spi_config);

Signature · 1 line

1.8.3.2

Lee Jones

Applied

2014-03-03 07:36 UTC #2 lore raw

PATCH 2/4 mfd: mc13xxx: Limit maximum SPI speed

Quoted 14 lines

The patch adds the maximum speed limit in accordance with the PMIC datasheet if other value is not given in the devicetree description or board data.
Signed-off-by Alexander Shiyan <[email protected]>
---
 drivers/mfd/mc13xxx-spi.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/mfd/mc13xxx-spi.c +0 -0
index f88087f..cd27c32 100644

drivers/mfd/mc13xxx-spi.c static int mc13xxx_spi_probe(struct spi_device *spi) · line 145

Quoted 8 lines

drivers/mfd/mc13xxx-spi.c +2 -0

@@ -138,6 +138,8 @@ static int mc13xxx_spi_probe(struct spi_device *spi)
 
 	mc13xxx->irq = spi->irq;
 
+	spi->max_speed_hz = spi->max_speed_hz ? : 26000000;
+
 	mc13xxx->regmap = devm_regmap_init(&spi->dev, &regmap_mc13xxx_bus,
 					   &spi->dev,
 					   &mc13xxx_regmap_spi_config);

Applied, thanks.

Signature · 4 lines

Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Alexander Shiyan

author

2014-03-02 07:44 UTC #3 lore raw

PATCH 3/4 mfd: mc13xxx: Add missing spi_setup()

In reply to Alexander Shiyan

The probe routine should call spi_setup() to configure the SPI bus so it can properly communicate with the device.

Signed-off-by Alexander Shiyan <[email protected]>

---
 drivers/mfd/mc13xxx-spi.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/mfd/mc13xxx-spi.c +3 -0

index cd27c32..f35624a 100644
@@ -139,6 +139,9 @@ static int mc13xxx_spi_probe(struct spi_device *spi)
 	mc13xxx->irq = spi->irq;
 
 	spi->max_speed_hz = spi->max_speed_hz ? : 26000000;
+	ret = spi_setup(spi);
+	if (ret)
+		return ret;
 
 	mc13xxx->regmap = devm_regmap_init(&spi->dev, &regmap_mc13xxx_bus,
 					   &spi->dev,

Signature · 1 line

1.8.3.2

Lee Jones

Applied

2014-03-03 07:37 UTC #4 lore raw

PATCH 3/4 mfd: mc13xxx: Add missing spi_setup()

Quoted 7 lines

The probe routine should call spi_setup() to configure the SPI bus so it can properly communicate with the device.
Signed-off-by Alexander Shiyan <[email protected]>
---
 drivers/mfd/mc13xxx-spi.c | 3 +++
 1 file changed, 3 insertions(+)

Applied, thanks.

Signature · 4 lines

Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Alexander Shiyan

author

2014-03-02 07:44 UTC #5 lore raw

PATCH 4/4 regulator: mc13xxx: Fix probing with DT

In reply to Alexander Shiyan

The nodes of regulators should be retrieved from parent device. Bug was be introduced by commit (regulator: mc13xxx: Fix NULL pointer error in non-DT mode) in conjuction with (mfd: Revert "mfd: Always assign of_node in mfd_add_device()").

Signed-off-by Alexander Shiyan <[email protected]>

---
 drivers/regulator/mc13xxx-regulator-core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/regulator/mc13xxx-regulator-core.c +4 -4

index bf75fca..05b9717 100644
@@ -167,10 +167,10 @@ int mc13xxx_get_num_regulators_dt(struct platform_device *pdev)
 	struct device_node *parent;
 	int num;
 
-	if (!pdev->dev.of_node)
+	if (!pdev->dev.parent->of_node)
 		return -ENODEV;
 
-	parent = of_get_child_by_name(pdev->dev.of_node, "regulators");
+	parent = of_get_child_by_name(pdev->dev.parent->of_node, "regulators");
 	if (!parent)
 		return -ENODEV;

@@ -189,10 +189,10 @@ struct mc13xxx_regulator_init_data *mc13xxx_parse_regulators_dt(
 	struct device_node *parent, *child;
 	int i, parsed = 0;
 
-	if (!pdev->dev.of_node)
+	if (!pdev->dev.parent->of_node)
 		return NULL;
 
-	parent = of_get_child_by_name(pdev->dev.of_node, "regulators");
+	parent = of_get_child_by_name(pdev->dev.parent->of_node, "regulators");
 	if (!parent)
 		return NULL;

Signature · 1 line

1.8.3.2

Mark Brown

maintainer

2014-03-03 01:58 UTC #6 lore raw

PATCH 4/4 regulator: mc13xxx: Fix probing with DT

Quoted 4 lines · Alexander Shiyan

The nodes of regulators should be retrieved from parent device. Bug was be introduced by commit (regulator: mc13xxx: Fix NULL pointer error in non-DT mode) in conjuction with (mfd: Revert "mfd: Always assign of_node in mfd_add_device()").

Applied, thanks.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140303/1da1bfe4/attachment-0001.sig>

Lee Jones

2014-03-03 07:35 UTC #7 lore raw

PATCH 1/4 mfd: mc13xxx: Honor designed SPI mode

In reply to Alexander Shiyan

Quoted 10 lines

Instead of using hard coded SPI mode, use mode described in devicetree or defined for the board.
Additionally, patch updates all users of this PMIC to use correct SPI mode.
Signed-off-by Alexander Shiyan <[email protected]>
---
 arch/arm/mach-imx/mach-mx31lilly.c | 1 +
 arch/arm/mach-imx/mach-mx31lite.c  | 1 +
 drivers/mfd/mc13xxx-spi.c          | 2 --

There's no reason for these changes to be in the same patch.

Please split them up by subsystem and resubmit.

Signature · 4 lines

Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Alexander Shiyan

author

2014-03-03 07:55 UTC #8 lore raw

???????????, 3 ????? 2014, 15:35 +08:00 ?? Lee Jones <[email protected]>:

Quoted 11 lines

Quoted 10 lines
Instead of using hard coded SPI mode, use mode described in devicetree or defined for the board.
Additionally, patch updates all users of this PMIC to use correct SPI mode.
Signed-off-by Alexander Shiyan <[email protected]>
---
 arch/arm/mach-imx/mach-mx31lilly.c | 1 +
 arch/arm/mach-imx/mach-mx31lite.c  | 1 +
 drivers/mfd/mc13xxx-spi.c          | 2 --

Quoted 3 lines

There's no reason for these changes to be in the same patch.
Please split them up by subsystem and resubmit.

This should be in one patch, since it cause to break functionality of these boards due missing SPI_CS_HIGH property in the spi_board_info.

---

Lee Jones

2014-03-03 09:23 UTC #9 lore raw

PATCH 1/4 mfd: mc13xxx: Honor designed SPI mode

Quoted 12 lines

???????????, 3 ????? 2014, 15:35 +08:00 ?? Lee Jones <[email protected]>:
Quoted 11 lines
Quoted 10 lines
Instead of using hard coded SPI mode, use mode described in devicetree or defined for the board.
Additionally, patch updates all users of this PMIC to use correct SPI mode.
Signed-off-by Alexander Shiyan <[email protected]>
---
 arch/arm/mach-imx/mach-mx31lilly.c | 1 +
 arch/arm/mach-imx/mach-mx31lite.c  | 1 +
 drivers/mfd/mc13xxx-spi.c          | 2 --

Quoted 6 lines

Quoted 3 lines
There's no reason for these changes to be in the same patch.
Please split them up by subsystem and resubmit.
This should be in one patch, since it cause to break functionality of these boards due missing SPI_CS_HIGH property in the spi_board_info.

Okay, I'll take your word for it.

For the MFD part:

Acked-by Lee Jones <[email protected]>

I'm happy to take this through the MFD tree and can provide an immutable branch for the other Maintainers if requested.

Signature · 4 lines

Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Sascha Hauer

maintainer

2014-03-03 09:41 UTC #10 lore raw

PATCH 1/4 mfd: mc13xxx: Honor designed SPI mode

In reply to Alexander Shiyan

Quoted 4 lines · Alexander Shiyan

Instead of using hard coded SPI mode, use mode described in devicetree or defined for the board.
Additionally, patch updates all users of this PMIC to use correct SPI mode.

What's the reason for this change? Without it the driver just does the correct thing, this only adds a chance for boards do get it wrong. This for example happens with the i.MX27 phyCORE in this patch, see happens with arch/arm/boot/dts/imx27-phytec-phycore-som.dts whih doesn't have the spi-cs-high property.

Sascha

Quoted 49 lines

Signed-off-by Alexander Shiyan <[email protected]>

---
 arch/arm/mach-imx/mach-mx31lilly.c | 1 +
 arch/arm/mach-imx/mach-mx31lite.c  | 1 +
 drivers/mfd/mc13xxx-spi.c          | 2 --
 3 files changed, 2 insertions(+), 2 deletions(-)

arch/arm/mach-imx/mach-mx31lilly.c +1 -0

index 832b1e2..16c688e 100644
@@ -231,6 +231,7 @@ static struct spi_board_info mc13783_dev __initdata = {
 	.bus_num	= 1,
 	.chip_select	= 0,
 	.platform_data	= &mc13783_pdata,
+	.mode		= SPI_MODE_0 | SPI_CS_HIGH,
 	/* irq number is run-time assigned */
 };

arch/arm/mach-imx/mach-mx31lite.c +1 -0

index bea0729..bf3e61e 100644
@@ -121,6 +121,7 @@ static struct spi_board_info mc13783_spi_dev __initdata = {
 	.bus_num	= 1,
 	.chip_select    = 0,
 	.platform_data  = &mc13783_pdata,
+	.mode		= SPI_MODE_0 | SPI_CS_HIGH,
 	/* irq number is run-time assigned */
 };

drivers/mfd/mc13xxx-spi.c +0 -2

index 38ab678..f88087f 100644
@@ -136,8 +136,6 @@ static int mc13xxx_spi_probe(struct spi_device *spi)
 
 	dev_set_drvdata(&spi->dev, mc13xxx);
 
-	spi->mode = SPI_MODE_0 | SPI_CS_HIGH;
-
 	mc13xxx->irq = spi->irq;
 
 	mc13xxx->regmap = devm_regmap_init(&spi->dev, &regmap_mc13xxx_bus,

Signature · 1 line

1.8.3.2

Signature · 4 lines

Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Alexander Shiyan

author

2014-03-03 09:51 UTC #11 lore raw

???????????, 3 ????? 2014, 10:41 +01:00 ?? Sascha Hauer <[email protected]>:

Quoted 11 lines · Alexander Shiyan

Quoted 4 lines
Instead of using hard coded SPI mode, use mode described in devicetree or defined for the board.
Additionally, patch updates all users of this PMIC to use correct SPI mode.
What's the reason for this change? Without it the driver just does the correct thing, this only adds a chance for boards do get it wrong. This for example happens with the i.MX27 phyCORE in this patch, see happens with arch/arm/boot/dts/imx27-phytec-phycore-som.dts whih doesn't have the spi-cs-high property.

How specify proper functionality if chipselect connected through inverter?

For i.MX27 phyCORE:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/arch/arm/boot/dts/imx27-phytec-phycore-som.dtsi?id=986cc4920412d8a7b5c9dba85601651f6373f45e

---

Sascha Hauer

maintainer

2014-03-04 20:13 UTC #12 lore raw

PATCH 1/4 mfd: mc13xxx: Honor designed SPI mode

Quoted 7 lines · Alexander Shiyan

???????????, 3 ????? 2014, 10:41 +01:00 ?? Sascha Hauer <[email protected]>:
Quoted 6 lines · Alexander Shiyan
Quoted 4 lines
Instead of using hard coded SPI mode, use mode described in devicetree or defined for the board.
Additionally, patch updates all users of this PMIC to use correct SPI mode.

Quoted 7 lines

Quoted 5 lines
What's the reason for this change? Without it the driver just does the correct thing, this only adds a chance for boards do get it wrong. This for example happens with the i.MX27 phyCORE in this patch, see happens with arch/arm/boot/dts/imx27-phytec-phycore-som.dts whih doesn't have the spi-cs-high property.
How specify proper functionality if chipselect connected through inverter?

Is there such a hardware around?

Quoted 3 lines

For i.MX27 phyCORE:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/arch/arm/boot/dts/imx27-phytec-phycore-som.dtsi?id=986cc4920412d8a7b5c9dba85601651f6373f45e

Ok, I missed that one.

Sascha

Signature · 4 lines

Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Alexander Shiyan

author

2014-03-04 20:51 UTC #13 lore raw

???????, 4 ????? 2014, 21:13 +01:00 ?? Sascha Hauer <[email protected]>:

Quoted 14 lines · Alexander Shiyan

Quoted 13 lines
???????????, 3 ????? 2014, 10:41 +01:00 ?? Sascha Hauer <[email protected]>:
Quoted 11 lines · Alexander Shiyan
Quoted 4 lines
Instead of using hard coded SPI mode, use mode described in devicetree or defined for the board.
Additionally, patch updates all users of this PMIC to use correct SPI mode.
What's the reason for this change? Without it the driver just does the correct thing, this only adds a chance for boards do get it wrong. This for example happens with the i.MX27 phyCORE in this patch, see happens with arch/arm/boot/dts/imx27-phytec-phycore-som.dts whih doesn't have the spi-cs-high property.

Quoted 3 lines

Quoted 1 line
How specify proper functionality if chipselect connected through inverter?
Is there such a hardware around?

AFAIK no, but it is a protection for the future. Since this driver handle several PMICs, we must anticipate the addition of new types of ICs and the emergence of new devices using these PMICs, which could use external logic for chipselect. Instead use tricks, use proper definitions (or DT description), from my point of view, better.

Quoted 4 lines

Quoted 2 lines
For i.MX27 phyCORE:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/arch/arm/boot/dts/imx27-phytec-phycore-som.dtsi?id=986cc4920412d8a7b5c9dba85601651f6373f45e
Ok, I missed that one.

---

Sascha Hauer

maintainer

2014-03-05 11:57 UTC #14 lore raw

PATCH 1/4 mfd: mc13xxx: Honor designed SPI mode

Quoted 17 lines · Alexander Shiyan

???????, 4 ????? 2014, 21:13 +01:00 ?? Sascha Hauer <[email protected]>:
Quoted 16 lines · Alexander Shiyan
Quoted 14 lines
???????????, 3 ????? 2014, 10:41 +01:00 ?? Sascha Hauer <[email protected]>:
Quoted 11 lines · Alexander Shiyan
Quoted 4 lines
Instead of using hard coded SPI mode, use mode described in devicetree or defined for the board.
Additionally, patch updates all users of this PMIC to use correct SPI mode.
What's the reason for this change? Without it the driver just does the correct thing, this only adds a chance for boards do get it wrong. This for example happens with the i.MX27 phyCORE in this patch, see happens with arch/arm/boot/dts/imx27-phytec-phycore-som.dts whih doesn't have the spi-cs-high property.
How specify proper functionality if chipselect connected through inverter?

Quoted 7 lines

Quoted 1 line
Is there such a hardware around?
AFAIK no, but it is a protection for the future. Since this driver handle several PMICs, we must anticipate the addition of new types of ICs and the emergence of new devices using these PMICs, which could use external logic for chipselect. Instead use tricks, use proper definitions (or DT description), from my point of view, better.

In that case rather than breaking devicetree compatibility we really should leave this problem to someone who actually has this problem. He could introduce an explicit spi-cs-low property to overwrite the driver preference.

Sascha

Signature · 4 lines

Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |