Re: [PATCH 11/11] iio: pressure-core: st: Provide correct regulator support

2013-09-04 Thread Mark Brown 
On Wed, Sep 04, 2013 at 02:26:38PM +0100, Lee Jones wrote:
> On Wed, 04 Sep 2013, Lars-Peter Clausen wrote:

> > > I think this would be better handled with something like Mark Brown's
> > > suggested regulator_get_optional [1,2].

> Thanks Mark, I didn't know that existed.

It's only just gone into Linus' tree this merge window so it's very new.


signature.asc
Description: Digital signature

Re: [PATCH 11/11] iio: pressure-core: st: Provide correct regulator support

2013-09-04 Thread Lee Jones 
On Wed, 04 Sep 2013, Lars-Peter Clausen wrote:

> On 09/04/2013 03:11 PM, Mark Rutland wrote:
> > Hi Lee,
> > 
> > On Wed, Sep 04, 2013 at 10:31:43AM +0100, Lee Jones wrote:
> >> The power to some of the sensors are controlled by regulators. In most
> >> cases these are 'always on', but if not they will fail to work until
> >> the regulator is enabled using the relevant APIs.
> >>
> >> Signed-off-by: Lee Jones 
> >> ---
> >>  drivers/iio/pressure/st_pressure_core.c | 13 +
> >>  include/linux/iio/common/st_sensors.h   |  3 +++
> >>  2 files changed, 16 insertions(+)
> >>
> >> diff --git a/drivers/iio/pressure/st_pressure_core.c 
> >> b/drivers/iio/pressure/st_pressure_core.c
> >> index f452417..7beed89 100644
> >> --- a/drivers/iio/pressure/st_pressure_core.c
> >> +++ b/drivers/iio/pressure/st_pressure_core.c
> >> @@ -23,6 +23,7 @@
> >>  #include 
> >>  #include 
> >>  #include 
> >> +#include 
> >>  #include 
> >>  
> >>  #include 
> >> @@ -315,6 +316,15 @@ int st_press_common_probe(struct iio_dev *indio_dev)
> >>indio_dev->modes = INDIO_DIRECT_MODE;
> >>indio_dev->info = _info;
> >>  
> >> +  /* Regulator not mandatory, but if requested we should enable it. */
> >> +  pdata->regulator = regulator_get(_dev->dev, "vdd");
> >> +  if (!IS_ERR_OR_NULL(pdata->regulator)) {
> > 
> > Can regulator_get return NULL? As far as I can see, it either returns a
> > valid reulator pointer or an ERR_PTR value.
> > 
> > When you say "if requested", do you mean "if described in the dt"? If
> > so, the above doesn't distunguish between a regulator not being listed
> > and one failing to be got (e.g. if we got EPROBE_DEFER from
> > regulator_get).
> > 
> > I think this would be better handled with something like Mark Brown's
> > suggested regulator_get_optional [1,2].

Thanks Mark, I didn't know that existed.

> It can return NULL, but NULL is actually a valid regulator in that case, so
> the check should only be IS_ERR. And yes regulator_get_optional is what
> should be used here.

Okay, I'll use that instead.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 11/11] iio: pressure-core: st: Provide correct regulator support

2013-09-04 Thread Mark Brown 
On Wed, Sep 04, 2013 at 02:11:11PM +0100, Mark Rutland wrote:
> On Wed, Sep 04, 2013 at 10:31:43AM +0100, Lee Jones wrote:

> > +   /* Regulator not mandatory, but if requested we should enable it. */
> > +   pdata->regulator = regulator_get(_dev->dev, "vdd");
> > +   if (!IS_ERR_OR_NULL(pdata->regulator)) {

> Can regulator_get return NULL? As far as I can see, it either returns a
> valid reulator pointer or an ERR_PTR value.

Yes, NULL is a valid regulator.

> When you say "if requested", do you mean "if described in the dt"? If
> so, the above doesn't distunguish between a regulator not being listed
> and one failing to be got (e.g. if we got EPROBE_DEFER from
> regulator_get).

> I think this would be better handled with something like Mark Brown's
> suggested regulator_get_optional [1,2].

If the regulator may genuinely be absent from the system then it should
be being requested using regulator_get_optional().  Otherwise it should
be being requested using regulator_get().  In both cases it is important 
that the driver pays attention to errors.


signature.asc
Description: Digital signature

Re: [PATCH 11/11] iio: pressure-core: st: Provide correct regulator support

2013-09-04 Thread Lars-Peter Clausen 
On 09/04/2013 03:11 PM, Mark Rutland wrote:
> Hi Lee,
> 
> On Wed, Sep 04, 2013 at 10:31:43AM +0100, Lee Jones wrote:
>> The power to some of the sensors are controlled by regulators. In most
>> cases these are 'always on', but if not they will fail to work until
>> the regulator is enabled using the relevant APIs.
>>
>> Signed-off-by: Lee Jones 
>> ---
>>  drivers/iio/pressure/st_pressure_core.c | 13 +
>>  include/linux/iio/common/st_sensors.h   |  3 +++
>>  2 files changed, 16 insertions(+)
>>
>> diff --git a/drivers/iio/pressure/st_pressure_core.c 
>> b/drivers/iio/pressure/st_pressure_core.c
>> index f452417..7beed89 100644
>> --- a/drivers/iio/pressure/st_pressure_core.c
>> +++ b/drivers/iio/pressure/st_pressure_core.c
>> @@ -23,6 +23,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>  
>>  #include 
>> @@ -315,6 +316,15 @@ int st_press_common_probe(struct iio_dev *indio_dev)
>>  indio_dev->modes = INDIO_DIRECT_MODE;
>>  indio_dev->info = _info;
>>  
>> +/* Regulator not mandatory, but if requested we should enable it. */
>> +pdata->regulator = regulator_get(_dev->dev, "vdd");
>> +if (!IS_ERR_OR_NULL(pdata->regulator)) {
> 
> Can regulator_get return NULL? As far as I can see, it either returns a
> valid reulator pointer or an ERR_PTR value.
> 
> When you say "if requested", do you mean "if described in the dt"? If
> so, the above doesn't distunguish between a regulator not being listed
> and one failing to be got (e.g. if we got EPROBE_DEFER from
> regulator_get).
> 
> I think this would be better handled with something like Mark Brown's
> suggested regulator_get_optional [1,2].

It can return NULL, but NULL is actually a valid regulator in that case, so
the check should only be IS_ERR. And yes regulator_get_optional is what
should be used here.

- Lars
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 11/11] iio: pressure-core: st: Provide correct regulator support

2013-09-04 Thread Mark Rutland 
Hi Lee,

On Wed, Sep 04, 2013 at 10:31:43AM +0100, Lee Jones wrote:
> The power to some of the sensors are controlled by regulators. In most
> cases these are 'always on', but if not they will fail to work until
> the regulator is enabled using the relevant APIs.
> 
> Signed-off-by: Lee Jones 
> ---
>  drivers/iio/pressure/st_pressure_core.c | 13 +
>  include/linux/iio/common/st_sensors.h   |  3 +++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/iio/pressure/st_pressure_core.c 
> b/drivers/iio/pressure/st_pressure_core.c
> index f452417..7beed89 100644
> --- a/drivers/iio/pressure/st_pressure_core.c
> +++ b/drivers/iio/pressure/st_pressure_core.c
> @@ -23,6 +23,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #include 
> @@ -315,6 +316,15 @@ int st_press_common_probe(struct iio_dev *indio_dev)
>   indio_dev->modes = INDIO_DIRECT_MODE;
>   indio_dev->info = _info;
>  
> + /* Regulator not mandatory, but if requested we should enable it. */
> + pdata->regulator = regulator_get(_dev->dev, "vdd");
> + if (!IS_ERR_OR_NULL(pdata->regulator)) {

Can regulator_get return NULL? As far as I can see, it either returns a
valid reulator pointer or an ERR_PTR value.

When you say "if requested", do you mean "if described in the dt"? If
so, the above doesn't distunguish between a regulator not being listed
and one failing to be got (e.g. if we got EPROBE_DEFER from
regulator_get).

I think this would be better handled with something like Mark Brown's
suggested regulator_get_optional [1,2].

Thanks,
Mark.

[1] https://lkml.org/lkml/2013/7/30/328 (cover)
[2] https://lkml.org/lkml/2013/7/30/334 (patches)

> + err = regulator_enable(pdata->regulator);
> + if (err != 0)
> + dev_warn(_dev->dev,
> +  "Failed to enable specified vdd regulator\n");
> + }
> +
>   err = st_sensors_check_device_support(indio_dev,
> ARRAY_SIZE(st_press_sensors),
> st_press_sensors);
> @@ -363,6 +373,9 @@ void st_press_common_remove(struct iio_dev *indio_dev)
>  {
>   struct st_sensor_data *pdata = iio_priv(indio_dev);
>  
> + if (!IS_ERR_OR_NULL(pdata->regulator))
> + regulator_disable(pdata->regulator);
> +
>   iio_device_unregister(indio_dev);
>   if (pdata->get_irq_data_ready(indio_dev) > 0) {
>   st_sensors_deallocate_trigger(indio_dev);
> diff --git a/include/linux/iio/common/st_sensors.h 
> b/include/linux/iio/common/st_sensors.h
> index 4aef925..eb6ef3f 100644
> --- a/include/linux/iio/common/st_sensors.h
> +++ b/include/linux/iio/common/st_sensors.h
> @@ -16,6 +16,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #define ST_SENSORS_TX_MAX_LENGTH 2
>  #define ST_SENSORS_RX_MAX_LENGTH 6
> @@ -197,6 +198,7 @@ struct st_sensors {
>   * @trig: The trigger in use by the core driver.
>   * @sensor: Pointer to the current sensor struct in use.
>   * @current_fullscale: Maximum range of measure by the sensor.
> + * @regulator: Pointer to sensor's power supply
>   * @enabled: Status of the sensor (false->off, true->on).
>   * @multiread_bit: Use or not particular bit for [I2C/SPI] multiread.
>   * @buffer_data: Data used by buffer part.
> @@ -211,6 +213,7 @@ struct st_sensor_data {
>   struct iio_trigger *trig;
>   struct st_sensors *sensor;
>   struct st_sensor_fullscale_avl *current_fullscale;
> + struct regulator *regulator;
>  
>   bool enabled;
>   bool multiread_bit;
> -- 
> 1.8.1.2
> 
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 11/11] iio: pressure-core: st: Provide correct regulator support

2013-09-04 Thread Mark Rutland 
Hi Lee,

On Wed, Sep 04, 2013 at 10:31:43AM +0100, Lee Jones wrote:
 The power to some of the sensors are controlled by regulators. In most
 cases these are 'always on', but if not they will fail to work until
 the regulator is enabled using the relevant APIs.
 
 Signed-off-by: Lee Jones lee.jo...@linaro.org
 ---
  drivers/iio/pressure/st_pressure_core.c | 13 +
  include/linux/iio/common/st_sensors.h   |  3 +++
  2 files changed, 16 insertions(+)
 
 diff --git a/drivers/iio/pressure/st_pressure_core.c 
 b/drivers/iio/pressure/st_pressure_core.c
 index f452417..7beed89 100644
 --- a/drivers/iio/pressure/st_pressure_core.c
 +++ b/drivers/iio/pressure/st_pressure_core.c
 @@ -23,6 +23,7 @@
  #include linux/iio/sysfs.h
  #include linux/iio/trigger.h
  #include linux/iio/buffer.h
 +#include linux/regulator/consumer.h
  #include asm/unaligned.h
  
  #include linux/iio/common/st_sensors.h
 @@ -315,6 +316,15 @@ int st_press_common_probe(struct iio_dev *indio_dev)
   indio_dev-modes = INDIO_DIRECT_MODE;
   indio_dev-info = press_info;
  
 + /* Regulator not mandatory, but if requested we should enable it. */
 + pdata-regulator = regulator_get(indio_dev-dev, vdd);
 + if (!IS_ERR_OR_NULL(pdata-regulator)) {

Can regulator_get return NULL? As far as I can see, it either returns a
valid reulator pointer or an ERR_PTR value.

When you say if requested, do you mean if described in the dt? If
so, the above doesn't distunguish between a regulator not being listed
and one failing to be got (e.g. if we got EPROBE_DEFER from
regulator_get).

I think this would be better handled with something like Mark Brown's
suggested regulator_get_optional [1,2].

Thanks,
Mark.

[1] https://lkml.org/lkml/2013/7/30/328 (cover)
[2] https://lkml.org/lkml/2013/7/30/334 (patches)

 + err = regulator_enable(pdata-regulator);
 + if (err != 0)
 + dev_warn(indio_dev-dev,
 +  Failed to enable specified vdd regulator\n);
 + }
 +
   err = st_sensors_check_device_support(indio_dev,
 ARRAY_SIZE(st_press_sensors),
 st_press_sensors);
 @@ -363,6 +373,9 @@ void st_press_common_remove(struct iio_dev *indio_dev)
  {
   struct st_sensor_data *pdata = iio_priv(indio_dev);
  
 + if (!IS_ERR_OR_NULL(pdata-regulator))
 + regulator_disable(pdata-regulator);
 +
   iio_device_unregister(indio_dev);
   if (pdata-get_irq_data_ready(indio_dev)  0) {
   st_sensors_deallocate_trigger(indio_dev);
 diff --git a/include/linux/iio/common/st_sensors.h 
 b/include/linux/iio/common/st_sensors.h
 index 4aef925..eb6ef3f 100644
 --- a/include/linux/iio/common/st_sensors.h
 +++ b/include/linux/iio/common/st_sensors.h
 @@ -16,6 +16,7 @@
  #include linux/irqreturn.h
  #include linux/iio/trigger.h
  #include linux/bitops.h
 +#include linux/regulator/consumer.h
  
  #define ST_SENSORS_TX_MAX_LENGTH 2
  #define ST_SENSORS_RX_MAX_LENGTH 6
 @@ -197,6 +198,7 @@ struct st_sensors {
   * @trig: The trigger in use by the core driver.
   * @sensor: Pointer to the current sensor struct in use.
   * @current_fullscale: Maximum range of measure by the sensor.
 + * @regulator: Pointer to sensor's power supply
   * @enabled: Status of the sensor (false-off, true-on).
   * @multiread_bit: Use or not particular bit for [I2C/SPI] multiread.
   * @buffer_data: Data used by buffer part.
 @@ -211,6 +213,7 @@ struct st_sensor_data {
   struct iio_trigger *trig;
   struct st_sensors *sensor;
   struct st_sensor_fullscale_avl *current_fullscale;
 + struct regulator *regulator;
  
   bool enabled;
   bool multiread_bit;
 -- 
 1.8.1.2
 
 
 ___
 linux-arm-kernel mailing list
 linux-arm-ker...@lists.infradead.org
 http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
 
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 11/11] iio: pressure-core: st: Provide correct regulator support

2013-09-04 Thread Lars-Peter Clausen 
On 09/04/2013 03:11 PM, Mark Rutland wrote:
 Hi Lee,
 
 On Wed, Sep 04, 2013 at 10:31:43AM +0100, Lee Jones wrote:
 The power to some of the sensors are controlled by regulators. In most
 cases these are 'always on', but if not they will fail to work until
 the regulator is enabled using the relevant APIs.

 Signed-off-by: Lee Jones lee.jo...@linaro.org
 ---
  drivers/iio/pressure/st_pressure_core.c | 13 +
  include/linux/iio/common/st_sensors.h   |  3 +++
  2 files changed, 16 insertions(+)

 diff --git a/drivers/iio/pressure/st_pressure_core.c 
 b/drivers/iio/pressure/st_pressure_core.c
 index f452417..7beed89 100644
 --- a/drivers/iio/pressure/st_pressure_core.c
 +++ b/drivers/iio/pressure/st_pressure_core.c
 @@ -23,6 +23,7 @@
  #include linux/iio/sysfs.h
  #include linux/iio/trigger.h
  #include linux/iio/buffer.h
 +#include linux/regulator/consumer.h
  #include asm/unaligned.h
  
  #include linux/iio/common/st_sensors.h
 @@ -315,6 +316,15 @@ int st_press_common_probe(struct iio_dev *indio_dev)
  indio_dev-modes = INDIO_DIRECT_MODE;
  indio_dev-info = press_info;
  
 +/* Regulator not mandatory, but if requested we should enable it. */
 +pdata-regulator = regulator_get(indio_dev-dev, vdd);
 +if (!IS_ERR_OR_NULL(pdata-regulator)) {
 
 Can regulator_get return NULL? As far as I can see, it either returns a
 valid reulator pointer or an ERR_PTR value.
 
 When you say if requested, do you mean if described in the dt? If
 so, the above doesn't distunguish between a regulator not being listed
 and one failing to be got (e.g. if we got EPROBE_DEFER from
 regulator_get).
 
 I think this would be better handled with something like Mark Brown's
 suggested regulator_get_optional [1,2].

It can return NULL, but NULL is actually a valid regulator in that case, so
the check should only be IS_ERR. And yes regulator_get_optional is what
should be used here.

- Lars
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 11/11] iio: pressure-core: st: Provide correct regulator support

2013-09-04 Thread Mark Brown 
On Wed, Sep 04, 2013 at 02:11:11PM +0100, Mark Rutland wrote:
 On Wed, Sep 04, 2013 at 10:31:43AM +0100, Lee Jones wrote:

  +   /* Regulator not mandatory, but if requested we should enable it. */
  +   pdata-regulator = regulator_get(indio_dev-dev, vdd);
  +   if (!IS_ERR_OR_NULL(pdata-regulator)) {

 Can regulator_get return NULL? As far as I can see, it either returns a
 valid reulator pointer or an ERR_PTR value.

Yes, NULL is a valid regulator.

 When you say if requested, do you mean if described in the dt? If
 so, the above doesn't distunguish between a regulator not being listed
 and one failing to be got (e.g. if we got EPROBE_DEFER from
 regulator_get).

 I think this would be better handled with something like Mark Brown's
 suggested regulator_get_optional [1,2].

If the regulator may genuinely be absent from the system then it should
be being requested using regulator_get_optional().  Otherwise it should
be being requested using regulator_get().  In both cases it is important 
that the driver pays attention to errors.


signature.asc
Description: Digital signature

Re: [PATCH 11/11] iio: pressure-core: st: Provide correct regulator support

2013-09-04 Thread Lee Jones 
On Wed, 04 Sep 2013, Lars-Peter Clausen wrote:

 On 09/04/2013 03:11 PM, Mark Rutland wrote:
  Hi Lee,
  
  On Wed, Sep 04, 2013 at 10:31:43AM +0100, Lee Jones wrote:
  The power to some of the sensors are controlled by regulators. In most
  cases these are 'always on', but if not they will fail to work until
  the regulator is enabled using the relevant APIs.
 
  Signed-off-by: Lee Jones lee.jo...@linaro.org
  ---
   drivers/iio/pressure/st_pressure_core.c | 13 +
   include/linux/iio/common/st_sensors.h   |  3 +++
   2 files changed, 16 insertions(+)
 
  diff --git a/drivers/iio/pressure/st_pressure_core.c 
  b/drivers/iio/pressure/st_pressure_core.c
  index f452417..7beed89 100644
  --- a/drivers/iio/pressure/st_pressure_core.c
  +++ b/drivers/iio/pressure/st_pressure_core.c
  @@ -23,6 +23,7 @@
   #include linux/iio/sysfs.h
   #include linux/iio/trigger.h
   #include linux/iio/buffer.h
  +#include linux/regulator/consumer.h
   #include asm/unaligned.h
   
   #include linux/iio/common/st_sensors.h
  @@ -315,6 +316,15 @@ int st_press_common_probe(struct iio_dev *indio_dev)
 indio_dev-modes = INDIO_DIRECT_MODE;
 indio_dev-info = press_info;
   
  +  /* Regulator not mandatory, but if requested we should enable it. */
  +  pdata-regulator = regulator_get(indio_dev-dev, vdd);
  +  if (!IS_ERR_OR_NULL(pdata-regulator)) {
  
  Can regulator_get return NULL? As far as I can see, it either returns a
  valid reulator pointer or an ERR_PTR value.
  
  When you say if requested, do you mean if described in the dt? If
  so, the above doesn't distunguish between a regulator not being listed
  and one failing to be got (e.g. if we got EPROBE_DEFER from
  regulator_get).
  
  I think this would be better handled with something like Mark Brown's
  suggested regulator_get_optional [1,2].

Thanks Mark, I didn't know that existed.

 It can return NULL, but NULL is actually a valid regulator in that case, so
 the check should only be IS_ERR. And yes regulator_get_optional is what
 should be used here.

Okay, I'll use that instead.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 11/11] iio: pressure-core: st: Provide correct regulator support

2013-09-04 Thread Mark Brown 
On Wed, Sep 04, 2013 at 02:26:38PM +0100, Lee Jones wrote:
 On Wed, 04 Sep 2013, Lars-Peter Clausen wrote:

   I think this would be better handled with something like Mark Brown's
   suggested regulator_get_optional [1,2].

 Thanks Mark, I didn't know that existed.

It's only just gone into Linus' tree this merge window so it's very new.


signature.asc
Description: Digital signature