All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ryan Mallon <ryan@bluewatersys.com>
To: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Cc: linux-ide@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	Sergei Shtylyov <sshtylyov@ru.montavista.com>,
	Joao Ramos <joao.ramos@inov.pt>,
	H Hartley Sweeten <hsweeten@visionengravers.com>
Subject: Re: [PATCH] add PATA host controller support for Cirrus Logic's EP93xx CPUs
Date: Wed, 02 Dec 2009 13:53:53 +1300	[thread overview]
Message-ID: <4B15BAA1.2010408@bluewatersys.com> (raw)
In-Reply-To: <200911261651.40928.bzolnier@gmail.com>

Bartlomiej Zolnierkiewicz wrote:
> Based on the older IDE host driver by Joao Ramos and review comments
> for it from Sergei Shtylyov.  Not yet tested with the real hardware.
>

Hi Bartlomiej,

I have got as far as patching this into my kernel and doing a build test
(still need to find a hard-disk to test). I got some build errors, see
below:

> +static void pata_ep93xx_set_piomode(struct ata_port *ap,
> +				    struct ata_device *adev)
> +{
> +	void __iomem *base = ap->host->private_data;
> +	struct ata_device *pair = ata_dev_pair(adev);
> +	const struct ata_timing *t = ata_timing_find_mode(adev->pio_mode);
> +	const struct ata_timing *cmd_t = t;
> +	u32 reg = IDECFG_IDEEN | IDECFG_PIO;
> +	u8 pio = adev->pio_mode - XFER_PIO_0;
> +
> +	if (pair && pair->pio_mode < adev->pio_mode)
> +		cmd_t = ata_timing_find_mode(pair->pio_mode);
> +
> +	/*
> +	 * store the adequate PIO mode timings, to be used later
> +	 * by pata_ep93xx_{read,write}
> +	 */
> +	adev->private_data = (void *)t;
> +	ap->private_data = (void *)cmd_t;

struct ata_device has no member called private_data. Do we need to store
both t and cmd_t, or can we just store cmd_t in ap->private_data?

If we need both, can we have something like this (not sure if the member
names are sensible):

	struct ep93xx_ata_timing {
		struct ata_timing *dev_timing;
		struct ata_timing *pair_timing;
	};

and store that struct in ap?

> +unsigned int pata_ep93xx_data_xfer(struct ata_device *adev, unsigned char *buf,
> +				   unsigned int buflen, int rw)
> +{
> +	struct ata_port *ap = adev->link->ap;
> +	void __iomem *base = ap->host->private_data;
> +	void *data_addr = ap->ioaddr.data_addr;
> +	u16 *data = (u16 *)buf;
> +	struct ata_timing *t = adev->private_data;

This is the only place that uses the ata_timing structure stored in
adev->private_data, can this be changed to:

	struct ata_timing *t = ap->private_data;

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan@bluewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934

WARNING: multiple messages have this Message-ID (diff)
From: ryan@bluewatersys.com (Ryan Mallon)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] add PATA host controller support for Cirrus Logic's EP93xx CPUs
Date: Wed, 02 Dec 2009 13:53:53 +1300	[thread overview]
Message-ID: <4B15BAA1.2010408@bluewatersys.com> (raw)
In-Reply-To: <200911261651.40928.bzolnier@gmail.com>

Bartlomiej Zolnierkiewicz wrote:
> Based on the older IDE host driver by Joao Ramos and review comments
> for it from Sergei Shtylyov.  Not yet tested with the real hardware.
>

Hi Bartlomiej,

I have got as far as patching this into my kernel and doing a build test
(still need to find a hard-disk to test). I got some build errors, see
below:

> +static void pata_ep93xx_set_piomode(struct ata_port *ap,
> +				    struct ata_device *adev)
> +{
> +	void __iomem *base = ap->host->private_data;
> +	struct ata_device *pair = ata_dev_pair(adev);
> +	const struct ata_timing *t = ata_timing_find_mode(adev->pio_mode);
> +	const struct ata_timing *cmd_t = t;
> +	u32 reg = IDECFG_IDEEN | IDECFG_PIO;
> +	u8 pio = adev->pio_mode - XFER_PIO_0;
> +
> +	if (pair && pair->pio_mode < adev->pio_mode)
> +		cmd_t = ata_timing_find_mode(pair->pio_mode);
> +
> +	/*
> +	 * store the adequate PIO mode timings, to be used later
> +	 * by pata_ep93xx_{read,write}
> +	 */
> +	adev->private_data = (void *)t;
> +	ap->private_data = (void *)cmd_t;

struct ata_device has no member called private_data. Do we need to store
both t and cmd_t, or can we just store cmd_t in ap->private_data?

If we need both, can we have something like this (not sure if the member
names are sensible):

	struct ep93xx_ata_timing {
		struct ata_timing *dev_timing;
		struct ata_timing *pair_timing;
	};

and store that struct in ap?

> +unsigned int pata_ep93xx_data_xfer(struct ata_device *adev, unsigned char *buf,
> +				   unsigned int buflen, int rw)
> +{
> +	struct ata_port *ap = adev->link->ap;
> +	void __iomem *base = ap->host->private_data;
> +	void *data_addr = ap->ioaddr.data_addr;
> +	u16 *data = (u16 *)buf;
> +	struct ata_timing *t = adev->private_data;

This is the only place that uses the ata_timing structure stored in
adev->private_data, can this be changed to:

	struct ata_timing *t = ap->private_data;

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan at bluewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934

  parent reply	other threads:[~2009-12-02  0:53 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-11-26 15:51 [PATCH] add PATA host controller support for Cirrus Logic's EP93xx CPUs Bartlomiej Zolnierkiewicz
2009-11-26 15:51 ` Bartlomiej Zolnierkiewicz
2009-11-26 17:03 ` Alan Cox
2009-11-26 17:03   ` Alan Cox
2009-11-26 19:46 ` Ryan Mallon
2009-11-26 19:46   ` Ryan Mallon
2009-11-26 22:15   ` João Ramos
2009-11-26 22:15     ` João Ramos
2009-12-02  0:53 ` Ryan Mallon [this message]
2009-12-02  0:53   ` Ryan Mallon
2009-12-02  1:06   ` Bartlomiej Zolnierkiewicz
2009-12-02  1:06     ` Bartlomiej Zolnierkiewicz
2009-12-02  1:16     ` H Hartley Sweeten
2009-12-02  1:16       ` H Hartley Sweeten
2009-12-02  1:16       ` H Hartley Sweeten
2009-12-02  1:26       ` Ryan Mallon
2009-12-02  1:26         ` Ryan Mallon
2009-12-14 21:40     ` Ryan Mallon
2009-12-14 21:40       ` Ryan Mallon
2009-12-14 22:06       ` Jeff Garzik
2009-12-14 22:06         ` Jeff Garzik
2010-01-05 18:56         ` Bartlomiej Zolnierkiewicz
2010-01-05 18:56           ` Bartlomiej Zolnierkiewicz
2010-01-05 18:56           ` Bartlomiej Zolnierkiewicz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4B15BAA1.2010408@bluewatersys.com \
    --to=ryan@bluewatersys.com \
    --cc=bzolnier@gmail.com \
    --cc=hsweeten@visionengravers.com \
    --cc=joao.ramos@inov.pt \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-ide@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=sshtylyov@ru.montavista.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.