From: John Syne <john3909@gmail.com>
To: Jonathan Cameron <jic23@kernel.org>
Cc: devel@driverdev.osuosl.org, Lars-Peter Clausen <lars@metafoo.de>,
Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>,
linux-iio@vger.kernel.org,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
linux-kernel@vger.kernel.org,
Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
Hartmut Knaack <knaack.h@gmx.de>,
daniel.baluta@nxp.com
Subject: Re: [PATCH v2 1/3] staging:iio:meter: Replaces IIO_DEV_ATTR_CH_OFF by IIO_DEVICE_ATTR
Date: Wed, 14 Mar 2018 23:12:02 -0700 [thread overview]
Message-ID: <79D3051B-FF2F-4DD3-AF75-F6A4BAD81838@gmail.com> (raw)
In-Reply-To: <20180310151045.4570e85d@archlinux>
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 <jic23@kernel.org> wrote:
>
> On Thu, 8 Mar 2018 21:37:33 -0300
> Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote:
>
>> On 03/07, Jonathan Cameron wrote:
>>> On Tue, 6 Mar 2018 21:43:47 -0300
>>> Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> 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 <rodrigosiqueiramelo@gmail.com>
>>>
>>> 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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
next prev parent reply other threads:[~2018-03-15 6:12 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-07 0:43 [PATCH v2 0/3] staging:iio:meter: Checkpatch cleanup for meter Rodrigo Siqueira
2018-03-07 0:43 ` [PATCH v2 1/3] staging:iio:meter: Replaces IIO_DEV_ATTR_CH_OFF by IIO_DEVICE_ATTR Rodrigo Siqueira
2018-03-07 20:07 ` Jonathan Cameron
2018-03-09 0:37 ` Rodrigo Siqueira
2018-03-10 15:10 ` Jonathan Cameron
2018-03-15 6:10 ` John Syne
2018-03-15 6:12 ` John Syne [this message]
2018-03-17 20:30 ` Jonathan Cameron
2018-03-18 6:11 ` John Syne
2018-03-18 12:23 ` meter ABI: (was Re: [PATCH v2 1/3] staging:iio:meter: Replaces IIO_DEV_ATTR_CH_OFF by IIO_DEVICE_ATTR) Jonathan Cameron
2018-03-20 5:57 ` John Syne
2018-03-24 15:02 ` Jonathan Cameron
2018-03-24 22:45 ` John Syne
2018-03-25 16:29 ` Jonathan Cameron
2018-03-25 20:36 ` John Syne
2018-03-30 9:13 ` Jonathan Cameron
2018-03-20 6:28 ` John Syne
2018-03-24 15:18 ` Jonathan Cameron
2018-03-24 23:06 ` John Syne
2018-03-25 16:44 ` Jonathan Cameron
2018-03-25 20:43 ` John Syne
2018-03-25 20:44 ` John Syne
2018-03-24 23:18 ` John Syne
2018-03-25 7:10 ` John Syne
2018-03-25 7:13 ` John Syne
2018-03-25 8:26 ` John Syne
2018-03-25 8:29 ` John Syne
2018-03-25 16:54 ` Jonathan Cameron
2018-03-25 20:53 ` John Syne
2018-03-30 9:16 ` Jonathan Cameron
2018-03-07 0:44 ` [PATCH v2 2/3] staging:iio:meter: Remove unused macro IIO_DEV_ATTR_CH_OFF Rodrigo Siqueira
2018-03-07 20:09 ` Jonathan Cameron
2018-03-07 0:44 ` [PATCH v2 3/3] staging:iio:meter: Aligns open parenthesis Rodrigo Siqueira
2018-03-07 20:12 ` Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=79D3051B-FF2F-4DD3-AF75-F6A4BAD81838@gmail.com \
--to=john3909@gmail.com \
--cc=daniel.baluta@nxp.com \
--cc=devel@driverdev.osuosl.org \
--cc=gregkh@linuxfoundation.org \
--cc=jic23@kernel.org \
--cc=knaack.h@gmx.de \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pmeerw@pmeerw.net \
--cc=rodrigosiqueiramelo@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).