Re: Work on iio: stating: frequency: ad9832
On 04/02, Alexandru Ardelean wrote: > On Mon, Apr 1, 2019 at 7:13 PM Jonathan Cameron > wrote: > > > > On Mon, 1 Apr 2019 11:25:29 -0300 > > Marcelo Schmitt 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===*=ad9834=both= > > 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Work on iio: stating: frequency: ad9832
On Mon, Apr 1, 2019 at 7:13 PM Jonathan Cameron wrote: > > On Mon, 1 Apr 2019 11:25:29 -0300 > Marcelo Schmitt 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===*=ad9834=both= 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 > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Work on iio: stating: frequency: ad9832
On Mon, 1 Apr 2019 11:25:29 -0300 Marcelo Schmitt 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel