LinuxLists.cc - [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[permalink] [raw]

Subject: Re: [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

> The MFD driver has now been added, so this driver is now being adopted to be a
> subdevice driver on top of it. This means, the i2c driver usage is being
> converted to platform driver usage all around.
>
> Signed-off-by: Laszlo Papp <[email protected]>
> ---
> This patch has been compile tested only and will be tested with real hardware,
> but early reviews to catch any trivial issues would be welcome.
> drivers/hwmon/Kconfig | 2 +-
> drivers/hwmon/max6650.c | 155 ++++++++++++++++++++++++------------------------
> 2 files changed, 79 insertions(+), 78 deletions(-)

<snip>

> /*
> * Insmod parameters
> @@ -105,24 +108,23 @@ module_param(clock, int, S_IRUGO);
>
> #define DIV_FROM_REG(reg) (1 << (reg & 7))
>
> -static int max6650_probe(struct i2c_client *client,
> - const struct i2c_device_id *id);
> -static int max6650_init_client(struct i2c_client *client);
> -static int max6650_remove(struct i2c_client *client);
> +static int max6650_probe(struct platform_device *pdev);
> +static int max6650_init_client(struct platform_device *pdev);
> +static int max6650_remove(struct platform_device *pdev);
> static struct max6650_data *max6650_update_device(struct device *dev);

It would be good to remove these forward declarations in the future.

If no one volunteers I'll happily do it.

> /*
> * Driver data (common to all clients)
> */
>
> -static const struct i2c_device_id max6650_id[] = {
> +static const struct platform_device_id max6650_id[] = {
> { "max6650", 1 },
> { "max6651", 4 },
> { }
> };
> -MODULE_DEVICE_TABLE(i2c, max6650_id);
> +MODULE_DEVICE_TABLE(platform, max6650_id);

I thought you were going to do the matching in the MFD driver?

If not, what's the point in the exported 'type' attribute?

> -static struct i2c_driver max6650_driver = {
> +static struct platform_driver max6650_driver = {
> .driver = {
> .name = "max6650",

This should be changed as it now supports max665{0,1} right?

<snip>

> - i2c_smbus_write_byte_data(client, MAX6650_REG_SPEED, data->speed);
> + regmap_write(data->iodev->map, MAX6650_REG_SPEED, data->speed);

Ensure all of the regmap stuff is fully tested.

<snip>

> @@ -484,10 +482,12 @@ static umode_t max6650_attrs_visible(struct kobject *kobj, struct attribute *a,
> int n)
> {
> struct device *dev = container_of(kobj, struct device, kobj);
> - struct i2c_client *client = to_i2c_client(dev);
> - u8 alarm_en = i2c_smbus_read_byte_data(client, MAX6650_REG_ALARM_EN);
> + struct max6650_data *data = dev_get_drvdata(dev);
> + int alarm_en;
> struct device_attribute *devattr;
>
> + regmap_read(data->iodev->map, MAX6650_REG_ALARM_EN, &alarm_en);
> +

Where is this then used?

> -static int max6650_probe(struct i2c_client *client,
> - const struct i2c_device_id *id)
> +static int max6650_probe(struct platform_device *pdev)
> {
> + struct max665x_dev *max665x = dev_get_drvdata(pdev->dev.parent);
> struct max6650_data *data;
> + const struct platform_device_id *id = platform_get_device_id(pdev);

What's the point in 'type' in the global container?

It's looking as though you're not going to need it to be global after
all, right?

<snip>

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

2014-02-13 10:15:38

[permalink] [raw]

Subject: Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

On Thu, 13 Feb 2014 09:58:17 +0000, Lee Jones wrote:
> > The MFD driver has now been added, so this driver is now being adopted to be a
> > subdevice driver on top of it. This means, the i2c driver usage is being
> > converted to platform driver usage all around.
> >
> > Signed-off-by: Laszlo Papp <[email protected]>
> > ---
> > This patch has been compile tested only and will be tested with real hardware,
> > but early reviews to catch any trivial issues would be welcome.
> > drivers/hwmon/Kconfig | 2 +-
> > drivers/hwmon/max6650.c | 155 ++++++++++++++++++++++++------------------------
> > 2 files changed, 79 insertions(+), 78 deletions(-)
>
> <snip>
>
> > /*
> > * Insmod parameters
> > @@ -105,24 +108,23 @@ module_param(clock, int, S_IRUGO);
> >
> > #define DIV_FROM_REG(reg) (1 << (reg & 7))
> >
> > -static int max6650_probe(struct i2c_client *client,
> > - const struct i2c_device_id *id);
> > -static int max6650_init_client(struct i2c_client *client);
> > -static int max6650_remove(struct i2c_client *client);
> > +static int max6650_probe(struct platform_device *pdev);
> > +static int max6650_init_client(struct platform_device *pdev);
> > +static int max6650_remove(struct platform_device *pdev);
> > static struct max6650_data *max6650_update_device(struct device *dev);
>
> It would be good to remove these forward declarations in the future.
>
> If no one volunteers I'll happily do it.

Guenter just did:

http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html

Any change to the max6650 driver should go on top of his patch series
to avoid conflicts:

http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html

--
Jean Delvare
Suse L3 Support

2014-02-13 10:38:05

[permalink] [raw]

Subject: Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

On Thu, Feb 13, 2014 at 10:15 AM, Jean Delvare <[email protected]> wrote:
> On Thu, 13 Feb 2014 09:58:17 +0000, Lee Jones wrote:
>> > The MFD driver has now been added, so this driver is now being adopted to be a
>> > subdevice driver on top of it. This means, the i2c driver usage is being
>> > converted to platform driver usage all around.
>> >
>> > Signed-off-by: Laszlo Papp <[email protected]>
>> > ---
>> > This patch has been compile tested only and will be tested with real hardware,
>> > but early reviews to catch any trivial issues would be welcome.
>> > drivers/hwmon/Kconfig | 2 +-
>> > drivers/hwmon/max6650.c | 155 ++++++++++++++++++++++++------------------------
>> > 2 files changed, 79 insertions(+), 78 deletions(-)
>>
>> <snip>
>>
>> > /*
>> > * Insmod parameters
>> > @@ -105,24 +108,23 @@ module_param(clock, int, S_IRUGO);
>> >
>> > #define DIV_FROM_REG(reg) (1 << (reg & 7))
>> >
>> > -static int max6650_probe(struct i2c_client *client,
>> > - const struct i2c_device_id *id);
>> > -static int max6650_init_client(struct i2c_client *client);
>> > -static int max6650_remove(struct i2c_client *client);
>> > +static int max6650_probe(struct platform_device *pdev);
>> > +static int max6650_init_client(struct platform_device *pdev);
>> > +static int max6650_remove(struct platform_device *pdev);
>> > static struct max6650_data *max6650_update_device(struct device *dev);
>>
>> It would be good to remove these forward declarations in the future.
>>
>> If no one volunteers I'll happily do it.
>
> Guenter just did:
>
> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html
>
> Any change to the max6650 driver should go on top of his patch series
> to avoid conflicts:
>
> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html

As far as I can see, that patch set was not even tested, so how can it
go in? I was told that any patch should be _runtime_ tested, too.
Fwiw, I do not have time to test those personally, he would need to
find someone else if that requirement really holds true.

I would not really like to fix bugs appearing in that code to get my
features in.

2014-02-13 10:46:06

[permalink] [raw]

Subject: Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

On Thu, Feb 13, 2014 at 10:38 AM, Laszlo Papp <[email protected]> wrote:
> On Thu, Feb 13, 2014 at 10:15 AM, Jean Delvare <[email protected]> wrote:
>> On Thu, 13 Feb 2014 09:58:17 +0000, Lee Jones wrote:
>>> > The MFD driver has now been added, so this driver is now being adopted to be a
>>> > subdevice driver on top of it. This means, the i2c driver usage is being
>>> > converted to platform driver usage all around.
>>> >
>>> > Signed-off-by: Laszlo Papp <[email protected]>
>>> > ---
>>> > This patch has been compile tested only and will be tested with real hardware,
>>> > but early reviews to catch any trivial issues would be welcome.
>>> > drivers/hwmon/Kconfig | 2 +-
>>> > drivers/hwmon/max6650.c | 155 ++++++++++++++++++++++++------------------------
>>> > 2 files changed, 79 insertions(+), 78 deletions(-)
>>>
>>> <snip>
>>>
>>> > /*
>>> > * Insmod parameters
>>> > @@ -105,24 +108,23 @@ module_param(clock, int, S_IRUGO);
>>> >
>>> > #define DIV_FROM_REG(reg) (1 << (reg & 7))
>>> >
>>> > -static int max6650_probe(struct i2c_client *client,
>>> > - const struct i2c_device_id *id);
>>> > -static int max6650_init_client(struct i2c_client *client);
>>> > -static int max6650_remove(struct i2c_client *client);
>>> > +static int max6650_probe(struct platform_device *pdev);
>>> > +static int max6650_init_client(struct platform_device *pdev);
>>> > +static int max6650_remove(struct platform_device *pdev);
>>> > static struct max6650_data *max6650_update_device(struct device *dev);
>>>
>>> It would be good to remove these forward declarations in the future.
>>>
>>> If no one volunteers I'll happily do it.
>>
>> Guenter just did:
>>
>> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html
>>
>> Any change to the max6650 driver should go on top of his patch series
>> to avoid conflicts:
>>
>> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html
>
> As far as I can see, that patch set was not even tested, so how can it
> go in? I was told that any patch should be _runtime_ tested, too.
> Fwiw, I do not have time to test those personally, he would need to
> find someone else if that requirement really holds true.
>
> I would not really like to fix bugs appearing in that code to get my
> features in.

Also, since my change has been around for 2-3 months now, I would
really prefer not to be forced to rewrite it again from scratch.
Surely, you can wait with those, more or less, cosmetic non-runtime
tested changes?

This would impose me a lot of additional work again, and I personally
do not see the benefit of it. In my book at least, feature is over
internal polishing.

2014-02-13 10:55:58

[permalink] [raw]

Subject: Re: [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

On Thu, Feb 13, 2014 at 9:58 AM, Lee Jones <[email protected]> wrote:
>> The MFD driver has now been added, so this driver is now being adopted to be a
>> subdevice driver on top of it. This means, the i2c driver usage is being
>> converted to platform driver usage all around.
>>
>> Signed-off-by: Laszlo Papp <[email protected]>
>> ---
>> This patch has been compile tested only and will be tested with real hardware,
>> but early reviews to catch any trivial issues would be welcome.
>> drivers/hwmon/Kconfig | 2 +-
>> drivers/hwmon/max6650.c | 155 ++++++++++++++++++++++++------------------------
>> 2 files changed, 79 insertions(+), 78 deletions(-)
>
> <snip>
>
>> /*
>> * Insmod parameters
>> @@ -105,24 +108,23 @@ module_param(clock, int, S_IRUGO);
>>
>> #define DIV_FROM_REG(reg) (1 << (reg & 7))
>>
>> -static int max6650_probe(struct i2c_client *client,
>> - const struct i2c_device_id *id);
>> -static int max6650_init_client(struct i2c_client *client);
>> -static int max6650_remove(struct i2c_client *client);
>> +static int max6650_probe(struct platform_device *pdev);
>> +static int max6650_init_client(struct platform_device *pdev);
>> +static int max6650_remove(struct platform_device *pdev);
>> static struct max6650_data *max6650_update_device(struct device *dev);
>
> It would be good to remove these forward declarations in the future.
>
> If no one volunteers I'll happily do it.

I personally do not see any problem with the code either way, hence I
would not personally touch what works. :)

But if it is a strong opinionated style restriction in the codebase,
then yeah, someone could do it, except me.

>> /*
>> * Driver data (common to all clients)
>> */
>>
>> -static const struct i2c_device_id max6650_id[] = {
>> +static const struct platform_device_id max6650_id[] = {
>> { "max6650", 1 },
>> { "max6651", 4 },
>> { }
>> };
>> -MODULE_DEVICE_TABLE(i2c, max6650_id);
>> +MODULE_DEVICE_TABLE(platform, max6650_id);
>
> I thought you were going to do the matching in the MFD driver?
>
> If not, what's the point in the exported 'type' attribute?

I am yet to understand the concept here. You were objecting to those,
so I removed this in MFD. I could add it to that back in this patch as
proposed.

>> -static struct i2c_driver max6650_driver = {
>> +static struct platform_driver max6650_driver = {
>> .driver = {
>> .name = "max6650",
>
> This should be changed as it now supports max665{0,1} right?

This is a strange historical driver. It has always supported both, yet
it was named max6650 weirdly enough as you note.

> <snip>
>
>> - i2c_smbus_write_byte_data(client, MAX6650_REG_SPEED, data->speed);
>> + regmap_write(data->iodev->map, MAX6650_REG_SPEED, data->speed);
>
> Ensure all of the regmap stuff is fully tested.

Yes, sure.

> <snip>
>
>> @@ -484,10 +482,12 @@ static umode_t max6650_attrs_visible(struct kobject *kobj, struct attribute *a,
>> int n)
>> {
>> struct device *dev = container_of(kobj, struct device, kobj);
>> - struct i2c_client *client = to_i2c_client(dev);
>> - u8 alarm_en = i2c_smbus_read_byte_data(client, MAX6650_REG_ALARM_EN);
>> + struct max6650_data *data = dev_get_drvdata(dev);
>> + int alarm_en;
>> struct device_attribute *devattr;
>>
>> + regmap_read(data->iodev->map, MAX6650_REG_ALARM_EN, &alarm_en);
>> +
>
> Where is this then used?

Nowhere, so it was a sub-optimal situation in the old code. It is just
a direct platform device port of it whatever it was. Perhaps it is the
right time to clean it a bit, I agree.

>> -static int max6650_probe(struct i2c_client *client,
>> - const struct i2c_device_id *id)
>> +static int max6650_probe(struct platform_device *pdev)
>> {
>> + struct max665x_dev *max665x = dev_get_drvdata(pdev->dev.parent);
>> struct max6650_data *data;
>> + const struct platform_device_id *id = platform_get_device_id(pdev);
>
> What's the point in 'type' in the global container?
>
> It's looking as though you're not going to need it to be global after
> all, right?

I would need it for the bit fiddling, too, I think.

2014-02-13 11:07:39

[permalink] [raw]

Subject: Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

Hi Laszlo,

Le Thursday 13 February 2014 à 10:46 +0000, Laszlo Papp a écrit :
> On Thu, Feb 13, 2014 at 10:38 AM, Laszlo Papp <[email protected]> wrote:
> > On Thu, Feb 13, 2014 at 10:15 AM, Jean Delvare <[email protected]> wrote:
> >> Any change to the max6650 driver should go on top of his patch series
> >> to avoid conflicts:
> >>
> >> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html
> >
> > As far as I can see, that patch set was not even tested, so how can it
> > go in? I was told that any patch should be _runtime_ tested, too.
> > Fwiw, I do not have time to test those personally, he would need to
> > find someone else if that requirement really holds true.

I find it _very_ funny that you dare to complain here, when you sent a
totally untested patch no later than 2 days ago:

http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041180.html

There's no way that patch can work.

And, actually, Guenter's patches have been reviewed and tested by
myself, to some degree (I don't have access to a physical MAX6650 or
MAX6651 chip so I used an emulation, which I think was good enough given
the nature of the changes.)

> > I would not really like to fix bugs appearing in that code to get my
> > features in.

I have no idea what you mean here.

> Also, since my change has been around for 2-3 months now, I would
> really prefer not to be forced to rewrite it again from scratch.

I'm sure Gunter would have preferred if you could write proper patches
so he wouldn't have to do it himself.

Seriously, nobody here cares about your personal preferences. You said
you want some significant changes done to the max6650 driver, it takes
many steps to get there, either you take them, or you can leave right
now. If you're not going to listen to (and subsequently obey) people who
have been working on this project for years and are well-known and
respected by the vast majority of their peers, then bye bye.

> Surely, you can wait with those, more or less, cosmetic non-runtime
> tested changes?

I see you once again failed to read (or understand) something I repeated
many times already. One of these changes (the one moving the hwmon
attributes from i2c device to hwmon device) is _mandatory_ to get your
own changes accepted. Guenter did you an immense favor by writing these
patches, so if anything you should be very grateful to him.

> This would impose me a lot of additional work again, and I personally
> do not see the benefit of it. In my book at least, feature is over
> internal polishing.

Change books then, yours is just wrong. Bug fixes come first, then
cleanups, then features. Adding features on top of ugly code is a pain
for everyone. Plus cleaning up the code helps you to understand it, so
I'd say this is time well invested. You should try, that would certainly
help you avoid some mistakes you did in the past.

I would like to add a more general comment on the way you behave with
the community and that has been bothering me for days. You apparently
act first and think second. I can no longer count the number of times
you replied to a post only to reply to yourself a few minutes later.
Last occurrence of this is in this thread: first reply from you at
11:38, and then an addition at 11:46, i.e. 8 minutes later. And you do
that all the time.

And it holds for patches too, for example.
http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041179.html
posted at 11:24, then
http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041180.html
v2 of the same posted at 11:28.

So please listen to this piece of advice: take your time. Think more,
and only act after you have thought thoroughly about everything. It will
save you a lot of trouble, and the community a lot of time.

Thanks,
--
Jean Delvare
Suse L3 Support

2014-02-13 11:16:16

[permalink] [raw]

Subject: Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

On Thu, 13 Feb 2014, Jean Delvare wrote:

> On Thu, 13 Feb 2014 09:58:17 +0000, Lee Jones wrote:
> > > The MFD driver has now been added, so this driver is now being adopted to be a
> > > subdevice driver on top of it. This means, the i2c driver usage is being
> > > converted to platform driver usage all around.
> > >
> > > Signed-off-by: Laszlo Papp <[email protected]>
> > > ---
> > > This patch has been compile tested only and will be tested with real hardware,
> > > but early reviews to catch any trivial issues would be welcome.
> > > drivers/hwmon/Kconfig | 2 +-
> > > drivers/hwmon/max6650.c | 155 ++++++++++++++++++++++++------------------------
> > > 2 files changed, 79 insertions(+), 78 deletions(-)
> >
> > <snip>
> >
> > > /*
> > > * Insmod parameters
> > > @@ -105,24 +108,23 @@ module_param(clock, int, S_IRUGO);
> > >
> > > #define DIV_FROM_REG(reg) (1 << (reg & 7))
> > >
> > > -static int max6650_probe(struct i2c_client *client,
> > > - const struct i2c_device_id *id);
> > > -static int max6650_init_client(struct i2c_client *client);
> > > -static int max6650_remove(struct i2c_client *client);
> > > +static int max6650_probe(struct platform_device *pdev);
> > > +static int max6650_init_client(struct platform_device *pdev);
> > > +static int max6650_remove(struct platform_device *pdev);
> > > static struct max6650_data *max6650_update_device(struct device *dev);
> >
> > It would be good to remove these forward declarations in the future.
> >
> > If no one volunteers I'll happily do it.
>
> Guenter just did:
>
> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html

Nice, FWIW:
Acked-by: Lee Jones <[email protected]>

> Any change to the max6650 driver should go on top of his patch series
> to avoid conflicts:
>
> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html

Do you have a tree Laszlo can rebase on top of?

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

2014-02-13 11:29:14

[permalink] [raw]

Subject: Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

I will try hard to concentrate on the technical and fruitful stuff in
the reply...

On Thu, Feb 13, 2014 at 11:07 AM, Jean Delvare <[email protected]> wrote:
> Hi Laszlo,
>
> Le Thursday 13 February 2014 ? 10:46 +0000, Laszlo Papp a ?crit :
>> On Thu, Feb 13, 2014 at 10:38 AM, Laszlo Papp <[email protected]> wrote:
>> > On Thu, Feb 13, 2014 at 10:15 AM, Jean Delvare <[email protected]> wrote:
>> >> Any change to the max6650 driver should go on top of his patch series
>> >> to avoid conflicts:
>> >>
>> >> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html
>> >
>> > As far as I can see, that patch set was not even tested, so how can it
>> > go in? I was told that any patch should be _runtime_ tested, too.
>> > Fwiw, I do not have time to test those personally, he would need to
>> > find someone else if that requirement really holds true.
>
> I find it _very_ funny that you dare to complain here, when you sent a
> totally untested patch no later than 2 days ago:
>
> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041180.html
>
> There's no way that patch can work.
>
> And, actually, Guenter's patches have been reviewed and tested by
> myself, to some degree (I don't have access to a physical MAX6650 or
> MAX6651 chip so I used an emulation, which I think was good enough given
> the nature of the changes.)

If you read the thread carefully, I did test the change several times,
but not after every minor change. I even apologized for my mistake of
not testing after every minor change. I am not sure what I could do
about it, but let me know if you have an idea.

>> > I would not really like to fix bugs appearing in that code to get my
>> > features in.
>
> I have no idea what you mean here.

The fact that the series is quite intrusive, and most of it seems to
be unnecessary for getting the features done. Hence, it can contain
bugs which I potentially need to debug in _real_ hardware environment
for hours or even days.

>> Also, since my change has been around for 2-3 months now, I would
>> really prefer not to be forced to rewrite it again from scratch.
>
> I'm sure Gunter would have preferred if you could write proper patches
> so he wouldn't have to do it himself.

This is not how an open source community works in my opinion, but see below.

> Seriously, nobody here cares about your personal preferences. You said
> you want some significant changes done to the max6650 driver, it takes
> many steps to get there, either you take them, or you can leave right
> now. If you're not going to listen to (and subsequently obey) people who
> have been working on this project for years and are well-known and
> respected by the vast majority of their peers, then bye bye.
>
>> Surely, you can wait with those, more or less, cosmetic non-runtime
>> tested changes?
>
> I see you once again failed to read (or understand) something I repeated
> many times already. One of these changes (the one moving the hwmon
> attributes from i2c device to hwmon device) is _mandatory_ to get your
> own changes accepted. Guenter did you an immense favor by writing these
> patches, so if anything you should be very grateful to him.

I submitted a change for trying to address the _real_ issue a couple
of days ago _right after_ the suggestion. You can check the times, it
was literally within a couple of hours. Then, someone submits the same
with better content. This is what I meant by not how a community
works. You can leave comments for others, and then they can increase
their expertise.

The patch set is very likely to contain bugs because it is a
significant re-factoring, whereas I focused on as minimal changes as
possible to get the feature reliably in.

Please help me to be involved, and not excluded. It will be hard for
me to feel helpful, and the inherent consequence is that my project
will not work with the kernel project which, I think, we both would
like to avoid.

2014-02-13 11:33:21

[permalink] [raw]

Subject: Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

> >>> > -static int max6650_probe(struct i2c_client *client,
> >>> > - const struct i2c_device_id *id);
> >>> > -static int max6650_init_client(struct i2c_client *client);
> >>> > -static int max6650_remove(struct i2c_client *client);
> >>> > +static int max6650_probe(struct platform_device *pdev);
> >>> > +static int max6650_init_client(struct platform_device *pdev);
> >>> > +static int max6650_remove(struct platform_device *pdev);
> >>> > static struct max6650_data *max6650_update_device(struct device *dev);
> >>>
> >>> It would be good to remove these forward declarations in the future.
> >>>
> >>> If no one volunteers I'll happily do it.
> >>
> >> Guenter just did:
> >>
> >> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html
> >>
> >> Any change to the max6650 driver should go on top of his patch series
> >> to avoid conflicts:
> >>
> >> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html
> >
> As far as I can see, that patch set was not even tested, so how can it
> go in? I was told that any patch should be _runtime_ tested, too.
> Fwiw, I do not have time to test those personally, he would need to
> find someone else if that requirement really holds true.
>
> I would not really like to fix bugs appearing in that code to get my
> features in.
>
> Also, since my change has been around for 2-3 months now, I would
> really prefer not to be forced to rewrite it again from scratch.
> Surely, you can wait with those, more or less, cosmetic non-runtime
> tested changes?
>
> This would impose me a lot of additional work again, and I personally
> do not see the benefit of it. In my book at least, feature is over
> internal polishing.

Right, I've had enough. I'm removing your patch from the MFD tree.

I've asked too many people to give you a second chance and asked you
privately to behave yourself and treat others with respect. So far I
haven't seen an ounce of self control or depomacy from you.

This is how it's going to work from now on:

- You submit a patch
- It gets reviewed <----\
- You fix up the review comments as requested -----/
- Non-compliance or arguments with the _experts_ results in:
`$INTEREST > /dev/null || \
grep "From: Laszio Papp" ~/.mail | xargs rm -rf`

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

2014-02-13 11:58:18

[permalink] [raw]

Subject: Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

Hi Lee,

On Thu, 13 Feb 2014 11:16:07 +0000, Lee Jones wrote:
> On Thu, 13 Feb 2014, Jean Delvare wrote:
> > Guenter just did:
> >
> > http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html
>
> Nice, FWIW:
> Acked-by: Lee Jones <[email protected]>
>
> > Any change to the max6650 driver should go on top of his patch series
> > to avoid conflicts:
> >
> > http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html
>
> Do you have a tree Laszlo can rebase on top of?

Now that the patches have my Reviewed-by and your Acked-by, I believe
Guenter will add them shortly to:

http://git.kernel.org/cgit/linux/kernel/git/groeck/linux-staging.git/log/?h=hwmon-staging

--
Jean Delvare
Suse L3 Support

2014-02-13 12:27:30

[permalink] [raw]

Subject: Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

On Thu, Feb 13, 2014 at 11:33 AM, Lee Jones <[email protected]> wrote:
>> >>> > -static int max6650_probe(struct i2c_client *client,
>> >>> > - const struct i2c_device_id *id);
>> >>> > -static int max6650_init_client(struct i2c_client *client);
>> >>> > -static int max6650_remove(struct i2c_client *client);
>> >>> > +static int max6650_probe(struct platform_device *pdev);
>> >>> > +static int max6650_init_client(struct platform_device *pdev);
>> >>> > +static int max6650_remove(struct platform_device *pdev);
>> >>> > static struct max6650_data *max6650_update_device(struct device *dev);
>> >>>
>> >>> It would be good to remove these forward declarations in the future.
>> >>>
>> >>> If no one volunteers I'll happily do it.
>> >>
>> >> Guenter just did:
>> >>
>> >> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html
>> >>
>> >> Any change to the max6650 driver should go on top of his patch series
>> >> to avoid conflicts:
>> >>
>> >> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html
>> >
>> As far as I can see, that patch set was not even tested, so how can it
>> go in? I was told that any patch should be _runtime_ tested, too.
>> Fwiw, I do not have time to test those personally, he would need to
>> find someone else if that requirement really holds true.
>>
>> I would not really like to fix bugs appearing in that code to get my
>> features in.
>>
>> Also, since my change has been around for 2-3 months now, I would
>> really prefer not to be forced to rewrite it again from scratch.
>> Surely, you can wait with those, more or less, cosmetic non-runtime
>> tested changes?
>>
>> This would impose me a lot of additional work again, and I personally
>> do not see the benefit of it. In my book at least, feature is over
>> internal polishing.
>
> Right, I've had enough. I'm removing your patch from the MFD tree.
>
> I've asked too many people to give you a second chance and asked you
> privately to behave yourself and treat others with respect. So far I
> haven't seen an ounce of self control or depomacy from you.
>
> This is how it's going to work from now on:
>
> - You submit a patch
> - It gets reviewed <----\
> - You fix up the review comments as requested -----/
> - Non-compliance or arguments with the _experts_ results in:
> `$INTEREST > /dev/null || \
> grep "From: Laszio Papp" ~/.mail | xargs rm -rf`

http://comments.gmane.org/gmane.linux.kernel/1645251

Step 2 did not happen. I did not get any review for my change. I
literally submitted that within a couple of hours after the request.

Could you please tell me what was wrong with that change, and why I
did not get any respect not to "xargs rm -rf" my work in that area? I
believe I was ignored instead of improving the change, and someone
else tried to address the same thing. There was no argument in that
thread. It was a technical change. I personally do not feel happy
about it.

2014-02-13 12:40:11

[permalink] [raw]

Subject: Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

> http://comments.gmane.org/gmane.linux.kernel/1645251
>
> Step 2 did not happen. I did not get any review for my change. I
> literally submitted that within a couple of hours after the request.
>
> Could you please tell me what was wrong with that change, and why I
> did not get any respect not to "xargs rm -rf" my work in that area? I
> believe I was ignored instead of improving the change, and someone
> else tried to address the same thing. There was no argument in that
> thread. It was a technical change. I personally do not feel happy
> about it.

Let's start again.

Rebase your work on top of the HWMON tree on kernel.org and resubmit
the entire set. If rebasing takes you more than 20 mins, you're
probably doing it wrong. Ensure you submit the entire patchset with a
nice cover letter and all the patches dangling (shallow threaded) from
it. If you don't know how to do that look at `git send-email --help`
and search for "thread", "annotate" and "compose". Keep submitting
them to your own (and only your own) email address until it looks
correct, _then_ submit to the MLs CC'ing all of the maintainers on all
of the patches.

Submission should look like this:

[PATCH 0/x] Patch-set title
\->[PATCH 1/x] mfd: Patch title
|->[PATCH ./x] hwmon: Patch title
|->[PATCH x/x] gpio/pinctrl: Patch title

We'll give you constructive reviews and you'll ask questions (not
arguments) and resubmit.

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

2014-02-13 12:57:38

[permalink] [raw]

Subject: Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

Hi Laszlo,

On Thu, 13 Feb 2014 12:27:28 +0000, Laszlo Papp wrote:
> On Thu, Feb 13, 2014 at 11:33 AM, Lee Jones <[email protected]> wrote:
> > Right, I've had enough. I'm removing your patch from the MFD tree.
> >
> > I've asked too many people to give you a second chance and asked you
> > privately to behave yourself and treat others with respect. So far I
> > haven't seen an ounce of self control or depomacy from you.
> >
> > This is how it's going to work from now on:
> >
> > - You submit a patch
> > - It gets reviewed <----\
> > - You fix up the review comments as requested -----/
> > - Non-compliance or arguments with the _experts_ results in:
> > `$INTEREST > /dev/null || \
> > grep "From: Laszio Papp" ~/.mail | xargs rm -rf`
>
> http://comments.gmane.org/gmane.linux.kernel/1645251
>
> Step 2 did not happen. I did not get any review for my change. I
> literally submitted that within a couple of hours after the request.

Yes, twice even, and broken each time. And without a changelog on v2
(despite Documentation/SubmittingPatches explaining this is a good
practice - see section 15.)

> Could you please tell me what was wrong with that change, and why I
> did not get any respect not to "xargs rm -rf" my work in that area? I
> believe I was ignored instead of improving the change, and someone
> else tried to address the same thing. There was no argument in that
> thread. It was a technical change. I personally do not feel happy
> about it.

The change itself was so wrong that I don't even know where to start.

But the main problem really was you. You had pissed me (and I suspect,
everybody else) off so much that day that I really didn't want to deal
with your rants or code any longer. As you can imagine, I have more
than enough on my plate, so I just moved on to another task.

Then by the time I may have been willing to give you another chance and
review your code, Guenter wrote a more complete, better patch set. So I
thought I'd just review that one. And it was good, and it took me less
time to review and test it than to (attempt to) teach you how to behave.

Working with Guenter is a pleasure. Working with you is a pain, really.
And guess what, I get to choose who I'm working with.

If people no longer want to work with you, well, blame it on yourself.

--
Jean Delvare
Suse L3 Support

2014-02-13 13:19:12

[permalink] [raw]

Subject: Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

On Thu, Feb 13, 2014 at 12:57 PM, Jean Delvare <[email protected]> wrote:
> Hi Laszlo,
>
> On Thu, 13 Feb 2014 12:27:28 +0000, Laszlo Papp wrote:
>> On Thu, Feb 13, 2014 at 11:33 AM, Lee Jones <[email protected]> wrote:
>> > Right, I've had enough. I'm removing your patch from the MFD tree.
>> >
>> > I've asked too many people to give you a second chance and asked you
>> > privately to behave yourself and treat others with respect. So far I
>> > haven't seen an ounce of self control or depomacy from you.
>> >
>> > This is how it's going to work from now on:
>> >
>> > - You submit a patch
>> > - It gets reviewed <----\
>> > - You fix up the review comments as requested -----/
>> > - Non-compliance or arguments with the _experts_ results in:
>> > `$INTEREST > /dev/null || \
>> > grep "From: Laszio Papp" ~/.mail | xargs rm -rf`
>>
>> http://comments.gmane.org/gmane.linux.kernel/1645251
>>
>> Step 2 did not happen. I did not get any review for my change. I
>> literally submitted that within a couple of hours after the request.
>
> Yes, twice even, and broken each time. And without a changelog on v2
> (despite Documentation/SubmittingPatches explaining this is a good
> practice - see section 15.)
>
>> Could you please tell me what was wrong with that change, and why I
>> did not get any respect not to "xargs rm -rf" my work in that area? I
>> believe I was ignored instead of improving the change, and someone
>> else tried to address the same thing. There was no argument in that
>> thread. It was a technical change. I personally do not feel happy
>> about it.
>
> The change itself was so wrong that I don't even know where to start.
>
> But the main problem really was you. You had pissed me (and I suspect,
> everybody else) off so much that day that I really didn't want to deal
> with your rants or code any longer. As you can imagine, I have more
> than enough on my plate, so I just moved on to another task.
>
> Then by the time I may have been willing to give you another chance and
> review your code, Guenter wrote a more complete, better patch set. So I
> thought I'd just review that one. And it was good, and it took me less
> time to review and test it than to (attempt to) teach you how to behave.
>
> Working with Guenter is a pleasure. Working with you is a pain, really.
> And guess what, I get to choose who I'm working with.
>
> If people no longer want to work with you, well, blame it on yourself.

I think the question was more addressed to Guenter because he did not
try to help me to get involved with that patch. I did not mean to say
that I would have expected reviews within a week, but I would have
hoped for being more inclusive, i.e. trying to tell me what I am
doing, why, even if it is clearly broken. Please forgive me my
technical shortcoming. I am sure this would improve over time, just
like for anyone else here.

Currently, I am not there mentally to submit the patch set requested
by Lee because this situation is making me being afraid of a newbie
and submitting patches with issues. An expert may just come and
rewrite it, and then my effort is going again to "/dev/null". I guess
I will need some time and break to get over this...

That being said, I hope I will come back with peace of mind after the break.

PS.: I have submitted a patch set in December by the way, and got no
review from the hwmon maintainers for the hwmon parts. That patch is
still valid, and review would still be very welcome. The current
submission is pretty much just a direct port of it on top of the
latest mfd and upstream changes.

2014-02-13 16:16:50

by Guenter Roeck

[permalink] [raw]

Subject: Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

On 02/13/2014 04:27 AM, Laszlo Papp wrote:
> On Thu, Feb 13, 2014 at 11:33 AM, Lee Jones <[email protected]> wrote:
>>>>>>> -static int max6650_probe(struct i2c_client *client,
>>>>>>> - const struct i2c_device_id *id);
>>>>>>> -static int max6650_init_client(struct i2c_client *client);
>>>>>>> -static int max6650_remove(struct i2c_client *client);
>>>>>>> +static int max6650_probe(struct platform_device *pdev);
>>>>>>> +static int max6650_init_client(struct platform_device *pdev);
>>>>>>> +static int max6650_remove(struct platform_device *pdev);
>>>>>>> static struct max6650_data *max6650_update_device(struct device *dev);
>>>>>>
>>>>>> It would be good to remove these forward declarations in the future.
>>>>>>
>>>>>> If no one volunteers I'll happily do it.
>>>>>
>>>>> Guenter just did:
>>>>>
>>>>> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html
>>>>>
>>>>> Any change to the max6650 driver should go on top of his patch series
>>>>> to avoid conflicts:
>>>>>
>>>>> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html
>>>>
>>> As far as I can see, that patch set was not even tested, so how can it
>>> go in? I was told that any patch should be _runtime_ tested, too.
>>> Fwiw, I do not have time to test those personally, he would need to
>>> find someone else if that requirement really holds true.
>>>
>>> I would not really like to fix bugs appearing in that code to get my
>>> features in.
>>>
>>> Also, since my change has been around for 2-3 months now, I would
>>> really prefer not to be forced to rewrite it again from scratch.
>>> Surely, you can wait with those, more or less, cosmetic non-runtime
>>> tested changes?
>>>
>>> This would impose me a lot of additional work again, and I personally
>>> do not see the benefit of it. In my book at least, feature is over
>>> internal polishing.
>>
>> Right, I've had enough. I'm removing your patch from the MFD tree.
>>
>> I've asked too many people to give you a second chance and asked you
>> privately to behave yourself and treat others with respect. So far I
>> haven't seen an ounce of self control or depomacy from you.
>>
>> This is how it's going to work from now on:
>>
>> - You submit a patch
>> - It gets reviewed <----\
>> - You fix up the review comments as requested -----/
>> - Non-compliance or arguments with the _experts_ results in:
>> `$INTEREST > /dev/null || \
>> grep "From: Laszio Papp" ~/.mail | xargs rm -rf`
>
> http://comments.gmane.org/gmane.linux.kernel/1645251
>
> Step 2 did not happen. I did not get any review for my change. I
> literally submitted that within a couple of hours after the request.
>

If you had tested your patch on real or simulated hardware,
you might have noticed a crash whenever you accessed any
of the attributes. So you did not test your patch.

Instead of trying to educate you how the conversion to the
new API works, I decided to help you out a bit and do
the conversion myself. I did some cleanup before, since
that made the actual feature patch easier for me to implement,
and I did some more cleanup afterwards just because I like
cleaning up code.

I had hoped that you might find the time to test the result,
but it appears that won't happen. I am gracious to Jean that
he took the time to review the changes and even test the
result in simulation, even though I know he is very busy.
So I consider the changes to be good enough to be made
available in my -staging tree, which I did by now.
I'll move them over to -next once I have the chance to test
on real hardware or after I get a Tested-by: from someone
with real hardware.

Guenter

2014-02-13 16:29:25

by Guenter Roeck

[permalink] [raw]

Subject: Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

On 02/13/2014 03:58 AM, Jean Delvare wrote:
> Hi Lee,
>
> On Thu, 13 Feb 2014 11:16:07 +0000, Lee Jones wrote:
>> On Thu, 13 Feb 2014, Jean Delvare wrote:
>>> Guenter just did:
>>>
>>> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html
>>
>> Nice, FWIW:
>> Acked-by: Lee Jones <[email protected]>
>>
>>> Any change to the max6650 driver should go on top of his patch series
>>> to avoid conflicts:
>>>
>>> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html
>>
>> Do you have a tree Laszlo can rebase on top of?
>
> Now that the patches have my Reviewed-by and your Acked-by, I believe
> Guenter will add them shortly to:
>
> http://git.kernel.org/cgit/linux/kernel/git/groeck/linux-staging.git/log/?h=hwmon-staging
>
It is there now (in the hwmon-staging branch as Jean suggested).

Jean, Lee, thanks a lot for the reviews and simulation testing.
I'll try to get samples and test on real hardware before moving
the patches into hwmon-next.

Thanks,
Guenter

2014-02-13 16:53:26

[permalink] [raw]

Subject: Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

On Thu, Feb 13, 2014 at 4:16 PM, Guenter Roeck <[email protected]> wrote:
> On 02/13/2014 04:27 AM, Laszlo Papp wrote:
>>
>> On Thu, Feb 13, 2014 at 11:33 AM, Lee Jones <[email protected]> wrote:
>>>>>>>>
>>>>>>>> -static int max6650_probe(struct i2c_client *client,
>>>>>>>> - const struct i2c_device_id *id);
>>>>>>>> -static int max6650_init_client(struct i2c_client *client);
>>>>>>>> -static int max6650_remove(struct i2c_client *client);
>>>>>>>> +static int max6650_probe(struct platform_device *pdev);
>>>>>>>> +static int max6650_init_client(struct platform_device *pdev);
>>>>>>>> +static int max6650_remove(struct platform_device *pdev);
>>>>>>>> static struct max6650_data *max6650_update_device(struct device
>>>>>>>> *dev);
>>>>>>>
>>>>>>>
>>>>>>> It would be good to remove these forward declarations in the future.
>>>>>>>
>>>>>>> If no one volunteers I'll happily do it.
>>>>>>
>>>>>>
>>>>>> Guenter just did:
>>>>>>
>>>>>>
>>>>>> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html
>>>>>>
>>>>>> Any change to the max6650 driver should go on top of his patch series
>>>>>> to avoid conflicts:
>>>>>>
>>>>>>
>>>>>> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html
>>>>>
>>>>>
>>>> As far as I can see, that patch set was not even tested, so how can it
>>>> go in? I was told that any patch should be _runtime_ tested, too.
>>>> Fwiw, I do not have time to test those personally, he would need to
>>>> find someone else if that requirement really holds true.
>>>>
>>>> I would not really like to fix bugs appearing in that code to get my
>>>> features in.
>>>>
>>>> Also, since my change has been around for 2-3 months now, I would
>>>> really prefer not to be forced to rewrite it again from scratch.
>>>> Surely, you can wait with those, more or less, cosmetic non-runtime
>>>> tested changes?
>>>>
>>>> This would impose me a lot of additional work again, and I personally
>>>> do not see the benefit of it. In my book at least, feature is over
>>>> internal polishing.
>>>
>>>
>>> Right, I've had enough. I'm removing your patch from the MFD tree.
>>>
>>> I've asked too many people to give you a second chance and asked you
>>> privately to behave yourself and treat others with respect. So far I
>>> haven't seen an ounce of self control or depomacy from you.
>>>
>>> This is how it's going to work from now on:
>>>
>>> - You submit a patch
>>> - It gets reviewed <----\
>>> - You fix up the review comments as requested -----/
>>> - Non-compliance or arguments with the _experts_ results in:
>>> `$INTEREST > /dev/null || \
>>> grep "From: Laszio Papp" ~/.mail | xargs rm -rf`
>>
>>
>> http://comments.gmane.org/gmane.linux.kernel/1645251
>>
>> Step 2 did not happen. I did not get any review for my change. I
>> literally submitted that within a couple of hours after the request.
>>
>
> If you had tested your patch on real or simulated hardware,
> you might have noticed a crash whenever you accessed any
> of the attributes. So you did not test your patch.
>
> Instead of trying to educate you how the conversion to the
> new API works, I decided to help you out a bit and do
> the conversion myself.

I am afraid that with this attitude and approach towards potential new
contributors, you will put more and more work on you making it more
difficult to get things done at large. "Educating" new people is a win
in the long run, and this is a general practice out there before
claiming that I am crazy (e.g. someone even told me in the kernel
circles that is "rude").

I am surprised that mentoring new people is considered bad idea in
here. Since there is no guarantee you would not do it next time if I
tried to put effort into contributing, I feel more comfortable to skip
this project for now, and work where I know this cannot happen.
Currently, I do not feel inclusive in this project due to this
approach.

I do not even claim that you would be this to me intentionally, but it
has happened either way.

> I did some cleanup before, since
> that made the actual feature patch easier for me to implement,
> and I did some more cleanup afterwards just because I like
> cleaning up code.

IMO, it is not worth making the git history noisy with needless
re-arranging like forward headers, but that is just my opinion. Please
do not say, it is rude and not respectful. I definitely respect your
opinion, I just do not agree with it, but that is fine, you are a
maintainer so you get to decide, I guess. That being said, perhaps I
missed something why such re-arranging outweighs the git history
disadvantage.

2014-02-14 07:03:30

[permalink] [raw]

Subject: Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

On Thu, Feb 13, 2014 at 12:40 PM, Lee Jones <[email protected]> wrote:
>> http://comments.gmane.org/gmane.linux.kernel/1645251
>>
>> Step 2 did not happen. I did not get any review for my change. I
>> literally submitted that within a couple of hours after the request.
>>
>> Could you please tell me what was wrong with that change, and why I
>> did not get any respect not to "xargs rm -rf" my work in that area? I
>> believe I was ignored instead of improving the change, and someone
>> else tried to address the same thing. There was no argument in that
>> thread. It was a technical change. I personally do not feel happy
>> about it.
>
> Let's start again.
>
> Rebase your work on top of the HWMON tree on kernel.org and resubmit
> the entire set. If rebasing takes you more than 20 mins, you're
> probably doing it wrong.

I tried, but I could not manage it within 20 minutes, so I guess I am
doing something wrong. Can you please provide some pointers how not to
do it wrong? Perhaps, I am not aware of some tricks.

2014-02-14 09:02:49

[permalink] [raw]

Subject: Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

> >> http://comments.gmane.org/gmane.linux.kernel/1645251
> >>
> >> Step 2 did not happen. I did not get any review for my change. I
> >> literally submitted that within a couple of hours after the request.
> >>
> >> Could you please tell me what was wrong with that change, and why I
> >> did not get any respect not to "xargs rm -rf" my work in that area? I
> >> believe I was ignored instead of improving the change, and someone
> >> else tried to address the same thing. There was no argument in that
> >> thread. It was a technical change. I personally do not feel happy
> >> about it.
> >
> > Let's start again.
> >
> > Rebase your work on top of the HWMON tree on kernel.org and resubmit
> > the entire set. If rebasing takes you more than 20 mins, you're
> > probably doing it wrong.
>
> I tried, but I could not manage it within 20 minutes, so I guess I am
> doing something wrong. Can you please provide some pointers how not to
> do it wrong? Perhaps, I am not aware of some tricks.

One question, are you still working on this stuff or not? I'm confused
by the disparity in your messages. I'm going to guess that you're in
for now.

Do:
`git rebase -i <base> --onto <newbase>`
Where:
<base> is the SHA1 of the first patch below your changes in `git log`
<new_base> is Guenter's staging tree on kernel.org

Ensure you're rebasing all of your patches (and patches that aren't
yours) when your $EDITOR pops up. If they are wrong, delete all the
lines in the file and the rebase will be aborted. If they're correct
save and close your $EDITOR.

You'll receive conflicts. You can see the state of the conflicts using
`git status` Open the file, find the conflict markers and make a choice
from the HEAD section or the section from your patch. Sometimes
you'll need to manually merge the two, if there are changes from both
refs that you want to keep. Once you're happy `git commit -a` and `git
rebase --continue`. Each conflict should not take you long, but if it
does, keep at it, as it's good practice. After a time of doing it,
you'll be able to fix merge conflicts in no time at all.

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

2014-02-14 09:14:14

[permalink] [raw]

Subject: Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

> >>>>>>>> -static int max6650_probe(struct i2c_client *client,
> >>>>>>>> - const struct i2c_device_id *id);
> >>>>>>>> -static int max6650_init_client(struct i2c_client *client);
> >>>>>>>> -static int max6650_remove(struct i2c_client *client);
> >>>>>>>> +static int max6650_probe(struct platform_device *pdev);
> >>>>>>>> +static int max6650_init_client(struct platform_device *pdev);
> >>>>>>>> +static int max6650_remove(struct platform_device *pdev);
> >>>>>>>> static struct max6650_data *max6650_update_device(struct device
> >>>>>>>> *dev);
> >>>>>>>
> >>>>>>>
> >>>>>>> It would be good to remove these forward declarations in the future.
> >>>>>>>
> >>>>>>> If no one volunteers I'll happily do it.
> >>>>>>
> >>>>>>
> >>>>>> Guenter just did:
> >>>>>>
> >>>>>>
> >>>>>> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html
> >>>>>>
> >>>>>> Any change to the max6650 driver should go on top of his patch series
> >>>>>> to avoid conflicts:
> >>>>>>
> >>>>>>
> >>>>>> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html
> >>>>>
> >>>>>
> >>>> As far as I can see, that patch set was not even tested, so how can it
> >>>> go in? I was told that any patch should be _runtime_ tested, too.
> >>>> Fwiw, I do not have time to test those personally, he would need to
> >>>> find someone else if that requirement really holds true.
> >>>>
> >>>> I would not really like to fix bugs appearing in that code to get my
> >>>> features in.
> >>>>
> >>>> Also, since my change has been around for 2-3 months now, I would
> >>>> really prefer not to be forced to rewrite it again from scratch.
> >>>> Surely, you can wait with those, more or less, cosmetic non-runtime
> >>>> tested changes?
> >>>>
> >>>> This would impose me a lot of additional work again, and I personally
> >>>> do not see the benefit of it. In my book at least, feature is over
> >>>> internal polishing.
> >>>
> >>>
> >>> Right, I've had enough. I'm removing your patch from the MFD tree.
> >>>
> >>> I've asked too many people to give you a second chance and asked you
> >>> privately to behave yourself and treat others with respect. So far I
> >>> haven't seen an ounce of self control or depomacy from you.
> >>>
> >>> This is how it's going to work from now on:
> >>>
> >>> - You submit a patch
> >>> - It gets reviewed <----\
> >>> - You fix up the review comments as requested -----/
> >>> - Non-compliance or arguments with the _experts_ results in:
> >>> `$INTEREST > /dev/null || \
> >>> grep "From: Laszio Papp" ~/.mail | xargs rm -rf`
> >>
> >>
> >> http://comments.gmane.org/gmane.linux.kernel/1645251
> >>
> >> Step 2 did not happen. I did not get any review for my change. I
> >> literally submitted that within a couple of hours after the request.
> >>
> >
> > If you had tested your patch on real or simulated hardware,
> > you might have noticed a crash whenever you accessed any
> > of the attributes. So you did not test your patch.
> >
> > Instead of trying to educate you how the conversion to the
> > new API works, I decided to help you out a bit and do
> > the conversion myself.
>
> I am afraid that with this attitude and approach towards potential new
> contributors, you will put more and more work on you making it more
> difficult to get things done at large. "Educating" new people is a win
> in the long run, and this is a general practice out there before
> claiming that I am crazy (e.g. someone even told me in the kernel
> circles that is "rude").
>
> I am surprised that mentoring new people is considered bad idea in
> here. Since there is no guarantee you would not do it next time if I
> tried to put effort into contributing, I feel more comfortable to skip
> this project for now, and work where I know this cannot happen.
> Currently, I do not feel inclusive in this project due to this
> approach.
>
> I do not even claim that you would be this to me intentionally, but it
> has happened either way.

I agree that if Guenter was to go ahead and convert this driver to an
MFD one, then that would be rude conduct. However, the work undertaken
by him was 'add-on' work which you clearly weren't comfortable with. I
think he was genuinely doing you a favour rather than 'stealing your
thunder'. His work has paved the way for yours and should make your
work easier in the long run.

NB: Rebasing on top of different HEADs and other people's work is just
a fact of life in the kernel. If you find it difficult, stick
around. You'll get better at it with practice.

> > I did some cleanup before, since
> > that made the actual feature patch easier for me to implement,
> > and I did some more cleanup afterwards just because I like
> > cleaning up code.
>
> IMO, it is not worth making the git history noisy with needless
> re-arranging like forward headers, but that is just my opinion. Please
> do not say, it is rude and not respectful. I definitely respect your
> opinion, I just do not agree with it, but that is fine, you are a
> maintainer so you get to decide, I guess. That being said, perhaps I
> missed something why such re-arranging outweighs the git history
> disadvantage.

=:-D

We have nearly half a million commits in the Mainline kernel since
v2.6.12. I don't think Git history is even a consideration here. Also,
we really don't like forward declarations in the kernel. Guenter's
clean-ups where good ones. We can't stop cleaning up and developing in
the fear that someone else is working on that same driver. We have
lots of people making changes to the same files all over the kernel.
Get good at `git rebase`ing and there is no issue, it's just another
day in the life of a Kernel Developer.

If you'd spend more time rebasing and coding as you do writing emails,
you'd have had this done by now. :)

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

2014-02-14 09:20:55

[permalink] [raw]

Subject: Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

On Fri, Feb 14, 2014 at 9:02 AM, Lee Jones <[email protected]> wrote:
>> >> http://comments.gmane.org/gmane.linux.kernel/1645251
>> >>
>> >> Step 2 did not happen. I did not get any review for my change. I
>> >> literally submitted that within a couple of hours after the request.
>> >>
>> >> Could you please tell me what was wrong with that change, and why I
>> >> did not get any respect not to "xargs rm -rf" my work in that area? I
>> >> believe I was ignored instead of improving the change, and someone
>> >> else tried to address the same thing. There was no argument in that
>> >> thread. It was a technical change. I personally do not feel happy
>> >> about it.
>> >
>> > Let's start again.
>> >
>> > Rebase your work on top of the HWMON tree on kernel.org and resubmit
>> > the entire set. If rebasing takes you more than 20 mins, you're
>> > probably doing it wrong.
>>
>> I tried, but I could not manage it within 20 minutes, so I guess I am
>> doing something wrong. Can you please provide some pointers how not to
>> do it wrong? Perhaps, I am not aware of some tricks.
>
> One question, are you still working on this stuff or not? I'm confused
> by the disparity in your messages. I'm going to guess that you're in
> for now.
>
> Do:
> `git rebase -i <base> --onto <newbase>`
> Where:
> <base> is the SHA1 of the first patch below your changes in `git log`
> <new_base> is Guenter's staging tree on kernel.org
>
> Ensure you're rebasing all of your patches (and patches that aren't
> yours) when your $EDITOR pops up. If they are wrong, delete all the
> lines in the file and the rebase will be aborted. If they're correct
> save and close your $EDITOR.
>
> You'll receive conflicts. You can see the state of the conflicts using
> `git status` Open the file, find the conflict markers and make a choice
> from the HEAD section or the section from your patch. Sometimes
> you'll need to manually merge the two, if there are changes from both
> refs that you want to keep. Once you're happy `git commit -a` and `git
> rebase --continue`. Each conflict should not take you long, but if it
> does, keep at it, as it's good practice. After a time of doing it,
> you'll be able to fix merge conflicts in no time at all.

Right, that is what I have been following myself for a couple of
years. Why it took me more time because I had to go through his
changes and to understand all in details to make reasonably good
decisions what to keep and what to drop at the conflicts.

Thank you for your answer.

2014-02-14 10:17:33