[PATCH] hwmon: (gpio-fan) allow to use alarm support alone from DT

[PATCH] hwmon: (gpio-fan) allow to use alarm support alone from DT

[Simon Guinot]

On some boards, such as the LaCie 2Big Network v2 or 2Big NAS (based on
Marvell Kirkwood SoCs), an I2C fan controller is used but the alarm
signal is wired to a separate GPIO. Unfortunately, the gpio-fan driver
can't be used to handle GPIO alarm alone from DT: an error is returned
if the "gpios" DT property is missing.

This patch allows to use the gpio-fan driver even if the "alarm-gpios"
DT property is defined alone.

Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
---
 .../devicetree/bindings/gpio/gpio-fan.txt          |  6 ++-
 drivers/hwmon/gpio-fan.c                           | 44 +++++++++++-----------
 2 files changed, 27 insertions(+), 23 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/gpio-fan.txt b/Documentation/devicetree/bindings/gpio/gpio-fan.txt
index 2dd457a3469a..a4c5d15b72e6 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-fan.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio-fan.txt
@@ -2,16 +2,18 @@ Bindings for fan connected to GPIO lines
 
 Required properties:
 - compatible : "gpio-fan"
+
+Optional properties:
 - gpios: Specifies the pins that map to bits in the control value,
   ordered MSB-->LSB.
 - gpio-fan,speed-map: A mapping of possible fan RPM speeds and the
   control value that should be set to achieve them. This array
   must have the RPM values in ascending order.
-
-Optional properties:
 - alarm-gpios: This pin going active indicates something is wrong with
   the fan, and a udev event will be fired.
 
