Re: [PATCH] hwmon: (max6650) Add support for gpiodef

Re: [PATCH] hwmon: (max6650) Add support for gpiodef

[Laszlo Papp]

On Thu, Nov 14, 2013 at 5:24 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Thu, Nov 14, 2013 at 02:51:01PM +0000, Laszlo Papp wrote:
> > It is necessary for end users when they are using the gpio pins connected to the
> > fan controller. There is a separate gpiodef register provided, but it is unused
> > under the usual circumstances and that is probably the reason why this feature
> > has not been added before. It is necessary for our board to function properly.
> >
> > Signed-off-by: Laszlo Papp <lpapp@kde.org>
> > ---
> >  Documentation/hwmon/max6650 |   5 +++
> >  drivers/hwmon/max6650.c     | 107 ++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 112 insertions(+)
> >
> > diff --git a/Documentation/hwmon/max6650 b/Documentation/hwmon/max6650
> > index 58d9644..32c69a8 100644
> > --- a/Documentation/hwmon/max6650
> > +++ b/Documentation/hwmon/max6650
> > @@ -39,6 +39,11 @@ pwm1               rw      relative speed (0-255), 255=max. speed.
> >  fan1_div     rw      sets the speed range the inputs can handle. Legal
> >                       values are 1, 2, 4, and 8. Use lower values for
> >                       faster fans.
> > +gpio0        rw      sets the gpio 0 PIN. Legal values are 0, 1, 2, and 3.
> > +gpio1        rw      sets the gpio 1 PIN. Legal values are 0, 1, 2, and 3.
> > +gpio2        rw      sets the gpio 2 PIN. Legal values are 0, 1, 2, and 3.
> > +gpio3        rw      sets the gpio 3 PIN. Legal values are 0, and 1.
> > +gpio4        rw      sets the gpio 4 PIN. Legal values are 0, and 1.
> >
> gpio pins should be exported through the gpio subsystem, usually with a gpio
> driver. In this case, it may be acceptable to have the driver register with
> the gpio subsystem to export the pins. Using local sysfs attributes is wrong
> and unacceptable.

In short: I am not sure.

My concern is that these are not generic gpio pins. They seem to have
chip specific functionality, like alarm, full-on, and clock (internal
and external). Strictly speaking, one could even mention that to
expose the GPIODEF register as is without splitting it into five
separate gpio pin entries. Even those five pins behave slightly
differently.

I considered both, but these are the reasons why I decided to keep it
chip specific rather than separately in a generic subsystem.

Cheers,
Laszlo

Re: [PATCH] hwmon: (max6650) Add support for gpiodef

[Guenter Roeck]

On Tue, Nov 19, 2013 at 04:42:49PM +0000, Laszlo Papp wrote:
> On Fri, Nov 15, 2013 at 7:52 PM, Marcus Folkesson
> <marcus.folkesson@gmail.com> wrote:
> >
> >> This is just one use case of those, you could also use it for
> >> non-generic gpio functionality, like alarm, "full-on", internal clock,
> >> external clock, etc. I believe it is always a bit tricky with MFD. I
> >> personally prefer to put it into the chip driver because this is not
> >> clearly a generic gpio interface here, and I need to drive it
> >> dynamically.
> >
> > I agree.
> >
> > I think the solution with expose the "GPIOs" in sysfs is the right way to
> > go.
> > The chip-function is of a dynamic nature and should therefor not be set in
> > platform data / devicetree.
> >
> > As mentioned before, GPIOs should use the gpio subsystem whenever possible,
> > but the the gpio-functionality is just a subset of
> > functions these pins may be set to.
> >
> > Also, the I think the *real* reason why the entries is called "gpio" is that
> > it is so the registers are are mentioned in the datasheet.
> > Everyone that is working with the device will know what it is all about.
> > I see it more as an register expose than a gpio interface...
> >
> > I agree that the entries does not really fit here. But they does not fit
> > better elsewhere either.
> > And I don't think they fit worse than the alarm-entries that is already in
> > mainline.
> >
> > Anyway, I think the documentation file should mention what function each
> > valid value represent.
> 
> Yes, makes sense to make the documentation more comprehensive. Thanks.
> 
> Any other issues from anyone before submitting a polished version?
> 
You'll have to get feedback from Jean. I won't accept the patch.

