Re: [Device-drivers-devel] Oddities and how to handle them.

[Jonathan Cameron <jic23@cam.ac.uk>]

On 05/03/11 10:46, Jonathan Cameron wrote:
> On 05/03/11 10:26, Michael Hennerich wrote:
>> On 05/02/2011 04:50 PM, Jonathan Cameron wrote:
>>>   
>>>>>>>>             
>>>>>>> We could prefix all inputs with in and all outputs with out, assuming
>>>>>>> we move voltages out of the way. Ultimately we didn't have any output
>>>>>>> devices when we started hammering the interfaces into shape.
>>>>>>>
>>>>>>>
>>>>>>>           
>>>>>> That sounds right to me.
>>>>>>
>>>>>>         
>>>>> We may need to do this gradually, or on the move from staging out into the
>>>>> main tree.  Whilst we are in staging, I know there are mainstream users
>>>>> of a few drivers.  Perhaps we just support old interface for them on a
>>>>> case by case basis.
>>>>>
>>>>> This will want a full proposal to lkml.
>>>>>
>>>>>       
>>>>>>>           
>>>>>>>> In addition we need to proper naming for what is input or output -
>>>>>>>> current, voltage, etc.
>>>>>>>>
>>>>>>>> The three power values can't be three different channels.
>>>>>>>> They are alternatives all on the same physical input channel, and the
>>>>>>>> naming should express this.
>>>>>>>>
>>>>>>>>
>>>>>>>>             
>>>>>>> Then it will have to be as modifiers.  Right now we tend to use them to
>>>>>>> group things.  So for accelerometers we can in theory have:
>>>>>>>
>>>>>>> accel0_x,
>>>>>>> accel0_y,
>>>>>>> accel1_x, etc. for chips implementing more than one sensor in a given
>>>>>>> direction.
>>>>>>>
>>>>>>> If we insist on same number meaning same physical ping then for typical
>>>>>>> inertial sensor the channel number would have to be unique.
>>>>>>> Thus take adis16400 we would need.
>>>>>>>
>>>>>>> in0_supply_raw
>>>>>>> gyro1_x_raw
>>>>>>> gyro2_y_raw
>>>>>>> gyro3_z_raw
>>>>>>> accel4_x_raw
>>>>>>> etc...
>>>>>>> which, whilst looking odd, wouldn't be a fundamental problem.
>>>>>>>
>>>>>>>
>>>>>>>           
>>>>>> Agreed - that looks odd. And yes modifiers should work as well.
>>>>>> So we get to -
>>>>>>
>>>>>> in_powerX_Y_apparent_raw
>>>>>>
>>>>>> in_volatgeX_Y_rms_raw
>>>>>>
>>>>>> or
>>>>>>
>>>>>> inX_powerY_apparent_raw
>>>>>> inX_volatgeY_rms_raw
>>>>>> outX_volatgeY_raw
>>>>>>
>>>>>>
>>>>>>         
>>>>> I'm a little confused on what the Y is?  I would imagine we can only have
>>>>> one apparent power measure per channel.  The modifier will be into an enum
>>>>> associated with that 'apparent' label, much as we have 'x'
>>>>> for axis in devices where that makes sense.  We may want to move away from
>>>>> the passing a character approach for those modifiers as well so we have
>>>>> just one path.
>>>>>
>>>>>       
>>>> Hi Jonathan,
>>>>
>>>> I'm now getting confused as well.
>>>> Yes one apparent power measure per channel is enough.
>>>> Didn't you say that the 3 power values will need to be different channels?
>>>> My point was that we need a modifier that identifies the physical
>>>> input/output channel.
>>>>     
>>> I was thinking of this other way around. We have perfectly good channel
>>> numbers. Lets use them for the physical channel, then use the modifiers
>>> to distinguish what we are dealing with.  Thus, here we have:
>>>
>>> Channel types
>>> Power,
>>> Voltage,
>>> Current,
>>> (for now keep voltage as inX as it will easier to do a separate series converting
>>> all drivers to new naming)
>>>
>>> for power, we define modifiers, apparent, active, reactive.
>>>
>>> for voltage and current we will define the modifier rms
>>>
>>> (this is a change to what I proposed earlier so as to allow for
>>> events on RMS values. For consistency we will probably want to move
>>> the existing channel info element peak_raw over to be a modifier
>>> as well - what we currently do with that is a dirty hack that will
>>> bite us at some point)
>>>
>>> We then define channel numbering, to be an 'indicator' of shared physical
>>> channel (i.e. pin).  I only say indicator so as to avoid a mass change of
>>> the tree in this driver patch. As with the channel renames, that wants
>>> to be a separate series.  It actually effects only a few channels on a few
>>> devices so isn't a big problem.
>>>
>>> By saying channel numbers indicate physical channels iff they are present
>>> we get around having to assign the to axes on the IMU's and accelerometers.
>>>
>>> So we end up with here (I've gone for raw everywhere to avoid reading the
>>> datasheet thoroughly!)
>>>
>>> power0_apparent_raw
>>> power0_active_raw
>>> power0_reactive_raw
>>> in0_raw (probably become voltage0_raw at a later date, from waveform register?)
>>>   
>> Not sure if we need voltage0_raw and current0_raw as a none buffer channel.
>> These actual values are only interesting when they are sampled at a
>> fixed frequency.
> Cool. I wasn't sure about those.  Can conceive of devices that look at the exact
> wave form which want to do this, but agreed, it doesn't make sense for this one..
> (and I have no idea if such a detailed device exists - can only think of being useful
> for looking at various DC to AC convertors...)
>>> in0_rms_raw
>>> in0_peak_raw (max value from set number of wave cycles - probably needs in0_peak_cycles as well?)
>>> curr0_raw (from waveform register?)
>>> curr0_rms_raw
>>> curr0_peak_raw (max value from set number of wave cycles..)
>>>
>>> Would this cover your requirements?  It generalizes well (I think) so I'm quite
>>> keen on doing it roughly like this...
>>>   
>> Thanks, this covers things - and makes a lot of sense.
> I'm pushing the updated code all the way through the tree.  It will take a little while
> as this touches about half the driver updates. Note I'm also scrapping all but one of
> the IIO_CHAN macros as per the other branch of this thread.  As Arnd predicted they have
> turned into a maintenance nightmare!

I've pushed a fairly rough branch 'work' on the iio-onwards tree.  As the names of the last
few commits show, it has some elements I should have hacked off the top, but I've run out
of time (pesky students) + not sure what time I'll have for a few days.

Still basic principal of what I described is there in the meantime.
Sorry it's touch messy!
> 
>>> As a follow up series, I'll (or some one else) also move the accelerometers etc
>>> to not specify their modifiers with 'x' as channel but rather the modifier
>>> code in channel2 of iio_chan_spec.
>>>
>>>  Thanks for knocking this driver into shape!
>>>
>>> Hope it doesn't prove too painful.
>>>
>>> Jonathan
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>>   
>>
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>