+Note: At least one the "gpios" and "alarm-gpios" properties should be set.
+
 Examples:
 
 	gpio_fan {
diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
index 36abf814b8c7..c241f5b0b7cf 100644
--- a/drivers/hwmon/gpio-fan.c
+++ b/drivers/hwmon/gpio-fan.c
@@ -404,10 +404,32 @@ static int gpio_fan_get_of_pdata(struct device *dev,
 
 	node = dev->of_node;
 
+	/* Alarm GPIO if one exists */
+	if (of_gpio_named_count(node, "alarm-gpios") > 0) {
+		struct gpio_fan_alarm *alarm;
+		int val;
+		enum of_gpio_flags flags;
+
+		alarm = devm_kzalloc(dev, sizeof(struct gpio_fan_alarm),
+					GFP_KERNEL);
+		if (!alarm)
+			return -ENOMEM;
+
+		val = of_get_named_gpio_flags(node, "alarm-gpios", 0, &flags);
+		if (val < 0)
+			return val;
+		alarm->gpio = val;
+		alarm->active_low = flags & OF_GPIO_ACTIVE_LOW;
+
+		pdata->alarm = alarm;
+	}
+
 	/* Fill GPIO pin array */
 	pdata->num_ctrl = of_gpio_count(node);
 	if (pdata->num_ctrl <= 0) {
-		dev_err(dev, "gpios DT property empty / missing");
+		if (pdata->alarm)
+			return 0;
+		dev_err(dev, "DT properties empty / missing");
 		return -ENODEV;
 	}
 	ctrl = devm_kzalloc(dev, pdata->num_ctrl * sizeof(unsigned),
@@ -460,26 +482,6 @@ static int gpio_fan_get_of_pdata(struct device *dev,
 	}
 	pdata->speed = speed;
 
-	/* Alarm GPIO if one exists */
-	if (of_gpio_named_count(node, "alarm-gpios") > 0) {
-		struct gpio_fan_alarm *alarm;
-		int val;
-		enum of_gpio_flags flags;
-
-		alarm = devm_kzalloc(dev, sizeof(struct gpio_fan_alarm),
-					GFP_KERNEL);
-		if (!alarm)
-			return -ENOMEM;
-
-		val = of_get_named_gpio_flags(node, "alarm-gpios", 0, &flags);
-		if (val < 0)
-			return val;
-		alarm->gpio = val;
-		alarm->active_low = flags & OF_GPIO_ACTIVE_LOW;
-
-		pdata->alarm = alarm;
-	}
-
 	return 0;
 }
 
-- 
2.1.1

[PATCH] hwmon: (gpio-fan) allow to use alarm support alone from DT

[Andrew Lunn]

On Mon, Feb 23, 2015 at 01:58:54PM +0100, Simon Guinot wrote:
> On some boards, such as the LaCie 2Big Network v2 or 2Big NAS (based on
> Marvell Kirkwood SoCs), an I2C fan controller is used but the alarm
> signal is wired to a separate GPIO. Unfortunately, the gpio-fan driver
> can't be used to handle GPIO alarm alone from DT: an error is returned
> if the "gpios" DT property is missing.
> 
> This patch allows to use the gpio-fan driver even if the "alarm-gpios"
> DT property is defined alone.
> 
> Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
> ---
>  .../devicetree/bindings/gpio/gpio-fan.txt          |  6 ++-
>  drivers/hwmon/gpio-fan.c                           | 44 +++++++++++-----------
>  2 files changed, 27 insertions(+), 23 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-fan.txt b/Documentation/devicetree/bindings/gpio/gpio-fan.txt
> index 2dd457a3469a..a4c5d15b72e6 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio-fan.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio-fan.txt
> @@ -2,16 +2,18 @@ Bindings for fan connected to GPIO lines
>  
>  Required properties:
>  - compatible : "gpio-fan"
> +
> +Optional properties:
>  - gpios: Specifies the pins that map to bits in the control value,
>    ordered MSB-->LSB.
>  - gpio-fan,speed-map: A mapping of possible fan RPM speeds and the
>    control value that should be set to achieve them. This array
>    must have the RPM values in ascending order.
> -
> -Optional properties:
>  - alarm-gpios: This pin going active indicates something is wrong with
>    the fan, and a udev event will be fired.
>  
> +Note: At least one the "gpios" and "alarm-gpios" properties should be set.

I think you have this sentence wrong. What i think you mean is

Note: At least one of "gpios" or "alarm-gpios" properties should be set.

      Andrew

[PATCH] hwmon: (gpio-fan) allow to use alarm support alone from DT

[Guenter Roeck]

On 02/23/2015 04:58 AM, Simon Guinot wrote:
> On some boards, such as the LaCie 2Big Network v2 or 2Big NAS (based on
> Marvell Kirkwood SoCs), an I2C fan controller is used but the alarm
> signal is wired to a separate GPIO. Unfortunately, the gpio-fan driver
> can't be used to handle GPIO alarm alone from DT: an error is returned
> if the "gpios" DT property is missing.
>
> This patch allows to use the gpio-fan driver even if the "alarm-gpios"
> DT property is defined alone.
>

That is the wrong solution. The gpio alarm signal should be handled
by the fan controller driver.

Guenter

[PATCH] hwmon: (gpio-fan) allow to use alarm support alone from DT

[Simon Guinot]

On Mon, Feb 23, 2015 at 06:06:12AM -0800, Guenter Roeck wrote:
> On 02/23/2015 04:58 AM, Simon Guinot wrote:
> >On some boards, such as the LaCie 2Big Network v2 or 2Big NAS (based on
> >Marvell Kirkwood SoCs), an I2C fan controller is used but the alarm
> >signal is wired to a separate GPIO. Unfortunately, the gpio-fan driver
> >can't be used to handle GPIO alarm alone from DT: an error is returned
> >if the "gpios" DT property is missing.
> >
> >This patch allows to use the gpio-fan driver even if the "alarm-gpios"
> >DT property is defined alone.
> >
> 
> That is the wrong solution. The gpio alarm signal should be handled
> by the fan controller driver.

Hi Guenter,

Sure it should, but unfortunately it is not the case. I have several
boards using this mechanism (ie: a separate fan alarm GPIO). I think the
idea was to reduce the board cost...

Then this means I need a way to support this alarm signal and I can't
find a better one than using gpio-fan. Note that this was possible with
the original gpio-fan implementation (before the DT binding addition).

Let me know what you think.

Simon

> 
> Guenter
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150223/d5db44a8/attachment.sig>

[PATCH] hwmon: (gpio-fan) allow to use alarm support alone from DT

[Guenter Roeck]

On 02/23/2015 06:34 AM, Simon Guinot wrote:
> On Mon, Feb 23, 2015 at 06:06:12AM -0800, Guenter Roeck wrote:
>> On 02/23/2015 04:58 AM, Simon Guinot wrote:
>>> On some boards, such as the LaCie 2Big Network v2 or 2Big NAS (based on
>>> Marvell Kirkwood SoCs), an I2C fan controller is used but the alarm
>>> signal is wired to a separate GPIO. Unfortunately, the gpio-fan driver
>>> can't be used to handle GPIO alarm alone from DT: an error is returned
>>> if the "gpios" DT property is missing.
>>>
>>> This patch allows to use the gpio-fan driver even if the "alarm-gpios"
>>> DT property is defined alone.
>>>
>>
>> That is the wrong solution. The gpio alarm signal should be handled
>> by the fan controller driver.
>
> Hi Guenter,
>
> Sure it should, but unfortunately it is not the case. I have several
> boards using this mechanism (ie: a separate fan alarm GPIO). I think the
> idea was to reduce the board cost...
>
Well, yes, the driver for the fan controller chip needs to be updated
to support interrupts.

> Then this means I need a way to support this alarm signal and I can't
> find a better one than using gpio-fan. Note that this was possible with
> the original gpio-fan implementation (before the DT binding addition).
>

That doesn't help.

Guenter

[PATCH] hwmon: (gpio-fan) allow to use alarm support alone from DT

[Simon Guinot]

On Mon, Feb 23, 2015 at 06:43:16AM -0800, Guenter Roeck wrote:
> On 02/23/2015 06:34 AM, Simon Guinot wrote:
> >On Mon, Feb 23, 2015 at 06:06:12AM -0800, Guenter Roeck wrote:
> >>On 02/23/2015 04:58 AM, Simon Guinot wrote:
> >>>On some boards, such as the LaCie 2Big Network v2 or 2Big NAS (based on
> >>>Marvell Kirkwood SoCs), an I2C fan controller is used but the alarm
> >>>signal is wired to a separate GPIO. Unfortunately, the gpio-fan driver
> >>>can't be used to handle GPIO alarm alone from DT: an error is returned
> >>>if the "gpios" DT property is missing.
> >>>
> >>>This patch allows to use the gpio-fan driver even if the "alarm-gpios"
> >>>DT property is defined alone.
> >>>
> >>
> >>That is the wrong solution. The gpio alarm signal should be handled
> >>by the fan controller driver.
> >
> >Hi Guenter,
> >
> >Sure it should, but unfortunately it is not the case. I have several
> >boards using this mechanism (ie: a separate fan alarm GPIO). I think the
> >idea was to reduce the board cost...
> >
> Well, yes, the driver for the fan controller chip needs to be updated
> to support interrupts.

This will not help for the boards I mentioned. As an attempt to reduce
the board cost, a fan with 2 wires has been used. This means we don't
have any tachymeter feedback and then the controller alarm mechanism
can't be used. Instead a kind of hardware hack allows to have a fan
alarm on a separate GPIO.

> 
> >Then this means I need a way to support this alarm signal and I can't
> >find a better one than using gpio-fan. Note that this was possible with
> >the original gpio-fan implementation (before the DT binding addition).
> >
> 
> That doesn't help.

Handle the GPIO fan alarm feature from the fan controller driver don't
look good either to me. This alarm mechanism is not a part of the fan
controller itself but rather something apart. Also I am afraid that the
result would really look like a hack.

Simon

> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150225/7fcd800a/attachment.sig>

[PATCH] hwmon: (gpio-fan) allow to use alarm support alone from DT

[Andrew Lunn]

> Handle the GPIO fan alarm feature from the fan controller driver don't
> look good either to me. This alarm mechanism is not a part of the fan
> controller itself but rather something apart. Also I am afraid that the
> result would really look like a hack.

Hi Simon

It sounds like you need to extract the alarm code from gpio-fan into a
little library. Then put a wrapper around it to make a gpio-fan-alarm
driver.

	Andrew

[PATCH] hwmon: (gpio-fan) allow to use alarm support alone from DT

[Simon Guinot]

On Wed, Feb 25, 2015 at 02:50:15PM +0100, Andrew Lunn wrote:
> > Handle the GPIO fan alarm feature from the fan controller driver don't
> > look good either to me. This alarm mechanism is not a part of the fan
> > controller itself but rather something apart. Also I am afraid that the
> > result would really look like a hack.
> 
> Hi Simon
> 
> It sounds like you need to extract the alarm code from gpio-fan into a
> little library. Then put a wrapper around it to make a gpio-fan-alarm
> driver.

Hi Andrew,

Yes, I am working on that. I'll come back with some patches based on
this approach.

Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150225/794db90a/attachment.sig>

[PATCH] hwmon: (gpio-fan) allow to use alarm support alone from DT

[Guenter Roeck]

On 02/25/2015 05:50 AM, Andrew Lunn wrote:
>> Handle the GPIO fan alarm feature from the fan controller driver don't
>> look good either to me. This alarm mechanism is not a part of the fan
>> controller itself but rather something apart. Also I am afraid that the
>> result would really look like a hack.
>
> Hi Simon
>
> It sounds like you need to extract the alarm code from gpio-fan into a
> little library. Then put a wrapper around it to make a gpio-fan-alarm
> driver.
>

Please, the intend is to do the right thing, not to cause code bloat.

If it is in fact correct that the alarm mechanism in this case is not tied
to the fan controller, using the gpio-fan driver is ok. However, we need to
state and check in the code that _some_ property is mandatory. A driver with
only optional properties doesn't make sense.

Guenter

[PATCH] hwmon: (gpio-fan) allow to use alarm support alone from DT

[Guenter Roeck]

On 02/25/2015 06:29 AM, Simon Guinot wrote:
> On Wed, Feb 25, 2015 at 02:50:15PM +0100, Andrew Lunn wrote:
>>> Handle the GPIO fan alarm feature from the fan controller driver don't
>>> look good either to me. This alarm mechanism is not a part of the fan
>>> controller itself but rather something apart. Also I am afraid that the
>>> result would really look like a hack.
>>
>> Hi Simon
>>
>> It sounds like you need to extract the alarm code from gpio-fan into a
>> little library. Then put a wrapper around it to make a gpio-fan-alarm
>> driver.
>
> Hi Andrew,
>
> Yes, I am working on that. I'll come back with some patches based on
> this approach.
>
I have not seen your patches, but from the context it sounds like a NACK.

Guenter

[PATCH] hwmon: (gpio-fan) allow to use alarm support alone from DT

[Andrew Lunn]

> However, we need to state and check in the code that _some_ property
> is mandatory. A driver with only optional properties doesn't make
> sense.

The patch to the binding documentation said:

Note: At least one the "gpios" and "alarm-gpios" properties should be set.

which i then suggested should be:

Note: At least one of "gpios" or "alarm-gpios" properties should be set.

What we could do is move this sentence into the Required Properties of
the documentation.

The patch Simon submitted also does check that this condition is
fulfilled.

	Andrew

[PATCH] hwmon: (gpio-fan) allow to use alarm support alone from DT

[Guenter Roeck]

On Wed, Feb 25, 2015 at 04:04:59PM +0100, Andrew Lunn wrote:
> > However, we need to state and check in the code that _some_ property
> > is mandatory. A driver with only optional properties doesn't make
> > sense.
> 
> The patch to the binding documentation said:
> 
> Note: At least one the "gpios" and "alarm-gpios" properties should be set.
> 
> which i then suggested should be:
> 
> Note: At least one of "gpios" or "alarm-gpios" properties should be set.
> 
"should" is not "must".

Guenter

> What we could do is move this sentence into the Required Properties of
> the documentation.
> 
> The patch Simon submitted also does check that this condition is
> fulfilled.
> 
> 	Andrew
> 
> 
>