Guenter

Re: [PATCH] hwmon: (max6650) Add support for gpiodef

[Laszlo Papp]

On Fri, Nov 15, 2013 at 7:52 PM, Marcus Folkesson
<marcus.folkesson@gmail.com> wrote:
>
>> This is just one use case of those, you could also use it for
>> non-generic gpio functionality, like alarm, "full-on", internal clock,
>> external clock, etc. I believe it is always a bit tricky with MFD. I
>> personally prefer to put it into the chip driver because this is not
>> clearly a generic gpio interface here, and I need to drive it
>> dynamically.
>
> I agree.
>
> I think the solution with expose the "GPIOs" in sysfs is the right way to
> go.
> The chip-function is of a dynamic nature and should therefor not be set in
> platform data / devicetree.
>
> As mentioned before, GPIOs should use the gpio subsystem whenever possible,
> but the the gpio-functionality is just a subset of
> functions these pins may be set to.
>
> Also, the I think the *real* reason why the entries is called "gpio" is that
> it is so the registers are are mentioned in the datasheet.
> Everyone that is working with the device will know what it is all about.
> I see it more as an register expose than a gpio interface...
>
> I agree that the entries does not really fit here. But they does not fit
> better elsewhere either.
> And I don't think they fit worse than the alarm-entries that is already in
> mainline.
>
> Anyway, I think the documentation file should mention what function each
> valid value represent.

Yes, makes sense to make the documentation more comprehensive. Thanks.

Any other issues from anyone before submitting a polished version?

Re: [PATCH] hwmon: (max6650) Add support for gpiodef

[Laszlo Papp]

On Thu, Nov 14, 2013 at 7:00 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> What are those pins used for ?

I would like to drive them dynamically like the rest of the features
exposed to the sysfs in a similar way.

For instance, one may need to output a logic-low level on the pin 0 to
get an operation working. One use case could be (and is) enabling or
disabling the fans dynamically based on the temperature, etc
condition. Imagine that if you have a 12 V fan with 5 V board power
supply, and such a pin would be useful to establish the bridge between
them with diodes.

This is just one use case of those, you could also use it for
non-generic gpio functionality, like alarm, "full-on", internal clock,
external clock, etc. I believe it is always a bit tricky with MFD. I
personally prefer to put it into the chip driver because this is not
clearly a generic gpio interface here, and I need to drive it
dynamically.

Re: [PATCH] hwmon: (max6650) Add support for gpiodef

[Guenter Roeck]

