Message-ID: <20080920130006.69d50cbb@mjolnir.drzeus.cx>
Date:	Sat, 20 Sep 2008 13:00:06 +0200
From:	Pierre Ossman <drzeus-mmc@...eus.cx>
To:	Michał Mirosław <mirq-linux@...e.qmqm.pl>
Cc:	linux-kernel@...r.kernel.org, Alex Dubov <oakad@...oo.com>
Subject: Re: RFC: Driver for CB710/720 memory card reader (MMC part)

On Sat, 13 Sep 2008 01:43:02 +0200
Michał Mirosław <mirq-linux@...e.qmqm.pl> wrote:

> Hello again,
> 
> Here is next version of CB710 MMC-host driver. Since I don't like
> copying code I used platform device "bus" instead of duplicating
> tifm_core and modifying it to cb710 specifics. Debugging printk()s
> were retained for now.
> 
> Please comment,
> Michał Mirosław
> 
> diff -urN empty/cb710.h cb710-pre-20080913/cb710.h
> --- empty/cb710.h	1970-01-01 01:00:00.000000000 +0100
> +++ cb710-pre-20080913/cb710.h	2008-09-13 00:30:51.000000000 +0200
> @@ -0,0 +1,186 @@
> +/*
> + *  cb710/cb710.h
> + *
> + *  Copyleft by Michał Mirosław, 2008
> + *

You should probably stick with "Copyright" even though it is a free
software license.

> +
> +#define RDPORT(t, p) \
> +	ioread##t(chip->iobase + (p))
> +#define WRPORT(t, p, v) \
> +	do { iowrite##t((v), chip->iobase + (p)); \
> +		(void)ioread8(chip->iobase + 0x13); } while (0)
> +#define UPDPORT(t, p, v, m) \
> +	do { \
> +		iowrite##t( \
> +			(ioread##t(chip->iobase + (p)) & ~(m)) | (v), \
> +			chip->iobase + (p)); \
> +		(void)ioread8(chip->iobase + 0x13); \
> +	} while (0)
> +#define S_RDPORT(t, p, b, c) \
> +	ioread##t##_rep(chip->iobase + (p), (b), (c))
> +#define S_WRPORT(t, p, b, c) \
> +	do { \
> +		iowrite##t##_rep(chip->iobase + (p), (b), (c)); \
> +		(void)ioread8(chip->iobase + 0x13); \
> +	} while (0)
> +

This is a pretty bad case of obfuscation. Don't construct macros that
reference local variables (chip in this case). Also,
iowrite16_rep(chip->iobase + CB700_FOO, buffer, 12); is quite readable
as it is. Using standard kernel functions allows your code to be more
accessible to other kernel hackers.

> +
> +void __cb710_pci_update_config_reg(struct pci_dev *pdev,
> +	int reg, uint32_t mask, uint32_t xor);
> +#define cb710_pci_update_config_reg(d, r, m, x) \
> +	__cb710_pci_update_config_reg((d), (r), ~(m), (x))

Again, this just obfuscates. If you keep the ~ in calling code it
becomes more obvious that it is a mask.

> +/* sg-to-PIO buffer */
> +#define CB710_SG_BUFFER_BLOCK	16	/* power of two */
> +struct cb710_sg_chain {
> +	uint8_t bounce_buffer[CB710_SG_BUFFER_BLOCK];
> +	struct scatterlist *sg;
> +	unsigned int sg_num;
> +	struct page *page;
> +	void *mapped_page;
> +	size_t cur_offset;
> +	size_t need_advance;
> +	unsigned page_no;
> +	unsigned page_offset;
> +	unsigned page_left;
> +	unsigned need_bounce:1;
> +	unsigned use_bounce:1;
> +};
> +
> +void cb710_sg_init(struct cb710_sg_chain *buf,
> +	struct scatterlist *sg, size_t nelem);
> +int cb710_sg_next_buf(struct cb710_sg_chain *buf,
> +	void **dataptr, size_t *len, int to_sg);
> +void cb710_sg_abort(struct cb710_sg_chain *buf, int to_sg);
> +
> +#define cb710_sg_read_next(b, d, l) \
> +	cb710_sg_next_buf((b), (d), (l), 0)
> +#define cb710_sg_write_next(b, d, l) \
> +	cb710_sg_next_buf((b), (d), (l), 1)
> +#define cb710_sg_abort_read(b) \
> +	cb710_sg_abort((b), 0)
> +#define cb710_sg_abort_write(b) \
> +	cb710_sg_abort((b), 1)
> +

Why this complex system? Can't you use the handlers the kernel already
provides? You also get a lot of special handling with those, e.g.
highmem.

