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

[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

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

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

[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: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 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

[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

[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: [lm-sensors] [PATCH] hwmon: (max6650) Add support for gpiodef

[Guenter Roeck]

On Tue, Nov 19, 2013 at 08:54:38AM -0800, Guenter Roeck wrote:
> 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.
> 
To add to this: The datasheet clearly states that the pins are GPIO pins,
three of which can be configured as ALERT output, ALL_ON input, or clock
input/output. GPIO pins should be made available to the kernel through
the GPIO subsystem; any clock configuration should be configured through the
clock subsystem if needed. The pin configuration as ALERT output/ALL_ON
input/clock is clearly board specific and should thus be provided as
platform data and/or devicetree data if needed.

The existing GPIO alarm attributes should be removed. The pin values should be
reported as GPIO pin values instead.

Guenter

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

[Laszlo Papp]

On Tue, Nov 19, 2013 at 6:00 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Tue, Nov 19, 2013 at 08:54:38AM -0800, Guenter Roeck wrote:
>> 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.
>>
> To add to this: The datasheet clearly states that the pins are GPIO pins,
> three of which can be configured as ALERT output, ALL_ON input, or clock
> input/output.

Which is inline with what we wrote before. Although, you probably
meant FULL_ON rather than ALL_ON. There is no "ALL_ON" in this
context.

> GPIO pins should be made available to the kernel through
> the GPIO subsystem; any clock configuration should be configured through the
> clock subsystem if needed.

I believe we will agree to disagree there. I find it more convenient
(along with Markus, etc) to have clearly chip specific feature
available for the chip in one place - especially when that follows the
consistency - rather than distributed in several sub-spaces. Not that
this is only a minor feature. Splitting them into even tinier is
strange in my opinion.

> The pin configuration as ALERT output/ALL_ON
> input/clock is clearly board specific and should thus be provided as
> platform data and/or devicetree data if needed.

That is still static, not dynamic, hence inappropriate for the use case at hand.

> The existing GPIO alarm attributes should be removed. The pin values should be
> reported as GPIO pin values instead.

Feel free to provide a change for review if you wish.

I would like to note that I am not planning to rewrite an already
existing and tested feature as of now. Not sure if I could find the
motivation and time for doing that any soon. To me, it only looks
personal taste nitpicking so far, and the feature would be more
important to me. There are pro/cons for both sides, but if this
feature cannot get it in with this design, there might be no feature
like this for the posterity.

So unless there are good arguments with modulo critical answers why it
is unacceptably wrong as is for now, let us drop this change in
upstream, then.

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

[Laszlo Papp]

One week passed since the initial submit. Any feedback from the
maintainer who accepts patches for this?

On Tue, Nov 19, 2013 at 7:43 PM, Laszlo Papp <lpapp@kde.org> wrote:
> On Tue, Nov 19, 2013 at 6:00 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On Tue, Nov 19, 2013 at 08:54:38AM -0800, Guenter Roeck wrote:
>>> 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.
>>>
>> To add to this: The datasheet clearly states that the pins are GPIO pins,
>> three of which can be configured as ALERT output, ALL_ON input, or clock
>> input/output.
>
> Which is inline with what we wrote before. Although, you probably
> meant FULL_ON rather than ALL_ON. There is no "ALL_ON" in this
> context.
>
>> GPIO pins should be made available to the kernel through
>> the GPIO subsystem; any clock configuration should be configured through the
>> clock subsystem if needed.
>
> I believe we will agree to disagree there. I find it more convenient
> (along with Markus, etc) to have clearly chip specific feature
> available for the chip in one place - especially when that follows the
> consistency - rather than distributed in several sub-spaces. Not that
> this is only a minor feature. Splitting them into even tinier is
> strange in my opinion.
>
>> The pin configuration as ALERT output/ALL_ON
>> input/clock is clearly board specific and should thus be provided as
>> platform data and/or devicetree data if needed.
>
> That is still static, not dynamic, hence inappropriate for the use case at hand.
>
>> The existing GPIO alarm attributes should be removed. The pin values should be
>> reported as GPIO pin values instead.
>
> Feel free to provide a change for review if you wish.
>
> I would like to note that I am not planning to rewrite an already
> existing and tested feature as of now. Not sure if I could find the
> motivation and time for doing that any soon. To me, it only looks
> personal taste nitpicking so far, and the feature would be more
> important to me. There are pro/cons for both sides, but if this
> feature cannot get it in with this design, there might be no feature
> like this for the posterity.
>
> So unless there are good arguments with modulo critical answers why it
> is unacceptably wrong as is for now, let us drop this change in
> upstream, then.

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

[Guenter Roeck]

On Thu, Nov 21, 2013 at 03:20:34PM +0000, Laszlo Papp wrote:
> One week passed since the initial submit. Any feedback from the
> maintainer who accepts patches for this?
> 
Last time I checked that was either Jean Delvare or me.

As I already told you, I won't accept the patch as-is,
and I told you what would need to be changed to have it accepted.

In general, we don't support adding non-standard sysfs attributes in hwmon
drivers unless really needed and discussed. As I see it, there is no need
for non-standard sysfs attributes in this driver; you _could_ use
the gpio subsystem. You chose not to and provide non-standard sysfs
attributes instead, essentially duplicating gpio subsystem functionality.

I see it as even more important to use the gpio subsystem for the intended use
case, which is to use gpio pins for fan control. In that case, providing access
through the gpio subsystem would enable using the gpio-fan driver to actually
control the fans.

You may consider that to be personal taste nitpicking. I don't.

Guenter

> On Tue, Nov 19, 2013 at 7:43 PM, Laszlo Papp <lpapp@kde.org> wrote:
> > On Tue, Nov 19, 2013 at 6:00 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> >> On Tue, Nov 19, 2013 at 08:54:38AM -0800, Guenter Roeck wrote:
> >>> 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.
> >>>
> >> To add to this: The datasheet clearly states that the pins are GPIO pins,
> >> three of which can be configured as ALERT output, ALL_ON input, or clock
> >> input/output.
> >
> > Which is inline with what we wrote before. Although, you probably
> > meant FULL_ON rather than ALL_ON. There is no "ALL_ON" in this
> > context.
> >
> >> GPIO pins should be made available to the kernel through
> >> the GPIO subsystem; any clock configuration should be configured through the
> >> clock subsystem if needed.
> >
> > I believe we will agree to disagree there. I find it more convenient
> > (along with Markus, etc) to have clearly chip specific feature
> > available for the chip in one place - especially when that follows the
> > consistency - rather than distributed in several sub-spaces. Not that
> > this is only a minor feature. Splitting them into even tinier is
> > strange in my opinion.
> >
> >> The pin configuration as ALERT output/ALL_ON
> >> input/clock is clearly board specific and should thus be provided as
> >> platform data and/or devicetree data if needed.
> >
> > That is still static, not dynamic, hence inappropriate for the use case at hand.
> >
> >> The existing GPIO alarm attributes should be removed. The pin values should be
> >> reported as GPIO pin values instead.
> >
> > Feel free to provide a change for review if you wish.
> >
> > I would like to note that I am not planning to rewrite an already
> > existing and tested feature as of now. Not sure if I could find the
> > motivation and time for doing that any soon. To me, it only looks
> > personal taste nitpicking so far, and the feature would be more
> > important to me. There are pro/cons for both sides, but if this
> > feature cannot get it in with this design, there might be no feature
> > like this for the posterity.
> >
> > So unless there are good arguments with modulo critical answers why it
> > is unacceptably wrong as is for now, let us drop this change in
> > upstream, then.
> 

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

[Laszlo Papp]

On Thu, Nov 21, 2013 at 5:34 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Thu, Nov 21, 2013 at 03:20:34PM +0000, Laszlo Papp wrote:
>> One week passed since the initial submit. Any feedback from the
>> maintainer who accepts patches for this?
>>
> Last time I checked that was either Jean Delvare or me.
>
> As I already told you, I won't accept the patch as-is,
> and I told you what would need to be changed to have it accepted.
>
> In general, we don't support adding non-standard sysfs attributes in hwmon
> drivers unless really needed and discussed. As I see it, there is no need
> for non-standard sysfs attributes in this driver; you _could_ use
> the gpio subsystem. You chose not to and provide non-standard sysfs
> attributes instead, essentially duplicating gpio subsystem functionality.

MFD != gpio subsystem, but for some reason or another you continuously
overlook that. You also disregard what Markus wrote: this change is
just following the existing convention in there. Basically, your
suggestion would lead to a mixed interface where some feature of the
chip is exposed in 3-4 other places, and some centrally and in a
compact manner in hwmon.

> I see it as even more important to use the gpio subsystem for the intended use
> case, which is to use gpio pins for fan control. In that case, providing access
> through the gpio subsystem would enable using the gpio-fan driver to actually
> control the fans.

That is clearly incorrect. To write a proper userspace middleware
would imply fishing stuff from several subspaces rather than using the
same compact interface. I called that a nightmare from end user point
of view.

> You may consider that to be personal taste nitpicking. I don't.

I consider it worse than nitpicking eventually: imho, it is rejecting
a validated feature without providing a better change. It is a shame,
but we cannot do anything more at this point to provide remedy here
without getting financial loss, further time spent on a full rewrite,
and relevant study, etc. The kernel will remain without this feature
probably. I see it as a loss/loss for both parties. You will save
maintaining it (even though it is me who would probably need to
maintain this feature for the next few years...) for the cost of not
having the feature at all, most likely.

Well, I guess we will need to stick to a more feature-rich forked
version for us then.

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

[Laszlo Papp]

Just to clarify: you want to have ./gpio/gpio-max6650.c?

On Thu, Nov 21, 2013 at 8:52 PM, Laszlo Papp <lpapp@kde.org> wrote:
> On Thu, Nov 21, 2013 at 5:34 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On Thu, Nov 21, 2013 at 03:20:34PM +0000, Laszlo Papp wrote:
>>> One week passed since the initial submit. Any feedback from the
>>> maintainer who accepts patches for this?
>>>
>> Last time I checked that was either Jean Delvare or me.
>>
>> As I already told you, I won't accept the patch as-is,
>> and I told you what would need to be changed to have it accepted.
>>
>> In general, we don't support adding non-standard sysfs attributes in hwmon
>> drivers unless really needed and discussed. As I see it, there is no need
>> for non-standard sysfs attributes in this driver; you _could_ use
>> the gpio subsystem. You chose not to and provide non-standard sysfs
>> attributes instead, essentially duplicating gpio subsystem functionality.
>
> MFD != gpio subsystem, but for some reason or another you continuously
> overlook that. You also disregard what Markus wrote: this change is
> just following the existing convention in there. Basically, your
> suggestion would lead to a mixed interface where some feature of the
> chip is exposed in 3-4 other places, and some centrally and in a
> compact manner in hwmon.
>
>> I see it as even more important to use the gpio subsystem for the intended use
>> case, which is to use gpio pins for fan control. In that case, providing access
>> through the gpio subsystem would enable using the gpio-fan driver to actually
>> control the fans.
>
> That is clearly incorrect. To write a proper userspace middleware
> would imply fishing stuff from several subspaces rather than using the
> same compact interface. I called that a nightmare from end user point
> of view.
>
>> You may consider that to be personal taste nitpicking. I don't.
>
> I consider it worse than nitpicking eventually: imho, it is rejecting
> a validated feature without providing a better change. It is a shame,
> but we cannot do anything more at this point to provide remedy here
> without getting financial loss, further time spent on a full rewrite,
> and relevant study, etc. The kernel will remain without this feature
> probably. I see it as a loss/loss for both parties. You will save
> maintaining it (even though it is me who would probably need to
> maintain this feature for the next few years...) for the cost of not
> having the feature at all, most likely.
>
> Well, I guess we will need to stick to a more feature-rich forked
> version for us then.

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

[Guenter Roeck]

On 11/22/2013 01:23 AM, Laszlo Papp wrote:
> Just to clarify: you want to have ./gpio/gpio-max6650.c?
>
No, I never said that. I wanted you to register the gpio pins
with the gpio subsystem. I didn't ask you to write a separate
driver for it.

Sure, strictly speaking one could write a top level mfd driver
and separate gpio and hwmon drivers, but at least in my opinion
that would be overkill. I also never suggested this; you brought
the term mfd into the discussion.

Guenter

> On Thu, Nov 21, 2013 at 8:52 PM, Laszlo Papp <lpapp@kde.org> wrote:
>> On Thu, Nov 21, 2013 at 5:34 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>> On Thu, Nov 21, 2013 at 03:20:34PM +0000, Laszlo Papp wrote:
>>>> One week passed since the initial submit. Any feedback from the
>>>> maintainer who accepts patches for this?
>>>>
>>> Last time I checked that was either Jean Delvare or me.
>>>
>>> As I already told you, I won't accept the patch as-is,
>>> and I told you what would need to be changed to have it accepted.
>>>
>>> In general, we don't support adding non-standard sysfs attributes in hwmon
>>> drivers unless really needed and discussed. As I see it, there is no need
>>> for non-standard sysfs attributes in this driver; you _could_ use
>>> the gpio subsystem. You chose not to and provide non-standard sysfs
>>> attributes instead, essentially duplicating gpio subsystem functionality.
>>
>> MFD != gpio subsystem, but for some reason or another you continuously
>> overlook that. You also disregard what Markus wrote: this change is
>> just following the existing convention in there. Basically, your
>> suggestion would lead to a mixed interface where some feature of the
>> chip is exposed in 3-4 other places, and some centrally and in a
>> compact manner in hwmon.
>>
>>> I see it as even more important to use the gpio subsystem for the intended use
>>> case, which is to use gpio pins for fan control. In that case, providing access
>>> through the gpio subsystem would enable using the gpio-fan driver to actually
>>> control the fans.
>>
>> That is clearly incorrect. To write a proper userspace middleware
>> would imply fishing stuff from several subspaces rather than using the
>> same compact interface. I called that a nightmare from end user point
>> of view.
>>
>>> You may consider that to be personal taste nitpicking. I don't.
>>
>> I consider it worse than nitpicking eventually: imho, it is rejecting
>> a validated feature without providing a better change. It is a shame,
>> but we cannot do anything more at this point to provide remedy here
>> without getting financial loss, further time spent on a full rewrite,
>> and relevant study, etc. The kernel will remain without this feature
>> probably. I see it as a loss/loss for both parties. You will save
>> maintaining it (even though it is me who would probably need to
>> maintain this feature for the next few years...) for the cost of not
>> having the feature at all, most likely.
>>
>> Well, I guess we will need to stick to a more feature-rich forked
>> version for us then.
>
>

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

[Laszlo Papp]

On Fri, Nov 22, 2013 at 2:35 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 11/22/2013 01:23 AM, Laszlo Papp wrote:
>>
>> Just to clarify: you want to have ./gpio/gpio-max6650.c?
>>
> No, I never said that. I wanted you to register the gpio pins
> with the gpio subsystem. I didn't ask you to write a separate
> driver for it.
>
> Sure, strictly speaking one could write a top level mfd driver
> and separate gpio and hwmon drivers, but at least in my opinion
> that would be overkill. I also never suggested this; you brought
> the term mfd into the discussion.

Why is it an overkill to write a separate gpio driver for this, and
potentially also for the gpio status register of this particular MAXIM
chip?

Other gpio drivers in the gpio folder are also short. This would not
become long either.

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

[Guenter Roeck]

On Fri, Nov 22, 2013 at 02:50:50PM +0000, Laszlo Papp wrote:
> On Fri, Nov 22, 2013 at 2:35 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On 11/22/2013 01:23 AM, Laszlo Papp wrote:
> >>
> >> Just to clarify: you want to have ./gpio/gpio-max6650.c?
> >>
> > No, I never said that. I wanted you to register the gpio pins
> > with the gpio subsystem. I didn't ask you to write a separate
> > driver for it.
> >
> > Sure, strictly speaking one could write a top level mfd driver
> > and separate gpio and hwmon drivers, but at least in my opinion
> > that would be overkill. I also never suggested this; you brought
> > the term mfd into the discussion.
> 
> Why is it an overkill to write a separate gpio driver for this, and
> potentially also for the gpio status register of this particular MAXIM
> chip?
> 
> Other gpio drivers in the gpio folder are also short. This would not
> become long either.
> 
No one prevents you from writing a separate gpio driver. I will certainly
not prevent you from doing it. Just don't claim that I requested it.
Same is true for an mfd driver; I won't prevent you from writing one.
But don't claim that I told you to do it.

Guenter

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

[Laszlo Papp]

On Fri, Nov 22, 2013 at 4:04 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Fri, Nov 22, 2013 at 02:50:50PM +0000, Laszlo Papp wrote:
>> On Fri, Nov 22, 2013 at 2:35 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> > On 11/22/2013 01:23 AM, Laszlo Papp wrote:
>> >>
>> >> Just to clarify: you want to have ./gpio/gpio-max6650.c?
>> >>
>> > No, I never said that. I wanted you to register the gpio pins
>> > with the gpio subsystem. I didn't ask you to write a separate
>> > driver for it.
>> >
>> > Sure, strictly speaking one could write a top level mfd driver
>> > and separate gpio and hwmon drivers, but at least in my opinion
>> > that would be overkill. I also never suggested this; you brought
>> > the term mfd into the discussion.
>>
>> Why is it an overkill to write a separate gpio driver for this, and
>> potentially also for the gpio status register of this particular MAXIM
>> chip?
>>
>> Other gpio drivers in the gpio folder are also short. This would not
>> become long either.
>>
> No one prevents you from writing a separate gpio driver. I will certainly
> not prevent you from doing it. Just don't claim that I requested it.
> Same is true for an mfd driver; I won't prevent you from writing one.
> But don't claim that I told you to do it.

I am not sure if you are trying to claim that with this statement a
separate gpio driver would be acceptable, but it would require a bit
more work, or whether that you would reject such a driver added.

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

[Guenter Roeck]

On Fri, Nov 22, 2013 at 05:47:33PM +0000, Laszlo Papp wrote:
> On Fri, Nov 22, 2013 at 4:04 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On Fri, Nov 22, 2013 at 02:50:50PM +0000, Laszlo Papp wrote:
> >> On Fri, Nov 22, 2013 at 2:35 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> >> > On 11/22/2013 01:23 AM, Laszlo Papp wrote:
> >> >>
> >> >> Just to clarify: you want to have ./gpio/gpio-max6650.c?
> >> >>
> >> > No, I never said that. I wanted you to register the gpio pins
> >> > with the gpio subsystem. I didn't ask you to write a separate
> >> > driver for it.
> >> >
> >> > Sure, strictly speaking one could write a top level mfd driver
> >> > and separate gpio and hwmon drivers, but at least in my opinion
> >> > that would be overkill. I also never suggested this; you brought
> >> > the term mfd into the discussion.
> >>
> >> Why is it an overkill to write a separate gpio driver for this, and
> >> potentially also for the gpio status register of this particular MAXIM
> >> chip?
> >>
> >> Other gpio drivers in the gpio folder are also short. This would not
> >> become long either.
> >>
> > No one prevents you from writing a separate gpio driver. I will certainly
> > not prevent you from doing it. Just don't claim that I requested it.
> > Same is true for an mfd driver; I won't prevent you from writing one.
> > But don't claim that I told you to do it.
> 
> I am not sure if you are trying to claim that with this statement a
> separate gpio driver would be acceptable, but it would require a bit
> more work, or whether that you would reject such a driver added.
> 
Please explain, for my education, what makes you believe that I would
object to or reject to anyone submitting such a driver.

Thanks,
Guenter

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

[Laszlo Papp]

On Fri, Nov 22, 2013 at 6:05 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Fri, Nov 22, 2013 at 05:47:33PM +0000, Laszlo Papp wrote:
>> On Fri, Nov 22, 2013 at 4:04 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> > On Fri, Nov 22, 2013 at 02:50:50PM +0000, Laszlo Papp wrote:
>> >> On Fri, Nov 22, 2013 at 2:35 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> >> > On 11/22/2013 01:23 AM, Laszlo Papp wrote:
>> >> >>
>> >> >> Just to clarify: you want to have ./gpio/gpio-max6650.c?
>> >> >>
>> >> > No, I never said that. I wanted you to register the gpio pins
>> >> > with the gpio subsystem. I didn't ask you to write a separate
>> >> > driver for it.
>> >> >
>> >> > Sure, strictly speaking one could write a top level mfd driver
>> >> > and separate gpio and hwmon drivers, but at least in my opinion
>> >> > that would be overkill. I also never suggested this; you brought
>> >> > the term mfd into the discussion.
>> >>
>> >> Why is it an overkill to write a separate gpio driver for this, and
>> >> potentially also for the gpio status register of this particular MAXIM
>> >> chip?
>> >>
>> >> Other gpio drivers in the gpio folder are also short. This would not
>> >> become long either.
>> >>
>> > No one prevents you from writing a separate gpio driver. I will certainly
>> > not prevent you from doing it. Just don't claim that I requested it.
>> > Same is true for an mfd driver; I won't prevent you from writing one.
>> > But don't claim that I told you to do it.
>>
>> I am not sure if you are trying to claim that with this statement a
>> separate gpio driver would be acceptable, but it would require a bit
>> more work, or whether that you would reject such a driver added.
>>
> Please explain, for my education, what makes you believe that I would
> object to or reject to anyone submitting such a driver.

After the current objection, I am not sure anymore. I thought the last
change would have been just as acceptable. ;-)

So, in short, I would not like to waste more time with unacceptable
approaches for the maintainer. Anyway, if you feel happy about
accepting such an approach once stabilized, I think I will go for
that.

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

[Laszlo Papp]

So, Mr. Roeck, how do you suggest to handle input or output port
configuration? Should it be coming from platform data, or should it be
more dynamic? I would say, it is usually static, but I do not know if
it is common to be dynamic. I am not a hardware designer.

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

[Guenter Roeck]

On Mon, Dec 16, 2013 at 03:56:33PM +0000, Laszlo Papp wrote:
> So, Mr. Roeck, how do you suggest to handle input or output port
> configuration? Should it be coming from platform data, or should it be
> more dynamic? I would say, it is usually static, but I do not know if
> it is common to be dynamic. I am not a hardware designer.
> 

Platform and/or devicetree.

Guenter

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

[Laszlo Papp]

On Mon, Dec 16, 2013 at 5:27 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Mon, Dec 16, 2013 at 03:56:33PM +0000, Laszlo Papp wrote:
>> So, Mr. Roeck, how do you suggest to handle input or output port
>> configuration? Should it be coming from platform data, or should it be
>> more dynamic? I would say, it is usually static, but I do not know if
>> it is common to be dynamic. I am not a hardware designer.
>>
>
> Platform and/or devicetree.

Works for me. Should I apply the gpio and mfd drivers separately or in
one single patch?

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

[Laszlo Papp]

On Mon, Dec 16, 2013 at 6:28 PM, Laszlo Papp <lpapp@kde.org> wrote:
> On Mon, Dec 16, 2013 at 5:27 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On Mon, Dec 16, 2013 at 03:56:33PM +0000, Laszlo Papp wrote:
>>> So, Mr. Roeck, how do you suggest to handle input or output port
>>> configuration? Should it be coming from platform data, or should it be
>>> more dynamic? I would say, it is usually static, but I do not know if
>>> it is common to be dynamic. I am not a hardware designer.
>>>
>>
>> Platform and/or devicetree.
>
> Works for me. Should I apply the gpio and mfd drivers separately or in
> one single patch?

s/apply/send/

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

[Guenter Roeck]

On Mon, Dec 16, 2013 at 06:28:34PM +0000, Laszlo Papp wrote:
> On Mon, Dec 16, 2013 at 6:28 PM, Laszlo Papp <lpapp@kde.org> wrote:
> > On Mon, Dec 16, 2013 at 5:27 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> >> On Mon, Dec 16, 2013 at 03:56:33PM +0000, Laszlo Papp wrote:
> >>> So, Mr. Roeck, how do you suggest to handle input or output port
> >>> configuration? Should it be coming from platform data, or should it be
> >>> more dynamic? I would say, it is usually static, but I do not know if
> >>> it is common to be dynamic. I am not a hardware designer.
> >>>
> >>
> >> Platform and/or devicetree.
> >
> > Works for me. Should I apply the gpio and mfd drivers separately or in
> > one single patch?
> 
> s/apply/send/
> 
Separately.

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 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