On Thu, Nov 14, 2013 at 06:35:57PM +0000, Laszlo Papp wrote:
> On Thu, Nov 14, 2013 at 6:18 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On Thu, Nov 14, 2013 at 06:13:45PM +0000, Laszlo Papp wrote:
> >> On Thu, Nov 14, 2013 at 5:24 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> >>
> >> > On Thu, Nov 14, 2013 at 02:51:01PM +0000, Laszlo Papp wrote:
> >> > > It is necessary for end users when they are using the gpio pins
> >> > connected to the
> >> > > fan controller. There is a separate gpiodef register provided, but it is
> >> > unused
> >> > > under the usual circumstances and that is probably the reason why this
> >> > feature
> >> > > has not been added before. It is necessary for our board to function
> >> > properly.
> >> > >
> >> > > Signed-off-by: Laszlo Papp <lpapp@kde.org>
> >> > > ---
> >> > >  Documentation/hwmon/max6650 |   5 +++
> >> > >  drivers/hwmon/max6650.c     | 107
> >> > ++++++++++++++++++++++++++++++++++++++++++++
> >> > >  2 files changed, 112 insertions(+)
> >> > >
> >> > > diff --git a/Documentation/hwmon/max6650 b/Documentation/hwmon/max6650
> >> > > index 58d9644..32c69a8 100644
> >> > > --- a/Documentation/hwmon/max6650
> >> > > +++ b/Documentation/hwmon/max6650
> >> > > @@ -39,6 +39,11 @@ pwm1               rw      relative speed (0-255),
> >> > 255=max. speed.
> >> > >  fan1_div     rw      sets the speed range the inputs can handle. Legal
> >> > >                       values are 1, 2, 4, and 8. Use lower values for
> >> > >                       faster fans.
> >> > > +gpio0        rw      sets the gpio 0 PIN. Legal values are 0, 1, 2, and
> >> > 3.
> >> > > +gpio1        rw      sets the gpio 1 PIN. Legal values are 0, 1, 2, and
> >> > 3.
> >> > > +gpio2        rw      sets the gpio 2 PIN. Legal values are 0, 1, 2, and
> >> > 3.
> >> > > +gpio3        rw      sets the gpio 3 PIN. Legal values are 0, and 1.
> >> > > +gpio4        rw      sets the gpio 4 PIN. Legal values are 0, and 1.
> >> > >
> >> > gpio pins should be exported through the gpio subsystem, usually with a
> >> > gpio
> >> > driver. In this case, it may be acceptable to have the driver register with
> >> > the gpio subsystem to export the pins. Using local sysfs attributes is
> >> > wrong
> >> > and unacceptable.
> >> >
> >>
> >> In short: I am not sure.
> >>
> >> My concern is that these are not generic gpio pins. They seem to have chip
> >> specific functionality, like alarm, full-on, and clock (internal and
> >> external). Strictly speaking, one could even mention that to expose the
> >> GPIODEF register as is without splitting it into five separate gpio pin
> >> entries. Even those five pins behave slightly differently.
> >>
> >> I considered both, but these are the reasons why I decided to keep it chip
> >> specific rather than separately in a generic subsystem.
> >>
> > If the registers are not really gpio pins but are needed / used for chip
> > configuration, the other option would be to configure the values using platform
> > data and/or devicetree data. Either case, exporting the pins to user space via
> > sysfs attribute files in the device directory is the wrong approach.
> 
> Hmm, that would not allow dynamic settings on its own - if I am not
> mistaken - which we are in need of. I might be mistaken tough, so
> please forgive my shortcomings.
> 
> I was also thinking of module parameters before, as well. Perhaps that
> is a better approach. I am not sure. What do you think about it?
> 
Problem with that is that it applies to all MAX6650's.

What are those pins used for ?
If they need to be set at runtime, gpio may be a choice after all. 
I think I may need to get a better understanding of your use case.

Thanks,
Guenter

Re: [PATCH] hwmon: (max6650) Add support for gpiodef

[Laszlo Papp]

On Thu, Nov 14, 2013 at 2:51 PM, Laszlo Papp <lpapp@kde.org> wrote:
> +       case 1:
> +               data->gpiodef &= ~MAX6650_GPIO_DEF_GPIO1_LOW;
> +               data->gpiodef &= (gpio << 2);

Yikes, that should have been "|=" of course. I will fix it if needed
once the right approach is found and agreed upon...

Re: [PATCH] hwmon: (max6650) Add support for gpiodef

[Laszlo Papp]

On Thu, Nov 14, 2013 at 6:18 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Thu, Nov 14, 2013 at 06:13:45PM +0000, Laszlo Papp wrote:
>> On Thu, Nov 14, 2013 at 5:24 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> > On Thu, Nov 14, 2013 at 02:51:01PM +0000, Laszlo Papp wrote:
>> > > It is necessary for end users when they are using the gpio pins
>> > connected to the
>> > > fan controller. There is a separate gpiodef register provided, but it is
>> > unused
>> > > under the usual circumstances and that is probably the reason why this
>> > feature
>> > > has not been added before. It is necessary for our board to function
>> > properly.
>> > >
>> > > Signed-off-by: Laszlo Papp <lpapp@kde.org>
>> > > ---
>> > >  Documentation/hwmon/max6650 |   5 +++
>> > >  drivers/hwmon/max6650.c     | 107
>> > ++++++++++++++++++++++++++++++++++++++++++++
>> > >  2 files changed, 112 insertions(+)
>> > >
>> > > diff --git a/Documentation/hwmon/max6650 b/Documentation/hwmon/max6650
>> > > index 58d9644..32c69a8 100644
>> > > --- a/Documentation/hwmon/max6650
>> > > +++ b/Documentation/hwmon/max6650
>> > > @@ -39,6 +39,11 @@ pwm1               rw      relative speed (0-255),
>> > 255=max. speed.
>> > >  fan1_div     rw      sets the speed range the inputs can handle. Legal
>> > >                       values are 1, 2, 4, and 8. Use lower values for
>> > >                       faster fans.
>> > > +gpio0        rw      sets the gpio 0 PIN. Legal values are 0, 1, 2, and
>> > 3.
>> > > +gpio1        rw      sets the gpio 1 PIN. Legal values are 0, 1, 2, and
>> > 3.
>> > > +gpio2        rw      sets the gpio 2 PIN. Legal values are 0, 1, 2, and
>> > 3.
>> > > +gpio3        rw      sets the gpio 3 PIN. Legal values are 0, and 1.
>> > > +gpio4        rw      sets the gpio 4 PIN. Legal values are 0, and 1.
>> > >
>> > gpio pins should be exported through the gpio subsystem, usually with a
>> > gpio
>> > driver. In this case, it may be acceptable to have the driver register with
>> > the gpio subsystem to export the pins. Using local sysfs attributes is
>> > wrong
>> > and unacceptable.
>> >
>>
>> In short: I am not sure.
>>
>> My concern is that these are not generic gpio pins. They seem to have chip
>> specific functionality, like alarm, full-on, and clock (internal and
>> external). Strictly speaking, one could even mention that to expose the
>> GPIODEF register as is without splitting it into five separate gpio pin
>> entries. Even those five pins behave slightly differently.
>>
>> I considered both, but these are the reasons why I decided to keep it chip
>> specific rather than separately in a generic subsystem.
>>
> If the registers are not really gpio pins but are needed / used for chip
> configuration, the other option would be to configure the values using platform
> data and/or devicetree data. Either case, exporting the pins to user space via
> sysfs attribute files in the device directory is the wrong approach.

Hmm, that would not allow dynamic settings on its own - if I am not
mistaken - which we are in need of. I might be mistaken tough, so
please forgive my shortcomings.

I was also thinking of module parameters before, as well. Perhaps that
is a better approach. I am not sure. What do you think about it?

Re: [PATCH] hwmon: (max6650) Add support for gpiodef

[Guenter Roeck]

On Thu, Nov 14, 2013 at 06:13:45PM +0000, Laszlo Papp wrote:
> On Thu, Nov 14, 2013 at 5:24 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> 
> > On Thu, Nov 14, 2013 at 02:51:01PM +0000, Laszlo Papp wrote:
> > > It is necessary for end users when they are using the gpio pins
> > connected to the
> > > fan controller. There is a separate gpiodef register provided, but it is
> > unused
> > > under the usual circumstances and that is probably the reason why this
> > feature
> > > has not been added before. It is necessary for our board to function
> > properly.
> > >
> > > Signed-off-by: Laszlo Papp <lpapp@kde.org>
> > > ---
> > >  Documentation/hwmon/max6650 |   5 +++
> > >  drivers/hwmon/max6650.c     | 107
> > ++++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 112 insertions(+)
> > >
> > > diff --git a/Documentation/hwmon/max6650 b/Documentation/hwmon/max6650
> > > index 58d9644..32c69a8 100644
> > > --- a/Documentation/hwmon/max6650
> > > +++ b/Documentation/hwmon/max6650
> > > @@ -39,6 +39,11 @@ pwm1               rw      relative speed (0-255),
> > 255=max. speed.
> > >  fan1_div     rw      sets the speed range the inputs can handle. Legal
> > >                       values are 1, 2, 4, and 8. Use lower values for
> > >                       faster fans.
> > > +gpio0        rw      sets the gpio 0 PIN. Legal values are 0, 1, 2, and
> > 3.
> > > +gpio1        rw      sets the gpio 1 PIN. Legal values are 0, 1, 2, and
> > 3.
> > > +gpio2        rw      sets the gpio 2 PIN. Legal values are 0, 1, 2, and
> > 3.
> > > +gpio3        rw      sets the gpio 3 PIN. Legal values are 0, and 1.
> > > +gpio4        rw      sets the gpio 4 PIN. Legal values are 0, and 1.
> > >
> > gpio pins should be exported through the gpio subsystem, usually with a
> > gpio
> > driver. In this case, it may be acceptable to have the driver register with
> > the gpio subsystem to export the pins. Using local sysfs attributes is
> > wrong
> > and unacceptable.
> >
> 
> In short: I am not sure.
> 
> My concern is that these are not generic gpio pins. They seem to have chip
> specific functionality, like alarm, full-on, and clock (internal and
> external). Strictly speaking, one could even mention that to expose the
> GPIODEF register as is without splitting it into five separate gpio pin
> entries. Even those five pins behave slightly differently.
> 
> I considered both, but these are the reasons why I decided to keep it chip
> specific rather than separately in a generic subsystem.
> 
If the registers are not really gpio pins but are needed / used for chip
configuration, the other option would be to configure the values using platform
data and/or devicetree data. Either case, exporting the pins to user space via
sysfs attribute files in the device directory is the wrong approach.

Guenter

Re: [PATCH] hwmon: (max6650) Add support for gpiodef

[Guenter Roeck]

On Thu, Nov 14, 2013 at 02:51:01PM +0000, Laszlo Papp wrote:
> It is necessary for end users when they are using the gpio pins connected to the
> fan controller. There is a separate gpiodef register provided, but it is unused
> under the usual circumstances and that is probably the reason why this feature
> has not been added before. It is necessary for our board to function properly.
> 
> Signed-off-by: Laszlo Papp <lpapp@kde.org>
> ---
>  Documentation/hwmon/max6650 |   5 +++
>  drivers/hwmon/max6650.c     | 107 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 112 insertions(+)
> 
> diff --git a/Documentation/hwmon/max6650 b/Documentation/hwmon/max6650
> index 58d9644..32c69a8 100644
> --- a/Documentation/hwmon/max6650
> +++ b/Documentation/hwmon/max6650
> @@ -39,6 +39,11 @@ pwm1		rw	relative speed (0-255), 255=max. speed.
>  fan1_div	rw	sets the speed range the inputs can handle. Legal
>  			values are 1, 2, 4, and 8. Use lower values for
>  			faster fans.
> +gpio0     	rw	sets the gpio 0 PIN. Legal values are 0, 1, 2, and 3.
> +gpio1     	rw	sets the gpio 1 PIN. Legal values are 0, 1, 2, and 3.
> +gpio2     	rw	sets the gpio 2 PIN. Legal values are 0, 1, 2, and 3.
> +gpio3     	rw	sets the gpio 3 PIN. Legal values are 0, and 1.
> +gpio4     	rw	sets the gpio 4 PIN. Legal values are 0, and 1.
>  
gpio pins should be exported through the gpio subsystem, usually with a gpio
driver. In this case, it may be acceptable to have the driver register with
the gpio subsystem to export the pins. Using local sysfs attributes is wrong
and unacceptable.

Guenter

[PATCH] hwmon: (max6650) Add support for gpiodef

[Laszlo Papp]

It is necessary for end users when they are using the gpio pins connected to the
fan controller. There is a separate gpiodef register provided, but it is unused
under the usual circumstances and that is probably the reason why this feature
has not been added before. It is necessary for our board to function properly.

Signed-off-by: Laszlo Papp <lpapp@kde.org>
---
 Documentation/hwmon/max6650 |   5 +++
 drivers/hwmon/max6650.c     | 107 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 112 insertions(+)

diff --git a/Documentation/hwmon/max6650 b/Documentation/hwmon/max6650
index 58d9644..32c69a8 100644
--- a/Documentation/hwmon/max6650
+++ b/Documentation/hwmon/max6650
@@ -39,6 +39,11 @@ pwm1		rw	relative speed (0-255), 255=max. speed.
 fan1_div	rw	sets the speed range the inputs can handle. Legal
 			values are 1, 2, 4, and 8. Use lower values for
 			faster fans.
+gpio0     	rw	sets the gpio 0 PIN. Legal values are 0, 1, 2, and 3.
+gpio1     	rw	sets the gpio 1 PIN. Legal values are 0, 1, 2, and 3.
+gpio2     	rw	sets the gpio 2 PIN. Legal values are 0, 1, 2, and 3.
+gpio3     	rw	sets the gpio 3 PIN. Legal values are 0, and 1.
+gpio4     	rw	sets the gpio 4 PIN. Legal values are 0, and 1.
 
 Usage notes
 -----------
diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index 0cafc39..2268142 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -90,6 +90,16 @@ module_param(clock, int, S_IRUGO);
 #define MAX6650_COUNT_MASK		0x03
 
 /*
+ * Gpio Def register bits
+ */
+
+#define MAX6650_GPIO_DEF_GPIO0_LOW  0x03
+#define MAX6650_GPIO_DEF_GPIO1_LOW  0x0C
+#define MAX6650_GPIO_DEF_GPIO2_LOW  0x30
+#define MAX6650_GPIO_DEF_GPIO3      0x40
+#define MAX6650_GPIO_DEF_GPIO4      0x80
+
+/*
  * Alarm status register bits
  */
 
