| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20091104183507.GH3607@sortiz.org>
Date: Wed, 4 Nov 2009 19:35:08 +0100
From: Samuel Ortiz <sameo@...ux.intel.com>
To: Uwe Kleine-König
<u.kleine-koenig@...gutronix.de>
Cc: linux-kernel@...r.kernel.org,
Sascha Hauer <s.hauer@...gutronix.de>,
Mark Brown <broonie@...nsource.wolfsonmicro.com>
Subject: Re: [PATCH] mfd/mc13783: near complete rewrite
Hi Uwe,
On Tue, Nov 03, 2009 at 08:31:13PM +0100, Uwe Kleine-König wrote:
> This fixes several things while still providing the old API:
>
> - simplify and fix locking
> - better error handling
> - don't ack all irqs making it impossible to detect a reset of the
> rtc
> - use a timeout variant to wait for completion of ADC conversion
> - provide platform-data to regulator subdevice (This allows making
> struct mc13783 opaque for other drivers after the regulator driver is
> updated to use its platform_data.)
> - expose all interrupts
> - use threaded irq
>
> After all users in mainline are converted to the new API, some things
> (e.g. mc13783-private.h) can go away.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
> Cc: Sascha Hauer <s.hauer@...gutronix.de>
> Cc: Mark Brown <broonie@...nsource.wolfsonmicro.com>
> Cc: Samuel Ortiz <sameo@...ux.intel.com>
> ---
> Hello,
>
> compared to the first submission I squashed in the patch
>
> mfd/mc13783: change type of irq handlers to irq_handler_t
>
> sent earlier in that thread and fixed a few whitespace issues reported
> by checkpatch.pl.
>
> I'd be happy if this patch would make it in now.
The patch looks mostly ok, thanks for this work.
I have a few comments though.
> - * Copyright 2009 Pengutronix, Sascha Hauer <s.hauer@...gutronix.de>
Even though this looks like a major rewrite, I still think it's unfair to
remove Sascha from there.
> +void mc13783_lock(struct mc13783 *mc13783)
> +{
> + if (!mutex_trylock(&mc13783->lock)) {
> + dev_dbg(&mc13783->spidev->dev, "wait for %s from %pf\n",
> + __func__, __builtin_return_address(0));
> +
> + mutex_lock(&mc13783->lock);
That is just for debugging purposes, right ?
> +static int mc13783_prep_read_transfer(struct mc13783 *mc13783,
> + struct spi_transfer *t, u32 *buf,
> + unsigned int offset, u32 *val
What is val used for in that function ?
)
> +{
> + if (offset > MC13783_NUMREGS)
> return -EINVAL;
> - return len - m.actual_length;
> +
> + buf[0] = offset << 25;
Could we have a define for that 25 ?
> + memset(t, 0, sizeof(*t));
> +
> + t->tx_buf = buf;
> + t->rx_buf = buf;
> + t->len = sizeof(u32);
> +
> + return 1;
> }
>
> -static int mc13783_read(struct mc13783 *mc13783, int reg_num, u32 *reg_val)
> +static int mc13783_eval_read_transfer(struct mc13783 *mc13783,
> + struct spi_transfer *t, u32 *buf,
> + unsigned int offset, u32 *val)
> {
> - unsigned int frame = 0;
> - int ret = 0;
> + BUG_ON(t->tx_buf != buf || t->rx_buf != buf);
your SPI read will be on t->rx_buf. I could understand that you want to check
for t->rx_buf not being NULL (although a BUG_ON() seems too much here), but
checking for t->rx_buf pointing to buf really looks akward to me.
why not:
BUG_ON(t->rx_buf == NULL)
*val = *((u32 *)t->rx_buf) & 0xffffff;
> -static int mc13783_write(struct mc13783 *mc13783, int reg_num, u32 reg_val)
> +static int mc13783_eval_write_transfer(struct mc13783 *mc13783,
> + struct spi_transfer *t, u32 *buf,
> + unsigned int offset, u32 val)
> {
> - unsigned int frame = 0;
> + BUG_ON(t->tx_buf != buf || t->rx_buf != buf);
>
> - if (reg_num > MC13783_MAX_REG_NUM)
> - return -EINVAL;
> + return 1;
> +}
I dont get the point of mc13783_eval_write_transfer().
> +int mc13783_reg_read(struct mc13783 *mc13783, unsigned int offset, u32 *val)
> +{
> + u32 buf;
> + struct spi_transfer t;
> + struct spi_message m;
> + int ret;
> +
> + BUG_ON(!mutex_is_locked(&mc13783->lock));
> +
> + ret = mc13783_prep_read_transfer(mc13783, &t, &buf, offset, val);
Do you really need buf here ?
I think mc13783_prep_read_transfer(mc13783, &t, val, offset); should be
enough.
> + if (ret < 0)
> + return ret;
> +
> + spi_message_init(&m);
> + spi_message_add_tail(&t, &m);
> +
> + ret = spi_sync(mc13783->spidev, &m);
>
> - frame |= (1 << MC13783_WRITE_BIT_SHIFT);
> - frame |= reg_num << MC13783_REG_NUM_SHIFT;
> - frame |= reg_val & MC13783_FRAME_MASK;
> + /* error in message.status implies error return from spi_sync */
> + BUG_ON(!ret && m.status);
So, you really want to crash your board because of an SPI inconsistency ?
Seems like an overkill to me.
> - return spi_rw(mc13783->spi_device, (u8 *)&frame, 4);
> + if (ret)
> + return ret;
> +
> + ret = mc13783_eval_read_transfer(mc13783, &t, &buf, offset, val);
> +
> + dev_vdbg(&mc13783->spidev->dev, "[0x%02x] -> 0x%06x\n", offset, *val);
> +
> + return ret < 0 ? ret : 0;
> }
> +EXPORT_SYMBOL(mc13783_reg_read);
>
> -int mc13783_reg_read(struct mc13783 *mc13783, int reg_num, u32 *reg_val)
> +int mc13783_reg_write(struct mc13783 *mc13783, unsigned int offset, u32 val)
> {
> + u32 buf;
> + struct spi_transfer t;
> + struct spi_message m;
> int ret;
>
> - mutex_lock(&mc13783->io_lock);
> - ret = mc13783_read(mc13783, reg_num, reg_val);
> - mutex_unlock(&mc13783->io_lock);
> + BUG_ON(!mutex_is_locked(&mc13783->lock));
>
> - return ret;
> + dev_vdbg(&mc13783->spidev->dev, "[0x%02x] <- 0x%06x\n", offset, val);
> +
> + ret = mc13783_prep_write_transfer(mc13783, &t, &buf, offset, val);
> +
> + if (ret < 0)
> + return ret;
> +
> + spi_message_init(&m);
> + spi_message_add_tail(&t, &m);
> +
> + ret = spi_sync(mc13783->spidev, &m);
> +
> + BUG_ON(!ret && m.status);
> +
> + if (ret)
> + return ret;
> +
> + ret = mc13783_eval_write_transfer(mc13783, &t, &buf, offset, val);
Again, I dont see the point of this function.
The rest of the code looks fine to me.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists