Re: [PATCH v4] iio: adc: Add TI ADS1015 ADC driver support

Re: [PATCH v4] iio: adc: Add TI ADS1015 ADC driver support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2100 bytes --]

On Fri, Feb 05, 2016 at 06:32:45PM -0600, Michael Welling wrote:
> On Fri, Feb 05, 2016 at 09:32:34PM +0200, Daniel Baluta wrote:
> > >> +static int ads1015_read_raw(struct iio_dev *indio_dev,
> > >> +                         struct iio_chan_spec const *chan, int *val,
> > >> +                         int *val2, long mask)
> > >> +{
> > >> +     int ret, idx;
> > >> +     struct ads1015_data *data = iio_priv(indio_dev);
> > >> +
> > >> +     mutex_lock(&data->lock);
> > >> +     switch (mask) {
> > >> +     case IIO_CHAN_INFO_RAW:
> > >> +             if (iio_buffer_enabled(indio_dev)) {
> > >> +                     ret = -EBUSY;
> > >> +                     break;
> > >> +             }
> > >> +
> > >> +             ret = ads1015_set_power_state(data, true);
> > >> +             if (ret < 0)
> > >> +                     break;
> > >
> > > Just tested the driver on a Dragonboard 410C with a robotics mezzanine that I
> > > designed.
> > >
> > > The above ads1015_set_power_state(data, true) is always returning -EINVAL.
> > >
> > > Any ideas why that would be happening?
> > > I think it may be the return from pm_runtime_get_sync?
> > 
> > Can you confirm that pm_runtime_get_sync fails? Using some printk?
> > 
> > Also adding printks in suspend/resume function would be helpful. Do
> > you have CONFIG_PM enabled?
> >
> 
> Indeed it is the pm_runtime_get_sync that fails with a -EINVAL.
> 
> > >
> > > When I comment out the break the readings come back but are not updated continually.
> > > If I read in_voltage0-voltage1_raw then in_voltage0_raw the value is updated.
> > 
> > I guess this is normal if set_power_state fails.
> 
> The hwmod driver works fine BTW.
> 
> My guess is there is an issue with the qup i2c driver seeing as it has worked on
> other system without issue.
> 
> CC'd some the latest developer on the qup i2c driver.
> 
> I2C guys have any ideas on this?
> 

Adding some more people who recently worked on this. Might be nice to
know which kernel version you are using.

> > 
> > thanks,
> > Daniel.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4] iio: adc: Add TI ADS1015 ADC driver support

[Daniel Baluta]

On Mon, Feb 8, 2016 at 12:25 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> On Fri, Feb 05, 2016 at 06:32:45PM -0600, Michael Welling wrote:
>> On Fri, Feb 05, 2016 at 09:32:34PM +0200, Daniel Baluta wrote:
>> > >> +static int ads1015_read_raw(struct iio_dev *indio_dev,
>> > >> +                         struct iio_chan_spec const *chan, int *val,
>> > >> +                         int *val2, long mask)
>> > >> +{
>> > >> +     int ret, idx;
>> > >> +     struct ads1015_data *data = iio_priv(indio_dev);
>> > >> +
>> > >> +     mutex_lock(&data->lock);
>> > >> +     switch (mask) {
>> > >> +     case IIO_CHAN_INFO_RAW:
>> > >> +             if (iio_buffer_enabled(indio_dev)) {
>> > >> +                     ret = -EBUSY;
>> > >> +                     break;
>> > >> +             }
>> > >> +
>> > >> +             ret = ads1015_set_power_state(data, true);
>> > >> +             if (ret < 0)
>> > >> +                     break;
>> > >
>> > > Just tested the driver on a Dragonboard 410C with a robotics mezzanine that I
>> > > designed.
>> > >
>> > > The above ads1015_set_power_state(data, true) is always returning -EINVAL.
>> > >
>> > > Any ideas why that would be happening?
>> > > I think it may be the return from pm_runtime_get_sync?
>> >
>> > Can you confirm that pm_runtime_get_sync fails? Using some printk?
>> >
>> > Also adding printks in suspend/resume function would be helpful. Do
>> > you have CONFIG_PM enabled?
>> >
>>
>> Indeed it is the pm_runtime_get_sync that fails with a -EINVAL.

Can you check the output of:
$ cat /sys/bus/iio/devices/iio:device0/power/runtime_status

* after insmod
* after a reading from sysfs

>>
>> > >
>> > > When I comment out the break the readings come back but are not updated continually.
>> > > If I read in_voltage0-voltage1_raw then in_voltage0_raw the value is updated.
>> >
>> > I guess this is normal if set_power_state fails.
>>
>> The hwmod driver works fine BTW.
>>
>> My guess is there is an issue with the qup i2c driver seeing as it has worked on
>> other system without issue.
>>
>> CC'd some the latest developer on the qup i2c driver.
>>
>> I2C guys have any ideas on this?
>>
>
> Adding some more people who recently worked on this. Might be nice to
> know which kernel version you are using.

Re: [PATCH v4] iio: adc: Add TI ADS1015 ADC driver support

[Daniel Baluta]

On Mon, Feb 8, 2016 at 4:44 PM, Daniel Baluta <daniel.baluta-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> On Mon, Feb 8, 2016 at 12:25 PM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
>> On Fri, Feb 05, 2016 at 06:32:45PM -0600, Michael Welling wrote:
>>> On Fri, Feb 05, 2016 at 09:32:34PM +0200, Daniel Baluta wrote:
>>> > >> +static int ads1015_read_raw(struct iio_dev *indio_dev,
>>> > >> +                         struct iio_chan_spec const *chan, int *val,
>>> > >> +                         int *val2, long mask)
>>> > >> +{
>>> > >> +     int ret, idx;
>>> > >> +     struct ads1015_data *data = iio_priv(indio_dev);
>>> > >> +
>>> > >> +     mutex_lock(&data->lock);
>>> > >> +     switch (mask) {
>>> > >> +     case IIO_CHAN_INFO_RAW:
>>> > >> +             if (iio_buffer_enabled(indio_dev)) {
>>> > >> +                     ret = -EBUSY;
>>> > >> +                     break;
>>> > >> +             }
>>> > >> +
>>> > >> +             ret = ads1015_set_power_state(data, true);
>>> > >> +             if (ret < 0)
>>> > >> +                     break;
>>> > >
>>> > > Just tested the driver on a Dragonboard 410C with a robotics mezzanine that I
>>> > > designed.
>>> > >
>>> > > The above ads1015_set_power_state(data, true) is always returning -EINVAL.
>>> > >
>>> > > Any ideas why that would be happening?
>>> > > I think it may be the return from pm_runtime_get_sync?
>>> >
>>> > Can you confirm that pm_runtime_get_sync fails? Using some printk?
>>> >
>>> > Also adding printks in suspend/resume function would be helpful. Do
>>> > you have CONFIG_PM enabled?
>>> >
>>>
>>> Indeed it is the pm_runtime_get_sync that fails with a -EINVAL.
>
> Can you check the output of:
> $ cat /sys/bus/iio/devices/iio:device0/power/runtime_status
>
> * after insmod
> * after a reading from sysfs
>
>>>
>>> > >
>>> > > When I comment out the break the readings come back but are not updated continually.
>>> > > If I read in_voltage0-voltage1_raw then in_voltage0_raw the value is updated.
>>> >
>>> > I guess this is normal if set_power_state fails.
>>>
>>> The hwmod driver works fine BTW.
>>>
>>> My guess is there is an issue with the qup i2c driver seeing as it has worked on
>>> other system without issue.
>>>
>>> CC'd some the latest developer on the qup i2c driver.
>>>
>>> I2C guys have any ideas on this?
>>>
>>
>> Adding some more people who recently worked on this. Might be nice to
>> know which kernel version you are using.

Jonathan,

I will send an update version with your comments addressed once
we sort out this runtime pm issue.

Michael,

If you questions we can discuss on #linux-iio IRC channel
under irc.oftc.net server.

thanks,
Daniel.

Re: [PATCH v4] iio: adc: Add TI ADS1015 ADC driver support

[Michael Welling]

On Mon, Feb 08, 2016 at 04:44:02PM +0200, Daniel Baluta wrote:
> On Mon, Feb 8, 2016 at 12:25 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> > On Fri, Feb 05, 2016 at 06:32:45PM -0600, Michael Welling wrote:
> >> On Fri, Feb 05, 2016 at 09:32:34PM +0200, Daniel Baluta wrote:
> >> > >> +static int ads1015_read_raw(struct iio_dev *indio_dev,
> >> > >> +                         struct iio_chan_spec const *chan, int *val,
> >> > >> +                         int *val2, long mask)
> >> > >> +{
> >> > >> +     int ret, idx;
> >> > >> +     struct ads1015_data *data = iio_priv(indio_dev);
> >> > >> +
> >> > >> +     mutex_lock(&data->lock);
> >> > >> +     switch (mask) {
> >> > >> +     case IIO_CHAN_INFO_RAW:
> >> > >> +             if (iio_buffer_enabled(indio_dev)) {
> >> > >> +                     ret = -EBUSY;
> >> > >> +                     break;
> >> > >> +             }
> >> > >> +
> >> > >> +             ret = ads1015_set_power_state(data, true);
> >> > >> +             if (ret < 0)
> >> > >> +                     break;
> >> > >
> >> > > Just tested the driver on a Dragonboard 410C with a robotics mezzanine that I
> >> > > designed.
> >> > >
> >> > > The above ads1015_set_power_state(data, true) is always returning -EINVAL.
> >> > >
> >> > > Any ideas why that would be happening?
> >> > > I think it may be the return from pm_runtime_get_sync?
> >> >
> >> > Can you confirm that pm_runtime_get_sync fails? Using some printk?
> >> >
> >> > Also adding printks in suspend/resume function would be helpful. Do
> >> > you have CONFIG_PM enabled?
> >> >
> >>
> >> Indeed it is the pm_runtime_get_sync that fails with a -EINVAL.
> 
> Can you check the output of:
> $ cat /sys/bus/iio/devices/iio:device0/power/runtime_status
> 
> * after insmod

root@dragonboard-410c:~# cat /sys/bus/iio/devices/iio:device0/power/runtime_status
unsupported

> * after a reading from sysfs

root@dragonboard-410c:~# cat /sys/bus/iio/devices/iio:device0/power/runtime_status
unsupported

> 
> >>
> >> > >
> >> > > When I comment out the break the readings come back but are not updated continually.
> >> > > If I read in_voltage0-voltage1_raw then in_voltage0_raw the value is updated.
> >> >
> >> > I guess this is normal if set_power_state fails.
> >>
> >> The hwmod driver works fine BTW.
> >>
> >> My guess is there is an issue with the qup i2c driver seeing as it has worked on
> >> other system without issue.
> >>
> >> CC'd some the latest developer on the qup i2c driver.
> >>
> >> I2C guys have any ideas on this?
> >>
> >
> > Adding some more people who recently worked on this. Might be nice to
> > know which kernel version you are using.

Re: [PATCH v4] iio: adc: Add TI ADS1015 ADC driver support

[Michael Welling]

On Mon, Feb 08, 2016 at 11:25:00AM +0100, Wolfram Sang wrote:
> On Fri, Feb 05, 2016 at 06:32:45PM -0600, Michael Welling wrote:
> > On Fri, Feb 05, 2016 at 09:32:34PM +0200, Daniel Baluta wrote:
> > > >> +static int ads1015_read_raw(struct iio_dev *indio_dev,
> > > >> +                         struct iio_chan_spec const *chan, int *val,
> > > >> +                         int *val2, long mask)
> > > >> +{
> > > >> +     int ret, idx;
> > > >> +     struct ads1015_data *data = iio_priv(indio_dev);
> > > >> +
> > > >> +     mutex_lock(&data->lock);
> > > >> +     switch (mask) {
> > > >> +     case IIO_CHAN_INFO_RAW:
> > > >> +             if (iio_buffer_enabled(indio_dev)) {
> > > >> +                     ret = -EBUSY;
> > > >> +                     break;
> > > >> +             }
> > > >> +
> > > >> +             ret = ads1015_set_power_state(data, true);
> > > >> +             if (ret < 0)
> > > >> +                     break;
> > > >
> > > > Just tested the driver on a Dragonboard 410C with a robotics mezzanine that I
> > > > designed.
> > > >
> > > > The above ads1015_set_power_state(data, true) is always returning -EINVAL.
> > > >
> > > > Any ideas why that would be happening?
> > > > I think it may be the return from pm_runtime_get_sync?
> > > 
> > > Can you confirm that pm_runtime_get_sync fails? Using some printk?
> > > 
> > > Also adding printks in suspend/resume function would be helpful. Do
> > > you have CONFIG_PM enabled?
> > >
> > 
> > Indeed it is the pm_runtime_get_sync that fails with a -EINVAL.
> > 
> > > >
> > > > When I comment out the break the readings come back but are not updated continually.
> > > > If I read in_voltage0-voltage1_raw then in_voltage0_raw the value is updated.
> > > 
> > > I guess this is normal if set_power_state fails.
> > 
> > The hwmod driver works fine BTW.
> > 
> > My guess is there is an issue with the qup i2c driver seeing as it has worked on
> > other system without issue.
> > 
> > CC'd some the latest developer on the qup i2c driver.
> > 
> > I2C guys have any ideas on this?
> > 
> 
> Adding some more people who recently worked on this. Might be nice to
> know which kernel version you are using.
>

4.5.0-rc2
 
> > > 
> > > thanks,
> > > Daniel.

RE: [PATCH v4] iio: adc: Add TI ADS1015 ADC driver support

[Sricharan]

Hi,

> -----Original Message-----
> From: Michael Welling [mailto:mwelling79@gmail.com] On Behalf Of Michael
> Welling
> Sent: Monday, February 08, 2016 10:07 PM
> To: Wolfram Sang
> Cc: Daniel Baluta; Jonathan Cameron; Hartmut Knaack; Lars-Peter Clausen;
> Peter Meerwald-Stadler; Linux Kernel Mailing List;
linux-iio@vger.kernel.org;
> Lucas De Marchi; Andy Gross; Pramod Gurav; Bjorn Andersson; Guenter
> Roeck; eibach@gdsys.de; Sricharan R; linux-arm-msm@vger.kernel.org
> Subject: Re: [PATCH v4] iio: adc: Add TI ADS1015 ADC driver support
> 
> On Mon, Feb 08, 2016 at 11:25:00AM +0100, Wolfram Sang wrote:
> > On Fri, Feb 05, 2016 at 06:32:45PM -0600, Michael Welling wrote:
> > > On Fri, Feb 05, 2016 at 09:32:34PM +0200, Daniel Baluta wrote:
> > > > >> +static int ads1015_read_raw(struct iio_dev *indio_dev,
> > > > >> +                         struct iio_chan_spec const *chan, int
*val,
> > > > >> +                         int *val2, long mask) {
> > > > >> +     int ret, idx;
> > > > >> +     struct ads1015_data *data = iio_priv(indio_dev);
> > > > >> +
> > > > >> +     mutex_lock(&data->lock);
> > > > >> +     switch (mask) {
> > > > >> +     case IIO_CHAN_INFO_RAW:
> > > > >> +             if (iio_buffer_enabled(indio_dev)) {
> > > > >> +                     ret = -EBUSY;
> > > > >> +                     break;
> > > > >> +             }
> > > > >> +
> > > > >> +             ret = ads1015_set_power_state(data, true);
> > > > >> +             if (ret < 0)
> > > > >> +                     break;
> > > > >
> > > > > Just tested the driver on a Dragonboard 410C with a robotics
> > > > > mezzanine that I designed.
> > > > >
> > > > > The above ads1015_set_power_state(data, true) is always returning
-
> EINVAL.
> > > > >
> > > > > Any ideas why that would be happening?
> > > > > I think it may be the return from pm_runtime_get_sync?
> > > >
> > > > Can you confirm that pm_runtime_get_sync fails? Using some printk?
> > > >
> > > > Also adding printks in suspend/resume function would be helpful.
> > > > Do you have CONFIG_PM enabled?
> > > >
> > >
> > > Indeed it is the pm_runtime_get_sync that fails with a -EINVAL.
> > >
> > > > >
> > > > > When I comment out the break the readings come back but are not
> updated continually.
> > > > > If I read in_voltage0-voltage1_raw then in_voltage0_raw the value
is
> updated.
> > > >
> > > > I guess this is normal if set_power_state fails.
> > >
> > > The hwmod driver works fine BTW.
> > >
> > > My guess is there is an issue with the qup i2c driver seeing as it
> > > has worked on other system without issue.
> > >
> > > CC'd some the latest developer on the qup i2c driver.
> > >
> > > I2C guys have any ideas on this?
> > >
> >
> > Adding some more people who recently worked on this. Might be nice to
> > know which kernel version you are using.
> >
 Which i2c bus is this connected to ? I can give a try with 410c to see why
pm_runtime_get_sync from qup
 fails.

Regards,
  Sricharan

Re: [PATCH v4] iio: adc: Add TI ADS1015 ADC driver support

[Michael Welling]

On Tue, Feb 09, 2016 at 12:41:35AM +0530, Sricharan wrote:
> Hi,
> 
> > -----Original Message-----
> > From: Michael Welling [mailto:mwelling79-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org] On Behalf Of Michael
> > Welling
> > Sent: Monday, February 08, 2016 10:07 PM
> > To: Wolfram Sang
> > Cc: Daniel Baluta; Jonathan Cameron; Hartmut Knaack; Lars-Peter Clausen;
> > Peter Meerwald-Stadler; Linux Kernel Mailing List;
> linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> > Lucas De Marchi; Andy Gross; Pramod Gurav; Bjorn Andersson; Guenter
> > Roeck; eibach-dJ+jgKLZIg4@public.gmane.org; Sricharan R; linux-arm-msm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Subject: Re: [PATCH v4] iio: adc: Add TI ADS1015 ADC driver support
> > 
> > On Mon, Feb 08, 2016 at 11:25:00AM +0100, Wolfram Sang wrote:
> > > On Fri, Feb 05, 2016 at 06:32:45PM -0600, Michael Welling wrote:
> > > > On Fri, Feb 05, 2016 at 09:32:34PM +0200, Daniel Baluta wrote:
> > > > > >> +static int ads1015_read_raw(struct iio_dev *indio_dev,
> > > > > >> +                         struct iio_chan_spec const *chan, int
> *val,
> > > > > >> +                         int *val2, long mask) {
> > > > > >> +     int ret, idx;
> > > > > >> +     struct ads1015_data *data = iio_priv(indio_dev);
> > > > > >> +
> > > > > >> +     mutex_lock(&data->lock);
> > > > > >> +     switch (mask) {
> > > > > >> +     case IIO_CHAN_INFO_RAW:
> > > > > >> +             if (iio_buffer_enabled(indio_dev)) {
> > > > > >> +                     ret = -EBUSY;
> > > > > >> +                     break;
> > > > > >> +             }
> > > > > >> +
> > > > > >> +             ret = ads1015_set_power_state(data, true);
> > > > > >> +             if (ret < 0)
> > > > > >> +                     break;
> > > > > >
> > > > > > Just tested the driver on a Dragonboard 410C with a robotics
> > > > > > mezzanine that I designed.
> > > > > >
> > > > > > The above ads1015_set_power_state(data, true) is always returning
> -
> > EINVAL.
> > > > > >
> > > > > > Any ideas why that would be happening?
> > > > > > I think it may be the return from pm_runtime_get_sync?
> > > > >
> > > > > Can you confirm that pm_runtime_get_sync fails? Using some printk?
> > > > >
> > > > > Also adding printks in suspend/resume function would be helpful.
> > > > > Do you have CONFIG_PM enabled?
> > > > >
> > > >
> > > > Indeed it is the pm_runtime_get_sync that fails with a -EINVAL.
> > > >
> > > > > >
> > > > > > When I comment out the break the readings come back but are not
> > updated continually.
> > > > > > If I read in_voltage0-voltage1_raw then in_voltage0_raw the value
> is
> > updated.
> > > > >
> > > > > I guess this is normal if set_power_state fails.
> > > >
> > > > The hwmod driver works fine BTW.
> > > >
> > > > My guess is there is an issue with the qup i2c driver seeing as it
> > > > has worked on other system without issue.
> > > >
> > > > CC'd some the latest developer on the qup i2c driver.
> > > >
> > > > I2C guys have any ideas on this?
> > > >
> > >
> > > Adding some more people who recently worked on this. Might be nice to
> > > know which kernel version you are using.
> > >
>  Which i2c bus is this connected to ? I can give a try with 410c to see why
> pm_runtime_get_sync from qup
>  fails.

It is on the lowspeed header. Here is my devicetree entry:

                 i2c@78b6000 {
                 /* On Low speed expansion */
                         label = "LS-I2C0";
                         status = "okay";
 
                         pca: pca@40 {
                                 compatible = "nxp,pca9685-pwm";
                                 #pwm-cells = <2>;
                                 reg = <0x40>;
                         };
 
                         adc: adc@48 {
                                 compatible = "ti,ads1015";
                                 reg = <0x48>;
                         };
                 };

> 
> Regards,
>   Sricharan
> 
> 
> 

Re: [PATCH v4] iio: adc: Add TI ADS1015 ADC driver support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 680 bytes --]

> > > On Mon, Feb 08, 2016 at 11:25:00AM +0100, Wolfram Sang wrote:
> > > > On Fri, Feb 05, 2016 at 06:32:45PM -0600, Michael Welling wrote:
> > > > > On Fri, Feb 05, 2016 at 09:32:34PM +0200, Daniel Baluta wrote:
> > > > > > >> +static int ads1015_read_raw(struct iio_dev *indio_dev,
> > > > > > >> +                         struct iio_chan_spec const *chan, int

Guys, please quote only relevant sections.

>                          pca: pca@40 {
>                                  compatible = "nxp,pca9685-pwm";
>                                  #pwm-cells = <2>;
>                                  reg = <0x40>;
>                          };

Does accessing the pca work?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4] iio: adc: Add TI ADS1015 ADC driver support

[Michael Welling]

On Mon, Feb 08, 2016 at 09:00:27PM +0100, Wolfram Sang wrote:
> > > > On Mon, Feb 08, 2016 at 11:25:00AM +0100, Wolfram Sang wrote:
> > > > > On Fri, Feb 05, 2016 at 06:32:45PM -0600, Michael Welling wrote:
> > > > > > On Fri, Feb 05, 2016 at 09:32:34PM +0200, Daniel Baluta wrote:
> > > > > > > >> +static int ads1015_read_raw(struct iio_dev *indio_dev,
> > > > > > > >> +                         struct iio_chan_spec const *chan, int
> 
> Guys, please quote only relevant sections.
>

Sorry. 

> >                          pca: pca@40 {
> >                                  compatible = "nxp,pca9685-pwm";
> >                                  #pwm-cells = <2>;
> >                                  reg = <0x40>;
> >                          };
> 
> Does accessing the pca work?
> 

Yes.

https://plus.google.com/+MichaelWelling79/posts/PdmGysBfZp4

RE: [PATCH v4] iio: adc: Add TI ADS1015 ADC driver support

[Sricharan]

Hi,

> -----Original Message-----
> From: linux-arm-msm-owner@vger.kernel.org [mailto:linux-arm-msm-
> owner@vger.kernel.org] On Behalf Of Michael Welling
> Sent: Tuesday, February 09, 2016 12:47 AM
> To: Sricharan
> Cc: 'Wolfram Sang'; 'Daniel Baluta'; 'Jonathan Cameron'; 'Hartmut Knaack';
> 'Lars-Peter Clausen'; 'Peter Meerwald-Stadler'; 'Linux Kernel Mailing
List';
> linux-iio@vger.kernel.org; 'Lucas De Marchi'; 'Andy Gross'; 'Pramod
Gurav';
> 'Bjorn Andersson'; 'Guenter Roeck'; eibach@gdsys.de; linux-arm-
> msm@vger.kernel.org
> Subject: Re: [PATCH v4] iio: adc: Add TI ADS1015 ADC driver support
> 
> On Tue, Feb 09, 2016 at 12:41:35AM +0530, Sricharan wrote:
> > Hi,
> >
> > > -----Original Message-----
> > > From: Michael Welling [mailto:mwelling79@gmail.com] On Behalf Of
> > > Michael Welling
> > > Sent: Monday, February 08, 2016 10:07 PM
> > > To: Wolfram Sang
> > > Cc: Daniel Baluta; Jonathan Cameron; Hartmut Knaack; Lars-Peter
> > > Clausen; Peter Meerwald-Stadler; Linux Kernel Mailing List;
> > linux-iio@vger.kernel.org;
> > > Lucas De Marchi; Andy Gross; Pramod Gurav; Bjorn Andersson; Guenter
> > > Roeck; eibach@gdsys.de; Sricharan R; linux-arm-msm@vger.kernel.org
> > > Subject: Re: [PATCH v4] iio: adc: Add TI ADS1015 ADC driver support
> > >
> > > On Mon, Feb 08, 2016 at 11:25:00AM +0100, Wolfram Sang wrote:
> > > > On Fri, Feb 05, 2016 at 06:32:45PM -0600, Michael Welling wrote:
> > > > > On Fri, Feb 05, 2016 at 09:32:34PM +0200, Daniel Baluta wrote:
> > > > > > >> +static int ads1015_read_raw(struct iio_dev *indio_dev,
> > > > > > >> +                         struct iio_chan_spec const *chan,
> > > > > > >> +int
> > *val,
> > > > > > >> +                         int *val2, long mask) {
> > > > > > >> +     int ret, idx;
> > > > > > >> +     struct ads1015_data *data = iio_priv(indio_dev);
> > > > > > >> +
> > > > > > >> +     mutex_lock(&data->lock);
> > > > > > >> +     switch (mask) {
> > > > > > >> +     case IIO_CHAN_INFO_RAW:
> > > > > > >> +             if (iio_buffer_enabled(indio_dev)) {
> > > > > > >> +                     ret = -EBUSY;
> > > > > > >> +                     break;
> > > > > > >> +             }
> > > > > > >> +
> > > > > > >> +             ret = ads1015_set_power_state(data, true);
> > > > > > >> +             if (ret < 0)
> > > > > > >> +                     break;
> > > > > > >
> > > > > > > Just tested the driver on a Dragonboard 410C with a robotics
> > > > > > > mezzanine that I designed.
> > > > > > >
> > > > > > > The above ads1015_set_power_state(data, true) is always
> > > > > > > returning
> > -
> > > EINVAL.
> > > > > > >
> > > > > > > Any ideas why that would be happening?
> > > > > > > I think it may be the return from pm_runtime_get_sync?
> > > > > >
> > > > > > Can you confirm that pm_runtime_get_sync fails? Using some
> printk?
> > > > > >
> > > > > > Also adding printks in suspend/resume function would be helpful.
> > > > > > Do you have CONFIG_PM enabled?
> > > > > >
> > > > >
> > > > > Indeed it is the pm_runtime_get_sync that fails with a -EINVAL.
> > > > >
> > > > > > >
> > > > > > > When I comment out the break the readings come back but are
> > > > > > > not
> > > updated continually.
> > > > > > > If I read in_voltage0-voltage1_raw then in_voltage0_raw the
> > > > > > > value
> > is
> > > updated.
> > > > > >
> > > > > > I guess this is normal if set_power_state fails.
> > > > >
> > > > > The hwmod driver works fine BTW.
> > > > >
> > > > > My guess is there is an issue with the qup i2c driver seeing as
> > > > > it has worked on other system without issue.
> > > > >
> > > > > CC'd some the latest developer on the qup i2c driver.
> > > > >
> > > > > I2C guys have any ideas on this?
> > > > >
> > > >
> > > > Adding some more people who recently worked on this. Might be nice
> > > > to know which kernel version you are using.
> > > >
> >  Which i2c bus is this connected to ? I can give a try with 410c to
> > see why pm_runtime_get_sync from qup  fails.
> 
> It is on the lowspeed header. Here is my devicetree entry:
> 
>                  i2c@78b6000 {
>                  /* On Low speed expansion */
>                          label = "LS-I2C0";
>                          status = "okay";
> 
>                          pca: pca@40 {
>                                  compatible = "nxp,pca9685-pwm";
>                                  #pwm-cells = <2>;
>                                  reg = <0x40>;
>                          };
> 
>                          adc: adc@48 {
>                                  compatible = "ti,ads1015";
>                                  reg = <0x48>;
>                          };
>                  };

Whats the sequence in which the failure happens ?

I tested on DB410c by adding the DT entry that you mentioned above on
4.5-rc2 and rc3.
I see that the i2c transfers call from pca9685  during  pca9685_pwm_probe
did
go through and no failure from pm_runtime_get_sync

Regards,
 Sricharan

Re: [PATCH v4] iio: adc: Add TI ADS1015 ADC driver support

[Daniel Baluta]

<snap headers>

>> > > > > > >> +static int ads1015_read_raw(struct iio_dev *indio_dev,
>> > > > > > >> +                         struct iio_chan_spec const *chan,
>> > > > > > >> +int
>> > *val,
>> > > > > > >> +                         int *val2, long mask) {
>> > > > > > >> +     int ret, idx;
>> > > > > > >> +     struct ads1015_data *data = iio_priv(indio_dev);
>> > > > > > >> +
>> > > > > > >> +     mutex_lock(&data->lock);
>> > > > > > >> +     switch (mask) {
>> > > > > > >> +     case IIO_CHAN_INFO_RAW:
>> > > > > > >> +             if (iio_buffer_enabled(indio_dev)) {
>> > > > > > >> +                     ret = -EBUSY;
>> > > > > > >> +                     break;
>> > > > > > >> +             }
>> > > > > > >> +
>> > > > > > >> +             ret = ads1015_set_power_state(data, true);
>> > > > > > >> +             if (ret < 0)
>> > > > > > >> +                     break;
>> > > > > > >
>> > > > > > > Just tested the driver on a Dragonboard 410C with a robotics
>> > > > > > > mezzanine that I designed.
>> > > > > > >
>> > > > > > > The above ads1015_set_power_state(data, true) is always
>> > > > > > > returning
>> > -
>> > > EINVAL.
>> > > > > > >
>> > > > > > > Any ideas why that would be happening?
>> > > > > > > I think it may be the return from pm_runtime_get_sync?
>> > > > > >
>> > > > > > Can you confirm that pm_runtime_get_sync fails? Using some
>> printk?
>> > > > > >
>> > > > > > Also adding printks in suspend/resume function would be helpful.
>> > > > > > Do you have CONFIG_PM enabled?
>> > > > > >
>> > > > >
>> > > > > Indeed it is the pm_runtime_get_sync that fails with a -EINVAL.
>> > > > >
>> > > > > > >
>> > > > > > > When I comment out the break the readings come back but are
>> > > > > > > not
>> > > updated continually.
>> > > > > > > If I read in_voltage0-voltage1_raw then in_voltage0_raw the
>> > > > > > > value
>> > is
>> > > updated.
>> > > > > >
>> > > > > > I guess this is normal if set_power_state fails.
>> > > > >
>> > > > > The hwmod driver works fine BTW.
>> > > > >
>> > > > > My guess is there is an issue with the qup i2c driver seeing as
>> > > > > it has worked on other system without issue.
>> > > > >
>> > > > > CC'd some the latest developer on the qup i2c driver.
>> > > > >
>> > > > > I2C guys have any ideas on this?
>> > > > >
>> > > >
>> > > > Adding some more people who recently worked on this. Might be nice
>> > > > to know which kernel version you are using.
>> > > >
>> >  Which i2c bus is this connected to ? I can give a try with 410c to
>> > see why pm_runtime_get_sync from qup  fails.
>>
>> It is on the lowspeed header. Here is my devicetree entry:
>>
>>                  i2c@78b6000 {
>>                  /* On Low speed expansion */
>>                          label = "LS-I2C0";
>>                          status = "okay";
>>
>>                          pca: pca@40 {
>>                                  compatible = "nxp,pca9685-pwm";
>>                                  #pwm-cells = <2>;
>>                                  reg = <0x40>;
>>                          };
>>
>>                          adc: adc@48 {
>>                                  compatible = "ti,ads1015";
>>                                  reg = <0x48>;
>>                          };
>>                  };
>
> Whats the sequence in which the failure happens ?
>
> I tested on DB410c by adding the DT entry that you mentioned above on
> 4.5-rc2 and rc3.
> I see that the i2c transfers call from pca9685  during  pca9685_pwm_probe
> did
> go through and no failure from pm_runtime_get_sync

Hi Sricharan,

Are you looking at pca9685_pwm_probe in drivers/pwm/pwm-pca9685.c right?

I'm asking this because this driver doesn't seem to support runtime
pm and there is no check for regmap_write/regmap_write return
code in the probe function.


Daniel.

RE: [PATCH v4] iio: adc: Add TI ADS1015 ADC driver support

[Sricharan]

<snip>...

> >> > > > >
> >> > > > > Indeed it is the pm_runtime_get_sync that fails with a -EINVAL.
> >> > > > >
> >> > > > > > >
> >> > > > > > > When I comment out the break the readings come back but
> >> > > > > > > are not
> >> > > updated continually.
> >> > > > > > > If I read in_voltage0-voltage1_raw then in_voltage0_raw
> >> > > > > > > the value
> >> > is
> >> > > updated.
> >> > > > > >
> >> > > > > > I guess this is normal if set_power_state fails.
> >> > > > >
> >> > > > > The hwmod driver works fine BTW.
> >> > > > >
> >> > > > > My guess is there is an issue with the qup i2c driver seeing
> >> > > > > as it has worked on other system without issue.
> >> > > > >
> >> > > > > CC'd some the latest developer on the qup i2c driver.
> >> > > > >
> >> > > > > I2C guys have any ideas on this?
> >> > > > >
> >> > > >
> >> > > > Adding some more people who recently worked on this. Might be
> >> > > > nice to know which kernel version you are using.
> >> > > >
> >> >  Which i2c bus is this connected to ? I can give a try with 410c to
> >> > see why pm_runtime_get_sync from qup  fails.
> >>
> >> It is on the lowspeed header. Here is my devicetree entry:
> >>
> >>                  i2c@78b6000 {
> >>                  /* On Low speed expansion */
> >>                          label = "LS-I2C0";
> >>                          status = "okay";
> >>
> >>                          pca: pca@40 {
> >>                                  compatible = "nxp,pca9685-pwm";
> >>                                  #pwm-cells = <2>;
> >>                                  reg = <0x40>;
> >>                          };
> >>
> >>                          adc: adc@48 {
> >>                                  compatible = "ti,ads1015";
> >>                                  reg = <0x48>;
> >>                          };
> >>                  };
> >
> > Whats the sequence in which the failure happens ?
> >
> > I tested on DB410c by adding the DT entry that you mentioned above on
> > 4.5-rc2 and rc3.
> > I see that the i2c transfers call from pca9685  during
> > pca9685_pwm_probe did go through and no failure from
> > pm_runtime_get_sync
> 
> Hi Sricharan,
> 
> Are you looking at pca9685_pwm_probe in drivers/pwm/pwm-pca9685.c
> right?
>
 Yes.
 
> I'm asking this because this driver doesn't seem to support runtime pm and
> there is no check for regmap_write/regmap_write return code in the probe
> function.
   Hmm to be clear, so it’s the pm_runtime_getsync from i2c-qup which fails right ?
   I was tracking that when there are i2c_xfers from pwm. I did not see any failures there.
   So wanted to know the correct sequence to reproduce.
  
Regards,
  Sricharan
  

Re: [PATCH v4] iio: adc: Add TI ADS1015 ADC driver support

[Michael Welling]

On Wed, Feb 10, 2016 at 08:39:04PM +0530, Sricharan wrote:
> > Hi Sricharan,
> > 
> > Are you looking at pca9685_pwm_probe in drivers/pwm/pwm-pca9685.c
> > right?
> >
>  Yes.
>  
> > I'm asking this because this driver doesn't seem to support runtime pm and
> > there is no check for regmap_write/regmap_write return code in the probe
> > function.
>    Hmm to be clear, so it’s the pm_runtime_getsync from i2c-qup which fails right ?
>    I was tracking that when there are i2c_xfers from pwm. I did not see any failures there.
>    So wanted to know the correct sequence to reproduce.
>

The problem was discovered using the patch that this thread is on. The PWM driver does
not have the problem.

When the driver in this patch called pm_runtime_get_sync you got -EINVAL back.

> Regards,
>   Sricharan
>   
> 
> 

Re: [PATCH v4] iio: adc: Add TI ADS1015 ADC driver support

[Michael Welling]

On Wed, Feb 10, 2016 at 10:36:22AM -0600, Michael Welling wrote:
> On Wed, Feb 10, 2016 at 08:39:04PM +0530, Sricharan wrote:
> > > Hi Sricharan,
> > > 
> > > Are you looking at pca9685_pwm_probe in drivers/pwm/pwm-pca9685.c
> > > right?
> > >
> >  Yes.
> >  
> > > I'm asking this because this driver doesn't seem to support runtime pm and
> > > there is no check for regmap_write/regmap_write return code in the probe
> > > function.
> >    Hmm to be clear, so it’s the pm_runtime_getsync from i2c-qup which fails right ?
> >    I was tracking that when there are i2c_xfers from pwm. I did not see any failures there.
> >    So wanted to know the correct sequence to reproduce.
> >
> 
> The problem was discovered using the patch that this thread is on. The PWM driver does
> not have the problem.
> 
> When the driver in this patch called pm_runtime_get_sync you got -EINVAL back.

I noticed some patches for the QUP I2C driver in linux-next so I built against it.

The ADC driver now appears to work as desired.

root@dragonboard-410c:~# cat /sys/bus/iio/devices/iio\:device0/in_voltage0_raw 
287
root@dragonboard-410c:~# cat /sys/bus/iio/devices/iio\:device0/in_voltage1_raw 
269
root@dragonboard-410c:~# cat /sys/bus/iio/devices/iio\:device0/in_voltage2_raw 
270
root@dragonboard-410c:~# cat /sys/bus/iio/devices/iio\:device0/in_voltage3_raw 
271

> 
> > Regards,
> >   Sricharan
> >   
> > 
> > 

RE: [PATCH v4] iio: adc: Add TI ADS1015 ADC driver support

[Sricharan]

Hi,

> On Wed, Feb 10, 2016 at 10:36:22AM -0600, Michael Welling wrote:
> > On Wed, Feb 10, 2016 at 08:39:04PM +0530, Sricharan wrote:
> > > > Hi Sricharan,
> > > >
> > > > Are you looking at pca9685_pwm_probe in drivers/pwm/pwm-
> pca9685.c
> > > > right?
> > > >
> > >  Yes.
> > >
> > > > I'm asking this because this driver doesn't seem to support
> > > > runtime pm and there is no check for regmap_write/regmap_write
> > > > return code in the probe function.
> > >    Hmm to be clear, so it’s the pm_runtime_getsync from i2c-qup which
> fails right ?
> > >    I was tracking that when there are i2c_xfers from pwm. I did not see
> any failures there.
> > >    So wanted to know the correct sequence to reproduce.
> > >
> >
> > The problem was discovered using the patch that this thread is on. The
> > PWM driver does not have the problem.
> >
> > When the driver in this patch called pm_runtime_get_sync you got -EINVAL
> back.
> 
> I noticed some patches for the QUP I2C driver in linux-next so I built against
> it.
> 
> The ADC driver now appears to work as desired.
> 
   Ok, I suppose those were changes merged last week to add qup V2 tags support, but
   not sure how that is helping to solve the pm_runtime issue here. 

Regards,
  Sricharan