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/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) >>>> { >>> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-iio" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > _______________________________________________ > devel mailing list > devel@linuxdriverproject.org > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel