Patchwork [1/4] mfd: mc13xxx: Honor designed SPI mode

login
register
mail settings
Submitter Alexander Shiyan
Date March 2, 2014, 7:44 a.m.
Message ID <1393746275-9253-1-git-send-email-shc_work@mail.ru>
Download mbox | patch
Permalink /patch/325542/
State New
Headers show

Comments

Alexander Shiyan - March 2, 2014, 7:44 a.m.
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 <shc_work@mail.ru>
---
 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(-)
Mark Brown - March 3, 2014, 1:58 a.m.
On Sun, Mar 02, 2014 at 11:44:35AM +0400, Alexander Shiyan wrote:
> 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.
Lee Jones - March 3, 2014, 7:35 a.m.
> 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 <shc_work@mail.ru>
> ---
>  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.
Lee Jones - March 3, 2014, 7:36 a.m.
> 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 <shc_work@mail.ru>
> ---
>  drivers/mfd/mc13xxx-spi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/mfd/mc13xxx-spi.c b/drivers/mfd/mc13xxx-spi.c
> index f88087f..cd27c32 100644
> --- a/drivers/mfd/mc13xxx-spi.c
> +++ b/drivers/mfd/mc13xxx-spi.c
> @@ -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.
Lee Jones - March 3, 2014, 7:37 a.m.
> 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 <shc_work@mail.ru>
> ---
>  drivers/mfd/mc13xxx-spi.c | 3 +++
>  1 file changed, 3 insertions(+)

Applied, thanks.
Alexander Shiyan - March 3, 2014, 7:55 a.m.
Понедельник,  3 марта 2014, 15:35 +08:00 от Lee Jones <lee.jones@linaro.org>:
> > 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 <shc_work@mail.ru>
> > ---
> >  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.

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 - March 3, 2014, 9:23 a.m.
> Понедельник,  3 марта 2014, 15:35 +08:00 от Lee Jones <lee.jones@linaro.org>:
> > > 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 <shc_work@mail.ru>
> > > ---
> > >  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.
> 
> 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 <lee.jones@linaro.org>

I'm happy to take this through the MFD tree and can provide an
immutable branch for the other Maintainers if requested.
Sascha Hauer - March 3, 2014, 9:41 a.m.
On Sun, Mar 02, 2014 at 11:44:32AM +0400, Alexander Shiyan wrote:
> 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

> 
> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> ---
>  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(-)
> 
> diff --git a/arch/arm/mach-imx/mach-mx31lilly.c b/arch/arm/mach-imx/mach-mx31lilly.c
> index 832b1e2..16c688e 100644
> --- a/arch/arm/mach-imx/mach-mx31lilly.c
> +++ b/arch/arm/mach-imx/mach-mx31lilly.c
> @@ -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 */
>  };
>  
> diff --git a/arch/arm/mach-imx/mach-mx31lite.c b/arch/arm/mach-imx/mach-mx31lite.c
> index bea0729..bf3e61e 100644
> --- a/arch/arm/mach-imx/mach-mx31lite.c
> +++ b/arch/arm/mach-imx/mach-mx31lite.c
> @@ -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 */
>  };
>  
> diff --git a/drivers/mfd/mc13xxx-spi.c b/drivers/mfd/mc13xxx-spi.c
> index 38ab678..f88087f 100644
> --- a/drivers/mfd/mc13xxx-spi.c
> +++ b/drivers/mfd/mc13xxx-spi.c
> @@ -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,
> -- 
> 1.8.3.2
> 
>
Alexander Shiyan - March 3, 2014, 9:51 a.m.
Понедельник,  3 марта 2014, 10:41 +01:00 от Sascha Hauer <s.hauer@pengutronix.de>:
> On Sun, Mar 02, 2014 at 11:44:32AM +0400, Alexander Shiyan wrote:
> > 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 - March 4, 2014, 8:13 p.m.
On Mon, Mar 03, 2014 at 01:51:05PM +0400, Alexander Shiyan wrote:
> Понедельник,  3 марта 2014, 10:41 +01:00 от Sascha Hauer <s.hauer@pengutronix.de>:
> > On Sun, Mar 02, 2014 at 11:44:32AM +0400, Alexander Shiyan wrote:
> > > 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?

Is there such a hardware around?

> 
> 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
Alexander Shiyan - March 4, 2014, 8:51 p.m.
Вторник,  4 марта 2014, 21:13 +01:00 от Sascha Hauer <s.hauer@pengutronix.de>:
> On Mon, Mar 03, 2014 at 01:51:05PM +0400, Alexander Shiyan wrote:
> > Понедельник,  3 марта 2014, 10:41 +01:00 от Sascha Hauer <s.hauer@pengutronix.de>:
> > > On Sun, Mar 02, 2014 at 11:44:32AM +0400, Alexander Shiyan wrote:
> > > > 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?
> 
> 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.

> > 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 - March 5, 2014, 11:57 a.m.
On Wed, Mar 05, 2014 at 12:51:05AM +0400, Alexander Shiyan wrote:
> Вторник,  4 марта 2014, 21:13 +01:00 от Sascha Hauer <s.hauer@pengutronix.de>:
> > On Mon, Mar 03, 2014 at 01:51:05PM +0400, Alexander Shiyan wrote:
> > > Понедельник,  3 марта 2014, 10:41 +01:00 от Sascha Hauer <s.hauer@pengutronix.de>:
> > > > On Sun, Mar 02, 2014 at 11:44:32AM +0400, Alexander Shiyan wrote:
> > > > > 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?
> > 
> > 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

Patch

diff --git a/arch/arm/mach-imx/mach-mx31lilly.c b/arch/arm/mach-imx/mach-mx31lilly.c
index 832b1e2..16c688e 100644
--- a/arch/arm/mach-imx/mach-mx31lilly.c
+++ b/arch/arm/mach-imx/mach-mx31lilly.c
@@ -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 */
 };
 
diff --git a/arch/arm/mach-imx/mach-mx31lite.c b/arch/arm/mach-imx/mach-mx31lite.c
index bea0729..bf3e61e 100644
--- a/arch/arm/mach-imx/mach-mx31lite.c
+++ b/arch/arm/mach-imx/mach-mx31lite.c
@@ -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 */
 };
 
diff --git a/drivers/mfd/mc13xxx-spi.c b/drivers/mfd/mc13xxx-spi.c
index 38ab678..f88087f 100644
--- a/drivers/mfd/mc13xxx-spi.c
+++ b/drivers/mfd/mc13xxx-spi.c
@@ -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,