@@ -145,6 +155,7 @@ struct max6650_data {
 	/* register values */
 	u8 speed;
 	u8 config;
+	u8 gpiodef;
 	u8 tach[4];
 	u8 count;
 	u8 dac;
@@ -461,6 +472,90 @@ static ssize_t get_alarm(struct device *dev, struct device_attribute *devattr,
 	return sprintf(buf, "%d\n", alarm);
 }
 
+static ssize_t get_gpio(struct device *dev, struct device_attribute *devattr,
+			 char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct max6650_data *data = max6650_update_device(dev);
+
+	int gpio = 0;
+
+	switch (attr->index) {
+	case 0:
+		gpio = data->gpiodef & MAX6650_GPIO_DEF_GPIO0_LOW;
+		break;
+	case 1:
+		gpio = (data->gpiodef & MAX6650_GPIO_DEF_GPIO1_LOW) >> 2;
+		break;
+	case 2:
+		gpio = (data->gpiodef & MAX6650_GPIO_DEF_GPIO2_LOW) >> 4;
+		break;
+	case 3:
+		gpio = (data->gpiodef & MAX6650_GPIO_DEF_GPIO3) >> 6;
+		break;
+	case 4:
+		gpio = (data->gpiodef & MAX6650_GPIO_DEF_GPIO4) >> 7;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return sprintf(buf, "%d\n", gpio);
+}
+
+static ssize_t set_gpio(struct device *dev, struct device_attribute *devattr,
+			 char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct max6650_data *data = max6650_update_device(dev);
+	struct i2c_client *client = to_i2c_client(dev);
+	u8 gpio;
+	int err;
+
+	err = kstrtoul(buf, 10, &gpio);
+	if (err)
+		return err;
+
+	mutex_lock(&data->update_lock);
+
+	switch (attr->index) {
+	case 0:
+		data->gpiodef &= ~MAX6650_GPIO_DEF_GPIO0_LOW;
+		data->gpiodef |= gpio;
+		break;
+	case 1:
+		data->gpiodef &= ~MAX6650_GPIO_DEF_GPIO1_LOW;
+		data->gpiodef &= (gpio << 2);
+		break;
+	case 2:
+		data->gpiodef &= ~MAX6650_GPIO_DEF_GPIO2_LOW;
+		data->gpiodef &= (gpio << 4);
+		break;
+	case 3:
+		data->gpiodef &= ~MAX6650_GPIO_DEF_GPIO3;
+		data->gpiodef &= (gpio << 6);
+		break;
+	case 4:
+		data->gpiodef &= ~MAX6650_GPIO_DEF_GPIO4;
+		data->gpiodef &= (gpio << 7);
+		break;
+	default:
+	    mutex_unlock(&data->update_lock);
+		return -EINVAL;
+	}
+
+	if (i2c_smbus_write_byte_data(client, MAX6650_REG_GPIO_DEF,
+							data->gpiodef)) {
+		dev_err(&client->dev, "Gpiodef write error, aborting.\n");
+	    mutex_unlock(&data->update_lock);
+		return err;
+	}
+
+	mutex_unlock(&data->update_lock);
+
+	return count;
+}
+
 static SENSOR_DEVICE_ATTR(fan1_input, S_IRUGO, get_fan, NULL, 0);
 static SENSOR_DEVICE_ATTR(fan2_input, S_IRUGO, get_fan, NULL, 1);
 static SENSOR_DEVICE_ATTR(fan3_input, S_IRUGO, get_fan, NULL, 2);
@@ -479,6 +574,11 @@ static SENSOR_DEVICE_ATTR(gpio1_alarm, S_IRUGO, get_alarm, NULL,
 			  MAX6650_ALRM_GPIO1);
 static SENSOR_DEVICE_ATTR(gpio2_alarm, S_IRUGO, get_alarm, NULL,
 			  MAX6650_ALRM_GPIO2);
+static SENSOR_DEVICE_ATTR(gpio0, S_IWUSR | S_IRUGO, get_gpio, set_gpio, 0);
+static SENSOR_DEVICE_ATTR(gpio1, S_IWUSR | S_IRUGO, get_gpio, set_gpio, 1);
+static SENSOR_DEVICE_ATTR(gpio2, S_IWUSR | S_IRUGO, get_gpio, set_gpio, 2);
+static SENSOR_DEVICE_ATTR(gpio3, S_IWUSR | S_IRUGO, get_gpio, set_gpio, 3);
+static SENSOR_DEVICE_ATTR(gpio4, S_IWUSR | S_IRUGO, get_gpio, set_gpio, 4);
 
 static umode_t max6650_attrs_visible(struct kobject *kobj, struct attribute *a,
 				    int n)
@@ -516,6 +616,8 @@ static struct attribute *max6650_attrs[] = {
 	&sensor_dev_attr_fan1_fault.dev_attr.attr,
 	&sensor_dev_attr_gpio1_alarm.dev_attr.attr,
 	&sensor_dev_attr_gpio2_alarm.dev_attr.attr,
+	&sensor_dev_attr_gpio0.dev_attr.attr,
+	&sensor_dev_attr_gpio1.dev_attr.attr,
 	NULL
 };
 
@@ -528,6 +630,9 @@ static struct attribute *max6651_attrs[] = {
 	&sensor_dev_attr_fan2_input.dev_attr.attr,
 	&sensor_dev_attr_fan3_input.dev_attr.attr,
 	&sensor_dev_attr_fan4_input.dev_attr.attr,
+	&sensor_dev_attr_gpio2.dev_attr.attr,
+	&sensor_dev_attr_gpio3.dev_attr.attr,
+	&sensor_dev_attr_gpio4.dev_attr.attr,
 	NULL
 };
 
@@ -704,6 +809,8 @@ static struct max6650_data *max6650_update_device(struct device *dev)
 						       MAX6650_REG_SPEED);
 		data->config = i2c_smbus_read_byte_data(client,
 							MAX6650_REG_CONFIG);
+		data->gpiodef = i2c_smbus_read_byte_data(client,
+							MAX6650_REG_GPIO_DEF);
 		for (i = 0; i < data->nr_fans; i++) {
 			data->tach[i] = i2c_smbus_read_byte_data(client,
 								 tach_reg[i]);
-- 
1.8.4.2