Re: meter ABI: (was Re: [PATCH v2 1/3] staging:iio:meter: Replaces IIO_DEV_ATTR_CH_OFF by IIO_DEVICE_ATTR)
On Sun, 25 Mar 2018 13:53:02 -0700 John Syne wrote: > > On Mar 25, 2018, at 9:54 AM, Jonathan Cameron wrote: > > > > On Sun, 25 Mar 2018 01:29:41 -0700 > > John Syne wrote: > > > >> Hi Jonathan, > > Hi John, > > > > Please wrap your normal emails (excepting tables) to 80 chars. > Yeah, I’m trying to do that, but I use a Mac and Apple Mail doesn’t > have a feature to wrap at 80 automatically so I have to do > this manually. > > > >> > >> I was speaking with Rodrigo and here is what I think must be done to move > >> ADE7878 out of staging > >> > >> Here are the steps as I see them: > >> > >> 1) Define the IIO Attributes so they are consistent with the IIO ABI. This > >> should be pretty simple given agreement on the naming convention. > > Please don't go just changing attribute names - the driver needs to > > use the standard approach of iio_chan_spec and create the vast > > majority of attributes automatically via that. There 'may' be > > a few corner cases wee don't want to make generic and they 'might' > > be exposed as attributes. > > > > I suspect this change is the 'big' job. The core support necessary > > for RMS and MAV (computedtype or whatever we call it) will need adding > > as well. That is a separate patch set with support for some examples > > in the dummy driver. > Great that you mentioned this, because I though we only needed to > modify the IIO_ATTR naming. I’ll take a look at iio_chan_spec and > the examples in the dummy driver. That is how it was originally done and this is an 'old' driver, but the chan spec stuff does three things : 1) Saves on lots of boiler plate code. 2) Makes it sane to access the device from other driver (hwmon now does something similar btw). 3) Avoids a lot of ABI typos and careful review. > > > >> 2) Map the ADE7854 interrupt status to IIO events. This requires an > >> interrupt processing section. > >> 3) Add DeviceTree support. > >> 4) Create DeviceTree overlay for the ADE7854. > > More a case of bindings for now. If those are used via an overlay > > fine but given we don't have any boards with one one in mainline, > > this is an implementation detail for the user rather than part > > of moving this driver out of staging. > I was thinking more along the lines of documentation to show a > developer what was needed to get the driver working. Do it as an example in the device tree bindings doc. > > > >> 5) Update ADE7854 probe to read in the DeviceTree register settings. > >> 6) Add support for power modes (PM1, PM2). > > This isn't necessary for a move out of staging (nice to have though). > > > >> 7) Not sure if we will support measurement streaming on the ADE7854. > >> The problem is ADE7854 is designed as an SPI master, which means > >> it controls the SPI clock, so the driver must support SPI slave > >> mode. However, the Linux Kernel does not currently support SPI > >> slave mode. We have three choices to make this work and they > >> are all a lot of work: 1) Add support for SPI Slave mode to the > >> kernel, 2) Use hardware to convert SPI signals to I2S signals > >> and with the use of a custom codec, use the ALSA framework to > >> stream the samples (this is an approach I used, but I don’t like > >> it), 3) Move the I2S driver out of the sound subsystem and use it > >> together with DMA to stream samples directly into the ADE7854 driver > >> (my preferred solutions). Perhaps Mark Brown has some ideas on how > >>to make this work. > > I'll be honest, this is an end of line part and frankly more than > > a little crazy. I would go with simply not supporting the measurement > > streaming at all for this part. If you really need it we can then > > move onto the how part, but from what you have said I'm guessing you > > don't care except in an abstract 'it would be nice' sort of a way? > Yeah, I’m moving to the ADE9000 so I’ll drop this. Agreed. We let someone else pick it up if they care. > > > >> > >> The ADE9000 will be much easier because it uses an SPI Slave interface. > >> > >> I hope I have captured everything, but let me know if I have missed > >> anything. > >> > > > > That will do for now ;) I'm sure there will be details that need > > tidying up once we have the above done, but that's true for any new > > driver (and this will be nearly a new driver before things are done). > Thank you again for all the detailed feedback. > > > > Jonathan > > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: meter ABI: (was Re: [PATCH v2 1/3] staging:iio:meter: Replaces IIO_DEV_ATTR_CH_OFF by IIO_DEVICE_ATTR)
On Sun, 25 Mar 2018 13:36:40 -0700
John Syne wrote:
> > On Mar 25, 2018, at 9:29 AM, Jonathan Cameron wrote:
> >
> > On Sat, 24 Mar 2018 15:45:21 -0700
> > John Syne wrote:
> >
> >>> On Mar 24, 2018, at 8:02 AM, Jonathan Cameron wrote:
> >>>
> >>> On Mon, 19 Mar 2018 22:57:16 -0700
> >>> John Syne wrote:
> >>>
> Hi Jonathan,
>
> Thank you for all your hard work. Your feedback is really helpful. I’m
> surprised that no one from Analog Device has offered any suggestions.
>
> >>>
> >>> Sadly those active in the mainline linux kernel from ADI are focused in
> >>> other areas currently :(
> >> Good point. I will reach out to Analog Devices and get a name of someone
> >> who is knowledgeable in this area. Perhaps Lars-Peters has a suggestion.
> > Yes, Lars or Michael Hennerich (+CC) are my normal contact points.
> > I believe this is outside both of their areas, but they may be able to
> > put you in contact with the right person. If nothing else it would be
> > good to make someone in ADI aware that people care about linux support for
> > these parts!
> I sent in a request to ADI for a contact person to help with the review.
Cool.
> >
> Anyway, please see my comments inline below
>
> Regards,
> John
>
>
>
>
>
> > On Mar 18, 2018, at 5:23 AM, Jonathan Cameron wrote:
> >
> > On Sat, 17 Mar 2018 23:11:45 -0700
> > John Syne wrote:
> >
> >> Hi Jonathan,
> > Hi John and All,
> >
> > I'd love to get some additional input on this from anyone interested.
> > There are a lot of weird and wonderful derived quantities in an energy
> > meter and it seems we need to make some fundamental changes to support
> > them - including potentially a userspace breaking change to the event
> > code description.
> >
> >>
> >> Here is the complete list of registers for the ADE7878 which I copied
> >> from the data sheet. I added a column “IIO Attribute” which I hope
> >> follows your IIO ABI. Please make any changes you feel are incorrect.
> >> BTW, there are several registers that cannot be generalized and are
> >> used purely for chip configuration. I think we should add a new naming
> >> convention, namely {register}_{}. Also, I see in
> >> the sys_bus_iio doc
> >
> > No, registers can always be broken up in to real meaningful values if
> > they are exposed to userspace and userspace has a reason to tweak
> > them.
> >
> > We never expose raw registers to userspace except through
> > debugfs and the clue is in the name - purely for debug.
> OK
> >
> >
> >>>
> >>>
> >>>
> >>>
> >> in_accel_x_peak_raw
> >>
> >> so shouldn’t the phase be represented as follows:
> >>
> >> in_current_A_scale
> > I'm still confused. What does A represent here? I assumed that was a
> > wild
> > card for the channel number before but clearly not.
> >
> > Ah, you are labelling the 3 separate phases as A, B and C. Hmm.
> > I guess they sort of look like axis, and sort of like independent
> > channels.
> > So could be indexed or done via modifiers depending on how you look at
> > it.
> In metering terminology, the three phases are commonly referred to as
> RED, GREEN, BLUE or PhaseA, PhaseB, PhaseC and Neutral. Analog Devices
> uses the ABCN nomenclature so anyone using this driver will probably
> have referenced the Analog Devices datasheet so this terminology should
> be familiar.
> >>> Which is more commonly used in general? We are designing what we hope
> >>> will be a general interface and last thing we want is to have everyone
> >>> not using ADI parts confused! Personally not assigning 'colours' makes
> >>> more sense to me.
> >> I did a little research and here is the naming used by vendor:
> >>
> >> ST Microelectronics: P1, P2, P3, N
> >> http://www.st.com/content/ccc/resource/technical/document/application_note/5e/f4/22/4d/90/96/4c/c4/CD00153264.pdf/files/CD00153264.pdf/jcr:content/translations/en.CD00153264.pdf
> >>
> >> Teridian: ABCN
> >> https://www.mouser.com/ds/2/256/71M6513-71M6513H-22603.pdf
> >>
> >> Maxim Integrated: ABCN
> >> https://datasheets.maximintegrated.com/en/ds/78M6631.pdf
> >>
> >> Microchip: ABCN
> >> http://ww1.microchip.com/downloads/en/DeviceDoc/51643b.pdf
> >>
> >> NXP: L1, L2, L3, N
> >> https://www.nxp.com/support/developer-resources/reference-designs/kinetis-km3x-256-mcu-three-phase-metering-reference-design:RD-KM3x-256-3PH-METERING
> >>
> >> Renesas: ABCN
> >> https://www.renesas.com/en-us/solutions/home/metering/power-meter-three.html
> >>
> >> So the most consistent would be ABCN
> > Cool. Makes sense to go with that. Thanks for checking this out.
> >
> >>
> >>
> >> All vendor providing three phase energy metering ICs use the ABCN
> >> terminology.
Re: meter ABI: (was Re: [PATCH v2 1/3] staging:iio:meter: Replaces IIO_DEV_ATTR_CH_OFF by IIO_DEVICE_ATTR)
> On Mar 25, 2018, at 9:54 AM, Jonathan Cameron wrote: > > On Sun, 25 Mar 2018 01:29:41 -0700 > John Syne wrote: > >> Hi Jonathan, > Hi John, > > Please wrap your normal emails (excepting tables) to 80 chars. Yeah, I’m trying to do that, but I use a Mac and Apple Mail doesn’t have a feature to wrap at 80 automatically so I have to do this manually. > >> >> I was speaking with Rodrigo and here is what I think must be done to move >> ADE7878 out of staging >> >> Here are the steps as I see them: >> >> 1) Define the IIO Attributes so they are consistent with the IIO ABI. This >> should be pretty simple given agreement on the naming convention. > Please don't go just changing attribute names - the driver needs to > use the standard approach of iio_chan_spec and create the vast > majority of attributes automatically via that. There 'may' be > a few corner cases wee don't want to make generic and they 'might' > be exposed as attributes. > > I suspect this change is the 'big' job. The core support necessary > for RMS and MAV (computedtype or whatever we call it) will need adding > as well. That is a separate patch set with support for some examples > in the dummy driver. Great that you mentioned this, because I though we only needed to modify the IIO_ATTR naming. I’ll take a look at iio_chan_spec and the examples in the dummy driver. > >> 2) Map the ADE7854 interrupt status to IIO events. This requires an >> interrupt processing section. >> 3) Add DeviceTree support. >> 4) Create DeviceTree overlay for the ADE7854. > More a case of bindings for now. If those are used via an overlay > fine but given we don't have any boards with one one in mainline, > this is an implementation detail for the user rather than part > of moving this driver out of staging. I was thinking more along the lines of documentation to show a developer what was needed to get the driver working. > >> 5) Update ADE7854 probe to read in the DeviceTree register settings. >> 6) Add support for power modes (PM1, PM2). > This isn't necessary for a move out of staging (nice to have though). > >> 7) Not sure if we will support measurement streaming on the ADE7854. >> The problem is ADE7854 is designed as an SPI master, which means >> it controls the SPI clock, so the driver must support SPI slave >> mode. However, the Linux Kernel does not currently support SPI >> slave mode. We have three choices to make this work and they >> are all a lot of work: 1) Add support for SPI Slave mode to the >> kernel, 2) Use hardware to convert SPI signals to I2S signals >> and with the use of a custom codec, use the ALSA framework to >> stream the samples (this is an approach I used, but I don’t like >> it), 3) Move the I2S driver out of the sound subsystem and use it >> together with DMA to stream samples directly into the ADE7854 driver >> (my preferred solutions). Perhaps Mark Brown has some ideas on how >>to make this work. > I'll be honest, this is an end of line part and frankly more than > a little crazy. I would go with simply not supporting the measurement > streaming at all for this part. If you really need it we can then > move onto the how part, but from what you have said I'm guessing you > don't care except in an abstract 'it would be nice' sort of a way? Yeah, I’m moving to the ADE9000 so I’ll drop this. > >> >> The ADE9000 will be much easier because it uses an SPI Slave interface. >> >> I hope I have captured everything, but let me know if I have missed anything. >> > > That will do for now ;) I'm sure there will be details that need > tidying up once we have the above done, but that's true for any new > driver (and this will be nearly a new driver before things are done). Thank you again for all the detailed feedback. > > Jonathan ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: meter ABI: (was Re: [PATCH v2 1/3] staging:iio:meter: Replaces IIO_DEV_ATTR_CH_OFF by IIO_DEVICE_ATTR)
> > On Mar 25, 2018, at 9:44 AM, Jonathan Cameron wrote: > > On Sat, 24 Mar 2018 16:06:17 -0700 > John Syne wrote: > > This thread is becoming unmanageable so I am cropping this down to just > the questions that remain open. > Probably easier to copy and paste this table into a spreadsheet. Let me know if there is anything I got wrong. Thank you again for all your help. >>> Yeah, we need to shrink this if we do it again. >> I’ll send an updated copy after this e-mail. Can you accept a spreadsheet >> attachment or a CSV file? > We need to keep the discussion visible on list so it needs to stay in > plain text. Just need to drop any columns we aren't caring about to make > it easier to read. Done >>> > ... 0x439C CVAROS in_powerreactive0_phaseC_offset in powerreactive 0 phaseC offset R/W 24 32 ZPSE S 0x00Phase C total reactive power offset adjust (ADE7858, ADE7868, and ADE7878). 0x439D AFWGAIN in_power0_phaseA_fundamental_scale in power 0 phaseA_fundamental scale R/W 24 32 ZPSE S 0x00Phase A fundamental active power gain adjust. Location reserved for ADE7854, ADE7858, and ADE7868. >>> Hmm. Fundamental needs to be represented using a separate channel index >>> and description of the frequency filters applied. That should map it >>> a generic way. >> How do I do this? > Define additional channels with different index and for them > use the the infomask elements > IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY, > IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY > and provide suitable values from the read_raw callbacks for that > channel. > > ... 0x43C0 AIRMS in_current0_phaseA_rms in current 0 phaseA_rms R 24 32 ZP S N/A4Phase A current rms value. >>> in_current0_phaseA_rms_raw as otherwise we don't know we need to apply >>> in_current0_phaseA_rms_scale to it (or the shared value that maps to that). >>> >> Yeah, this is still confusion to me. This should read >> in_current0_phaseA_rms_gain >> as it directly affects the value in_current0_phaseA_rms_raw. We still have >> to apply >> a scale value to turn this cryptic number into something meaningful. > So I'm a little lost. We have variable gain fine. > Does it effect the necessary scale factor to go from raw to real value or not? Yes > 1) Yes it does - then roll it as appropriate into the _scale attribute. > It should not be separated. This often requires some interesting maths > but is a onetime thing as the value isn't changing dynamically. > > 2) No it doesn't - then it is calibgain as it represents a necessary > parameter to change the incoming circuit to compensate for external effects. The hardwaregain and calibgain are setup only once and do not vary. The ADE7854 can accept various input ranges such as 0-250mV, 0-5V, etc The calibgain is used to compensate for the inaccuracies of the sensors and the termination components. > > It is possible you have a mixture of the two and hence need both but that > is normally only the case with devices where the calibgain is about fixing > the factory calibration. > > ... ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: meter ABI: (was Re: [PATCH v2 1/3] staging:iio:meter: Replaces IIO_DEV_ATTR_CH_OFF by IIO_DEVICE_ATTR)
> On Mar 25, 2018, at 9:44 AM, Jonathan Cameron wrote: > > On Sat, 24 Mar 2018 16:06:17 -0700 > John Syne wrote: > > This thread is becoming unmanageable so I am cropping this down to just > the questions that remain open. > Probably easier to copy and paste this table into a spreadsheet. Let me know if there is anything I got wrong. Thank you again for all your help. >>> Yeah, we need to shrink this if we do it again. >> I’ll send an updated copy after this e-mail. Can you accept a spreadsheet >> attachment or a CSV file? > We need to keep the discussion visible on list so it needs to stay in > plain text. Just need to drop any columns we aren't caring about to make > it easier to read. Done >>> > ... 0x439C CVAROS in_powerreactive0_phaseC_offset in powerreactive 0 phaseC offset R/W 24 32 ZPSE S 0x00Phase C total reactive power offset adjust (ADE7858, ADE7868, and ADE7878). 0x439D AFWGAIN in_power0_phaseA_fundamental_scale in power 0 phaseA_fundamental scale R/W 24 32 ZPSE S 0x00Phase A fundamental active power gain adjust. Location reserved for ADE7854, ADE7858, and ADE7868. >>> Hmm. Fundamental needs to be represented using a separate channel index >>> and description of the frequency filters applied. That should map it >>> a generic way. >> How do I do this? > Define additional channels with different index and for them > use the the infomask elements > IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY, > IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY > and provide suitable values from the read_raw callbacks for that > channel. > > ... 0x43C0 AIRMS in_current0_phaseA_rms in current 0 phaseA_rms R 24 32 ZP S N/A4Phase A current rms value. >>> in_current0_phaseA_rms_raw as otherwise we don't know we need to apply >>> in_current0_phaseA_rms_scale to it (or the shared value that maps to that). >>> >> Yeah, this is still confusion to me. This should read >> in_current0_phaseA_rms_gain >> as it directly affects the value in_current0_phaseA_rms_raw. We still have >> to apply >> a scale value to turn this cryptic number into something meaningful. > So I'm a little lost. We have variable gain fine. > Does it effect the necessary scale factor to go from raw to real value or not? Yes > 1) Yes it does - then roll it as appropriate into the _scale attribute. > It should not be separated. This often requires some interesting maths > but is a onetime thing as the value isn't changing dynamically. > > 2) No it doesn't - then it is calibgain as it represents a necessary > parameter to change the incoming circuit to compensate for external effects. The hardwaregain and calibgain are setup only once and do not vary. The ADE7854 can accept various input ranges such as 0-250mV, 0-5V, etc The calibgain is used to compensate for the inaccuracies of the sensors and the termination components. > > It is possible you have a mixture of the two and hence need both but that > is normally only the case with devices where the calibgain is about fixing > the factory calibration. > > ... ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: meter ABI: (was Re: [PATCH v2 1/3] staging:iio:meter: Replaces IIO_DEV_ATTR_CH_OFF by IIO_DEVICE_ATTR)
> On Mar 25, 2018, at 9:29 AM, Jonathan Cameron wrote:
>
> On Sat, 24 Mar 2018 15:45:21 -0700
> John Syne wrote:
>
>>> On Mar 24, 2018, at 8:02 AM, Jonathan Cameron wrote:
>>>
>>> On Mon, 19 Mar 2018 22:57:16 -0700
>>> John Syne wrote:
>>>
Hi Jonathan,
Thank you for all your hard work. Your feedback is really helpful. I’m
surprised that no one from Analog Device has offered any suggestions.
>>>
>>> Sadly those active in the mainline linux kernel from ADI are focused in
>>> other areas currently :(
>> Good point. I will reach out to Analog Devices and get a name of someone
>> who is knowledgeable in this area. Perhaps Lars-Peters has a suggestion.
> Yes, Lars or Michael Hennerich (+CC) are my normal contact points.
> I believe this is outside both of their areas, but they may be able to
> put you in contact with the right person. If nothing else it would be
> good to make someone in ADI aware that people care about linux support for
> these parts!
I sent in a request to ADI for a contact person to help with the review.
>
Anyway, please see my comments inline below
Regards,
John
> On Mar 18, 2018, at 5:23 AM, Jonathan Cameron wrote:
>
> On Sat, 17 Mar 2018 23:11:45 -0700
> John Syne wrote:
>
>> Hi Jonathan,
> Hi John and All,
>
> I'd love to get some additional input on this from anyone interested.
> There are a lot of weird and wonderful derived quantities in an energy
> meter and it seems we need to make some fundamental changes to support
> them - including potentially a userspace breaking change to the event
> code description.
>
>>
>> Here is the complete list of registers for the ADE7878 which I copied
>> from the data sheet. I added a column “IIO Attribute” which I hope
>> follows your IIO ABI. Please make any changes you feel are incorrect.
>> BTW, there are several registers that cannot be generalized and are used
>> purely for chip configuration. I think we should add a new naming
>> convention, namely {register}_{}. Also, I see in the
>> sys_bus_iio doc
>
> No, registers can always be broken up in to real meaningful values if
> they are exposed to userspace and userspace has a reason to tweak
> them.
>
> We never expose raw registers to userspace except through
> debugfs and the clue is in the name - purely for debug.
OK
>
>
>>>
>>>
>>>
>>>
>> in_accel_x_peak_raw
>>
>> so shouldn’t the phase be represented as follows:
>>
>> in_current_A_scale
> I'm still confused. What does A represent here? I assumed that was a
> wild
> card for the channel number before but clearly not.
>
> Ah, you are labelling the 3 separate phases as A, B and C. Hmm.
> I guess they sort of look like axis, and sort of like independent
> channels.
> So could be indexed or done via modifiers depending on how you look at
> it.
In metering terminology, the three phases are commonly referred to as RED,
GREEN, BLUE or PhaseA, PhaseB, PhaseC and Neutral. Analog Devices uses the
ABCN nomenclature so anyone using this driver will probably have
referenced the Analog Devices datasheet so this terminology should be
familiar.
>>> Which is more commonly used in general? We are designing what we hope
>>> will be a general interface and last thing we want is to have everyone
>>> not using ADI parts confused! Personally not assigning 'colours' makes
>>> more sense to me.
>> I did a little research and here is the naming used by vendor:
>>
>> ST Microelectronics: P1, P2, P3, N
>> http://www.st.com/content/ccc/resource/technical/document/application_note/5e/f4/22/4d/90/96/4c/c4/CD00153264.pdf/files/CD00153264.pdf/jcr:content/translations/en.CD00153264.pdf
>>
>> Teridian: ABCN
>> https://www.mouser.com/ds/2/256/71M6513-71M6513H-22603.pdf
>>
>> Maxim Integrated: ABCN
>> https://datasheets.maximintegrated.com/en/ds/78M6631.pdf
>>
>> Microchip: ABCN
>> http://ww1.microchip.com/downloads/en/DeviceDoc/51643b.pdf
>>
>> NXP: L1, L2, L3, N
>> https://www.nxp.com/support/developer-resources/reference-designs/kinetis-km3x-256-mcu-three-phase-metering-reference-design:RD-KM3x-256-3PH-METERING
>>
>> Renesas: ABCN
>> https://www.renesas.com/en-us/solutions/home/metering/power-meter-three.html
>>
>> So the most consistent would be ABCN
> Cool. Makes sense to go with that. Thanks for checking this out.
>
>>
>>
>> All vendor providing three phase energy metering ICs use the ABCN
>> terminology.
>>>
>>> (btw please wrap any lines that don't need to be long to around 80
>>> characters).
>>>
>
> Hmm. With neutral in there as well I guess we need to make them
> modifiers (but might change my mind later ;)
>
> Particularly as we are using the the modifier for RMS under the previous
> plan... It appears we should
Re: meter ABI: (was Re: [PATCH v2 1/3] staging:iio:meter: Replaces IIO_DEV_ATTR_CH_OFF by IIO_DEVICE_ATTR)
On Sun, 25 Mar 2018 01:29:41 -0700 John Syne wrote: > Hi Jonathan, Hi John, Please wrap your normal emails (excepting tables) to 80 chars. > > I was speaking with Rodrigo and here is what I think must be done to move > ADE7878 out of staging > > Here are the steps as I see them: > > 1) Define the IIO Attributes so they are consistent with the IIO ABI. This >should be pretty simple given agreement on the naming convention. Please don't go just changing attribute names - the driver needs to use the standard approach of iio_chan_spec and create the vast majority of attributes automatically via that. There 'may' be a few corner cases wee don't want to make generic and they 'might' be exposed as attributes. I suspect this change is the 'big' job. The core support necessary for RMS and MAV (computedtype or whatever we call it) will need adding as well. That is a separate patch set with support for some examples in the dummy driver. > 2) Map the ADE7854 interrupt status to IIO events. This requires an >interrupt processing section. > 3) Add DeviceTree support. > 4) Create DeviceTree overlay for the ADE7854. More a case of bindings for now. If those are used via an overlay fine but given we don't have any boards with one one in mainline, this is an implementation detail for the user rather than part of moving this driver out of staging. > 5) Update ADE7854 probe to read in the DeviceTree register settings. > 6) Add support for power modes (PM1, PM2). This isn't necessary for a move out of staging (nice to have though). > 7) Not sure if we will support measurement streaming on the ADE7854. >The problem is ADE7854 is designed as an SPI master, which means >it controls the SPI clock, so the driver must support SPI slave >mode. However, the Linux Kernel does not currently support SPI >slave mode. We have three choices to make this work and they >are all a lot of work: 1) Add support for SPI Slave mode to the >kernel, 2) Use hardware to convert SPI signals to I2S signals >and with the use of a custom codec, use the ALSA framework to >stream the samples (this is an approach I used, but I don’t like >it), 3) Move the I2S driver out of the sound subsystem and use it >together with DMA to stream samples directly into the ADE7854 driver >(my preferred solutions). Perhaps Mark Brown has some ideas on how > to make this work. I'll be honest, this is an end of line part and frankly more than a little crazy. I would go with simply not supporting the measurement streaming at all for this part. If you really need it we can then move onto the how part, but from what you have said I'm guessing you don't care except in an abstract 'it would be nice' sort of a way? > > The ADE9000 will be much easier because it uses an SPI Slave interface. > > I hope I have captured everything, but let me know if I have missed anything. > That will do for now ;) I'm sure there will be details that need tidying up once we have the above done, but that's true for any new driver (and this will be nearly a new driver before things are done). Jonathan ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: meter ABI: (was Re: [PATCH v2 1/3] staging:iio:meter: Replaces IIO_DEV_ATTR_CH_OFF by IIO_DEVICE_ATTR)
On Sat, 24 Mar 2018 16:06:17 -0700 John Syne wrote: This thread is becoming unmanageable so I am cropping this down to just the questions that remain open. > >> Probably easier to copy and paste this table into a spreadsheet. Let me > >> know if there is anything I got wrong. Thank you again for all your help. > > Yeah, we need to shrink this if we do it again. > I’ll send an updated copy after this e-mail. Can you accept a spreadsheet > attachment or a CSV file? We need to keep the discussion visible on list so it needs to stay in plain text. Just need to drop any columns we aren't caring about to make it easier to read. > > ... > >> 0x439C CVAROS in_powerreactive0_phaseC_offset in > >> powerreactive 0 phaseC offset R/W 24 32 ZPSE S > >> 0x00Phase C total reactive power offset adjust (ADE7858, > >> ADE7868, and ADE7878). > >> 0x439D AFWGAIN in_power0_phaseA_fundamental_scale in > >> power 0 phaseA_fundamental scale R/W 24 32 ZPSE S > >> 0x00Phase A fundamental active power gain adjust. > >> Location reserved for ADE7854, ADE7858, and ADE7868. > > Hmm. Fundamental needs to be represented using a separate channel index > > and description of the frequency filters applied. That should map it > > a generic way. > How do I do this? Define additional channels with different index and for them use the the infomask elements IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY, IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY and provide suitable values from the read_raw callbacks for that channel. ... > >> 0x43C0 AIRMS in_current0_phaseA_rms in current 0 > >> phaseA_rms R 24 32 ZP S N/A4Phase A > >> current rms value. > > in_current0_phaseA_rms_raw as otherwise we don't know we need to apply > > in_current0_phaseA_rms_scale to it (or the shared value that maps to that). > > > Yeah, this is still confusion to me. This should read > in_current0_phaseA_rms_gain > as it directly affects the value in_current0_phaseA_rms_raw. We still have to > apply > a scale value to turn this cryptic number into something meaningful. So I'm a little lost. We have variable gain fine. Does it effect the necessary scale factor to go from raw to real value or not? 1) Yes it does - then roll it as appropriate into the _scale attribute. It should not be separated. This often requires some interesting maths but is a onetime thing as the value isn't changing dynamically. 2) No it doesn't - then it is calibgain as it represents a necessary parameter to change the incoming circuit to compensate for external effects. It is possible you have a mixture of the two and hence need both but that is normally only the case with devices where the calibgain is about fixing the factory calibration. ... ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: meter ABI: (was Re: [PATCH v2 1/3] staging:iio:meter: Replaces IIO_DEV_ATTR_CH_OFF by IIO_DEVICE_ATTR)
On Sat, 24 Mar 2018 15:45:21 -0700
John Syne wrote:
> > On Mar 24, 2018, at 8:02 AM, Jonathan Cameron wrote:
> >
> > On Mon, 19 Mar 2018 22:57:16 -0700
> > John Syne wrote:
> >
> >> Hi Jonathan,
> >>
> >> Thank you for all your hard work. Your feedback is really helpful. I’m
> >> surprised that no one from Analog Device has offered any suggestions.
> >>
> >
> > Sadly those active in the mainline linux kernel from ADI are focused in
> > other areas currently :(
> Good point. I will reach out to Analog Devices and get a name of someone
> who is knowledgeable in this area. Perhaps Lars-Peters has a suggestion.
Yes, Lars or Michael Hennerich (+CC) are my normal contact points.
I believe this is outside both of their areas, but they may be able to
put you in contact with the right person. If nothing else it would be
good to make someone in ADI aware that people care about linux support for
these parts!
> >> Anyway, please see my comments inline below
> >>
> >> Regards,
> >> John
> >>
> >>
> >>
> >>
> >>
> >>> On Mar 18, 2018, at 5:23 AM, Jonathan Cameron wrote:
> >>>
> >>> On Sat, 17 Mar 2018 23:11:45 -0700
> >>> John Syne wrote:
> >>>
> Hi Jonathan,
> >>> Hi John and All,
> >>>
> >>> I'd love to get some additional input on this from anyone interested.
> >>> There are a lot of weird and wonderful derived quantities in an energy
> >>> meter and it seems we need to make some fundamental changes to support
> >>> them - including potentially a userspace breaking change to the event
> >>> code description.
> >>>
>
> Here is the complete list of registers for the ADE7878 which I copied
> from the data sheet. I added a column “IIO Attribute” which I hope
> follows your IIO ABI. Please make any changes you feel are incorrect.
> BTW, there are several registers that cannot be generalized and are used
> purely for chip configuration. I think we should add a new naming
> convention, namely {register}_{}. Also, I see in the
> sys_bus_iio doc
No, registers can always be broken up in to real meaningful values if
they are exposed to userspace and userspace has a reason to tweak
them.
We never expose raw registers to userspace except through
debugfs and the clue is in the name - purely for debug.
> >
> >
> >
> >
> in_accel_x_peak_raw
>
> so shouldn’t the phase be represented as follows:
>
> in_current_A_scale
> >>> I'm still confused. What does A represent here? I assumed that was a
> >>> wild
> >>> card for the channel number before but clearly not.
> >>>
> >>> Ah, you are labelling the 3 separate phases as A, B and C. Hmm.
> >>> I guess they sort of look like axis, and sort of like independent
> >>> channels.
> >>> So could be indexed or done via modifiers depending on how you look at
> >>> it.
> >> In metering terminology, the three phases are commonly referred to as RED,
> >> GREEN, BLUE or PhaseA, PhaseB, PhaseC and Neutral. Analog Devices uses the
> >> ABCN nomenclature so anyone using this driver will probably have
> >> referenced the Analog Devices datasheet so this terminology should be
> >> familiar.
> > Which is more commonly used in general? We are designing what we hope
> > will be a general interface and last thing we want is to have everyone
> > not using ADI parts confused! Personally not assigning 'colours' makes
> > more sense to me.
> I did a little research and here is the naming used by vendor:
>
> ST Microelectronics: P1, P2, P3, N
> http://www.st.com/content/ccc/resource/technical/document/application_note/5e/f4/22/4d/90/96/4c/c4/CD00153264.pdf/files/CD00153264.pdf/jcr:content/translations/en.CD00153264.pdf
>
> Teridian: ABCN
> https://www.mouser.com/ds/2/256/71M6513-71M6513H-22603.pdf
>
> Maxim Integrated: ABCN
> https://datasheets.maximintegrated.com/en/ds/78M6631.pdf
>
> Microchip: ABCN
> http://ww1.microchip.com/downloads/en/DeviceDoc/51643b.pdf
>
> NXP: L1, L2, L3, N
> https://www.nxp.com/support/developer-resources/reference-designs/kinetis-km3x-256-mcu-three-phase-metering-reference-design:RD-KM3x-256-3PH-METERING
>
> Renesas: ABCN
> https://www.renesas.com/en-us/solutions/home/metering/power-meter-three.html
>
> So the most consistent would be ABCN
Cool. Makes sense to go with that. Thanks for checking this out.
>
>
> All vendor providing three phase energy metering ICs use the ABCN
> terminology.
> >
> > (btw please wrap any lines that don't need to be long to around 80
> > characters).
> >
> >>>
> >>> Hmm. With neutral in there as well I guess we need to make them
> >>> modifiers (but might change my mind later ;)
> >>>
> >>> Particularly as we are using the the modifier for RMS under the previous
> >>> plan... It appears we should treat that instead like we did for peak
> >>> and do it as an additional info mask element. The problem with doing that
> >>> on a continuous measurement is that we can't treat it as a ch
Re: meter ABI: (was Re: [PATCH v2 1/3] staging:iio:meter: Replaces IIO_DEV_ATTR_CH_OFF by IIO_DEVICE_ATTR)
Hi Jonathan, I was speaking with Rodrigo and here is what I think must be done to move ADE7878 out of staging Here are the steps as I see them: 1) Define the IIO Attributes so they are consistent with the IIO ABI. This should be pretty simple given agreement on the naming convention. 2) Map the ADE7854 interrupt status to IIO events. This requires an interrupt processing section. 3) Add DeviceTree support. 4) Create DeviceTree overlay for the ADE7854. 5) Update ADE7854 probe to read in the DeviceTree register settings. 6) Add support for power modes (PM1, PM2). 7) Not sure if we will support measurement streaming on the ADE7854. The problem is ADE7854 is designed as an SPI master, which means it controls the SPI clock, so the driver must support SPI slave mode. However, the Linux Kernel does not currently support SPI slave mode. We have three choices to make this work and they are all a lot of work: 1) Add support for SPI Slave mode to the kernel, 2) Use hardware to convert SPI signals to I2S signals and with the use of a custom codec, use the ALSA framework to stream the samples (this is an approach I used, but I don’t like it), 3) Move the I2S driver out of the sound subsystem and use it together with DMA to stream samples directly into the ADE7854 driver (my preferred solutions). Perhaps Mark Brown has some ideas on how to make this work. The ADE9000 will be much easier because it uses an SPI Slave interface. I hope I have captured everything, but let me know if I have missed anything. Regards, John ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: meter ABI: (was Re: [PATCH v2 1/3] staging:iio:meter: Replaces IIO_DEV_ATTR_CH_OFF by IIO_DEVICE_ATTR)
> On Mar 24, 2018, at 8:18 AM, Jonathan Cameron wrote:
>
> On Mon, 19 Mar 2018 23:28:45 -0700
> John Syne wrote:
>
>> Hi Jonathan,
>>
>> I broke out the {Direction}_{Type}_{Index}_{Modifier}_{Info_Mask} into
>> separate columns to make sure I understand your instructions. Good way to
>> check the results.
>>
>> Probably easier to copy and paste this table into a spreadsheet. Let me know
>> if there is anything I got wrong. Thank you again for all your help.
> Yeah, we need to shrink this if we do it again.
I’ll send an updated copy after this e-mail. Can you accept a spreadsheet
attachment or a CSV file?
>
> Can drop anything not related to ABI (so RW mask address etc and just have the
> register name and the bits around ABI.
Done
>
> Note I mentioned the altvoltage stuff in the previous email, so we need
> to think about whether that is useful to distinguish the instantaneous
> measurements from the multi cycle AC ones.
> Actually I think we are fine here as we explicitly describe all
> alt measurements as mav or rms which going to be handled in our
> new computedvalue description.
Yeah, I don’t see how anyone would use these registers, so I propose
we just drop them.
>
>
>>
>> Address RegisterIIO Attribute Device Tree or Code
>> Direction TypeIndex ModifierInfo Mask R/W Bit
>> Length Bit Length During CommunicationsTypeDefault Value
>> Description
>> 0x4380 AIGAIN in_current0_phaseA_scalein current
>> 0 phaseA scale R/W 24 32 ZPSE S 0x00
>> Phase A current gain adjust.
>> 0x4381 AVGAIN in_voltage0_phaseA_scalein voltage
>> 0 phaseA scale R/W 24 32 ZPSE S 0x00
>> Phase A voltage gain adjust.
>> 0x4382 BIGAIN in_current0_phaseB_scalein current
>> 0 phaseB scale R/W 24 32 ZPSE S 0x00
>> Phase B current gain adjust.
>> 0x4383 BVGAIN in_voltage0_phaseB_scalein voltage
>> 0 phaseB scale R/W 24 32 ZPSE S 0x00
>> Phase B voltage gain adjust.
>> 0x4384 CIGAIN in_current0_phaseC_scalein current
>> 0 phaseC scale R/W 24 32 ZPSE S 0x00
>> Phase C current gain adjust.
>> 0x4385 CVGAIN in_voltage0_phaseC_scalein voltage
>> 0 phaseC scale R/W 24 32 ZPSE S 0x00
>> Phase C voltage gain adjust.
>> 0x4386 NIGAIN in_current0_neutral_scale in current
>> 0 neutral scale R/W 24 32 ZPSE S 0x00
>> Neutral current gain adjust (ADE7868 and ADE7878 only).
>> 0x4387 AIRMSOS in_current0_phaseA_rms_offset in current
>> 0 phaseA offset R/W 24 32 ZPSE S 0x00
>> Phase A current rms offset.
>> 0x4388 AVRMSOS in_voltage0_phaseA_rms_offset in voltage
>> 0 phaseA offset R/W 24 32 ZPSE S 0x00
>> Phase A voltage rms offset.
>> 0x4389 BIRMSOS in_current0_phaseB_rms_offset in current
>> 0 phaseB offset R/W 24 32 ZPSE S 0x00
>> Phase B current rms offset.
>> 0x438A BVRMSOS in_voltage0_phaseB_rms_offset in voltage
>> 0 phaseB offset R/W 24 32 ZPSE S 0x00
>> Phase B voltage rms offset.
>> 0x438B CIRMSOS in_current0_phaseC_rms_offset in current
>> 0 phaseC offset R/W 24 32 ZPSE S 0x00
>> Phase C current rms offset.
>> 0x438C CVRMSOS in_voltage0_phaseC_rms_offset in voltage
>> 0 phaseC offset R/W 24 32 ZPSE S 0x00
>> Phase C voltage rms offset.
>> 0x438D NIRMSOS in_current0_neutral_rms_offset in current
>> 0 neutral offset R/W 24 32 ZPSE S 0x00
>> Neutral current rms offset (ADE7868 and ADE7878 only).
>> 0x438E AVAGAIN in_powerapparent0_phaseA_scale in
>> powerapparent 0 phaseA scale R/W 24 32 ZPSE S
>> 0x00Phase A apparent power gain adjust.
>> 0x438F BVAGAIN in_powerapparent0_phaseB_scale in
>> powerapparent 0 phaseB scale R/W 24 32 ZPSE S
>> 0x00Phase B apparent power gain adjust.
>> 0x4390 CVAGAIN in_powerapparent0_phaseC_scale in
>> powerapparent 0 phaseC scale R/W 24 32 ZPSE S
>> 0x00Phase C apparent power gain adjust.
>> 0x4391 AWGAIN in_power0_phaseA_scale in power 0
>> phaseA scale R/W 24 32 ZPSE S 0x00Phase A
>> total active power gain adjust.
>> 0x4392 AWATTOS in_power0_phaseA_
Re: meter ABI: (was Re: [PATCH v2 1/3] staging:iio:meter: Replaces IIO_DEV_ATTR_CH_OFF by IIO_DEVICE_ATTR)
Hi Jonathan, I have attached a CSV version of this table below. Hopefully that will make things easier for you. RegisterIIO Attribute Device Tree or Code Direction type Index ModifierInfo Mask AIGAIN in_current0_phaseA_scalein current 0 phaseA scale AVGAIN in_voltage0_phaseA_scalein voltage 0 phaseA scale BIGAIN in_current0_phaseB_scalein current 0 phaseB scale BVGAIN in_voltage0_phaseB_scalein voltage 0 phaseB scale CIGAIN in_current0_phaseC_scalein current 0 phaseC scale CVGAIN in_voltage0_phaseC_scalein voltage 0 phaseC scale NIGAIN in_current0_neutral_scale in current 0 neutral scale AIRMSOS in_current0_phaseA_rms_offset in current 0 phaseA offset AVRMSOS in_voltage0_phaseA_rms_offset in voltage 0 phaseA offset BIRMSOS in_current0_phaseB_rms_offset in current 0 phaseB offset BVRMSOS in_voltage0_phaseB_rms_offset in voltage 0 phaseB offset CIRMSOS in_current0_phaseC_rms_offset in current 0 phaseC offset CVRMSOS in_voltage0_phaseC_rms_offset in voltage 0 phaseC offset NIRMSOS in_current0_neutral_rms_offset in current 0 neutral offset AVAGAIN in_powerapparent0_phaseA_scale in powerapparent 0 phaseA scale BVAGAIN in_powerapparent0_phaseB_scale in powerapparent 0 phaseB scale CVAGAIN in_powerapparent0_phaseC_scale in powerapparent 0 phaseC scale AWGAIN in_power0_phaseA_scale in power 0 phaseA scale AWATTOS in_power0_phaseA_offset in power 0 phaseA offset BWGAIN in_power0_phaseB_scale in power 0 phaseB scale BWATTOS in_power0_phaseB_offset in power 0 phaseB offset CWGAIN in_power0_PhaseC_scale in power 0 phaseC scale CWATTOS in_power0_phaseC_offset in power 0 phaseC offset AVARGAINin_powerreactive0_phaseA_scale in powerreactive 0 phaseA scale AVAROS in_powerreactive0_phaseA_offset in powerreactive 0 phaseA offset BVARGAINin_powerreactive0_phaseB_scale in powerreactive 0 phaseB scale BVAROS in_powerreactive0_phaseB_offset in powerreactive 0 phaseB offset CVARGAINin_powerreactive0_phaseC_scale in powerreactive 0 phaseC scale CVAROS in_powerreactive0_phaseC_offset in powerreactive 0 phaseC offset AFWGAIN in_power0_phaseA_fundamental_scale in power 0 phaseA_fundamental scale AFWATTOSin_power0_phaseA_fundamental_offset in power 0 phaseA_fundamental offset BFWGAIN in_power0_phaseB_fundamental_scale in power 0 phaseB_fundamental scale BFWATTOSin_power0_phaseB_fundamental_offset in power 0 phaseB_fundamental offset CFWGAIN in_power0_phaseC_fundamental_scale in power 0 phaseC_fundamental scale CFWATTOSin_power0_phaseC_fundamental_offset in power 0 phaseC_fundamental offset AFVARGAIN in_powerreactive0_phaseA_fundamental_scale in powerreactive 0 phaseA_fundamental scale AFVAROS in_powerreactive0_phaseA_fundamental_offset in powerreactive 0 phaseA_fundamental offset BFVARGAIN in_powerreactive0_phaseB_fundamental_scale in powerreactive 0 phaseB_fundamental scale BFVAROS in_powerreactive0_phaseB_fundamental_offset in powerreactive 0 phaseB_fundamental offset CFVARGAIN in_powerreactive0_phaseC_fundamental_scale in powerreactive 0 phaseC_fundamental scale CFVAROS in_powerreactive0_phaseC_fundamental_offset in powerreactive 0 phaseC_fundamental offset VATHR1 VATHR1 DT VATHR0 VATHR0 DT WTHR1 WTHR1 DT WTHR0 WTHR0 DT VARTHR1 VARTHR1 DT VARTHR0 VARTHR0 DT ReservedReserved VANOLOADVANOLOADDT APNOLOADAPNOLOADDT VLEVEL VLEVEL DT DICOEFF DICOEFF DT HPFDIS HPFDIS DT
Re: meter ABI: (was Re: [PATCH v2 1/3] staging:iio:meter: Replaces IIO_DEV_ATTR_CH_OFF by IIO_DEVICE_ATTR)
> On Mar 24, 2018, at 8:02 AM, Jonathan Cameron wrote:
>
> On Mon, 19 Mar 2018 22:57:16 -0700
> John Syne wrote:
>
>> Hi Jonathan,
>>
>> Thank you for all your hard work. Your feedback is really helpful. I’m
>> surprised that no one from Analog Device has offered any suggestions.
>>
>
> Sadly those active in the mainline linux kernel from ADI are focused in
> other areas currently :(
Good point. I will reach out to Analog Devices and get a name of someone
who is knowledgeable in this area. Perhaps Lars-Peters has a suggestion.
>> Anyway, please see my comments inline below
>>
>> Regards,
>> John
>>
>>
>>
>>
>>
>>> On Mar 18, 2018, at 5:23 AM, Jonathan Cameron wrote:
>>>
>>> On Sat, 17 Mar 2018 23:11:45 -0700
>>> John Syne wrote:
>>>
Hi Jonathan,
>>> Hi John and All,
>>>
>>> I'd love to get some additional input on this from anyone interested.
>>> There are a lot of weird and wonderful derived quantities in an energy
>>> meter and it seems we need to make some fundamental changes to support
>>> them - including potentially a userspace breaking change to the event
>>> code description.
>>>
Here is the complete list of registers for the ADE7878 which I copied from
the data sheet. I added a column “IIO Attribute” which I hope follows your
IIO ABI. Please make any changes you feel are incorrect. BTW, there are
several registers that cannot be generalized and are used purely for chip
configuration. I think we should add a new naming convention, namely
{register}_{}. Also, I see in the sys_bus_iio doc
>
>
>
>
in_accel_x_peak_raw
so shouldn’t the phase be represented as follows:
in_current_A_scale
>>> I'm still confused. What does A represent here? I assumed that was a wild
>>> card for the channel number before but clearly not.
>>>
>>> Ah, you are labelling the 3 separate phases as A, B and C. Hmm.
>>> I guess they sort of look like axis, and sort of like independent channels.
>>> So could be indexed or done via modifiers depending on how you look at it.
>> In metering terminology, the three phases are commonly referred to as RED,
>> GREEN, BLUE or PhaseA, PhaseB, PhaseC and Neutral. Analog Devices uses the
>> ABCN nomenclature so anyone using this driver will probably have referenced
>> the Analog Devices datasheet so this terminology should be familiar.
> Which is more commonly used in general? We are designing what we hope
> will be a general interface and last thing we want is to have everyone
> not using ADI parts confused! Personally not assigning 'colours' makes
> more sense to me.
I did a little research and here is the naming used by vendor:
ST Microelectronics: P1, P2, P3, N
http://www.st.com/content/ccc/resource/technical/document/application_note/5e/f4/22/4d/90/96/4c/c4/CD00153264.pdf/files/CD00153264.pdf/jcr:content/translations/en.CD00153264.pdf
Teridian: ABCN
https://www.mouser.com/ds/2/256/71M6513-71M6513H-22603.pdf
Maxim Integrated: ABCN
https://datasheets.maximintegrated.com/en/ds/78M6631.pdf
Microchip: ABCN
http://ww1.microchip.com/downloads/en/DeviceDoc/51643b.pdf
NXP: L1, L2, L3, N
https://www.nxp.com/support/developer-resources/reference-designs/kinetis-km3x-256-mcu-three-phase-metering-reference-design:RD-KM3x-256-3PH-METERING
Renesas: ABCN
https://www.renesas.com/en-us/solutions/home/metering/power-meter-three.html
So the most consistent would be ABCN
All vendor providing three phase energy metering ICs use the ABCN terminology.
>
> (btw please wrap any lines that don't need to be long to around 80
> characters).
>
>>>
>>> Hmm. With neutral in there as well I guess we need to make them
>>> modifiers (but might change my mind later ;)
>>>
>>> Particularly as we are using the the modifier for RMS under the previous
>>> plan... It appears we should treat that instead like we did for peak
>>> and do it as an additional info mask element. The problem with doing that
>>> on a continuous measurement is that we can't treat it as a channel to
>>> be output through the buffered interface.
>> I’ve changed the layout of the spreadsheet to breakout the Direction, Type,
>> Index, Modifier and Info Mask to make sure there is no miss-understanding
>> and will help identify all the items we need to add.
>>
>> The ADE7878 channels that will use the buffer are IAWV, VAWV, IBWV, VBWV,
>> ICWV, VCWV, INWV, AVA, BVA, CVA, AWATT, BWATT, CWATT, AVAR, BVAR, and CVAR.
>> So I guess we have to add an index
> Probably a good idea anyway, we are sort of slowly moving away
> from index less channels. The ABI always allowed index assignment
> and it makes for easier userspace code.
>
>>
>> Address Register IIO Attribute
>> Dir TypeIndex ModifierInfo Mask R/W
>>Bit Bit Length TypeDefault Description
>>
Re: meter ABI: (was Re: [PATCH v2 1/3] staging:iio:meter: Replaces IIO_DEV_ATTR_CH_OFF by IIO_DEVICE_ATTR)
On Mon, 19 Mar 2018 23:28:45 -0700
John Syne wrote:
> Hi Jonathan,
>
> I broke out the {Direction}_{Type}_{Index}_{Modifier}_{Info_Mask} into
> separate columns to make sure I understand your instructions. Good way to
> check the results.
>
> Probably easier to copy and paste this table into a spreadsheet. Let me know
> if there is anything I got wrong. Thank you again for all your help.
Yeah, we need to shrink this if we do it again.
Can drop anything not related to ABI (so RW mask address etc and just have the
register name and the bits around ABI.
Note I mentioned the altvoltage stuff in the previous email, so we need
to think about whether that is useful to distinguish the instantaneous
measurements from the multi cycle AC ones.
Actually I think we are fine here as we explicitly describe all
alt measurements as mav or rms which going to be handled in our
new computedvalue description.
>
> Address RegisterIIO Attribute Device Tree or Code
> Direction TypeIndex ModifierInfo Mask R/W Bit
> Length Bit Length During CommunicationsTypeDefault Value
> Description
> 0x4380AIGAIN in_current0_phaseA_scalein current
> 0 phaseA scale R/W 24 32 ZPSE S 0x00Phase
> A current gain adjust.
> 0x4381AVGAIN in_voltage0_phaseA_scalein voltage
> 0 phaseA scale R/W 24 32 ZPSE S 0x00Phase
> A voltage gain adjust.
> 0x4382BIGAIN in_current0_phaseB_scalein current
> 0 phaseB scale R/W 24 32 ZPSE S 0x00Phase
> B current gain adjust.
> 0x4383BVGAIN in_voltage0_phaseB_scalein voltage
> 0 phaseB scale R/W 24 32 ZPSE S 0x00Phase
> B voltage gain adjust.
> 0x4384CIGAIN in_current0_phaseC_scalein current
> 0 phaseC scale R/W 24 32 ZPSE S 0x00Phase
> C current gain adjust.
> 0x4385CVGAIN in_voltage0_phaseC_scalein voltage
> 0 phaseC scale R/W 24 32 ZPSE S 0x00Phase
> C voltage gain adjust.
> 0x4386NIGAIN in_current0_neutral_scale in current
> 0 neutral scale R/W 24 32 ZPSE S 0x00
> Neutral current gain adjust (ADE7868 and ADE7878 only).
> 0x4387AIRMSOS in_current0_phaseA_rms_offset in current
> 0 phaseA offset R/W 24 32 ZPSE S 0x00Phase
> A current rms offset.
> 0x4388AVRMSOS in_voltage0_phaseA_rms_offset in voltage
> 0 phaseA offset R/W 24 32 ZPSE S 0x00Phase
> A voltage rms offset.
> 0x4389BIRMSOS in_current0_phaseB_rms_offset in current
> 0 phaseB offset R/W 24 32 ZPSE S 0x00Phase
> B current rms offset.
> 0x438ABVRMSOS in_voltage0_phaseB_rms_offset in voltage
> 0 phaseB offset R/W 24 32 ZPSE S 0x00Phase
> B voltage rms offset.
> 0x438BCIRMSOS in_current0_phaseC_rms_offset in current
> 0 phaseC offset R/W 24 32 ZPSE S 0x00Phase
> C current rms offset.
> 0x438CCVRMSOS in_voltage0_phaseC_rms_offset in voltage
> 0 phaseC offset R/W 24 32 ZPSE S 0x00Phase
> C voltage rms offset.
> 0x438DNIRMSOS in_current0_neutral_rms_offset in current
> 0 neutral offset R/W 24 32 ZPSE S 0x00
> Neutral current rms offset (ADE7868 and ADE7878 only).
> 0x438EAVAGAIN in_powerapparent0_phaseA_scale in
> powerapparent 0 phaseA scale R/W 24 32 ZPSE S
> 0x00Phase A apparent power gain adjust.
> 0x438FBVAGAIN in_powerapparent0_phaseB_scale in
> powerapparent 0 phaseB scale R/W 24 32 ZPSE S
> 0x00Phase B apparent power gain adjust.
> 0x4390CVAGAIN in_powerapparent0_phaseC_scale in
> powerapparent 0 phaseC scale R/W 24 32 ZPSE S
> 0x00Phase C apparent power gain adjust.
> 0x4391AWGAIN in_power0_phaseA_scale in power 0
> phaseA scale R/W 24 32 ZPSE S 0x00Phase A total
> active power gain adjust.
> 0x4392AWATTOS in_power0_phaseA_offset in power 0
> phaseA offset R/W 24 32 ZPSE S 0x00Phase A total
> active power offset adjust.
> 0x4393BWGAIN in_power0_phaseB_scale in power 0
> phaseB scale R/W 24 32 ZPSE S 0x00Phase B total
> active power gain a
Re: meter ABI: (was Re: [PATCH v2 1/3] staging:iio:meter: Replaces IIO_DEV_ATTR_CH_OFF by IIO_DEVICE_ATTR)
On Mon, 19 Mar 2018 22:57:16 -0700
John Syne wrote:
> Hi Jonathan,
>
> Thank you for all your hard work. Your feedback is really helpful. I’m
> surprised that no one from Analog Device has offered any suggestions.
>
Sadly those active in the mainline linux kernel from ADI are focused in
other areas currently :(
> Anyway, please see my comments inline below
>
> Regards,
> John
>
>
>
>
>
> > On Mar 18, 2018, at 5:23 AM, Jonathan Cameron wrote:
> >
> > On Sat, 17 Mar 2018 23:11:45 -0700
> > John Syne wrote:
> >
> >> Hi Jonathan,
> > Hi John and All,
> >
> > I'd love to get some additional input on this from anyone interested.
> > There are a lot of weird and wonderful derived quantities in an energy
> > meter and it seems we need to make some fundamental changes to support
> > them - including potentially a userspace breaking change to the event
> > code description.
> >
> >>
> >> Here is the complete list of registers for the ADE7878 which I copied from
> >> the data sheet. I added a column “IIO Attribute” which I hope follows your
> >> IIO ABI. Please make any changes you feel are incorrect. BTW, there are
> >> several registers that cannot be generalized and are used purely for chip
> >> configuration. I think we should add a new naming convention, namely
> >> {register}_{}. Also, I see in the sys_bus_iio doc
> >> in_accel_x_peak_raw
> >>
> >> so shouldn’t the phase be represented as follows:
> >>
> >> in_current_A_scale
> > I'm still confused. What does A represent here? I assumed that was a wild
> > card for the channel number before but clearly not.
> >
> > Ah, you are labelling the 3 separate phases as A, B and C. Hmm.
> > I guess they sort of look like axis, and sort of like independent channels.
> > So could be indexed or done via modifiers depending on how you look at it.
> In metering terminology, the three phases are commonly referred to as RED,
> GREEN, BLUE or PhaseA, PhaseB, PhaseC and Neutral. Analog Devices uses the
> ABCN nomenclature so anyone using this driver will probably have referenced
> the Analog Devices datasheet so this terminology should be familiar.
Which is more commonly used in general? We are designing what we hope
will be a general interface and last thing we want is to have everyone
not using ADI parts confused! Personally not assigning 'colours' makes
more sense to me.
(btw please wrap any lines that don't need to be long to around 80 characters).
> >
> > Hmm. With neutral in there as well I guess we need to make them
> > modifiers (but might change my mind later ;)
> >
> > Particularly as we are using the the modifier for RMS under the previous
> > plan... It appears we should treat that instead like we did for peak
> > and do it as an additional info mask element. The problem with doing that
> > on a continuous measurement is that we can't treat it as a channel to
> > be output through the buffered interface.
> I’ve changed the layout of the spreadsheet to breakout the Direction, Type,
> Index, Modifier and Info Mask to make sure there is no miss-understanding and
> will help identify all the items we need to add.
>
> The ADE7878 channels that will use the buffer are IAWV, VAWV, IBWV, VBWV,
> ICWV, VCWV, INWV, AVA, BVA, CVA, AWATT, BWATT, CWATT, AVAR, BVAR, and CVAR.
> So I guess we have to add an index
Probably a good idea anyway, we are sort of slowly moving away
from index less channels. The ABI always allowed index assignment
and it makes for easier userspace code.
>
> Address Register IIO Attribute
> Dir TypeIndex ModifierInfo Mask R/W
> Bit Bit Length TypeDefault Description
>
>
> Length During Comm
> Value
> 0xE50CIAWVin_current0_phaseA_instantaneousin
> current 0 phaseA instantaneous R 24
> 32 SE S N/A Instantaneous value of
> Phase A current.
I'm unconvinced by "instantaneous". That is the default assumption that you are
measuring current at this point in time. I'm still confused on what this
actually
is. Is it effectively the DC current? I.e. the wave form value for current?
You answer this below. They are DC measurements..
Which actually raises a point I'd forgotten. We have an explicit type
for altvoltage (defined for DDS devices as the waveform amplitude IIRC).
We should be consistent with that if possible.
So I think this should be.
in_current0_phaseA_raw
etc
A channel can be uniquely identified using index and modifier as as pair (if we
add
the new field for computation applied that will also allow separate
identification).
So the late
Re: meter ABI: (was Re: [PATCH v2 1/3] staging:iio:meter: Replaces IIO_DEV_ATTR_CH_OFF by IIO_DEVICE_ATTR)
Hi Jonathan,
I broke out the {Direction}_{Type}_{Index}_{Modifier}_{Info_Mask} into separate
columns to make sure I understand your instructions. Good way to check the
results.
Probably easier to copy and paste this table into a spreadsheet. Let me know if
there is anything I got wrong. Thank you again for all your help.
Address RegisterIIO Attribute Device Tree or Code Direction
TypeIndex ModifierInfo Mask R/W Bit Length Bit
Length During CommunicationsTypeDefault Value Description
0x4380 AIGAIN in_current0_phaseA_scalein current 0
phaseA scale R/W 24 32 ZPSE S 0x00Phase A current
gain adjust.
0x4381 AVGAIN in_voltage0_phaseA_scalein voltage 0
phaseA scale R/W 24 32 ZPSE S 0x00Phase A voltage
gain adjust.
0x4382 BIGAIN in_current0_phaseB_scalein current 0
phaseB scale R/W 24 32 ZPSE S 0x00Phase B current
gain adjust.
0x4383 BVGAIN in_voltage0_phaseB_scalein voltage 0
phaseB scale R/W 24 32 ZPSE S 0x00Phase B voltage
gain adjust.
0x4384 CIGAIN in_current0_phaseC_scalein current 0
phaseC scale R/W 24 32 ZPSE S 0x00Phase C current
gain adjust.
0x4385 CVGAIN in_voltage0_phaseC_scalein voltage 0
phaseC scale R/W 24 32 ZPSE S 0x00Phase C voltage
gain adjust.
0x4386 NIGAIN in_current0_neutral_scale in current 0
neutral scale R/W 24 32 ZPSE S 0x00Neutral current
gain adjust (ADE7868 and ADE7878 only).
0x4387 AIRMSOS in_current0_phaseA_rms_offset in current 0
phaseA offset R/W 24 32 ZPSE S 0x00Phase A current
rms offset.
0x4388 AVRMSOS in_voltage0_phaseA_rms_offset in voltage 0
phaseA offset R/W 24 32 ZPSE S 0x00Phase A voltage
rms offset.
0x4389 BIRMSOS in_current0_phaseB_rms_offset in current 0
phaseB offset R/W 24 32 ZPSE S 0x00Phase B current
rms offset.
0x438A BVRMSOS in_voltage0_phaseB_rms_offset in voltage 0
phaseB offset R/W 24 32 ZPSE S 0x00Phase B voltage
rms offset.
0x438B CIRMSOS in_current0_phaseC_rms_offset in current 0
phaseC offset R/W 24 32 ZPSE S 0x00Phase C current
rms offset.
0x438C CVRMSOS in_voltage0_phaseC_rms_offset in voltage 0
phaseC offset R/W 24 32 ZPSE S 0x00Phase C voltage
rms offset.
0x438D NIRMSOS in_current0_neutral_rms_offset in current 0
neutral offset R/W 24 32 ZPSE S 0x00Neutral current
rms offset (ADE7868 and ADE7878 only).
0x438E AVAGAIN in_powerapparent0_phaseA_scale in powerapparent
0 phaseA scale R/W 24 32 ZPSE S 0x00Phase A
apparent power gain adjust.
0x438F BVAGAIN in_powerapparent0_phaseB_scale in powerapparent
0 phaseB scale R/W 24 32 ZPSE S 0x00Phase B
apparent power gain adjust.
0x4390 CVAGAIN in_powerapparent0_phaseC_scale in powerapparent
0 phaseC scale R/W 24 32 ZPSE S 0x00Phase C
apparent power gain adjust.
0x4391 AWGAIN in_power0_phaseA_scale in power 0 phaseA
scale R/W 24 32 ZPSE S 0x00Phase A total active
power gain adjust.
0x4392 AWATTOS in_power0_phaseA_offset in power 0 phaseA
offset R/W 24 32 ZPSE S 0x00Phase A total active
power offset adjust.
0x4393 BWGAIN in_power0_phaseB_scale in power 0 phaseB
scale R/W 24 32 ZPSE S 0x00Phase B total active
power gain adjust.
0x4394 BWATTOS in_power0_phaseB_offset in power 0 phaseB
offset R/W 24 32 ZPSE S 0x00Phase B total active
power offset adjust.
0x4395 CWGAIN in_power0_PhaseC_scale in power 0 phaseC
scale R/W 24 32 ZPSE S 0x00Phase C total active
power gain adjust.
0x4396 CWATTOS in_power0_phaseC_offset in power 0 phaseC
offset R/W 24 32 ZPSE S 0x00Phase C total active
power offset adjust.
0x4397 AVARGAINin_powerreactive0_phaseA_scale in
powerreactive 0 phaseA scale R/W 24 32 ZPSE S
0x00Phase A total reactive power gain adjust (ADE7858, ADE7868, and
ADE7878).
0x4398 AVAROS in_powerreactive0_phaseA_offset in
Re: meter ABI: (was Re: [PATCH v2 1/3] staging:iio:meter: Replaces IIO_DEV_ATTR_CH_OFF by IIO_DEVICE_ATTR)
Hi Jonathan,
Thank you for all your hard work. Your feedback is really helpful. I’m
surprised that no one from Analog Device has offered any suggestions.
Anyway, please see my comments inline below
Regards,
John
> On Mar 18, 2018, at 5:23 AM, Jonathan Cameron wrote:
>
> On Sat, 17 Mar 2018 23:11:45 -0700
> John Syne wrote:
>
>> Hi Jonathan,
> Hi John and All,
>
> I'd love to get some additional input on this from anyone interested.
> There are a lot of weird and wonderful derived quantities in an energy
> meter and it seems we need to make some fundamental changes to support
> them - including potentially a userspace breaking change to the event
> code description.
>
>>
>> Here is the complete list of registers for the ADE7878 which I copied from
>> the data sheet. I added a column “IIO Attribute” which I hope follows your
>> IIO ABI. Please make any changes you feel are incorrect. BTW, there are
>> several registers that cannot be generalized and are used purely for chip
>> configuration. I think we should add a new naming convention, namely
>> {register}_{}. Also, I see in the sys_bus_iio doc
>> in_accel_x_peak_raw
>>
>> so shouldn’t the phase be represented as follows:
>>
>> in_current_A_scale
> I'm still confused. What does A represent here? I assumed that was a wild
> card for the channel number before but clearly not.
>
> Ah, you are labelling the 3 separate phases as A, B and C. Hmm.
> I guess they sort of look like axis, and sort of like independent channels.
> So could be indexed or done via modifiers depending on how you look at it.
In metering terminology, the three phases are commonly referred to as RED,
GREEN, BLUE or PhaseA, PhaseB, PhaseC and Neutral. Analog Devices uses the ABCN
nomenclature so anyone using this driver will probably have referenced the
Analog Devices datasheet so this terminology should be familiar.
>
> Hmm. With neutral in there as well I guess we need to make them
> modifiers (but might change my mind later ;)
>
> Particularly as we are using the the modifier for RMS under the previous
> plan... It appears we should treat that instead like we did for peak
> and do it as an additional info mask element. The problem with doing that
> on a continuous measurement is that we can't treat it as a channel to
> be output through the buffered interface.
I’ve changed the layout of the spreadsheet to breakout the Direction, Type,
Index, Modifier and Info Mask to make sure there is no miss-understanding and
will help identify all the items we need to add.
The ADE7878 channels that will use the buffer are IAWV, VAWV, IBWV, VBWV, ICWV,
VCWV, INWV, AVA, BVA, CVA, AWATT, BWATT, CWATT, AVAR, BVAR, and CVAR. So I
guess we have to add an index
Address RegisterIIO Attribute
Dir TypeIndex ModifierInfo Mask R/W
Bit Bit Length TypeDefault Description
Length During Comm
Value
0xE50C IAWVin_current0_phaseA_instantaneousin current
0 phaseA instantaneous R 24 32 SE
S N/A Instantaneous value of Phase A current.
0xE50D IBWVin_current1_phaseB_instantaneousin current
1 phaseB instantaneous R 24 32 SE
S N/A Instantaneous value of Phase B current.
0xE50E ICWVin_current2_phaseC_instantaneousin current
2 phaseC instantaneous R 24 32 SE
S N/A Instantaneous value of Phase C current.
0xE50F INWVin_current3_phaseN_instantaneousin current
3 neutral instantaneous R 24 32 SE
S N/A Instantaneous value of neutral current
(ADE7868 and ADE7878 only).
0xE510 VAWVin_voltage4_phaseA_instantaneousin voltage
4 phaseA instantaneous R 24 32 SE
S N/A Instantaneous value of Phase A voltage.
0xE511 VBWVin_voltage5_phaseB_instantaneousin voltage
5 phaseB instantaneous R 24 32 SE
S N/A Instantaneous value of Phase B voltage.
0xE512 VCWVin_voltage6_phaseC_instantaneousin voltage
6 phaseC instantaneous R 24 32 SE
S N/A Instantaneous value of Phase C voltage.
0xE513 AWATT in_power7_phaseA_instantaneous
meter ABI: (was Re: [PATCH v2 1/3] staging:iio:meter: Replaces IIO_DEV_ATTR_CH_OFF by IIO_DEVICE_ATTR)
On Sat, 17 Mar 2018 23:11:45 -0700
John Syne wrote:
> Hi Jonathan,
Hi John and All,
I'd love to get some additional input on this from anyone interested.
There are a lot of weird and wonderful derived quantities in an energy
meter and it seems we need to make some fundamental changes to support
them - including potentially a userspace breaking change to the event
code description.
>
> Here is the complete list of registers for the ADE7878 which I copied from
> the data sheet. I added a column “IIO Attribute” which I hope follows your
> IIO ABI. Please make any changes you feel are incorrect. BTW, there are
> several registers that cannot be generalized and are used purely for chip
> configuration. I think we should add a new naming convention, namely
> {register}_{}. Also, I see in the sys_bus_iio doc
> in_accel_x_peak_raw
>
> so shouldn’t the phase be represented as follows:
>
> in_current_A_scale
I'm still confused. What does A represent here? I assumed that was a wild
card for the channel number before but clearly not.
Ah, you are labelling the 3 separate phases as A, B and C. Hmm.
I guess they sort of look like axis, and sort of like independent channels.
So could be indexed or done via modifiers depending on how you look at it.
Hmm. With neutral in there as well I guess we need to make them
modifiers (but might change my mind later ;)
Particularly as we are using the the modifier for RMS under the previous
plan... It appears we should treat that instead like we did for peak
and do it as an additional info mask element. The problem with doing that
on a continuous measurement is that we can't treat it as a channel to
be output through the buffered interface.
So again we have run out of space. It's increasingly looking like we need
room for another field in the events - to cleanly represent computed values.
Hmm. What is the current usage? - it's been a while so I had to go
look in the header.
0-15 Channel (lots of channels)
31-16 Channel 2 (36 modifiers - lots of channels)
47-32 Channel Type (31 used so far - this looks most likely to run out of
space in the long run so leave this one alone).
54-48 Event Direction (4 used)
55 Differential (1: channel 2 as differential pair 0: as a modifier)
63-56 Event Type (6 used)
So I think we can pinch bit 53 as another flag to indicate we have
a computed value or possibly bit 63 as event types are few and
far between as well.
Probably reasonable to assume we never have 16 bits worth
of channels and computed channels at the same time?
Hence I think we can steal bits off the top of Channel.
How many do we need? Not sure unfortunately but feels like
8 should be plenty.
The other element of this is we add a new field to iio_chan_spec
to contain 'derived_type' or something like that which has
rms and sum squared etc. Over time we can move some of those
from the modifiers and free up a few entires there.
So modifier might be "X and Y and Z" with a derived_type of
sum_squared to give existing sum_squared_x_y_z but no
rush on that.
Anyhow so now we have an extra element to play that will result
in a different channel.
Whilst here we should think about any other mods needed to
that event structure. It is a little unfortunate that this
will be a breaking change for any old userspace code playing
with new drivers but it can't be helped as we didn't have
reserved values in the original definition (oops).
At somepoint we may need to add the 'shared by derived_value'
info mask but I think we can ignore that for now (seems
moderately unlikely to have anything in it!)
>
> But for now, I followed your instructions from your reply.
>
> After finalizing this one, I will work on the ADE9000, which as way more
> registers ;-)
>
> Once we can agree on the register naming, I will update the ADE7854 driver
> for Rodrigo, which will go a long way to getting it out of staging.
I'll edit to fit with new scheme and insert indexes which I think would be
preferred though optional under the ABI as we only have one of each type/
>
> Address RegisterIIO Attribute R/W Bit Length Bit
> Length During CommunicationsTypeDefault Value Description
> 0x4380AIGAIN in_current0_phaseA_scaleR/W 24 32 ZPSE
> S 0x00Phase A current gain adjust.
A, B, C, N aren't obvious to the lay reader so I suggest we burn a few
characters and stick phase in for ABC and just have neutral for
the neutral one. Not sure about capitalization or not though.
> 0x4381AVGAIN in_voltage0_phaseA_scaleR/W 24 32 ZPSE
> S 0x00Phase A voltage gain adjust.
> 0x4382BIGAIN in_current0_phaseB_scaleR/W 24 32 ZPSE
> S 0x00Phase B current gain adjust.
> 0x4383BVGAIN in_voltage0_phaseB_scaleR/W 24 32 ZPSE
> S 0x00Phase B voltage gain adjust.
> 0x4384CIGAIN in_current0_phaseC_scale
Re: [PATCH v2 1/3] staging:iio:meter: Replaces IIO_DEV_ATTR_CH_OFF by IIO_DEVICE_ATTR
Hi Jonathan,
Here is the complete list of registers for the ADE7878 which I copied from the
data sheet. I added a column “IIO Attribute” which I hope follows your IIO ABI.
Please make any changes you feel are incorrect. BTW, there are several
registers that cannot be generalized and are used purely for chip
configuration. I think we should add a new naming convention, namely
{register}_{}. Also, I see in the sys_bus_iio doc
in_accel_x_peak_raw
so shouldn’t the phase be represented as follows:
in_current_A_scale
But for now, I followed your instructions from your reply.
After finalizing this one, I will work on the ADE9000, which as way more
registers ;-)
Once we can agree on the register naming, I will update the ADE7854 driver for
Rodrigo, which will go a long way to getting it out of staging.
Address RegisterIIO Attribute R/W Bit Length Bit Length
During CommunicationsTypeDefault Value Description
0x4380 AIGAIN in_currentA_scale R/W 24 32 ZPSE S
0x00Phase A current gain adjust.
0x4381 AVGAIN in_voltageA_scale R/W 24 32 ZPSE S
0x00Phase A voltage gain adjust.
0x4382 BIGAIN in_currentB_scale R/W 24 32 ZPSE S
0x00Phase B current gain adjust.
0x4383 BVGAIN in_voltageB_scale R/W 24 32 ZPSE S
0x00Phase B voltage gain adjust.
0x4384 CIGAIN in_currentC_scale R/W 24 32 ZPSE S
0x00Phase C current gain adjust.
0x4385 CVGAIN in_voltageC_scale R/W 24 32 ZPSE S
0x00Phase C voltage gain adjust.
0x4386 NIGAIN in_currentN_scale R/W 24 32 ZPSE S
0x00Neutral current gain adjust (ADE7868 and ADE7878 only).
0x4387 AIRMSOS in_currentA_rms_offset R/W 24 32 ZPSE S
0x00Phase A current rms offset.
0x4388 AVRMSOS in_voltageA_rms_offset R/W 24 32 ZPSE S
0x00Phase A voltage rms offset.
0x4389 BIRMSOS in_currentB_rms_offset R/W 24 32 ZPSE S
0x00Phase B current rms offset.
0x438A BVRMSOS in_voltageB_rms_offset R/W 24 32 ZPSE S
0x00Phase B voltage rms offset.
0x438B CIRMSOS in_currentC_rms_offset R/W 24 32 ZPSE S
0x00Phase C current rms offset.
0x438C CVRMSOS in_voltageC_rms_offset R/W 24 32 ZPSE S
0x00Phase C voltage rms offset.
0x438D NIRMSOS in_currentN_rms_offset R/W 24 32 ZPSE S
0x00Neutral current rms offset (ADE7868 and ADE7878 only).
0x438E AVAGAIN in_powerapparentA_scale R/W 24 32 ZPSE S
0x00Phase A apparent power gain adjust.
0x438F BVAGAIN in_powerapparentB_scale R/W 24 32 ZPSE S
0x00Phase B apparent power gain adjust.
0x4390 CVAGAIN in_powerapparentC_scale R/W 24 32 ZPSE S
0x00Phase C apparent power gain adjust.
0x4391 AWGAIN in_powerA_scale R/W 24 32 ZPSE S 0x00
Phase A total active power gain adjust.
0x4392 AWATTOS in_powerA_offsetR/W 24 32 ZPSE S
0x00Phase A total active power offset adjust.
0x4393 BWGAIN in_powerB_scale R/W 24 32 ZPSE S 0x00
Phase B total active power gain adjust.
0x4394 BWATTOS in_powerB_offsetR/W 24 32 ZPSE S
0x00Phase B total active power offset adjust.
0x4395 CWGAIN in_powerC_scale R/W 24 32 ZPSE S 0x00
Phase C total active power gain adjust.
0x4396 CWATTOS in_powerC_offsetR/W 24 32 ZPSE S
0x00Phase C total active power offset adjust.
0x4397 AVARGAINin_powerreactiveA_scale R/W 24 32 ZPSE S
0x00Phase A total reactive power gain adjust (ADE7858, ADE7868, and
ADE7878).
0x4398 AVAROS in_powerreactiveA_offsetR/W 24 32 ZPSE S
0x00Phase A total reactive power offset adjust (ADE7858, ADE7868,
and ADE7878).
0x4399 BVARGAINin_powerreactiveB_scale R/W 24 32 ZPSE S
0x00Phase B total reactive power gain adjust (ADE7858, ADE7868, and
ADE7878).
0x439A BVAROS in_powerreactiveB_offsetR/W 24 32 ZPSE S
0x00Phase B total reactive power offset adjust (ADE7858, ADE7868,
and ADE7878).
0x439B CVARGAINin_powerreactiveC_scale R/W 24 32 ZPSE S
0x00Phase C total reactive power gain adjust (ADE7858, ADE7868, and
ADE7878).
0x439C CVAROS in_powerreactiveC_offsetR/W 24 32 ZPSE S
0x00Phase C total reactive power offset adjust (ADE7858, ADE7868,
and ADE7878).
0x439D AFWGAIN in_powerA_fundamental_scale R/W 24 32 ZPSE S
0x00Phase A fundamental active power gain adjust. Location reserved
for ADE7
Re: [PATCH v2 1/3] staging:iio:meter: Replaces IIO_DEV_ATTR_CH_OFF by IIO_DEVICE_ATTR
On Wed, 14 Mar 2018 23:12:02 -0700
John Syne wrote:
> Hi Jonathan,
>
> I have been looking at the IIO ABI docs and if I understand
> correctly, the idea is to use consistent naming conventions? So for
> example, looking at the ADE7854 datasheet, the naming matching the
> ADE7854 registers would be as follows:
Welcome to one of the big reasons no one tidied these drivers
up originally. Still we have moved on somewhat since then
so similar circumstances have come up in other types of sensor.
>
> {direction}_{type}_{index}_{modifier}_{info_mask}
>
> AIGAIN- In_current_a_gain
Other than the fact that gain isn't an ABI element and that index
doesn't have
_ before it that is right.
in_voltageA_scale
That was a weird quirk a long time back which we should probably
not have done (copied it from hwmon)
> AVGAIN- in_voltage_a_gain
> BIGAIN- in_current_b_gain
> BVGAIN- in_voltage_b_gain
> —
> How do we represent the rms and offset
> AIRMSOS - in_current_a_rmsoffset
> AVRMSOS - in_voltage_a_rmsoffset
Right now we can't unfortunately though this one is easier to fix.
We already have modifiers for multi axis devices doing sum_squared
so add one of those for root_mean_square - this one is well known
enough that rms is fine in the string.
It's a effectively a different channel be it one derived from a simple
one. This is going to get tricky however as we would normally use
modifier to specialise a channel type - thoughts on this below.
> —
> Here I don’t understand how to represent both the phase and the
> active/reactive/apparent power components. Do we combine the phase and
> quadrature part like this
> AVAGAIN - in_power_a_gain /*
> apparent power */
> —
> AWGAIN- in_power_ai_gain
> /* active power */
And that is the problem. How do we represent the various power types.
Hmm. We could do it with modifiers but above we show that we have already used
them.
It would be easy enough to add yet another field to the channel spec to specify
this but there is a problem - Events. The event format is already pretty full
so where do we put this extra element if we need to define a channel separated
only by it.
One thought is we could instead define these as different top level
IIO_CHAN_TYPES in a similar fashion to we do for relative humidity vs
the proposed absolute humidity.
in_powerreactiveA_scale
in_poweractivceA_scale
(or in_powerrealA_scale to match with what I got taught years ago?)
I presume we keep in_powerA_scale etc for the apparent power and
modify any docs to make that clear.
> —
> AVARGAIN - in_power_aq_gain/*
> reactive power */
> —
> Now here it becomes more complicated. Not sure how this gets handled.
> AFWATTOS - in_power_a_active/fundamental/offset
Yeah, some of these are getting odd.
If I read this correctly this is the active power estimate based on only
the fundamental frequency - so no harmonics?
Hmm. This then becomes a separate channel with additional properties
specifying it is only the fundamental. This feels a bit like a filter
be it an unusual one? Might just be necessary to add a _fundamental_only
element on the end (would be info_mask if this is common enough to
justify that rather than using the extended methods to define it.).
> —
> AWATTHR - in_energy_ai_accumulation
Great, just when I thought we had gone far enough they define reactive
energy which is presumably roughly the same as reactivepower * time?
In that case we need types for that as well.
in_energyreactiveA_*
in_energyactiveA_*
> —
> AVARHR- in_energy_aq_accumulation
> —
> IPEAK - in_current_peak
That one is easy as we have an info_mask element for peak and one
for peak_scale that has always been a bit odd but was needed somewhere.
> —
>
> I’ll leave it there, because there are some even more complicated register
> naming issues.
Something to look forward to. Gah, I always hated power engineering
though I taught it very briefly (I really pity those students :(
Jonathan
>
> Regards,
> John
>
>
>
>
>
> > On Mar 10, 2018, at 7:10 AM, Jonathan Cameron wrote:
> >
> > On Thu, 8 Mar 2018 21:37:33 -0300
> > Rodrigo Siqueira wrote:
> >
> >> On 03/07, Jonathan Cameron wrote:
> >>> On Tue, 6 Mar 2018 21:43:47 -0300
> >>> Rodrigo Siqueira wrote:
> >>>
> The macro IIO_DEV_ATTR_CH_OFF is a wrapper for IIO_DEVICE_ATTR, with a
> tiny change in the name definition. This extra macro does not improve
> the readability and also creates some checkpatch errors.
>
> This patch fixes the checkpatch.pl errors:
>
> staging/iio/meter/ade7753.c:391: ERROR: Use 4 digit octal (0777) not
> decimal permissions
> staging/iio/meter/ade7753.c:395: ERROR: Use 4 digit octal (0777) not
> decim
Re: [PATCH v2 1/3] staging:iio:meter: Replaces IIO_DEV_ATTR_CH_OFF by IIO_DEVICE_ATTR
Hi Jonathan,
I have been looking at the IIO ABI docs and if I understand correctly, the idea
is to use consistent naming conventions? So for example, looking at the ADE7854
datasheet, the naming matching the ADE7854 registers would be as follows:
{direction}_{type}_{index}_{modifier}_{info_mask}
AIGAIN - In_current_a_gain
AVGAIN - in_voltage_a_gain
BIGAIN - in_current_b_gain
BVGAIN - in_voltage_b_gain
—
How do we represent the rms and offset
AIRMSOS - in_current_a_rmsoffset
AVRMSOS - in_voltage_a_rmsoffset
—
Here I don’t understand how to represent both the phase and the
active/reactive/apparent power components. Do we combine the phase and
quadrature part like this
AVAGAIN - in_power_a_gain /* apparent
power */
—
AWGAIN - in_power_ai_gain/*
active power */
—
AVARGAIN- in_power_aq_gain/*
reactive power */
—
Now here it becomes more complicated. Not sure how this gets handled.
AFWATTOS- in_power_a_active/fundamental/offset
—
AWATTHR - in_energy_ai_accumulation
—
AVARHR - in_energy_aq_accumulation
—
IPEAK - in_current_peak
—
I’ll leave it there, because there are some even more complicated register
naming issues.
Regards,
John
> On Mar 10, 2018, at 7:10 AM, Jonathan Cameron wrote:
>
> On Thu, 8 Mar 2018 21:37:33 -0300
> Rodrigo Siqueira wrote:
>
>> On 03/07, Jonathan Cameron wrote:
>>> On Tue, 6 Mar 2018 21:43:47 -0300
>>> Rodrigo Siqueira wrote:
>>>
The macro IIO_DEV_ATTR_CH_OFF is a wrapper for IIO_DEVICE_ATTR, with a
tiny change in the name definition. This extra macro does not improve
the readability and also creates some checkpatch errors.
This patch fixes the checkpatch.pl errors:
staging/iio/meter/ade7753.c:391: ERROR: Use 4 digit octal (0777) not
decimal permissions
staging/iio/meter/ade7753.c:395: ERROR: Use 4 digit octal (0777) not
decimal permissions
staging/iio/meter/ade7759.c:331: ERROR: Use 4 digit octal (0777) not
decimal permissions
staging/iio/meter/ade7759.c:335: ERROR: Use 4 digit octal (0777) not
decimal permissions
Signed-off-by: Rodrigo Siqueira
>>>
>>> Hmm. I wondered a bit about this one. It's a correct patch in of
>>> itself but the interface in question doesn't even vaguely conform
>>> to any of defined IIO ABI. Anyhow, it's still and improvement so
>>> I'll take it.
>>
>> I am not sure if I understood the comment about the ABI. The meter
>> interface is wrong because it uses things like IIO_DEVICE_ATTR? It
>> should use iio_info together with *write_raw and *read_raw. Right? Is it
>> the ABI problem that you refer?
> The ABI is about the userspace interface of IIO. It is defined
> in Documentation/ABI/testing/sysfs-bus-iio*
> So this documents the naming of sysfs attributes and (more or less)
> describes a consistent interface to userspace across lots of different
> types of devices.
>
> A lot of these older drivers in staging involve a good deal of ABI that
> was not reviewed or discussed. That is one of the biggest reasons we
> didn't take them out of staging in the first place.
>
> In order for generic userspace programs to have any idea what to do
> with these devices this all needs to be fixed.
>
> There may well be cases where we need to expand the existing ABI to
> cover new things. That's fine, but it has to be done with full
> review of the relevant documentation patches.
>
> Incidentally if you want an easy driver to work on moving out of staging
> then first thing to do is to compare what it shows to userspace with these
> docs. If it's totally different then you have a big job on your hands
> as often ABI can take a lot of discussion and a long time to establish
> a consensus.
>
> Jonathan
>
>
>>
>> Thanks :)
>>
>>> Applied to the togreg branch of iio.git and pushed out as testing
>>> for the autobuilders to play with it.
>>>
>>> I also added the removal of the header define which is no
>>> longer used.
>>>
>>> Please note, following discussions with Michael, I am going to send
>>> an email announcing an intent to drop these meter drivers next
>>> cycle unless someone can provide testing for any attempt to
>>> move them out of staging. I'm still taking patches on the basis
>>> that 'might' happen - but I wouldn't focus on these until we
>>> have some certainty on whether they will be around long term!
>>>
>>> Jonathan
>>>
---
drivers/staging/iio/meter/ade7753.c | 18 ++
drivers/staging/iio/meter/ade7759.c | 18 ++
2 files changed, 20 insertions(+), 16 deletions(-)
diff --git a/drivers/staging/iio/meter/ade7753.c
b/drivers/staging/iio/meter/ade7753.c
index c44eb577dc35..275e8dfff836 100644
--- a/drivers/staging/iio/meter/ade7753.c
+++ b/dri
Re: [PATCH v2 1/3] staging:iio:meter: Replaces IIO_DEV_ATTR_CH_OFF by IIO_DEVICE_ATTR
On Thu, 8 Mar 2018 21:37:33 -0300
Rodrigo Siqueira wrote:
> On 03/07, Jonathan Cameron wrote:
> > On Tue, 6 Mar 2018 21:43:47 -0300
> > Rodrigo Siqueira wrote:
> >
> > > The macro IIO_DEV_ATTR_CH_OFF is a wrapper for IIO_DEVICE_ATTR, with a
> > > tiny change in the name definition. This extra macro does not improve
> > > the readability and also creates some checkpatch errors.
> > >
> > > This patch fixes the checkpatch.pl errors:
> > >
> > > staging/iio/meter/ade7753.c:391: ERROR: Use 4 digit octal (0777) not
> > > decimal permissions
> > > staging/iio/meter/ade7753.c:395: ERROR: Use 4 digit octal (0777) not
> > > decimal permissions
> > > staging/iio/meter/ade7759.c:331: ERROR: Use 4 digit octal (0777) not
> > > decimal permissions
> > > staging/iio/meter/ade7759.c:335: ERROR: Use 4 digit octal (0777) not
> > > decimal permissions
> > >
> > > Signed-off-by: Rodrigo Siqueira
> >
> > Hmm. I wondered a bit about this one. It's a correct patch in of
> > itself but the interface in question doesn't even vaguely conform
> > to any of defined IIO ABI. Anyhow, it's still and improvement so
> > I'll take it.
>
> I am not sure if I understood the comment about the ABI. The meter
> interface is wrong because it uses things like IIO_DEVICE_ATTR? It
> should use iio_info together with *write_raw and *read_raw. Right? Is it
> the ABI problem that you refer?
The ABI is about the userspace interface of IIO. It is defined
in Documentation/ABI/testing/sysfs-bus-iio*
So this documents the naming of sysfs attributes and (more or less)
describes a consistent interface to userspace across lots of different
types of devices.
A lot of these older drivers in staging involve a good deal of ABI that
was not reviewed or discussed. That is one of the biggest reasons we
didn't take them out of staging in the first place.
In order for generic userspace programs to have any idea what to do
with these devices this all needs to be fixed.
There may well be cases where we need to expand the existing ABI to
cover new things. That's fine, but it has to be done with full
review of the relevant documentation patches.
Incidentally if you want an easy driver to work on moving out of staging
then first thing to do is to compare what it shows to userspace with these
docs. If it's totally different then you have a big job on your hands
as often ABI can take a lot of discussion and a long time to establish
a consensus.
Jonathan
>
> Thanks :)
>
> > Applied to the togreg branch of iio.git and pushed out as testing
> > for the autobuilders to play with it.
> >
> > I also added the removal of the header define which is no
> > longer used.
> >
> > Please note, following discussions with Michael, I am going to send
> > an email announcing an intent to drop these meter drivers next
> > cycle unless someone can provide testing for any attempt to
> > move them out of staging. I'm still taking patches on the basis
> > that 'might' happen - but I wouldn't focus on these until we
> > have some certainty on whether they will be around long term!
> >
> > Jonathan
> >
> > > ---
> > > drivers/staging/iio/meter/ade7753.c | 18 ++
> > > drivers/staging/iio/meter/ade7759.c | 18 ++
> > > 2 files changed, 20 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/staging/iio/meter/ade7753.c
> > > b/drivers/staging/iio/meter/ade7753.c
> > > index c44eb577dc35..275e8dfff836 100644
> > > --- a/drivers/staging/iio/meter/ade7753.c
> > > +++ b/drivers/staging/iio/meter/ade7753.c
> > > @@ -388,14 +388,16 @@ static IIO_DEV_ATTR_VPERIOD(0444,
> > > ade7753_read_16bit,
> > > NULL,
> > > ADE7753_PERIOD);
> > > -static IIO_DEV_ATTR_CH_OFF(1, 0644,
> > > - ade7753_read_8bit,
> > > - ade7753_write_8bit,
> > > - ADE7753_CH1OS);
> > > -static IIO_DEV_ATTR_CH_OFF(2, 0644,
> > > - ade7753_read_8bit,
> > > - ade7753_write_8bit,
> > > - ADE7753_CH2OS);
> > > +
> > > +static IIO_DEVICE_ATTR(choff_1, 0644,
> > > + ade7753_read_8bit,
> > > + ade7753_write_8bit,
> > > + ADE7753_CH1OS);
> > > +
> > > +static IIO_DEVICE_ATTR(choff_2, 0644,
> > > + ade7753_read_8bit,
> > > + ade7753_write_8bit,
> > > + ADE7753_CH2OS);
> > >
> > > static int ade7753_set_irq(struct device *dev, bool enable)
> > > {
> > > diff --git a/drivers/staging/iio/meter/ade7759.c
> > > b/drivers/staging/iio/meter/ade7759.c
> > > index 1decb2b8afab..c078b770fa53 100644
> > > --- a/drivers/staging/iio/meter/ade7759.c
> > > +++ b/drivers/staging/iio/meter/ade7759.c
> > > @@ -328,14 +328,16 @@ static IIO_DEV_ATTR_ACTIVE_POWER_GAIN(0644,
> > > ade7759_read_16bit,
> > > ade7759_write_16bit,
> > > ADE7759_APGAIN);
> > > -static IIO_DEV_ATTR_CH_OFF(1, 0644,
> > > - ade7759_read_8bit,
> > > - ade7759_write_8bit,
> > > - ADE7
Re: [PATCH v2 1/3] staging:iio:meter: Replaces IIO_DEV_ATTR_CH_OFF by IIO_DEVICE_ATTR
On 03/07, Jonathan Cameron wrote:
> On Tue, 6 Mar 2018 21:43:47 -0300
> Rodrigo Siqueira wrote:
>
> > The macro IIO_DEV_ATTR_CH_OFF is a wrapper for IIO_DEVICE_ATTR, with a
> > tiny change in the name definition. This extra macro does not improve
> > the readability and also creates some checkpatch errors.
> >
> > This patch fixes the checkpatch.pl errors:
> >
> > staging/iio/meter/ade7753.c:391: ERROR: Use 4 digit octal (0777) not
> > decimal permissions
> > staging/iio/meter/ade7753.c:395: ERROR: Use 4 digit octal (0777) not
> > decimal permissions
> > staging/iio/meter/ade7759.c:331: ERROR: Use 4 digit octal (0777) not
> > decimal permissions
> > staging/iio/meter/ade7759.c:335: ERROR: Use 4 digit octal (0777) not
> > decimal permissions
> >
> > Signed-off-by: Rodrigo Siqueira
>
> Hmm. I wondered a bit about this one. It's a correct patch in of
> itself but the interface in question doesn't even vaguely conform
> to any of defined IIO ABI. Anyhow, it's still and improvement so
> I'll take it.
I am not sure if I understood the comment about the ABI. The meter
interface is wrong because it uses things like IIO_DEVICE_ATTR? It
should use iio_info together with *write_raw and *read_raw. Right? Is it
the ABI problem that you refer?
Thanks :)
> Applied to the togreg branch of iio.git and pushed out as testing
> for the autobuilders to play with it.
>
> I also added the removal of the header define which is no
> longer used.
>
> Please note, following discussions with Michael, I am going to send
> an email announcing an intent to drop these meter drivers next
> cycle unless someone can provide testing for any attempt to
> move them out of staging. I'm still taking patches on the basis
> that 'might' happen - but I wouldn't focus on these until we
> have some certainty on whether they will be around long term!
>
> Jonathan
>
> > ---
> > drivers/staging/iio/meter/ade7753.c | 18 ++
> > drivers/staging/iio/meter/ade7759.c | 18 ++
> > 2 files changed, 20 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/staging/iio/meter/ade7753.c
> > b/drivers/staging/iio/meter/ade7753.c
> > index c44eb577dc35..275e8dfff836 100644
> > --- a/drivers/staging/iio/meter/ade7753.c
> > +++ b/drivers/staging/iio/meter/ade7753.c
> > @@ -388,14 +388,16 @@ static IIO_DEV_ATTR_VPERIOD(0444,
> > ade7753_read_16bit,
> > NULL,
> > ADE7753_PERIOD);
> > -static IIO_DEV_ATTR_CH_OFF(1, 0644,
> > - ade7753_read_8bit,
> > - ade7753_write_8bit,
> > - ADE7753_CH1OS);
> > -static IIO_DEV_ATTR_CH_OFF(2, 0644,
> > - ade7753_read_8bit,
> > - ade7753_write_8bit,
> > - ADE7753_CH2OS);
> > +
> > +static IIO_DEVICE_ATTR(choff_1, 0644,
> > + ade7753_read_8bit,
> > + ade7753_write_8bit,
> > + ADE7753_CH1OS);
> > +
> > +static IIO_DEVICE_ATTR(choff_2, 0644,
> > + ade7753_read_8bit,
> > + ade7753_write_8bit,
> > + ADE7753_CH2OS);
> >
> > static int ade7753_set_irq(struct device *dev, bool enable)
> > {
> > diff --git a/drivers/staging/iio/meter/ade7759.c
> > b/drivers/staging/iio/meter/ade7759.c
> > index 1decb2b8afab..c078b770fa53 100644
> > --- a/drivers/staging/iio/meter/ade7759.c
> > +++ b/drivers/staging/iio/meter/ade7759.c
> > @@ -328,14 +328,16 @@ static IIO_DEV_ATTR_ACTIVE_POWER_GAIN(0644,
> > ade7759_read_16bit,
> > ade7759_write_16bit,
> > ADE7759_APGAIN);
> > -static IIO_DEV_ATTR_CH_OFF(1, 0644,
> > - ade7759_read_8bit,
> > - ade7759_write_8bit,
> > - ADE7759_CH1OS);
> > -static IIO_DEV_ATTR_CH_OFF(2, 0644,
> > - ade7759_read_8bit,
> > - ade7759_write_8bit,
> > - ADE7759_CH2OS);
> > +
> > +static IIO_DEVICE_ATTR(choff_1, 0644,
> > + ade7759_read_8bit,
> > + ade7759_write_8bit,
> > + ADE7759_CH1OS);
> > +
> > +static IIO_DEVICE_ATTR(choff_2, 0644,
> > + ade7759_read_8bit,
> > + ade7759_write_8bit,
> > + ADE7759_CH2OS);
> >
> > static int ade7759_set_irq(struct device *dev, bool enable)
> > {
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/3] staging:iio:meter: Replaces IIO_DEV_ATTR_CH_OFF by IIO_DEVICE_ATTR
On Tue, 6 Mar 2018 21:43:47 -0300
Rodrigo Siqueira wrote:
> The macro IIO_DEV_ATTR_CH_OFF is a wrapper for IIO_DEVICE_ATTR, with a
> tiny change in the name definition. This extra macro does not improve
> the readability and also creates some checkpatch errors.
>
> This patch fixes the checkpatch.pl errors:
>
> staging/iio/meter/ade7753.c:391: ERROR: Use 4 digit octal (0777) not
> decimal permissions
> staging/iio/meter/ade7753.c:395: ERROR: Use 4 digit octal (0777) not
> decimal permissions
> staging/iio/meter/ade7759.c:331: ERROR: Use 4 digit octal (0777) not
> decimal permissions
> staging/iio/meter/ade7759.c:335: ERROR: Use 4 digit octal (0777) not
> decimal permissions
>
> Signed-off-by: Rodrigo Siqueira
Hmm. I wondered a bit about this one. It's a correct patch in of
itself but the interface in question doesn't even vaguely conform
to any of defined IIO ABI. Anyhow, it's still and improvement so
I'll take it.
Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.
I also added the removal of the header define which is no
longer used.
Please note, following discussions with Michael, I am going to send
an email announcing an intent to drop these meter drivers next
cycle unless someone can provide testing for any attempt to
move them out of staging. I'm still taking patches on the basis
that 'might' happen - but I wouldn't focus on these until we
have some certainty on whether they will be around long term!
Jonathan
> ---
> drivers/staging/iio/meter/ade7753.c | 18 ++
> drivers/staging/iio/meter/ade7759.c | 18 ++
> 2 files changed, 20 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/staging/iio/meter/ade7753.c
> b/drivers/staging/iio/meter/ade7753.c
> index c44eb577dc35..275e8dfff836 100644
> --- a/drivers/staging/iio/meter/ade7753.c
> +++ b/drivers/staging/iio/meter/ade7753.c
> @@ -388,14 +388,16 @@ static IIO_DEV_ATTR_VPERIOD(0444,
> ade7753_read_16bit,
> NULL,
> ADE7753_PERIOD);
> -static IIO_DEV_ATTR_CH_OFF(1, 0644,
> - ade7753_read_8bit,
> - ade7753_write_8bit,
> - ADE7753_CH1OS);
> -static IIO_DEV_ATTR_CH_OFF(2, 0644,
> - ade7753_read_8bit,
> - ade7753_write_8bit,
> - ADE7753_CH2OS);
> +
> +static IIO_DEVICE_ATTR(choff_1, 0644,
> + ade7753_read_8bit,
> + ade7753_write_8bit,
> + ADE7753_CH1OS);
> +
> +static IIO_DEVICE_ATTR(choff_2, 0644,
> + ade7753_read_8bit,
> + ade7753_write_8bit,
> + ADE7753_CH2OS);
>
> static int ade7753_set_irq(struct device *dev, bool enable)
> {
> diff --git a/drivers/staging/iio/meter/ade7759.c
> b/drivers/staging/iio/meter/ade7759.c
> index 1decb2b8afab..c078b770fa53 100644
> --- a/drivers/staging/iio/meter/ade7759.c
> +++ b/drivers/staging/iio/meter/ade7759.c
> @@ -328,14 +328,16 @@ static IIO_DEV_ATTR_ACTIVE_POWER_GAIN(0644,
> ade7759_read_16bit,
> ade7759_write_16bit,
> ADE7759_APGAIN);
> -static IIO_DEV_ATTR_CH_OFF(1, 0644,
> - ade7759_read_8bit,
> - ade7759_write_8bit,
> - ADE7759_CH1OS);
> -static IIO_DEV_ATTR_CH_OFF(2, 0644,
> - ade7759_read_8bit,
> - ade7759_write_8bit,
> - ADE7759_CH2OS);
> +
> +static IIO_DEVICE_ATTR(choff_1, 0644,
> + ade7759_read_8bit,
> + ade7759_write_8bit,
> + ADE7759_CH1OS);
> +
> +static IIO_DEVICE_ATTR(choff_2, 0644,
> + ade7759_read_8bit,
> + ade7759_write_8bit,
> + ADE7759_CH2OS);
>
> static int ade7759_set_irq(struct device *dev, bool enable)
> {
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 1/3] staging:iio:meter: Replaces IIO_DEV_ATTR_CH_OFF by IIO_DEVICE_ATTR
The macro IIO_DEV_ATTR_CH_OFF is a wrapper for IIO_DEVICE_ATTR, with a
tiny change in the name definition. This extra macro does not improve
the readability and also creates some checkpatch errors.
This patch fixes the checkpatch.pl errors:
staging/iio/meter/ade7753.c:391: ERROR: Use 4 digit octal (0777) not
decimal permissions
staging/iio/meter/ade7753.c:395: ERROR: Use 4 digit octal (0777) not
decimal permissions
staging/iio/meter/ade7759.c:331: ERROR: Use 4 digit octal (0777) not
decimal permissions
staging/iio/meter/ade7759.c:335: ERROR: Use 4 digit octal (0777) not
decimal permissions
Signed-off-by: Rodrigo Siqueira
---
drivers/staging/iio/meter/ade7753.c | 18 ++
drivers/staging/iio/meter/ade7759.c | 18 ++
2 files changed, 20 insertions(+), 16 deletions(-)
diff --git a/drivers/staging/iio/meter/ade7753.c
b/drivers/staging/iio/meter/ade7753.c
index c44eb577dc35..275e8dfff836 100644
--- a/drivers/staging/iio/meter/ade7753.c
+++ b/drivers/staging/iio/meter/ade7753.c
@@ -388,14 +388,16 @@ static IIO_DEV_ATTR_VPERIOD(0444,
ade7753_read_16bit,
NULL,
ADE7753_PERIOD);
-static IIO_DEV_ATTR_CH_OFF(1, 0644,
- ade7753_read_8bit,
- ade7753_write_8bit,
- ADE7753_CH1OS);
-static IIO_DEV_ATTR_CH_OFF(2, 0644,
- ade7753_read_8bit,
- ade7753_write_8bit,
- ADE7753_CH2OS);
+
+static IIO_DEVICE_ATTR(choff_1, 0644,
+ ade7753_read_8bit,
+ ade7753_write_8bit,
+ ADE7753_CH1OS);
+
+static IIO_DEVICE_ATTR(choff_2, 0644,
+ ade7753_read_8bit,
+ ade7753_write_8bit,
+ ADE7753_CH2OS);
static int ade7753_set_irq(struct device *dev, bool enable)
{
diff --git a/drivers/staging/iio/meter/ade7759.c
b/drivers/staging/iio/meter/ade7759.c
index 1decb2b8afab..c078b770fa53 100644
--- a/drivers/staging/iio/meter/ade7759.c
+++ b/drivers/staging/iio/meter/ade7759.c
@@ -328,14 +328,16 @@ static IIO_DEV_ATTR_ACTIVE_POWER_GAIN(0644,
ade7759_read_16bit,
ade7759_write_16bit,
ADE7759_APGAIN);
-static IIO_DEV_ATTR_CH_OFF(1, 0644,
- ade7759_read_8bit,
- ade7759_write_8bit,
- ADE7759_CH1OS);
-static IIO_DEV_ATTR_CH_OFF(2, 0644,
- ade7759_read_8bit,
- ade7759_write_8bit,
- ADE7759_CH2OS);
+
+static IIO_DEVICE_ATTR(choff_1, 0644,
+ ade7759_read_8bit,
+ ade7759_write_8bit,
+ ADE7759_CH1OS);
+
+static IIO_DEVICE_ATTR(choff_2, 0644,
+ ade7759_read_8bit,
+ ade7759_write_8bit,
+ ADE7759_CH2OS);
static int ade7759_set_irq(struct device *dev, bool enable)
{
--
2.16.2
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
