[PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Laszlo Papp]

In the preparation of creating an mfd driver and then refactor this one into a
platform driver in order to add some pinctrl functionality to the chip, it is
necessary to start the series with this change so that the mfd driver can refer
to the proper name in the subsequent change without making changes in more than
one driver later.

This was a request from Lee Jones, the MFD subsystem maintainer.

Signed-off-by: Laszlo Papp <lpapp@kde.org>
---
 drivers/hwmon/max6650.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index 0cafc39..3c36edc 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -116,8 +116,8 @@ static struct max6650_data *max6650_update_device(struct device *dev);
  */
 
 static const struct i2c_device_id max6650_id[] = {
-	{ "max6650", 1 },
-	{ "max6651", 4 },
+	{ "max6650-hwmon", 1 },
+	{ "max6651-hwmon", 4 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, max6650_id);
-- 
1.8.5.3

Re: [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Lee Jones]

> In the preparation of creating an mfd driver and then refactor this one into a
> platform driver in order to add some pinctrl functionality to the chip, it is
> necessary to start the series with this change so that the mfd driver can refer
> to the proper name in the subsequent change without making changes in more than
> one driver later.
> 
> This was a request from Lee Jones, the MFD subsystem maintainer.
> 
> Signed-off-by: Laszlo Papp <lpapp@kde.org>
> ---
>  drivers/hwmon/max6650.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
> index 0cafc39..3c36edc 100644
> --- a/drivers/hwmon/max6650.c
> +++ b/drivers/hwmon/max6650.c
> @@ -116,8 +116,8 @@ static struct max6650_data *max6650_update_device(struct device *dev);
>   */
>  
>  static const struct i2c_device_id max6650_id[] = {
> -	{ "max6650", 1 },
> -	{ "max6651", 4 },
> +	{ "max6650-hwmon", 1 },
> +	{ "max6651-hwmon", 4 },

Might be worth taking the opportunity to swap out these magic numbers
now.

>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, max6650_id);

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Jean Delvare]

Hi Lee, Laszlo,

On Mon, 10 Feb 2014 16:08:42 +0000, Lee Jones wrote:
> > In the preparation of creating an mfd driver and then refactor this one into a
> > platform driver in order to add some pinctrl functionality to the chip, it is
> > necessary to start the series with this change so that the mfd driver can refer
> > to the proper name in the subsequent change without making changes in more than
> > one driver later.
> > 
> > This was a request from Lee Jones, the MFD subsystem maintainer.
> > 
> > Signed-off-by: Laszlo Papp <lpapp@kde.org>
> > ---
> >  drivers/hwmon/max6650.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
> > index 0cafc39..3c36edc 100644
> > --- a/drivers/hwmon/max6650.c
> > +++ b/drivers/hwmon/max6650.c
> > @@ -116,8 +116,8 @@ static struct max6650_data *max6650_update_device(struct device *dev);
> >   */
> >  
> >  static const struct i2c_device_id max6650_id[] = {
> > -	{ "max6650", 1 },
> > -	{ "max6651", 4 },
> > +	{ "max6650-hwmon", 1 },
> > +	{ "max6651-hwmon", 4 },

No, this is not acceptable, sorry. This will change the name of the
hwmon device as seen from user-space, breaking any configuration file
referring to it. Additionally, dashes are explicitly forbidden in hwmon
device names. And lastly this will break any explicit instantiation of
theses devices (which is the only way, as the driver doesn't support
device auto-detection), be it in the kernel itself or from user-space.

The change doesn't make sense anyway. If you move to the MFD framework,
the core driver will be an I2C driver binding to the I2C device, and it
will spawn the logical devices, presumably in the form of platform
devices. That's what the current max6650 driver would have to bind to.
Just renaming the device won't work, you also need to change the type.

If you want to turn this into an MFD driver, I believe you must first
convert the hwmon part to register using
devm_hwmon_device_register_with_groups(). This will dissociate the i2c
device name from the hwmon device name and create a clean name-space
for each function. Guenter, maybe you had a plan to do so already
anyway?

That being said, going with MFD in this case seems quite overkill to
me. MFD makes a lot of sense when each function has its own resources.
As this isn't the case here, a single driver registering both an hwmon
interface and a pinctrl interface would seem sufficient to me. But I
think Guenter already discussed this in the past so I'll let him
continue and decide.

> Might be worth taking the opportunity to swap out these magic numbers
> now.

There's nothing magic about them, they tell the driver how many fans
each device supports. If you don't pass them as driver_data you'll have
to derive them from the device name in the probe function.

> >  	{ }
> >  };
> >  MODULE_DEVICE_TABLE(i2c, max6650_id);
> 


-- 
Jean Delvare
Suse L3 Support

Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[linux]

Quoting Jean Delvare <jdelvare@suse.de>:

>
> That being said, going with MFD in this case seems quite overkill to
> me. MFD makes a lot of sense when each function has its own resources.
> As this isn't the case here, a single driver registering both an hwmon
> interface and a pinctrl interface would seem sufficient to me. But I
> think Guenter already discussed this in the past so I'll let him
> continue and decide.
>

That is what I had suggested as well (though we were talking gpio
at the time). Laszlo didn't want to do it this way for some reason.
Right now I don't really have an idea what to do.

Guenter

Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Laszlo Papp]

On Mon, Feb 10, 2014 at 4:53 PM,  <linux@roeck-us.net> wrote:
> Quoting Jean Delvare <jdelvare@suse.de>:
>
>>
>> That being said, going with MFD in this case seems quite overkill to
>> me. MFD makes a lot of sense when each function has its own resources.
>> As this isn't the case here, a single driver registering both an hwmon
>> interface and a pinctrl interface would seem sufficient to me. But I
>> think Guenter already discussed this in the past so I'll let him
>> continue and decide.
>>
>
> That is what I had suggested as well (though we were talking gpio
> at the time). Laszlo didn't want to do it this way for some reason.
> Right now I don't really have an idea what to do.

Right now I do not really have an idea what the concern here is.

I will quote you:

"Please explain, for my education, what makes you believe that I would
object to or reject to anyone submitting such a driver."

and then the next one in the thread:

"> > Works for me. Should I apply the gpio and mfd drivers separately or in
> > one single patch?
>
> s/apply/send/
>
Separately."

This happened about two months ago, and after two months of man work,
several reviews from various people, while you have been *explicitly*
included in the threads, are claiming that it is unacceptable? Do you
see how much time waste that would be for everyone who have been
involved.

What I currently do not understand is the point for rejecting the
contribution that does not have API drawback, etc, if you do not
provide anything better. You are more than welcome to rewrite my work
once the feature works, but I guess it is very likely that you would
not do that.

So, let me ask it: shall we continue the bike-shedding after months,
or there is a definite decision from the maintainers? Disagreement is
not a problem because people can move on if the maintainers actually
make it clear what is acceptable and what not, but here that did not
really happen. We are where we were months ago.

Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Guenter Roeck]

On Mon, Feb 10, 2014 at 06:59:55PM +0000, Laszlo Papp wrote:
> On Mon, Feb 10, 2014 at 4:53 PM,  <linux@roeck-us.net> wrote:
> > Quoting Jean Delvare <jdelvare@suse.de>:
> >
> >>
> >> That being said, going with MFD in this case seems quite overkill to
> >> me. MFD makes a lot of sense when each function has its own resources.
> >> As this isn't the case here, a single driver registering both an hwmon
> >> interface and a pinctrl interface would seem sufficient to me. But I
> >> think Guenter already discussed this in the past so I'll let him
> >> continue and decide.
> >>
> >
> > That is what I had suggested as well (though we were talking gpio
> > at the time). Laszlo didn't want to do it this way for some reason.
> > Right now I don't really have an idea what to do.
> 
> Right now I do not really have an idea what the concern here is.
> 
> I will quote you:
> 
> "Please explain, for my education, what makes you believe that I would
> object to or reject to anyone submitting such a driver."
> 
> and then the next one in the thread:
> 
> "> > Works for me. Should I apply the gpio and mfd drivers separately or in
> > > one single patch?
> >
> > s/apply/send/
> >
> Separately."
> 
> This happened about two months ago, and after two months of man work,
> several reviews from various people, while you have been *explicitly*
> included in the threads, are claiming that it is unacceptable? Do you
> see how much time waste that would be for everyone who have been
> involved.
> 
> What I currently do not understand is the point for rejecting the
> contribution that does not have API drawback, etc, if you do not
> provide anything better. You are more than welcome to rewrite my work
> once the feature works, but I guess it is very likely that you would
> not do that.
> 
> So, let me ask it: shall we continue the bike-shedding after months,
> or there is a definite decision from the maintainers? Disagreement is
> not a problem because people can move on if the maintainers actually
> make it clear what is acceptable and what not, but here that did not
> really happen. We are where we were months ago.
> 

I think I'll let Jean handle this one.

Guenter

Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Laszlo Papp]

On Mon, Feb 10, 2014 at 11:10 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Mon, Feb 10, 2014 at 06:59:55PM +0000, Laszlo Papp wrote:
> I think I'll let Jean handle this one.

Guys, please be a bit more definite.

We should get over this long ping-pong game. It has been clearly
stated that either way is fine, and there was no objection for months
to either way either, and now the feature is just not moving forward.
This is the first time I have this sorrow experience that I am having
here in hwmon, unfortunately. A lot of effort spent, and nothing got
done.

As I said before, disagreeing is fine and natural. What is not fine is
wasting people's time for months and not making it clear what the
hwmon maintainers want, and months later after the work, the opposite
is claimed than before.

I am sorry if it sounds harsh, but currently this is how I see the
situation. If the MFD solution gets rejected, I consider it a huge
maintainer mistake since both of you were involved, and have never
ever spoken up for months that this would not be acceptable. In fact,
as quoted earlier in this thread, the opposite had been said.

I would like to get over the hwmon situation as soon as possible, so
please guys kindly advise what you would *really* like to see. I do
not really mind who makes the decision, but rejecting months of work
due to miscommunication is still better than continuing the same!

Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Laszlo Papp]

On Tue, Feb 11, 2014 at 3:23 AM, Laszlo Papp <lpapp@kde.org> wrote:
> On Mon, Feb 10, 2014 at 11:10 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On Mon, Feb 10, 2014 at 06:59:55PM +0000, Laszlo Papp wrote:
>> I think I'll let Jean handle this one.
>
> Guys, please be a bit more definite.
>
> We should get over this long ping-pong game. It has been clearly
> stated that either way is fine, and there was no objection for months
> to either way either, and now the feature is just not moving forward.
> This is the first time I have this sorrow experience that I am having
> here in hwmon, unfortunately. A lot of effort spent, and nothing got
> done.
>
> As I said before, disagreeing is fine and natural. What is not fine is
> wasting people's time for months and not making it clear what the
> hwmon maintainers want, and months later after the work, the opposite
> is claimed than before.
>
> I am sorry if it sounds harsh, but currently this is how I see the
> situation. If the MFD solution gets rejected, I consider it a huge
> maintainer mistake since both of you were involved, and have never
> ever spoken up for months that this would not be acceptable. In fact,
> as quoted earlier in this thread, the opposite had been said.
>
> I would like to get over the hwmon situation as soon as possible, so
> please guys kindly advise what you would *really* like to see. I do
> not really mind who makes the decision, but rejecting months of work
> due to miscommunication is still better than continuing the same!

(Alternatively, who is the higher-level decision maker over the
drivers (and hence driver subsystem maintainers) to ask for making a
final decision if you cannot make it?
I would hope for this being the last resort, but there is a point
where it is necessary to remain productive, in my opinion.)

Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Lee Jones]

> > >  static const struct i2c_device_id max6650_id[] = {
> > > -	{ "max6650", 1 },
> > > -	{ "max6651", 4 },
> > > +	{ "max6650-hwmon", 1 },
> > > +	{ "max6651-hwmon", 4 },
> 
> No, this is not acceptable, sorry. This will change the name of the
> hwmon device as seen from user-space, breaking any configuration file
> referring to it. Additionally, dashes are explicitly forbidden in hwmon
> device names. And lastly this will break any explicit instantiation of
> theses devices (which is the only way, as the driver doesn't support
> device auto-detection), be it in the kernel itself or from user-space.
> 
> The change doesn't make sense anyway. If you move to the MFD framework,
> the core driver will be an I2C driver binding to the I2C device, and it
> will spawn the logical devices, presumably in the form of platform
> devices. That's what the current max6650 driver would have to bind to.
> Just renaming the device won't work, you also need to change the type.
> 
> If you want to turn this into an MFD driver, I believe you must first
> convert the hwmon part to register using
> devm_hwmon_device_register_with_groups(). This will dissociate the i2c
> device name from the hwmon device name and create a clean name-space
> for each function. Guenter, maybe you had a plan to do so already
> anyway?
> 
> That being said, going with MFD in this case seems quite overkill to
> me. MFD makes a lot of sense when each function has its own resources.
> As this isn't the case here, a single driver registering both an hwmon
> interface and a pinctrl interface would seem sufficient to me. But I
> think Guenter already discussed this in the past so I'll let him
> continue and decide.

I'll get you guys decide if you want to go the MFD route or not.

Either is okay with me, but if you do decide in favour, a name change
with the device type appended would be preferred. Else the core device
would have the same name as all of its children which would be quite
unworkable.

> > Might be worth taking the opportunity to swap out these magic numbers
> > now.
> 
> There's nothing magic about them, they tell the driver how many fans
> each device supports. If you don't pass them as driver_data you'll have
> to derive them from the device name in the probe function.

They're magic in that they're not easily identifiable. In the few
moments that I looked at the patch I assumed they were device
IDs. They should be clearly defined.

> > >  	{ }
> > >  };
> > >  MODULE_DEVICE_TABLE(i2c, max6650_id);

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Jean Delvare]

Hi Lee,

