Hi Mattias,
On Fri, Jan 21, 2011 at 01:07:40PM +0100, Mattias Wallin wrote:
> On 01/21/2011 11:31 AM, Arun MURTHY wrote:
> > The only clients for GPADC is the battery driver and the Audio Acc
> > detection. Both of these are sub-modules/clients of ab8500.
> > None other than these can use the GPADC.
> > Inputs to GPADC can only be one among the following:
> > /* GPADC source: From datasheet(ADCSwSel[4:0] in GPADCCtrl2) */
> > #define BAT_CTRL 0x01
> > #define BTEMP_BALL 0x02
> > #define MAIN_CHARGER_V 0x03
> > #define ACC_DETECT1 0x04
> > #define ACC_DETECT2 0x05
> > #define ADC_AUX1 0x06
> > #define ADC_AUX2 0x07
> > #define MAIN_BAT_V 0x08
> > #define VBUS_V 0x09
> > #define MAIN_CHARGER_C 0x0A
> > #define USB_CHARGER_C 0x0B
> > #define BK_BAT_V 0x0C
> > #define DIE_TEMP 0x0D
> >
> > Henceforth in order to secure the usage of GPADC, and in order to
> > restrict it to only EM and AUDIO sub-module, the gpadc device struct
> > was added to ab8500 struct. Also that the exported function
> > ab8500_gpadc_convert has an argument struct ab8500_gpadc, which can
> > be obtained be dereferencing the struct ab8500. This is possible only
> > with the ab8500 and its clients, thereby securing the usage to
> > battery driver and audio acc detect.
> Yes and I would like to remove this restriction and have a simpler more open api.
> First I don't like that users needs to do a lot of pointer dereferencing in their call like ab8500_gpadc_convert(dev->parent->ab8500->gpadc, USB_CHARGER) (an example).
>
As subdevices, I expect users to have an ab8500 pointer. So it would just be
one dereference.
> I prefer a get function that returns a handle that should be used as first argument in the convert calls.
> It also makes sense if this driver will be extended to use more channels.
> Second this driver will be used by for example accessories which likely will be called by 3 party drivers
> and to make their life easier I don't want to force them to this ab8500-core dependency.
>
As I'm not familiar with your HW architecture, could you please describe how
those accessories would wire into the ab8500 core ?
If those devices really are independent drivers (i.e. not subdevices) needing
to get an A/D conversion from the ab8500 adc (I don't see how that can happen,
hence my above question), then it might make sense to use a conversion API
independent from any ab8500 pointer. But otherwise, I prefer this API rather
than the one in v2 of this patch.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
On 02/01/2011 12:36 PM, Samuel Ortiz wrote:
> As subdevices, I expect users to have an ab8500 pointer. So it would just be
> one dereference.
I do not want users to have or use the ab8500 pointer at all. I would
like to move it from the .h file into the ab8500-core.c eventually.
> As I'm not familiar with your HW architecture, could you please describe how
> those accessories would wire into the ab8500 core ?
The accessories can for example be a simple phone headset, a carkit and
so on. A headset wire into the 3.5mm plug and gpadc can be used to
understand whats plugged in. Our analog baseband chip ab8500 is a
container of subfunctionality like audio codec, digital encoder, voltage
regulators and so on.
The idea behind ab8500-core driver is to provide register access and
interrupt management to the subdrivers implementing the
subfunctionality. The gpadc driver is one these subdrivers. A headset
driver becomes a subdriver of the gpadc wich is a subdriver to
ab8500-core so the question is how far we should enforce these hierarchy
of drivers.
In my opinion the line goes here. The gpadc provides a service to
convert. Open for not only subdrivers and the rational is to reduce
complexity.
> If those devices really are independent drivers (i.e. not subdevices) needing
> to get an A/D conversion from the ab8500 adc (I don't see how that can happen,
> hence my above question), then it might make sense to use a conversion API
> independent from any ab8500 pointer. But otherwise, I prefer this API rather
> than the one in v2 of this patch.
You are absolutely right that really independent drivers of ab8500 will
probably not be found and I understand your argument. But many parts in
our platform have connections to ab8500 via regulators, clocks or other
wires. The decision is instead based on design to reduce complexity. If
a driver uses direct register access to ab8500 then it should be a
subdriver (to enforce startup order for example) otherwise is is not
required (in my oppinion). An accessory driver should easily be ported
from other platforms and not be tied to ab8500.
Me and Arun got some feedback to keep our discussion internal first so
sorry for keeping you out the last mails but the result is patch v2.
Thanks and regards,
Mattias Wallin