Work on iio: stating: frequency: ad9832

Work on iio: stating: frequency: ad9832

[Marcelo Schmitt]

Hello,

I was looking for some work on staging: iio: ad9832 and made some
observations while reading the driver.

Apparently it had no devicetree documentation so I tried to elaborate
one. 
It uses a platform_data variable to load external clock
frequency (I tried to make it use linux's clock framework).
Some device attributes don't seem to be standardized on
Documentation/ABI/testing/sysfs-bus-iio and there's no specific ABI
for ad9832 nearby nor at staging/iio/Documentation. So maybe those
missing ABI could be documented.
The device has to set some internal registers to operate correctly,
AD9832_FREQXHM and AD9832_PHASEXH, would it be feasible to set iio
chanels for this?
I couldn't understand why checkpatch.pl gave errors on IIO_DEV_ATTR_*
macros. To me they seem to have no problem.
Also it has that platform_data to be moved to include/linux/iio. Is
there any special reason for it not being there already? Which are
the criterions a platform_data need to satisfy to be put there?

I'm sending a patchset with some things I've already done.

Is there something else that could be done in this device driver?
Please, tell if I've forgotten something.

Any advice is welcome.
Thanks,

Marcelo

Re: Work on iio: stating: frequency: ad9832

[Jonathan Cameron]

On Mon, 1 Apr 2019 11:25:29 -0300
Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:

> Hello,
> 
> I was looking for some work on staging: iio: ad9832 and made some
> observations while reading the driver.
> 
> Apparently it had no devicetree documentation so I tried to elaborate
> one. 
> It uses a platform_data variable to load external clock
> frequency (I tried to make it use linux's clock framework).
Good.

> Some device attributes don't seem to be standardized on
> Documentation/ABI/testing/sysfs-bus-iio and there's no specific ABI
> for ad9832 nearby nor at staging/iio/Documentation. So maybe those
> missing ABI could be documented.
Beware. It's an old driver, so it may be that we actually want to change
it's ABI rather than documenting what is there (I have haven't looked!)

> The device has to set some internal registers to operate correctly,
> AD9832_FREQXHM and AD9832_PHASEXH, would it be feasible to set iio
> chanels for this?

What are they?  If they correspond to output channels in some sensible
way then maybe...

> I couldn't understand why checkpatch.pl gave errors on IIO_DEV_ATTR_*
> macros. To me they seem to have no problem.
> Also it has that platform_data to be moved to include/linux/iio. Is
> there any special reason for it not being there already? Which are
> the criterions a platform_data need to satisfy to be put there?
A driver moving out of staging shouldn't have platform data. It needs
to be converted over to more modern mechanisms.   We don't have a problem
supporting platform data for devices that have old school device files
already in tree, but that shouldn't be the case for a driver in staging.

Hence we can clean it up and move forward with just DT bindings.
> 
> I'm sending a patchset with some things I've already done.
Cool. I'll look at those later in the week if no one beats me to them.

> 
> Is there something else that could be done in this device driver?
> Please, tell if I've forgotten something.

I'll take a look, but it may be a little while before I do.
Hopefully someone else gets there first!

Jonathan

> 
> Any advice is welcome.
> Thanks,
> 
> Marcelo

Re: Work on iio: stating: frequency: ad9832

[Alexandru Ardelean]

On Mon, Apr 1, 2019 at 7:13 PM Jonathan Cameron
<jonathan.cameron@huawei.com> wrote:
>
> On Mon, 1 Apr 2019 11:25:29 -0300
> Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:
>
> > Hello,
> >
> > I was looking for some work on staging: iio: ad9832 and made some
> > observations while reading the driver.
> >
> > Apparently it had no devicetree documentation so I tried to elaborate
> > one.
> > It uses a platform_data variable to load external clock
> > frequency (I tried to make it use linux's clock framework).
> Good.
>
> > Some device attributes don't seem to be standardized on
> > Documentation/ABI/testing/sysfs-bus-iio and there's no specific ABI
> > for ad9832 nearby nor at staging/iio/Documentation. So maybe those
> > missing ABI could be documented.
> Beware. It's an old driver, so it may be that we actually want to change
> it's ABI rather than documenting what is there (I have haven't looked!)
>

This one can actually be coupled a bit with the AD9834 driver.
There's been some work on trying to move that one out of staging as well.

You can take a look at the patches sent for that driver.
They should be find-able on patchwork
https://patchwork.kernel.org/project/linux-iio/list/?series=&submitter=&state=*&q=ad9834&archive=both&delegate=

There are ideas worth borrowing from there.

The issue with the AD9834 [if i recall correctly] is that it doesn't
quite fit the classical IIO channel model.
Meaning, you can only activate the output of one channel at one moment
in time, and not both.

> > The device has to set some internal registers to operate correctly,
> > AD9832_FREQXHM and AD9832_PHASEXH, would it be feasible to set iio
> > chanels for this?
>
> What are they?  If they correspond to output channels in some sensible
> way then maybe...
>
> > I couldn't understand why checkpatch.pl gave errors on IIO_DEV_ATTR_*
> > macros. To me they seem to have no problem.
> > Also it has that platform_data to be moved to include/linux/iio. Is
> > there any special reason for it not being there already? Which are
> > the criterions a platform_data need to satisfy to be put there?
> A driver moving out of staging shouldn't have platform data. It needs
> to be converted over to more modern mechanisms.   We don't have a problem
> supporting platform data for devices that have old school device files
> already in tree, but that shouldn't be the case for a driver in staging.
>
> Hence we can clean it up and move forward with just DT bindings.
> >
> > I'm sending a patchset with some things I've already done.
> Cool. I'll look at those later in the week if no one beats me to them.
>
> >
> > Is there something else that could be done in this device driver?
> > Please, tell if I've forgotten something.
>
> I'll take a look, but it may be a little while before I do.
> Hopefully someone else gets there first!
>
> Jonathan
>
> >
> > Any advice is welcome.
> > Thanks,
> >
> > Marcelo
>
>

Re: Work on iio: stating: frequency: ad9832

[Marcelo Schmitt]

On 04/02, Alexandru Ardelean wrote:
> On Mon, Apr 1, 2019 at 7:13 PM Jonathan Cameron
> <jonathan.cameron@huawei.com> wrote:
> >
> > On Mon, 1 Apr 2019 11:25:29 -0300
> > Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:
> >
> > > Hello,
> > >
> > > I was looking for some work on staging: iio: ad9832 and made some
> > > observations while reading the driver.
> > >
> > > Apparently it had no devicetree documentation so I tried to elaborate
> > > one.
> > > It uses a platform_data variable to load external clock
> > > frequency (I tried to make it use linux's clock framework).
> > Good.
> >
> > > Some device attributes don't seem to be standardized on
> > > Documentation/ABI/testing/sysfs-bus-iio and there's no specific ABI
> > > for ad9832 nearby nor at staging/iio/Documentation. So maybe those
> > > missing ABI could be documented.
> > Beware. It's an old driver, so it may be that we actually want to change
> > it's ABI rather than documenting what is there (I have haven't looked!)
> >

OK, I'll take more time studying the device's datasheet to better     
understand the current ABI.

> 
> This one can actually be coupled a bit with the AD9834 driver.
> There's been some work on trying to move that one out of staging as well.
> 
> You can take a look at the patches sent for that driver.
> They should be find-able on patchwork
> https://patchwork.kernel.org/project/linux-iio/list/?series=&submitter=&state=*&q=ad9834&archive=both&delegate=
> 
> There are ideas worth borrowing from there.
> 
> The issue with the AD9834 [if i recall correctly] is that it doesn't
> quite fit the classical IIO channel model.
> Meaning, you can only activate the output of one channel at one moment
> in time, and not both.

OK, I'll have a look at it.

> 
> > > The device has to set some internal registers to operate correctly,
> > > AD9832_FREQXHM and AD9832_PHASEXH, would it be feasible to set iio
> > > chanels for this?
> >
> > What are they?  If they correspond to output channels in some sensible
> > way then maybe...
> >
> > > I couldn't understand why checkpatch.pl gave errors on IIO_DEV_ATTR_*
> > > macros. To me they seem to have no problem.
> > > Also it has that platform_data to be moved to include/linux/iio. Is
> > > there any special reason for it not being there already? Which are
> > > the criterions a platform_data need to satisfy to be put there?
> > A driver moving out of staging shouldn't have platform data. It needs
> > to be converted over to more modern mechanisms.   We don't have a problem
> > supporting platform data for devices that have old school device files
> > already in tree, but that shouldn't be the case for a driver in staging.
> >
> > Hence we can clean it up and move forward with just DT bindings.
> > >

Understood. Thanks for the explanation.

> > > I'm sending a patchset with some things I've already done.
> > Cool. I'll look at those later in the week if no one beats me to them.
> >
> > >
> > > Is there something else that could be done in this device driver?
> > > Please, tell if I've forgotten something.
> >
> > I'll take a look, but it may be a little while before I do.
> > Hopefully someone else gets there first!
> >
> > Jonathan
> >
> > >
> > > Any advice is welcome.
> > > Thanks,
> > >
> > > Marcelo
> >
> >

Thanks for the pieces of advice.

Marcelo