On Wed, Jun 3, 2009 at 05:46, <[email protected]> wrote:
> +/* spi data irq handler */
> +static irqreturn_t bfin_spi_pio_irq_handler(int irq, void *dev_id)
> +{
> + struct driver_data *drv_data = dev_id;
> + struct chip_data *chip = drv_data->cur_chip;
> + struct spi_message *msg = drv_data->cur_msg;
> + int n_bytes = drv_data->n_bytes;
> +
> + /* wait until transfer finished. */
> + while (!(read_STAT(drv_data) & BIT_STAT_RXS))
> + cpu_relax();
since TIMOD is passed in through the board resources' ctl_reg, we
should really update the check in bfin_spi_setup(). otherwise, people
who set ctl_reg to 0x1 will probably end up with a system that
randomly hangs for no apparent reason.
at any rate, do we really need this polling here ? i thought RXS was
set and thus triggered the interrupt, so it isnt really possible to
get an interrupt without the RXS bit set ?
> + if (n_bytes == 2)
> + *(u16 *) (drv_data->rx) = read_RDBR(drv_data);
> + else if (n_bytes == 1)
> + *(u8 *) (drv_data->rx) = read_RDBR(drv_data);
considering we filter n_bytes explicitly, there is no need to check
for the n_bytes == 1 as it just adds overhead at runtime
maybe even better if written as the following since 8bit usage is much
more common:
if (likely(n_bytes == 1))
*(u8 *) (drv_data->rx) = read_RDBR(drv_data);
else
*(u16 *) (drv_data->rx) = read_RDBR(drv_data);
> + if (drv_data->rx && drv_data->tx) {
> + /* duplex */
> + dev_dbg(&drv_data->pdev->dev, "duplex: write_TDBR\n");
> + if (drv_data->n_bytes == 2) {
> + *(u16 *) (drv_data->rx) = read_RDBR(drv_data);
> + write_TDBR(drv_data, (*(u16 *) (drv_data->tx)));
> + } else if (drv_data->n_bytes == 1) {
> + *(u8 *) (drv_data->rx) = read_RDBR(drv_data);
> + write_TDBR(drv_data, (*(u8 *) (drv_data->tx)));
> + }
> + } else if (drv_data->rx) {
> + /* read */
> + dev_dbg(&drv_data->pdev->dev, "read: write_TDBR\n");
> + if (drv_data->n_bytes == 2)
> + *(u16 *) (drv_data->rx) = read_RDBR(drv_data);
> + else if (drv_data->n_bytes == 1)
> + *(u8 *) (drv_data->rx) = read_RDBR(drv_data);
> + write_TDBR(drv_data, chip->idle_tx_val);
> + } else if (drv_data->tx) {
> + /* write */
> + dev_dbg(&drv_data->pdev->dev, "write: write_TDBR\n");
> + bfin_spi_dummy_read(drv_data);
> + if (drv_data->n_bytes == 2)
> + write_TDBR(drv_data, (*(u16 *) (drv_data->tx)));
> + else if (drv_data->n_bytes == 1)
> + write_TDBR(drv_data, (*(u8 *) (drv_data->tx)));
> + }
> + if (drv_data->tx)
> + drv_data->tx += n_bytes;
> + if (drv_data->rx)
> + drv_data->rx += n_bytes;
looks like a lot of unnecessary logic. considering we have to always
read RDBR and write TDBR, why not do something like:
u16 rdbr = read_RDBR(drv_data);
u16 tdbr = chip->idle_tx_val;
if (drv_data->rx) {
if (likely(n_bytes == 1))
*(u8 *)drv_data->rx = rdbr;
else
*(u16 *)drv_data->rx = rdbr;
drv_data->rx += n_bytes;
}
if (drv_data->tx) {
if (likely(n_bytes == 1))
tdbr = *(u8 *)drv_data->tx;
else
tdbr = *(u16 *)drv_data->tx;
drv_data->tx += n_bytes;
}
write_TDBR(drv_data, tdbr);
-mike
_______________________________________________
Linux-kernel-commits mailing list
[email protected]
https://blackfin.uclinux.org/mailman/listinfo/linux-kernel-commits
