[PATCH] spi/mpc52xx: check for invalid PSC usage

[PATCH] spi/mpc52xx: check for invalid PSC usage

[Wolfram Sang]

Add checks to allow only PSCs capable of SPI.
Also turn printk to dev_err while we are here.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: David Brownell <dbrownell@users.sourceforge.net>
---

Grant, if the patch is OK, maybe it can also go via your tree?

 drivers/spi/mpc52xx_psc_spi.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/mpc52xx_psc_spi.c b/drivers/spi/mpc52xx_psc_spi.c
index 1b74d5c..1d11a6d 100644
--- a/drivers/spi/mpc52xx_psc_spi.c
+++ b/drivers/spi/mpc52xx_psc_spi.c
@@ -467,19 +467,18 @@ static int __init mpc52xx_psc_spi_of_probe(struct of_device *op,
 
 	regaddr_p = of_get_address(op->node, 0, &size64, NULL);
 	if (!regaddr_p) {
-		printk(KERN_ERR "Invalid PSC address\n");
+		dev_err(&op->dev, "Invalid PSC address\n");
 		return -EINVAL;
 	}
 	regaddr64 = of_translate_address(op->node, regaddr_p);
 
-	/* get PSC id (1..6, used by port_config) */
+	/* get PSC id (only 1,2,3,6 can do SPI) */
 	if (op->dev.platform_data == NULL) {
 		const u32 *psc_nump;
 
 		psc_nump = of_get_property(op->node, "cell-index", NULL);
-		if (!psc_nump || *psc_nump > 5) {
-			printk(KERN_ERR "mpc52xx_psc_spi: Device node %s has invalid "
-					"cell-index property\n", op->node->full_name);
+		if (!psc_nump || (*psc_nump > 2 && *psc_nump != 5)) {
+			dev_err(&op->dev, "PSC can't do SPI\n");
 			return -EINVAL;
 		}
 		id = *psc_nump + 1;
-- 
1.6.3.3

Re: [PATCH] spi/mpc52xx: check for invalid PSC usage

[Grant Likely]

On Fri, Oct 23, 2009 at 5:25 AM, Wolfram Sang <w.sang@pengutronix.de> wrote=
:
> Add checks to allow only PSCs capable of SPI.
> Also turn printk to dev_err while we are here.

Hi Wolfram,

I wouldn't even bother.  It's not actively dangerous to try and use
PSC{4,5} in SPI mode.  It just not going to work.  Besides, the
MPC5200 common code already checks for an invalid PSC number when
setting the clock divisor.

Have you seen cases of users trying to do the wrong thing with the
crippled PSCs?

g.

>
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: David Brownell <dbrownell@users.sourceforge.net>
> ---
>
> Grant, if the patch is OK, maybe it can also go via your tree?
>
> =A0drivers/spi/mpc52xx_psc_spi.c | =A0 =A09 ++++-----
> =A01 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/spi/mpc52xx_psc_spi.c b/drivers/spi/mpc52xx_psc_spi.=
c
> index 1b74d5c..1d11a6d 100644
> --- a/drivers/spi/mpc52xx_psc_spi.c
> +++ b/drivers/spi/mpc52xx_psc_spi.c
> @@ -467,19 +467,18 @@ static int __init mpc52xx_psc_spi_of_probe(struct o=
f_device *op,
>
> =A0 =A0 =A0 =A0regaddr_p =3D of_get_address(op->node, 0, &size64, NULL);
> =A0 =A0 =A0 =A0if (!regaddr_p) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 printk(KERN_ERR "Invalid PSC address\n");
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&op->dev, "Invalid PSC address\n");
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -EINVAL;
> =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0regaddr64 =3D of_translate_address(op->node, regaddr_p);
>
> - =A0 =A0 =A0 /* get PSC id (1..6, used by port_config) */
> + =A0 =A0 =A0 /* get PSC id (only 1,2,3,6 can do SPI) */
> =A0 =A0 =A0 =A0if (op->dev.platform_data =3D=3D NULL) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0const u32 *psc_nump;
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0psc_nump =3D of_get_property(op->node, "ce=
ll-index", NULL);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!psc_nump || *psc_nump > 5) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 printk(KERN_ERR "mpc52xx_ps=
c_spi: Device node %s has invalid "
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 "cell-index property\n", op->node->full_name);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!psc_nump || (*psc_nump > 2 && *psc_num=
p !=3D 5)) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&op->dev, "PSC can'=
t do SPI\n");
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -EINVAL;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0id =3D *psc_nump + 1;
> --
> 1.6.3.3
>
>



--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] spi/mpc52xx: check for invalid PSC usage

[Wolfram Sang]

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

> I wouldn't even bother.  It's not actively dangerous to try and use
> PSC{4,5} in SPI mode.  It just not going to work.  Besides, the
> MPC5200 common code already checks for an invalid PSC number when
> setting the clock divisor.
> 
> Have you seen cases of users trying to do the wrong thing with the
> crippled PSCs?

Yes, that was the reason for this patch :) How about this patch to give users a
better idea than just -ENODEV via set_psc_clkdiv? ...[/me hacks]...
Uuuh, there is even a bug which makes the case go unnoticed.

===

Subject: [PATCH] spi/mpc52xx-psc-spi: check for valid PSC

This driver calls mpc52xx_set_psc_clkdiv() but doesn't check its return value
to see if the PSC is actually valid for SPI use. Add the check and a hint for
the user.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Grant Likely <grant.likely@secretlab.ca>
---
 drivers/spi/mpc52xx_psc_spi.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/mpc52xx_psc_spi.c b/drivers/spi/mpc52xx_psc_spi.c
index 1b74d5c..e268437 100644
--- a/drivers/spi/mpc52xx_psc_spi.c
+++ b/drivers/spi/mpc52xx_psc_spi.c
@@ -313,11 +313,13 @@ static int mpc52xx_psc_spi_port_config(int psc_id, struct mpc52xx_psc_spi *mps)
 	struct mpc52xx_psc __iomem *psc = mps->psc;
 	struct mpc52xx_psc_fifo __iomem *fifo = mps->fifo;
 	u32 mclken_div;
-	int ret = 0;
+	int ret;
 
 	/* default sysclk is 512MHz */
 	mclken_div = (mps->sysclk ? mps->sysclk : 512000000) / MCLK;
-	mpc52xx_set_psc_clkdiv(psc_id, mclken_div);
+	ret = mpc52xx_set_psc_clkdiv(psc_id, mclken_div);
+	if (ret)
+		return ret;
 
 	/* Reset the PSC into a known state */
 	out_8(&psc->command, MPC52xx_PSC_RST_RX);
@@ -341,7 +343,7 @@ static int mpc52xx_psc_spi_port_config(int psc_id, struct mpc52xx_psc_spi *mps)
 
 	mps->bits_per_word = 8;
 
-	return ret;
+	return 0;
 }
 
 static irqreturn_t mpc52xx_psc_spi_isr(int irq, void *dev_id)
@@ -410,8 +412,10 @@ static int __init mpc52xx_psc_spi_do_probe(struct device *dev, u32 regaddr,
 		goto free_master;
 
 	ret = mpc52xx_psc_spi_port_config(master->bus_num, mps);
-	if (ret < 0)
+	if (ret < 0) {
+		dev_err(dev, "can't configure PSC! Is it capable of SPI?\n");
 		goto free_irq;
+	}
 
 	spin_lock_init(&mps->lock);
 	init_completion(&mps->done);

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] spi/mpc52xx: check for invalid PSC usage

[Grant Likely]

On Mon, Nov 2, 2009 at 6:53 AM, Wolfram Sang <w.sang@pengutronix.de> wrote:
>> I wouldn't even bother. =A0It's not actively dangerous to try and use
>> PSC{4,5} in SPI mode. =A0It just not going to work. =A0Besides, the
>> MPC5200 common code already checks for an invalid PSC number when
>> setting the clock divisor.
>>
>> Have you seen cases of users trying to do the wrong thing with the
>> crippled PSCs?
>
> Yes, that was the reason for this patch :) How about this patch to give u=
sers a
> better idea than just -ENODEV via set_psc_clkdiv? ...[/me hacks]...
> Uuuh, there is even a bug which makes the case go unnoticed.

Sure.  This looks okay to me.  I'll pick it up.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.