> +void cb710_dump_regs(struct cb710_chip *chip, unsigned select)
> +{
> +	const unsigned allow[8] = {
> +		0xFFF0, 0xFFFF, 0xFFFF, 0xFFFF,
> +		0xFFF0, 0xFFFF, 0xFFFF, 0xFFFF,
> +	};
> +	const char *const prefix[sizeof(allow)/sizeof(*allow)] = {
> +		"MMC", "MMC", "MMC", "MMC",
> +		"MS?", "MS?", "SM?", "SM?"
> +	};
> +	u32 regs[sizeof(allow)/sizeof(*allow) << 2];
> +
> +	int i, j;
> +	char msg[100], *p;
> +
> +	if (!select)
> +		select = 0xFF;
> +	if (!(select & 0x700))
> +		select |= 0x100;
> +
> +#define reg(b, i) \
> +	(((u##b*)regs)[(i) / (b/8)])
> +#define allowed(b, i, j) \
> +	(((allow[i >> 4] >> j) & ((1 << b/8)-1)) == ((1 << b/8)-1))
> +#define dumpregs(b, f, x) { \

This thing is a bit big. It'll be more readable if you turn it into a
function.

> +
> +static irqreturn_t cb710_irq_handler(int irq, void *data)
> +{
> +	struct cb710_chip *chip = data;
> +	irqreturn_t handled = IRQ_NONE;
> +	unsigned flags;
> +	int nr;
> +
> +	spin_lock_irqsave(&chip->irq_lock, flags);

You shouldn't be using the interrupt versions inside the interrupt
handler. Just do a normal spin_lock().


> +
> +	pci_read_config_dword(pdev, 0x48, &val);
> +	if (!(val & 0x80000000)) {
> +		pci_write_config_dword(pdev, 0x48, val|0x71000000);
> +		pci_read_config_dword(pdev, 0x48, &val);
> +	}
> +
> +	printk(KERN_INFO CB710_DRIVER_NAME
> +		": PCI config[0x48] = 0x%08X (%d %d %d %d %d %d)\n",
> +		val,
> +		!(val & 0x01000000),
> +		(val >> 8) & 7,
> +		!!(val & 0x10000000),
> +		!!(val & 0x20000000),
> +		!!(val & 0x40000000),
> +		!(val & 0x02000000)
> +	);
> +

If this still is unknown voodoo, then please add a comment explaining
that. Otherwise I expect to see more defines. :)

> +
> +static const struct pci_device_id cb710_pci_tbl[] = {
> +	{ PCI_VENDOR_ID_ENE, 0x510, PCI_ANY_ID, PCI_ANY_ID, },
> +	{ 0, }
> +};

Allocate a define for 0x510, either in this file or pci_ids.h.

> diff -urN empty/Makefile cb710-pre-20080913/Makefile
> --- empty/Makefile	1970-01-01 01:00:00.000000000 +0100
> +++ cb710-pre-20080913/Makefile	2008-09-12 20:56:39.000000000 +0200

It's easier if you develop against the real kernel tree and send diffs
for that.

> diff -urN empty/mmc.c cb710-pre-20080913/mmc.c
> --- empty/mmc.c	1970-01-01 01:00:00.000000000 +0100
> +++ cb710-pre-20080913/mmc.c	2008-09-13 01:28:10.000000000 +0200

> +#include <linux/mmc/mmc.h>

You should never have to include this in a host driver.

> +	printk(KERN_INFO CB710_DRIVER_NAME
> +		": %s: clock set to %d Hz, wanted %d Hz; flag = %d\n",
> +		mmc_hostname(mmc),
> +		src_hz >> cb710_clock_divider_log2[divider_idx & 7],
> +		hz, (divider_idx & 8) != 0);

Please use pr_debug() for things like this.

> +static void cb710_mmc_enable_irq(struct cb710_chip *chip, int enable)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&chip->irq_lock, flags);
> +	__cb710_mmc_enable_irq(chip, enable);
> +	spin_unlock_irqrestore(&chip->irq_lock, flags);
> +}

This is a fairly useless wrapper. Look over how it is called instead.

> +static int cb710_check(struct cb710_chip *chip, int what)
> +{
> +	uint8_t status1, status2;
> +
> +	/* all this is magic */
> +	BUG_ON(what < 2 || what > 4);
> +

You seem to know what some of the "what" values are. Put some defines
for those.

> +/*	if (flags & MMC_CMD_IS_APP) */
> +	if (reader->app_cmd) {
> +		/* original driver set this bit for MMC/SD application
> +		 * commands. It apparently works without it, but... oh well.
> +		 */
> +		cb_flags |= 0x4000;
> +		reader->app_cmd = 0;
> +	}

ACMD:s are no different from "normal" commands. As you've discovered,
you do not need this code.

> +	if (flags & MMC_RSP_PRESENT) {
> +		/* Windows driver set 01 at bits 4,3 except for
> +		 * MMC_SET_BLOCKLEN. I assume that 00 here means no
> +		 * response is expected.
> +		 */
> +		if (cmd->opcode != MMC_SET_BLOCKLEN)
> +			cb_flags |= CB710_MMC_RSP_PRESENT;
> +		else
> +			cb_flags |= CB710_MMC_RSP_PRESENT_X;

Looking at the opcode is not acceptable, so you need to figure out
what's really going on here. MMC_SET_BLOCKLEN has a common R1 response,
so I don't know why the Windows driver special treated it.

> +		if (flags & MMC_RSP_136)		/* R2 */
> +			cb_flags |= CB710_MMC_RSP_136;
> +		else if (!(flags & MMC_RSP_CRC))	/* R3 */
> +			cb_flags |= CB710_MMC_RSP_NO_CRC;

I don't see a need for the "else".

> +
> +	if (rsp_opcode != ((cmd->flags & MMC_RSP_OPCODE) ? cmd->opcode : 0x3F))
> +		cmd->error = -EILSEQ;

This isn't terribly readable. Something like this would be better:

	if (cmd->flags & MMC_RSP_OPCODE) {
		if (rsp_opcode != cmd->opcode)
			cmd->error = -EILSEQ;
	} else {
		if (rsp_opcode != 0x3F)
			cmd->error = -EILSEQ;
	}

The code generated by the compiler should be the same.

> +	if (!error)	/* TODO: proper counting */
> +		data->bytes_xfered = data->blksz * data->blocks;

You can probably never do proper counting when you don't know how the
hardware behaves.

> +static const struct mmc_host_ops cb710_mmc_host = {
> +	.request = cb710_mmc_request,
> +	.set_ios = cb710_mmc_set_ios,
> +	.get_ro = cb710_mmc_get_ro,
> +	.enable_sdio_irq = NULL,
> +};

NULL is the default so you don't need to specify that.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.
--
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