On Mon, 10 Feb 2014 16:58:53 +0000, Lee Jones wrote:
> > > >  static const struct i2c_device_id max6650_id[] = {
> > > > -	{ "max6650", 1 },
> > > > -	{ "max6651", 4 },
> > > > +	{ "max6650-hwmon", 1 },
> > > > +	{ "max6651-hwmon", 4 },
> > 
> > No, this is not acceptable, sorry. This will change the name of the
> > hwmon device as seen from user-space, breaking any configuration file
> > referring to it. Additionally, dashes are explicitly forbidden in hwmon
> > device names. And lastly this will break any explicit instantiation of
> > theses devices (which is the only way, as the driver doesn't support
> > device auto-detection), be it in the kernel itself or from user-space.
> > 
> > The change doesn't make sense anyway. If you move to the MFD framework,
> > the core driver will be an I2C driver binding to the I2C device, and it
> > will spawn the logical devices, presumably in the form of platform
> > devices. That's what the current max6650 driver would have to bind to.
> > Just renaming the device won't work, you also need to change the type.
> > 
> > If you want to turn this into an MFD driver, I believe you must first
> > convert the hwmon part to register using
> > devm_hwmon_device_register_with_groups(). This will dissociate the i2c
> > device name from the hwmon device name and create a clean name-space
> > for each function. Guenter, maybe you had a plan to do so already
> > anyway?
> > 
> > That being said, going with MFD in this case seems quite overkill to
> > me. MFD makes a lot of sense when each function has its own resources.
> > As this isn't the case here, a single driver registering both an hwmon
> > interface and a pinctrl interface would seem sufficient to me. But I
> > think Guenter already discussed this in the past so I'll let him
> > continue and decide.
> 
> I'll get you guys decide if you want to go the MFD route or not.
> 
> Either is okay with me, but if you do decide in favour, a name change
> with the device type appended would be preferred. Else the core device
> would have the same name as all of its children which would be quite
> unworkable.

No problem with that. The main (I2C) device should be named max6650 (or
max6651), the subdevices can be named whatever you want, as long as the
hwmon (class) device's name attribute is also "max6650" (or "max6651".)
As stated above, this is required to preserve compatibility with
existing users.

> > > Might be worth taking the opportunity to swap out these magic numbers
> > > now.
> > 
> > There's nothing magic about them, they tell the driver how many fans
> > each device supports. If you don't pass them as driver_data you'll have
> > to derive them from the device name in the probe function.
> 
> They're magic in that they're not easily identifiable. In the few
> moments that I looked at the patch I assumed they were device
> IDs. They should be clearly defined.

They could have been device IDs, some drivers do that, and that would
have been equally fine. driver_data can be anything. Best thing to do
is to document it right above the device id array if you really find it
confusing (I don't.) I don't know what else exactly you had in mind,
but #defining FOUR_FANS as 4 and ONE_FAN as 1 and using that doesn't
strike me as the best coding practice.

-- 
Jean Delvare
Suse L3 Support

Re: [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Lee Jones]

> > > > Might be worth taking the opportunity to swap out these magic numbers
> > > > now.
> > > 
> > > There's nothing magic about them, they tell the driver how many fans
> > > each device supports. If you don't pass them as driver_data you'll have
> > > to derive them from the device name in the probe function.
> > 
> > They're magic in that they're not easily identifiable. In the few
> > moments that I looked at the patch I assumed they were device
> > IDs. They should be clearly defined.
> 
> They could have been device IDs, some drivers do that, and that would
> have been equally fine. driver_data can be anything. Best thing to do
> is to document it right above the device id array if you really find it
> confusing (I don't.) I don't know what else exactly you had in mind,
> but #defining FOUR_FANS as 4 and ONE_FAN as 1 and using that doesn't
> strike me as the best coding practice.

On the contrary. Perhaps the nomenclature can be worked on a little,
but if I saw the aforementioned defines I would have known instantly
what was being defined without searching for co-located comments. Thus
elevating the requirement for me to even mention it. Even when we
use the .data element for very simple information such as device IDs
we do so with a #define.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Jean Delvare]

On Mon, 10 Feb 2014 18:01:59 +0000, Lee Jones wrote:
> > > > > Might be worth taking the opportunity to swap out these magic numbers
> > > > > now.
> > > > 
> > > > There's nothing magic about them, they tell the driver how many fans
> > > > each device supports. If you don't pass them as driver_data you'll have
> > > > to derive them from the device name in the probe function.
> > > 
> > > They're magic in that they're not easily identifiable. In the few
> > > moments that I looked at the patch I assumed they were device
> > > IDs. They should be clearly defined.
> > 
> > They could have been device IDs, some drivers do that, and that would
> > have been equally fine. driver_data can be anything. Best thing to do
> > is to document it right above the device id array if you really find it
> > confusing (I don't.) I don't know what else exactly you had in mind,
> > but #defining FOUR_FANS as 4 and ONE_FAN as 1 and using that doesn't
> > strike me as the best coding practice.
> 
> On the contrary. Perhaps the nomenclature can be worked on a little,
> but if I saw the aforementioned defines I would have known instantly
> what was being defined without searching for co-located comments. Thus
> elevating the requirement for me to even mention it. Even when we
> use the .data element for very simple information such as device IDs
> we do so with a #define.

Right, you have a point here.

I suppose it was deemed unneeded for a ~750 lines driver nobody really
cared about. But if the driver is becoming more complex and popular
then indeed it makes sense to clean it up a little. Starting with
reordering functions to kill forward declarations ^^

-- 
Jean Delvare
Suse L3 Support

Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Lee Jones]

> > > > > > Might be worth taking the opportunity to swap out these magic numbers
> > > > > > now.
> > > > > 
> > > > > There's nothing magic about them, they tell the driver how many fans
> > > > > each device supports. If you don't pass them as driver_data you'll have
> > > > > to derive them from the device name in the probe function.
> > > > 
> > > > They're magic in that they're not easily identifiable. In the few
> > > > moments that I looked at the patch I assumed they were device
> > > > IDs. They should be clearly defined.
> > > 
> > > They could have been device IDs, some drivers do that, and that would
> > > have been equally fine. driver_data can be anything. Best thing to do
> > > is to document it right above the device id array if you really find it
> > > confusing (I don't.) I don't know what else exactly you had in mind,
> > > but #defining FOUR_FANS as 4 and ONE_FAN as 1 and using that doesn't
> > > strike me as the best coding practice.
> > 
> > On the contrary. Perhaps the nomenclature can be worked on a little,
> > but if I saw the aforementioned defines I would have known instantly
> > what was being defined without searching for co-located comments. Thus
> > elevating the requirement for me to even mention it. Even when we
> > use the .data element for very simple information such as device IDs
> > we do so with a #define.
> 
> Right, you have a point here.
> 
> I suppose it was deemed unneeded for a ~750 lines driver nobody really
> cared about. But if the driver is becoming more complex and popular
> then indeed it makes sense to clean it up a little. Starting with
> reordering functions to kill forward declarations ^^

Another worthwhile endeavour. :)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Laszlo Papp]

On Mon, Feb 10, 2014 at 5:43 PM, Jean Delvare <jdelvare@suse.de> wrote:
> Hi Lee,
>
> On Mon, 10 Feb 2014 16:58:53 +0000, Lee Jones wrote:
>> > > >  static const struct i2c_device_id max6650_id[] = {
>> > > > -       { "max6650", 1 },
>> > > > -       { "max6651", 4 },
>> > > > +       { "max6650-hwmon", 1 },
>> > > > +       { "max6651-hwmon", 4 },
>> >
>> > No, this is not acceptable, sorry. This will change the name of the
>> > hwmon device as seen from user-space, breaking any configuration file
>> > referring to it. Additionally, dashes are explicitly forbidden in hwmon
>> > device names. And lastly this will break any explicit instantiation of
>> > theses devices (which is the only way, as the driver doesn't support
>> > device auto-detection), be it in the kernel itself or from user-space.
>> >
>> > The change doesn't make sense anyway. If you move to the MFD framework,
>> > the core driver will be an I2C driver binding to the I2C device, and it
>> > will spawn the logical devices, presumably in the form of platform
>> > devices. That's what the current max6650 driver would have to bind to.
>> > Just renaming the device won't work, you also need to change the type.
>> >
>> > If you want to turn this into an MFD driver, I believe you must first
>> > convert the hwmon part to register using
>> > devm_hwmon_device_register_with_groups(). This will dissociate the i2c
>> > device name from the hwmon device name and create a clean name-space
>> > for each function. Guenter, maybe you had a plan to do so already
>> > anyway?
>> >
>> > That being said, going with MFD in this case seems quite overkill to
>> > me. MFD makes a lot of sense when each function has its own resources.
>> > As this isn't the case here, a single driver registering both an hwmon
>> > interface and a pinctrl interface would seem sufficient to me. But I
>> > think Guenter already discussed this in the past so I'll let him
>> > continue and decide.
>>
>> I'll get you guys decide if you want to go the MFD route or not.
>>
>> Either is okay with me, but if you do decide in favour, a name change
>> with the device type appended would be preferred. Else the core device
>> would have the same name as all of its children which would be quite
>> unworkable.
>
> No problem with that. The main (I2C) device should be named max6650 (or
> max6651), the subdevices can be named whatever you want, as long as the
> hwmon (class) device's name attribute is also "max6650" (or "max6651".)
> As stated above, this is required to preserve compatibility with
> existing users.
>
>> > > Might be worth taking the opportunity to swap out these magic numbers
>> > > now.
>> >
>> > There's nothing magic about them, they tell the driver how many fans
>> > each device supports. If you don't pass them as driver_data you'll have
>> > to derive them from the device name in the probe function.
>>
>> They're magic in that they're not easily identifiable. In the few
>> moments that I looked at the patch I assumed they were device
>> IDs. They should be clearly defined.
>
> They could have been device IDs, some drivers do that, and that would
> have been equally fine. driver_data can be anything. Best thing to do
> is to document it right above the device id array if you really find it
> confusing (I don't.) I don't know what else exactly you had in mind,
> but #defining FOUR_FANS as 4 and ONE_FAN as 1 and using that doesn't
> strike me as the best coding practice.

Err... no. 1/4 fan is not the only difference between max6650 and
max6651 ... (might be worth looking up the datasheet).

Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Jean Delvare]

On Mon, 10 Feb 2014 18:27:07 +0000, Laszlo Papp wrote:
> On Mon, Feb 10, 2014 at 5:43 PM, Jean Delvare <jdelvare@suse.de> wrote:
> > On Mon, 10 Feb 2014 16:58:53 +0000, Lee Jones wrote:
> >> > > Might be worth taking the opportunity to swap out these magic numbers
> >> > > now.
> >> >
> >> > There's nothing magic about them, they tell the driver how many fans
> >> > each device supports. If you don't pass them as driver_data you'll have
> >> > to derive them from the device name in the probe function.
> >>
> >> They're magic in that they're not easily identifiable. In the few
> >> moments that I looked at the patch I assumed they were device
> >> IDs. They should be clearly defined.
> >
> > They could have been device IDs, some drivers do that, and that would
> > have been equally fine. driver_data can be anything. Best thing to do
> > is to document it right above the device id array if you really find it
> > confusing (I don't.) I don't know what else exactly you had in mind,
> > but #defining FOUR_FANS as 4 and ONE_FAN as 1 and using that doesn't
> > strike me as the best coding practice.
> 
> Err... no. 1/4 fan is not the only difference between max6650 and
> max6651 ... (might be worth looking up the datasheet).

This is the only difference the driver cared about so far, so the code
made sense. If the driver is extended to support features which differ
between the MAX6650 and MAX6651 then it will make sense to revisit, of
course.

-- 
Jean Delvare
Suse L3 Support

Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Laszlo Papp]

On Mon, Feb 10, 2014 at 4:38 PM, Jean Delvare <jdelvare@suse.de> wrote:
> Hi Lee, Laszlo,
>
> On Mon, 10 Feb 2014 16:08:42 +0000, Lee Jones wrote:
>> > In the preparation of creating an mfd driver and then refactor this one into a
>> > platform driver in order to add some pinctrl functionality to the chip, it is
>> > necessary to start the series with this change so that the mfd driver can refer
>> > to the proper name in the subsequent change without making changes in more than
>> > one driver later.
>> >
>> > This was a request from Lee Jones, the MFD subsystem maintainer.
>> >
>> > Signed-off-by: Laszlo Papp <lpapp@kde.org>
>> > ---
>> >  drivers/hwmon/max6650.c | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
>> > index 0cafc39..3c36edc 100644
>> > --- a/drivers/hwmon/max6650.c
>> > +++ b/drivers/hwmon/max6650.c
>> > @@ -116,8 +116,8 @@ static struct max6650_data *max6650_update_device(struct device *dev);
>> >   */
>> >
>> >  static const struct i2c_device_id max6650_id[] = {
>> > -   { "max6650", 1 },
>> > -   { "max6651", 4 },
>> > +   { "max6650-hwmon", 1 },
>> > +   { "max6651-hwmon", 4 },
>
> No, this is not acceptable, sorry. This will change the name of the
> hwmon device as seen from user-space, breaking any configuration file
> referring to it. Additionally, dashes are explicitly forbidden in hwmon
> device names. And lastly this will break any explicit instantiation of
> theses devices (which is the only way, as the driver doesn't support
> device auto-detection), be it in the kernel itself or from user-space.
>
> The change doesn't make sense anyway. If you move to the MFD framework,
> the core driver will be an I2C driver binding to the I2C device, and it
> will spawn the logical devices, presumably in the form of platform
> devices. That's what the current max6650 driver would have to bind to.
> Just renaming the device won't work, you also need to change the type.

Hmm, this paragraph seems to indicate that you have not seen my
previous patch set. I tried to summarize in this commit message that
the type in this subdriver would need to change, yes.

I am fine with not renaming, but appending if such a thing is
possible. What does not make sense to me is acquiring a "global"
max665x name in a sub-device driver. The children have to be
distinguished somehow!

> If you want to turn this into an MFD driver, I believe you must first
> convert the hwmon part to register using
> devm_hwmon_device_register_with_groups(). This will dissociate the i2c
> device name from the hwmon device name and create a clean name-space
> for each function. Guenter, maybe you had a plan to do so already
> anyway?
>
> That being said, going with MFD in this case seems quite overkill to
> me. MFD makes a lot of sense when each function has its own resources.
> As this isn't the case here, a single driver registering both an hwmon
> interface and a pinctrl interface would seem sufficient to me. But I
> think Guenter already discussed this in the past so I'll let him
> continue and decide.

Exactly. This had been overdiscussed. I took my personal preference
without any technical drawback. I prefer clean separation just like
several other mfd drivers are doing, really.

Tell me this is unacceptable, and I will stop helping with getting the
required functionality into the kernel. Frankly, I am getting tired of
having worked on it for a few months now, and we are still at
discussing personal preferences rather than getting features in ...

Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Laszlo Papp]

On Mon, Feb 10, 2014 at 5:06 PM, Laszlo Papp <lpapp@kde.org> wrote:
> On Mon, Feb 10, 2014 at 4:38 PM, Jean Delvare <jdelvare@suse.de> wrote:
>> Hi Lee, Laszlo,
>>
>> On Mon, 10 Feb 2014 16:08:42 +0000, Lee Jones wrote:
>>> > In the preparation of creating an mfd driver and then refactor this one into a
>>> > platform driver in order to add some pinctrl functionality to the chip, it is
>>> > necessary to start the series with this change so that the mfd driver can refer
>>> > to the proper name in the subsequent change without making changes in more than
>>> > one driver later.
>>> >
>>> > This was a request from Lee Jones, the MFD subsystem maintainer.
>>> >
>>> > Signed-off-by: Laszlo Papp <lpapp@kde.org>
>>> > ---
>>> >  drivers/hwmon/max6650.c | 4 ++--
>>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>>> >
>>> > diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
>>> > index 0cafc39..3c36edc 100644
>>> > --- a/drivers/hwmon/max6650.c
>>> > +++ b/drivers/hwmon/max6650.c
>>> > @@ -116,8 +116,8 @@ static struct max6650_data *max6650_update_device(struct device *dev);
>>> >   */
>>> >
>>> >  static const struct i2c_device_id max6650_id[] = {
>>> > -   { "max6650", 1 },
>>> > -   { "max6651", 4 },
>>> > +   { "max6650-hwmon", 1 },
>>> > +   { "max6651-hwmon", 4 },
>>
>> No, this is not acceptable, sorry. This will change the name of the
>> hwmon device as seen from user-space, breaking any configuration file
>> referring to it. Additionally, dashes are explicitly forbidden in hwmon
>> device names. And lastly this will break any explicit instantiation of
>> theses devices (which is the only way, as the driver doesn't support
>> device auto-detection), be it in the kernel itself or from user-space.
>>
>> The change doesn't make sense anyway. If you move to the MFD framework,
>> the core driver will be an I2C driver binding to the I2C device, and it
>> will spawn the logical devices, presumably in the form of platform
>> devices. That's what the current max6650 driver would have to bind to.
>> Just renaming the device won't work, you also need to change the type.
>
> Hmm, this paragraph seems to indicate that you have not seen my
> previous patch set. I tried to summarize in this commit message that
> the type in this subdriver would need to change, yes.
>
> I am fine with not renaming, but appending if such a thing is
> possible. What does not make sense to me is acquiring a "global"
> max665x name in a sub-device driver. The children have to be
> distinguished somehow!
>
>> If you want to turn this into an MFD driver, I believe you must first
>> convert the hwmon part to register using
>> devm_hwmon_device_register_with_groups(). This will dissociate the i2c
>> device name from the hwmon device name and create a clean name-space
>> for each function. Guenter, maybe you had a plan to do so already
>> anyway?
>>
>> That being said, going with MFD in this case seems quite overkill to
>> me. MFD makes a lot of sense when each function has its own resources.
>> As this isn't the case here, a single driver registering both an hwmon
>> interface and a pinctrl interface would seem sufficient to me. But I
>> think Guenter already discussed this in the past so I'll let him
>> continue and decide.
>
> Exactly. This had been overdiscussed. I took my personal preference
> without any technical drawback. I prefer clean separation just like
> several other mfd drivers are doing, really.
>
> Tell me this is unacceptable, and I will stop helping with getting the
> required functionality into the kernel. Frankly, I am getting tired of
> having worked on it for a few months now, and we are still at
> discussing personal preferences rather than getting features in ...

Moreover, I really do not see how this is an overkill. In my opinion,
quite the opposite, putting MFD functionality for different areas into
one particular area is more than peculiar.

Even more, Guenter has been included in the long
gpio/pinctrl/mfd/hwmon patchset discussions. Could you please make up
your mind finally? I am sorry if this sounds harsh, but I hope you
understand my position. Trying to help this stuff moving forward, and
even after months of discussion we are still hitting the same
opinionated gates.

Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Laszlo Papp]

On Mon, Feb 10, 2014 at 4:38 PM, Jean Delvare <jdelvare@suse.de> wrote:
> Additionally, dashes are explicitly forbidden in hwmon
> device names.

Also, where is that documented?

I do not think you can make such a decision, and you will realize that
once you begin to think a bit out of the box and look around. See how
other children are managed for MFD devices. "driver-subsystem" is a
pretty common a schema. I do not really see any point in forbidding
dashes currently. Please do elaborate about the reasons, and the fact
that why it is undocuemented.

Also, I currently do not understand what you are suggesting: just
leave this technically unreasonable situation as is for compatibility
reasons? There is no better support in place for appending further
alternative names?

In any case, at the very least, I hope the lesson is learnt for the
future from this past mistake. If a chip is MFD'ish, a subdevice
driver should not ever be added with such an id.

Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Jean Delvare]

Hi Laszlo,

On Tue, 11 Feb 2014 03:13:37 +0000, Laszlo Papp wrote:
> On Mon, Feb 10, 2014 at 4:38 PM, Jean Delvare <jdelvare@suse.de> wrote:
> > Additionally, dashes are explicitly forbidden in hwmon
> > device names.
> 
> Also, where is that documented?

In Documentation/hwmon/sysfs-interface:

*********************
* Global attributes *
*********************

name		The chip name.
		This should be a short, lowercase string, not containing
		spaces nor dashes, representing the chip name. This is
		the only mandatory attribute.
		I2C devices get this attribute created automatically.
		RO

> I do not think you can make such a decision, and you will realize that
> once you begin to think a bit out of the box and look around. See how
> other children are managed for MFD devices. "driver-subsystem" is a
> pretty common a schema. I do not really see any point in forbidding
> dashes currently. Please do elaborate about the reasons, and the fact
> that why it is undocuemented.

It is documented since August 2007 [1], and stop with this tone,
please. The more you talk, the less I want to help you. Guenter already
gave up on you, I might as well do the same.

I did not "make" the decision, it is a limitation implied by the way
libsensors creates unique and persistent identifiers for hwmon devices.
These identifiers are of the form ${name}-${bus}-${address}, where
${bus} can (but doesn't have to) be of the form ${type}-${number}. If
${name} contains a dash then parsing the identifier becomes ambiguous,
as you can no longer easily tell where ${name} ends and where ${bus}
begins.

> Also, I currently do not understand what you are suggesting: just
> leave this technically unreasonable situation as is for compatibility
> reasons? There is no better support in place for appending further
> alternative names?

There's nothing unreasonable about the situation. Yes, compatibility
matters, it matters a lot even, and we do our best to guarantee that
users can move to a more recent kernel without breaking anything that
was previously working. I certainly hope other open source project
maintainers and developers do the same.

I already explained (but apparently you were to busy yelling at Guenter
and myself to notice) that the hwmon device name (which is the one we
can't change) can now be dissociated from the i2c (or platform) device
name, thanks to Guenter's excellent work. This is exactly the solution
to your problem, so there's no need sound dramatic.

> In any case, at the very least, I hope the lesson is learnt for the
> future from this past mistake. If a chip is MFD'ish, a subdevice
> driver should not ever be added with such an id.

You can't always anticipate this kind of thing. If you wait until
you're certain your design (and code) is perfect and will never need
to be changed, then you'll never release any piece of software. That
code you just wrote and submitted, and you believe is perfect, guess
what? Next year someone will call it crap and blame it on you for not
putting enough thinking into it.

There was little reason to believe that the max6650 driver would become
a MFD driver at the time is was written (10 years ago [2].) The concept
of MFD driver did not even exist then. We have several hwmon drivers
implementing side features (such as watchdog) without using the MFD
framework. I still believe using the MFD framework to support the
MAX6650 and MAX6651 is overkill, BTW, so even today I wouldn't
anticipate it. Which does _not_ mean I'll reject the attempt to do so,
contrary to what you repeatedly claimed in various posts of this
discussion.

You sent one patch, I reviewed it, found it was broken, and explained
why. In great details, I believe. Did you actually read my review? Did
you understand it?

[1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/Documentation/hwmon/sysfs-interface?id=176544dc55b0a912a2e1bacb9f48ccbd4aabd55f
[2] http://www.lm-sensors.org/browser/lm-sensors/trunk/kernel/chips/max6650.c?rev=1987

-- 
Jean Delvare
Suse L3 Support

Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Laszlo Papp]

On Tue, Feb 11, 2014 at 7:50 AM, Jean Delvare <jdelvare@suse.de> wrote:
> Hi Laszlo,
>
> On Tue, 11 Feb 2014 03:13:37 +0000, Laszlo Papp wrote:
>> On Mon, Feb 10, 2014 at 4:38 PM, Jean Delvare <jdelvare@suse.de> wrote:
>> > Additionally, dashes are explicitly forbidden in hwmon
>> > device names.
>>
>> Also, where is that documented?
>
> In Documentation/hwmon/sysfs-interface:
>
> *********************
> * Global attributes *
> *********************
>
> name            The chip name.
>                 This should be a short, lowercase string, not containing
>                 spaces nor dashes, representing the chip name. This is
>                 the only mandatory attribute.
>                 I2C devices get this attribute created automatically.
>                 RO
>
>> I do not think you can make such a decision, and you will realize that
>> once you begin to think a bit out of the box and look around. See how
>> other children are managed for MFD devices. "driver-subsystem" is a
>> pretty common a schema. I do not really see any point in forbidding
>> dashes currently. Please do elaborate about the reasons, and the fact
>> that why it is undocuemented.
>
> It is documented since August 2007 [1], and stop with this tone,
> please. The more you talk, the less I want to help you. Guenter already
> gave up on you, I might as well do the same.

My reply is reflecting the frustration coming from the wasted man
months caused by the miscommunication. I cannot step over it easily I
am afraid. Please assume good faith though... I was writing this in
the hope of this situation never ever happening again, so everyone
feels better in the future.

If I can also drop in my two cents, I would like to note that I also
feel bad about the no sign of regret.

> I did not "make" the decision, it is a limitation implied by the way
> libsensors creates unique and persistent identifiers for hwmon devices.
> These identifiers are of the form ${name}-${bus}-${address}, where
> ${bus} can (but doesn't have to) be of the form ${type}-${number}. If
> ${name} contains a dash then parsing the identifier becomes ambiguous,
> as you can no longer easily tell where ${name} ends and where ${bus}
> begins.
>
>> Also, I currently do not understand what you are suggesting: just
>> leave this technically unreasonable situation as is for compatibility
>> reasons? There is no better support in place for appending further
>> alternative names?
>
> There's nothing unreasonable about the situation. Yes, compatibility
> matters, it matters a lot even, and we do our best to guarantee that
> users can move to a more recent kernel without breaking anything that
> was previously working. I certainly hope other open source project
> maintainers and developers do the same.

My post was not actually about compatibility... It was about the fact
that there was no alternative way presented what should be done. That
is, not in an understandable way for me at least.

In any case, please consider rejecting "global names" for MFD chips in
the future for hwmon. This is my personal two cents.

> I already explained (but apparently you were to busy yelling at Guenter
> and myself to notice) that the hwmon device name (which is the one we
> can't change) can now be dissociated from the i2c (or platform) device
> name, thanks to Guenter's excellent work. This is exactly the solution
> to your problem, so there's no need sound dramatic.

Sorry, but I wished to raise the maintainer issue regardless how you
take them. I can assure you that I have done it only with good faith,
so the same mistake never ever happens again.

That being said, I really do not understand what you are writing, so I
guess I would need more details to proceed. Please give a more
thorough explanation that newcomers can also understand. Currently, it
is unclear to me what you are trying to write.

I already asked in the beginning of this thread if it is possible to
add the alternative ids to the list instead of renaming them. I do not
seem to have gotten replies to that.

>> In any case, at the very least, I hope the lesson is learnt for the
>> future from this past mistake. If a chip is MFD'ish, a subdevice
>> driver should not ever be added with such an id.
>
> You can't always anticipate this kind of thing. If you wait until
> you're certain your design (and code) is perfect and will never need
> to be changed, then you'll never release any piece of software. That
> code you just wrote and submitted, and you believe is perfect, guess
> what? Next year someone will call it crap and blame it on you for not
> putting enough thinking into it.

I personally think you are underestimating the issue here. As far as I
can tell, the datasheet is pretty short for this chip, and someone
reading it through surely gets the idea this chip is not at all about
just hwmon. I am not saying people do not do mistakes, but I am
encouraging you to learn the lesson with good intention so that this
can be avoided in the future.

Perhaps the datasheet was not read completely, and the feature was
added haskily... I do not know, but currently I cannot imagine it
otherwise.

> There was little reason to believe that the max6650 driver would become
> a MFD driver at the time is was written (10 years ago [2].) The concept
> of MFD driver did not even exist then. We have several hwmon drivers
> implementing side features (such as watchdog) without using the MFD
> framework. I still believe using the MFD framework to support the
> MAX6650 and MAX6651 is overkill, BTW, so even today I wouldn't
> anticipate it. Which does _not_ mean I'll reject the attempt to do so,
> contrary to what you repeatedly claimed in various posts of this
> discussion.
>
> You sent one patch, I reviewed it, found it was broken, and explained
> why. In great details, I believe. Did you actually read my review? Did
> you understand it?

No, I have not understand it, sorry. Could you please elaborate more?

> [1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/Documentation/hwmon/sysfs-interface?id=176544dc55b0a912a2e1bacb9f48ccbd4aabd55f
> [2] http://www.lm-sensors.org/browser/lm-sensors/trunk/kernel/chips/max6650.c?rev=1987
>
> --
> Jean Delvare
> Suse L3 Support

Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Laszlo Papp]

On Tue, Feb 11, 2014 at 7:50 AM, Jean Delvare <jdelvare@suse.de> wrote:
> Hi Laszlo,
>
> On Tue, 11 Feb 2014 03:13:37 +0000, Laszlo Papp wrote:
>> On Mon, Feb 10, 2014 at 4:38 PM, Jean Delvare <jdelvare@suse.de> wrote:
>> > Additionally, dashes are explicitly forbidden in hwmon
>> > device names.
>>
>> Also, where is that documented?
>
> In Documentation/hwmon/sysfs-interface:
>
> *********************
> * Global attributes *
> *********************
>
> name            The chip name.
>                 This should be a short, lowercase string, not containing
>                 spaces nor dashes, representing the chip name. This is
>                 the only mandatory attribute.
>                 I2C devices get this attribute created automatically.
>                 RO

Time to revisit this decision....

So, based on the fact that children device names usually contain
dashes, I do not understand why hwmon would be any special in this
regard. It is possible that the hwmon developers have not faced much
MFD situation before, and so, this was not considered to be handled
like in other subsystems.

I am proposing to change this "rule"...  Any objection?

Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Jean Delvare]

Le Tuesday 11 February 2014 à 08:28 +0000, Laszlo Papp a écrit :
> On Tue, Feb 11, 2014 at 7:50 AM, Jean Delvare <jdelvare@suse.de> wrote:
> > Hi Laszlo,
> >
> > On Tue, 11 Feb 2014 03:13:37 +0000, Laszlo Papp wrote:
> >> On Mon, Feb 10, 2014 at 4:38 PM, Jean Delvare <jdelvare@suse.de> wrote:
> >> > Additionally, dashes are explicitly forbidden in hwmon
> >> > device names.
> >>
> >> Also, where is that documented?
> >
> > In Documentation/hwmon/sysfs-interface:
> >
> > *********************
> > * Global attributes *
> > *********************
> >
> > name            The chip name.
> >                 This should be a short, lowercase string, not containing
> >                 spaces nor dashes, representing the chip name. This is
> >                 the only mandatory attribute.
> >                 I2C devices get this attribute created automatically.
> >                 RO
> 
> Time to revisit this decision....
> 
> So, based on the fact that children device names usually contain
> dashes, I do not understand why hwmon would be any special in this
> regard. It is possible that the hwmon developers have not faced much
> MFD situation before, and so, this was not considered to be handled
> like in other subsystems.
> 
> I am proposing to change this "rule"...  Any objection?

I'm giving up here, sorry. There's no point in me writing any more on
the topic as you are not listening to me. I do not have the impression
you are doing any effort to understand what I'm saying. Plus you keep
focusing on things that do not matter and problems for which a solution
has already been provided. So wherever you're going, this is without me.

-- 
Jean Delvare
Suse L3 Support

Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Laszlo Papp]

On Tue, Feb 11, 2014 at 8:49 AM, Jean Delvare <jdelvare@suse.de> wrote:
> Le Tuesday 11 February 2014 à 08:28 +0000, Laszlo Papp a écrit :
>> On Tue, Feb 11, 2014 at 7:50 AM, Jean Delvare <jdelvare@suse.de> wrote:
>> > Hi Laszlo,
>> >
>> > On Tue, 11 Feb 2014 03:13:37 +0000, Laszlo Papp wrote:
>> >> On Mon, Feb 10, 2014 at 4:38 PM, Jean Delvare <jdelvare@suse.de> wrote:
>> >> > Additionally, dashes are explicitly forbidden in hwmon
>> >> > device names.
>> >>
>> >> Also, where is that documented?
>> >
>> > In Documentation/hwmon/sysfs-interface:
>> >
>> > *********************
>> > * Global attributes *
>> > *********************
>> >
>> > name            The chip name.
>> >                 This should be a short, lowercase string, not containing
>> >                 spaces nor dashes, representing the chip name. This is
>> >                 the only mandatory attribute.
>> >                 I2C devices get this attribute created automatically.
>> >                 RO
>>
>> Time to revisit this decision....
>>
>> So, based on the fact that children device names usually contain
>> dashes, I do not understand why hwmon would be any special in this
>> regard. It is possible that the hwmon developers have not faced much
>> MFD situation before, and so, this was not considered to be handled
>> like in other subsystems.
>>
>> I am proposing to change this "rule"...  Any objection?
>
> I'm giving up here, sorry. There's no point in me writing any more on
> the topic as you are not listening to me. I do not have the impression
> you are doing any effort to understand what I'm saying. Plus you keep
> focusing on things that do not matter and problems for which a solution
> has already been provided.

Does it mean the people cannot come up with ideas if they think
something may be better than some provided way? Surely, software
evolves over time and people are sometimes right, and sometimes wrong
right?

I can assure you that I do listen, please assume good faith with ideas
thrown in. If you think it is that bad an idea, you could have
explained why so, so that I and others can also understand it who do
not yet, right?

Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Lee Jones]

> >> Time to revisit this decision....
> >>
> >> So, based on the fact that children device names usually contain
> >> dashes, I do not understand why hwmon would be any special in this
> >> regard. It is possible that the hwmon developers have not faced much
> >> MFD situation before, and so, this was not considered to be handled
> >> like in other subsystems.
> >>
> >> I am proposing to change this "rule"...  Any objection?
> >
> > I'm giving up here, sorry. There's no point in me writing any more on
> > the topic as you are not listening to me. I do not have the impression
> > you are doing any effort to understand what I'm saying. Plus you keep
> > focusing on things that do not matter and problems for which a solution
> > has already been provided.
> 
> Does it mean the people cannot come up with ideas if they think
> something may be better than some provided way? Surely, software
> evolves over time and people are sometimes right, and sometimes wrong
> right?
> 
> I can assure you that I do listen, please assume good faith with ideas
> thrown in. If you think it is that bad an idea, you could have
> explained why so, so that I and others can also understand it who do
> not yet, right?

In the small amount of time you've been working in the kernel I've
seen 3-4 Maintainers completely give up on you and 2-3 driven close to
the end of their tether (myself included).

Ranting and demanding will not work well for you here. Please, for
your own sake, learn some diplomacy and be courteous to others on the
list. If people are replying it means they're trying to help. Jumping
down their throats is not conducive to your success and will not
result in a win.  

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Laszlo Papp]

On Tue, Feb 11, 2014 at 9:57 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> >> Time to revisit this decision....
>> >>
>> >> So, based on the fact that children device names usually contain
>> >> dashes, I do not understand why hwmon would be any special in this
>> >> regard. It is possible that the hwmon developers have not faced much
>> >> MFD situation before, and so, this was not considered to be handled
>> >> like in other subsystems.
>> >>
>> >> I am proposing to change this "rule"...  Any objection?
>> >
>> > I'm giving up here, sorry. There's no point in me writing any more on
>> > the topic as you are not listening to me. I do not have the impression
>> > you are doing any effort to understand what I'm saying. Plus you keep
>> > focusing on things that do not matter and problems for which a solution
>> > has already been provided.
>>
>> Does it mean the people cannot come up with ideas if they think
>> something may be better than some provided way? Surely, software
>> evolves over time and people are sometimes right, and sometimes wrong
>> right?
>>
>> I can assure you that I do listen, please assume good faith with ideas
>> thrown in. If you think it is that bad an idea, you could have
>> explained why so, so that I and others can also understand it who do
>> not yet, right?
>
> In the small amount of time you've been working in the kernel I've
> seen 3-4 Maintainers completely give up on you and 2-3 driven close to
> the end of their tether (myself included).
>
> Ranting and demanding will not work well for you here. Please, for
> your own sake, learn some diplomacy and be courteous to others on the
> list. If people are replying it means they're trying to help. Jumping
> down their throats is not conducive to your success and will not
> result in a win.

I have not intended to demand anything, but I must confess that I was
frustrated about my long work being wasted, and I could probably have
spent my time with sipping some tea instead of replying. :-)

Apologies if I had hurt anyone involved. It is far from my intention.
I just really wished to proceed with this feature. Will try to grab my
cup next time earlier. Cheers.

Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Lee Jones]

> >> On Mon, Feb 10, 2014 at 4:38 PM, Jean Delvare <jdelvare@suse.de> wrote:
> >> > Additionally, dashes are explicitly forbidden in hwmon
> >> > device names.
> >>
> >> Also, where is that documented?
> >
> > In Documentation/hwmon/sysfs-interface:
> >
> > *********************
> > * Global attributes *
> > *********************
> >
> > name            The chip name.
> >                 This should be a short, lowercase string, not containing
> >                 spaces nor dashes, representing the chip name. This is
> >                 the only mandatory attribute.
> >                 I2C devices get this attribute created automatically.
> >                 RO
> 
> Time to revisit this decision....
> 
> So, based on the fact that children device names usually contain
> dashes, I do not understand why hwmon would be any special in this
> regard. It is possible that the hwmon developers have not faced much
> MFD situation before, and so, this was not considered to be handled
> like in other subsystems.
> 
> I am proposing to change this "rule"...  Any objection?

Prior to proposing such an invasive change which is highly likely to
come up against heavy opposition, why don't you try to work _with_ the
current ruling and the Maintainers to see what others have done to
solve the problem.  I highly doubt you are the first/only developer who
has had this issue.

Do:
  `git grep "\-hwmon"`

... and have a good look around for an accepted solution.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Laszlo Papp]

