No subject

screen
controller mode or in a direct operation mode (ADC mode).  The mode is
selected with the TS_SETUP_ENABLE bit.  Both modes are not able to =
operate
at the same time.  Plus, it really doesn't make any sense for them to =
work
together.  Either a touch screen will be connected or the signals will =
be
used to measure analog voltages.

The way you have partitioned this code it appears that the low-level i/o
is all handled in arch/arm/mach-ep93xx/adc.c.  The high-level stuff for
ADC mode is then handled by drivers/hwmon/ep93xx-hwmon.c.

This approach looks good to me.  When a touch screen driver is finally
created it should be able to reuse the functionality in adc.c in the =
same
way (hopefully).

Following are replies to your questions:

[snip]

>>> +	adc->clk =3D clk_get(dev, "ep93xx-analog");
>>=20
>> Did you try:
>> 	adc->clk =3D clk_get(dev, NULL);
>
> No I didn't, I guess that it will work if the names match. The goal=20
> was to define the clock "analog", and the two devices "adc" and=20
> "touchscreen", I don't know what's the best strategy to adopt here. I=20
> know that ADC and TS share the same ressources (regs, clock, irq, ...) =

> but as I said above I don't have the proper hardware to make any =
tests.
>
> So perhaps in the mean time, I should define everything as ADC centric =

> and forget completely the touchscreen controller.

I did some thinking on this.

The adc.c module is the only place that actually "gets" the clock.  The
upper-level hwmon, and eventually touchscreen, module don't even know
that the clock exists.  Based on that, the adc.c driver should be =
getting
the clock based on the dev_id since it is the only user.

>>=20
>>> +	if (IS_ERR(adc->clk)) {
>>> +		dev_err(dev, "failed to get ADC clock\n");
>>> +		ret =3D PTR_ERR(adc->clk);
>>> +		goto err_irq;
>>> +	}
>>> +
>>> +	regs =3D platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +	if (!regs) {
>>> +		dev_err(dev, "failed to find registers\n");
>>> +		ret =3D -ENXIO;
>>> +		goto err_clk;
>>> +	}
>>=20
>> request_mem_region() before ioremap().
>
> Oops, didn't spot that one, thanks.
> BTW, doesn't platform_get_resource() do it automatically?

No.  From drivers/base/platform.c

/**
 * platform_get_resource - get a resource for a device
 * @dev: platform device
 * @type: resource type
 * @num: resource index
 */
struct resource *platform_get_resource(struct platform_device *dev,
				       unsigned int type, unsigned int num)

The function just walks thru the passed platform_device->resource[]
looking for the 'num' resource of 'type'.  Then the resource is
returned unmodified.

[snip]

>>> +static struct clk clk_analog =3D {
>>=20
>> 	.parent	=3D &clk_xtali,
>
> The parent clock patch is still not in Linus tree.

It just went it.

commit ebd00c08e28a0ab4dcb715d222214625fff6d62a
Author: Hartley Sweeten <hartleys at visionengravers.com>
Date:   Thu Oct 8 23:44:41 2009 +0100

    ARM: 5756/1: ep93xx: introduce clk parent

[snip]

>>> +#define EP93XX_TS_PHYS_BASE		(EP93XX_APB_PHYS_BASE + 0x00100000)
>>=20
>> Should be:
>>=20
>> #define EP93XX_TS_PHYS_BASE			EP93XX_APB_PHYS(0x00100000)
>
> My current version (linus rc4) doesn't have EP93XX_APB_PHYS().

This also just went in:

commit 591006f830bcc8edf2841750d7543c5c5a672f89
Author: Hartley Sweeten <hartleys at visionengravers.com>
Date:   Fri Sep 25 17:54:31 2009 +0100

    ARM: 5729/1: ep93xx: define EP93XX_*_PHYS_BASE with macros

[snip]

>>> +	ep93xx_hwmon_device.dev.platform_data =3D &ts72xx_hwmon_info;
>>>  }
>>=20
>> Ugly... You shouldn't be setting up any type of device data in =
(*map_io).
>
> Yes, I don't remember the details but I think I ran into a problem of=20
> runtime order dependency, and doing this here have solved it. I will=20
> try to fix it.
> Well, the whole story is that it's what the S3c code is doing, I tried =

> to move it somewhere else for the same reason (it's ugly) and it=20
> didn't work so ...

Hmmm... This should not cause any problems as long as it is set before =
the:

	platform_device_register(&ep93xx_hwmon_device);

I'll take another look at it when you repost the updated patch.

[snip]

>> Can you just use struct ep93xx_hwmon_chcfg directly without using the
>> struct ep93xx_hwmon_data as a container?  Something like:
>
> Yes, actually i copied the S3c code and try to avoid to make any=20
> cosmetic change to keep both similar so that it will help if someday=20
> someone want to make the adc code ARM generic.

I'm not sure if that's possible but you never know...

I look forward to seeing the update patch.

Regards,
Hartley