On Tue, 3 Apr 2018, Christoph Hellwig wrote:
> > +static void zorro_esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
> > + u32 dma_count, int write, u8 cmd)
> > +{
> > + struct zorro_esp_priv *zep = dev_get_drvdata(esp->dev);
> > + u8 __iomem *fifo = esp->regs + ESP_FDATA * 16;
> > + u8 phase = esp->sreg & ESP_STAT_PMASK;
> > +
> > + cmd &= ~ESP_CMD_DMA;
> > +
> > + if (write) {
>
> It seems like this routine would benefit from being split into a read
> and a write one.
>
I'd prefer to leave this unchanged, for a couple of reasons.
The main reason is that zorro_esp_send_pio_cmd() is supposed to be a copy
of mac_esp_send_pio_cmd(). Identical code is easy to refactor in order to
eliminate the duplication, whereas small variations complicate review.
I've already started work on patches to resolve the duplicated code.
That effort is complicated by the question that Michael has raised about
the use of certain interrupt flags in the shared code. So I didn't simply
put a refactoring patch before this one. Merging Michael's driver first
seemed to make it easier for us to collaborate on this shared code.
Also, the calling convention here is just the send_dma_cmd method from
struct esp_driver_ops. The change you suggest would lead to this,
static void zorro_esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
u32 dma_count, int write, u8 cmd)
{
if (write)
zorro_esp_send_pio_cmd_write(esp, addr, esp_count, dma_count,
cmd);
else
zorro_esp_send_pio_cmd_read(esp, addr, esp_count, dma_count,
cmd);
}
The reader who is trying to understand the call chain starting from the
core driver learns nothing from this routine. Instead they have to jump
through another level of indirection to find the actual implementation.
Given that some esp drivers have multiple implementations of this
send_dma_cmd method, your version would have three static functions where
one is sufficient. Drivers like mac_esp that have two implementations of
the send_dma_cmd method would end up with six functions instead of two.
--