On Tue, Feb 11, 2014 at 8:50 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> >> On Mon, Feb 10, 2014 at 4:38 PM, Jean Delvare <jdelvare@suse.de> wrote:
>> >> > Additionally, dashes are explicitly forbidden in hwmon
>> >> > device names.
>> >>
>> >> Also, where is that documented?
>> >
>> > In Documentation/hwmon/sysfs-interface:
>> >
>> > *********************
>> > * Global attributes *
>> > *********************
>> >
>> > name            The chip name.
>> >                 This should be a short, lowercase string, not containing
>> >                 spaces nor dashes, representing the chip name. This is
>> >                 the only mandatory attribute.
>> >                 I2C devices get this attribute created automatically.
>> >                 RO
>>
>> Time to revisit this decision....
>>
>> So, based on the fact that children device names usually contain
>> dashes, I do not understand why hwmon would be any special in this
>> regard. It is possible that the hwmon developers have not faced much
>> MFD situation before, and so, this was not considered to be handled
>> like in other subsystems.
>>
>> I am proposing to change this "rule"...  Any objection?
>
> Prior to proposing such an invasive change which is highly likely to
> come up against heavy opposition,

It is possible that someone does not understand why you think it may
be invasive, right? Could you please explain the reason for that?

Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Laszlo Papp]

On Tue, Feb 11, 2014 at 8:58 AM, Laszlo Papp <lpapp@kde.org> wrote:
> On Tue, Feb 11, 2014 at 8:50 AM, Lee Jones <lee.jones@linaro.org> wrote:
>>> >> On Mon, Feb 10, 2014 at 4:38 PM, Jean Delvare <jdelvare@suse.de> wrote:
>>> >> > Additionally, dashes are explicitly forbidden in hwmon
>>> >> > device names.
>>> >>
>>> >> Also, where is that documented?
>>> >
>>> > In Documentation/hwmon/sysfs-interface:
>>> >
>>> > *********************
>>> > * Global attributes *
>>> > *********************
>>> >
>>> > name            The chip name.
>>> >                 This should be a short, lowercase string, not containing
>>> >                 spaces nor dashes, representing the chip name. This is
>>> >                 the only mandatory attribute.
>>> >                 I2C devices get this attribute created automatically.
>>> >                 RO
>>>
>>> Time to revisit this decision....
>>>
>>> So, based on the fact that children device names usually contain
>>> dashes, I do not understand why hwmon would be any special in this
>>> regard. It is possible that the hwmon developers have not faced much
>>> MFD situation before, and so, this was not considered to be handled
>>> like in other subsystems.
>>>
>>> I am proposing to change this "rule"...  Any objection?
>>
>> Prior to proposing such an invasive change which is highly likely to
>> come up against heavy opposition,
>
> It is possible that someone does not understand why you think it may
> be invasive, right? Could you please explain the reason for that?

Perhaps you think I would like to refactor all the existing
interfaces? That would be intrusive yes, but changing rules is
orthogonal to keeping compatibility in my opinion. It would be
possible to sanitize this for the feature and make consistent with
other subsystems while keeping the old drivers around as they are.

Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Lee Jones]

> >> >> On Mon, Feb 10, 2014 at 4:38 PM, Jean Delvare <jdelvare@suse.de> wrote:
> >> >> > Additionally, dashes are explicitly forbidden in hwmon
> >> >> > device names.
> >> >>
> >> >> Also, where is that documented?
> >> >
> >> > In Documentation/hwmon/sysfs-interface:
> >> >
> >> > *********************
> >> > * Global attributes *
> >> > *********************
> >> >
> >> > name            The chip name.
> >> >                 This should be a short, lowercase string, not containing
> >> >                 spaces nor dashes, representing the chip name. This is
> >> >                 the only mandatory attribute.
> >> >                 I2C devices get this attribute created automatically.
> >> >                 RO
> >>
> >> Time to revisit this decision....
> >>
> >> So, based on the fact that children device names usually contain
> >> dashes, I do not understand why hwmon would be any special in this
> >> regard. It is possible that the hwmon developers have not faced much
> >> MFD situation before, and so, this was not considered to be handled
> >> like in other subsystems.
> >>
> >> I am proposing to change this "rule"...  Any objection?
> >
> > Prior to proposing such an invasive change which is highly likely to
> > come up against heavy opposition,
> 
> It is possible that someone does not understand why you think it may
> be invasive, right? Could you please explain the reason for that?

The reason is a good one. In the kernel we make every attempt not to
break userspace. By that I mean _any_ userspace application. Userspace
applications which interface with the kernel can do so via a variety of
methods. One of those is Sysfs, where this name you are attempting to
change appears. The userspace applications already mentioned parse for
these devices, regex:ing for '-' separators. If you add an additional
'-' separator there is a chance that these applications will get
confused and break without warning. 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Laszlo Papp]

On Tue, Feb 11, 2014 at 9:47 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> >> >> On Mon, Feb 10, 2014 at 4:38 PM, Jean Delvare <jdelvare@suse.de> wrote:
>> >> >> > Additionally, dashes are explicitly forbidden in hwmon
>> >> >> > device names.
>> >> >>
>> >> >> Also, where is that documented?
>> >> >
>> >> > In Documentation/hwmon/sysfs-interface:
>> >> >
>> >> > *********************
>> >> > * Global attributes *
>> >> > *********************
>> >> >
>> >> > name            The chip name.
>> >> >                 This should be a short, lowercase string, not containing
>> >> >                 spaces nor dashes, representing the chip name. This is
>> >> >                 the only mandatory attribute.
>> >> >                 I2C devices get this attribute created automatically.
>> >> >                 RO
>> >>
>> >> Time to revisit this decision....
>> >>
>> >> So, based on the fact that children device names usually contain
>> >> dashes, I do not understand why hwmon would be any special in this
>> >> regard. It is possible that the hwmon developers have not faced much
>> >> MFD situation before, and so, this was not considered to be handled
>> >> like in other subsystems.
>> >>
>> >> I am proposing to change this "rule"...  Any objection?
>> >
>> > Prior to proposing such an invasive change which is highly likely to
>> > come up against heavy opposition,
>>
>> It is possible that someone does not understand why you think it may
>> be invasive, right? Could you please explain the reason for that?
>
> The reason is a good one. In the kernel we make every attempt not to
> break userspace. By that I mean _any_ userspace application. Userspace
> applications which interface with the kernel can do so via a variety of
> methods. One of those is Sysfs, where this name you are attempting to
> change appears. The userspace applications already mentioned parse for
> these devices, regex:ing for '-' separators. If you add an additional
> '-' separator there is a chance that these applications will get
> confused and break without warning.

Only for new devices that they have not supported before, right?
Provided, we apply the logic only for new drivers.

Is it unacceptable to have an interim "obsolete" period for a while
and the recommendation is the dash for the long future?

Also, how about applications that would expect hwmon to behave the
same way like all the rest? They also break with the current hwmon
schema, right?

>
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Laszlo Papp]

On Tue, Feb 11, 2014 at 8:50 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> >> On Mon, Feb 10, 2014 at 4:38 PM, Jean Delvare <jdelvare@suse.de> wrote:
>> >> > Additionally, dashes are explicitly forbidden in hwmon
>> >> > device names.
>> >>
>> >> Also, where is that documented?
>> >
>> > In Documentation/hwmon/sysfs-interface:
>> >
>> > *********************
>> > * Global attributes *
>> > *********************
>> >
>> > name            The chip name.
>> >                 This should be a short, lowercase string, not containing
>> >                 spaces nor dashes, representing the chip name. This is
>> >                 the only mandatory attribute.
>> >                 I2C devices get this attribute created automatically.
>> >                 RO
>>
>> Time to revisit this decision....
>>
>> So, based on the fact that children device names usually contain
>> dashes, I do not understand why hwmon would be any special in this
>> regard. It is possible that the hwmon developers have not faced much
>> MFD situation before, and so, this was not considered to be handled
>> like in other subsystems.
>>
>> I am proposing to change this "rule"...  Any objection?
>
> Prior to proposing such an invasive change which is highly likely to
> come up against heavy opposition, why don't you try to work _with_ the
> current ruling and the Maintainers to see what others have done to
> solve the problem.  I highly doubt you are the first/only developer who
> has had this issue.
>
> Do:
>   `git grep "\-hwmon"`
>
> ... and have a good look around for an accepted solution.

Well, this did not bring up relevant results, at least for my
understanding, but I will send a new patch with the
devm_hwmon_device_register_
with_groups() stuff based on my little understanding without further help.

Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

[Laszlo Papp]

On Tue, Feb 11, 2014 at 10:22 AM, Laszlo Papp <lpapp@kde.org> wrote:
> On Tue, Feb 11, 2014 at 8:50 AM, Lee Jones <lee.jones@linaro.org> wrote:
>>> >> On Mon, Feb 10, 2014 at 4:38 PM, Jean Delvare <jdelvare@suse.de> wrote:
>>> >> > Additionally, dashes are explicitly forbidden in hwmon
>>> >> > device names.
>>> >>
>>> >> Also, where is that documented?
>>> >
>>> > In Documentation/hwmon/sysfs-interface:
>>> >
>>> > *********************
>>> > * Global attributes *
>>> > *********************
>>> >
>>> > name            The chip name.
>>> >                 This should be a short, lowercase string, not containing
>>> >                 spaces nor dashes, representing the chip name. This is
>>> >                 the only mandatory attribute.
>>> >                 I2C devices get this attribute created automatically.
>>> >                 RO
>>>
>>> Time to revisit this decision....
>>>
>>> So, based on the fact that children device names usually contain
>>> dashes, I do not understand why hwmon would be any special in this
>>> regard. It is possible that the hwmon developers have not faced much
>>> MFD situation before, and so, this was not considered to be handled
>>> like in other subsystems.
>>>
>>> I am proposing to change this "rule"...  Any objection?
>>
>> Prior to proposing such an invasive change which is highly likely to
>> come up against heavy opposition, why don't you try to work _with_ the
>> current ruling and the Maintainers to see what others have done to
>> solve the problem.  I highly doubt you are the first/only developer who
>> has had this issue.
>>
>> Do:
>>   `git grep "\-hwmon"`
>>
>> ... and have a good look around for an accepted solution.
>
> Well, this did not bring up relevant results, at least for my
> understanding, but I will send a new patch with the
> devm_hwmon_device_register_
> with_groups() stuff based on my little understanding without further help.

https://lkml.org/lkml/2014/2/11/155

but I do not know how to enable the different devices conditionally.
What is the best practice for this in the kernel? (We can continue it
in there)