| Message ID | 1424696334-14767-1-git-send-email-simon.guinot@sequanux.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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
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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 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
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
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
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
> 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
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 > > >
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; }
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(-)