Message-ID: <CAOMwXhMRtFnAy33v90rZ-Mpa+yksH0054Pwr9M_VqeoJQWnPbw@mail.gmail.com>
Date:	Thu, 9 Jan 2014 11:11:45 +0000
From:	Laszlo Papp <lpapp@....org>
To:	Lee Jones <lee.jones@...aro.org>
Cc:	Guenter Roeck <linux@...ck-us.net>,
	Linus Walleij <linus.walleij@...aro.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] mfd: MAX6650/6651 support

On Thu, Jan 9, 2014 at 11:06 AM, Lee Jones <lee.jones@...aro.org> wrote:
>> > Styling i.e nice, neat, easily readable/maintainable code should be
>> > your bread and butter. If styling tires you, perhaps a new career
>> > might be in order. ;)
>>
>> or a new tool to be more professional ...
>
> Patches accepted.
>
>> >> >> +#include <linux/i2c.h>
>> >> >> +#include <linux/export.h>
>> >> >
>> >> > Why is this in here?
>> >>
>> >> Because this series was meant for a design review and overall
>> >> direction as opposed to a completely fine tuned patch set. Naturally,
>> >> I agree with the feedback of removing unnecessary header inclusion.
>> >
>> > Don't do that.
>>
>> I will guess that you mean "unnecessary header inclusion is OK"...
>
> No, I mean don't send sub-standard patches which crap you don't need
> or a copy and pasted mess contained within them.
>
>> >> >> +struct max6651_dev {
>> >> >> +    struct device *dev;
>> >> >> +    struct mutex iolock;
>> >> >> +
>> >> >> +    struct i2c_client *i2c;
>> >> >
>> >> > Is this used?
>> >>
>> >> Yes, heavily, for reading and writing the registers in the subdevice drivers.
>> >
>> > Can you show me where?
>>
>> Check the gpio driver or even the hwmon in this series. Look at the
>> places where it is using the read/write_reg functions, or you can just
>> check their signature in this patch. I am in the process of
>> refactoring it into regmap as we speak, but it is not painless because
>> I need to get it working for 3.2, too ...
>
> Can you show me the line where you initialise it?

max6651->dev = &i2c->dev;
max6651->i2c = i2c;

The last line is missing from the patch submitted as I fixed that only
last week while debugging it hard.

>> >> >> +     int type;
>> >> >
>> >> > Or this?
>> >>
>> >> Absolutely, this identifies the type, which is necessary for
>> >> initializing some corresponding data.
>> >
>> > Can you show me where?
>>
>> Well, you have different number of GPIO pins for starter ...
>
> Can you show me the line where this variable is used?

It is not yet, as explained in my previous email.

>> >> >> +};
>> >> >> +
>> >> >> +enum max6651_types {
>> >> >> +     TYPE_MAX6650,
>> >> >> +     TYPE_MAX6651,
>> >> >> +};
>> >> >
>> >> > What are you using these for?
>> >>
>> >> See above.
>> >
>> > Can you show me where you are using them?
>>
>> Perhaps, I was not while submitting this change, but the upcoming
>> changes should.
>
> Again, this adds confusion. Send your best, cleanest, most up-to-date
> code, or all you're doing is wasting people's time.

Well, that is the point why I was clarifying it.

Note that it is not like I can pull off an update within such a short
while after so many comments.... Especially since there are new
concepts revealed like "regmap", etc. I also need to make sure it
works with both latest and 3.2 where the regmap i2c init convenience
resource manager is not present, etc. It is a time-consuming full-time
job. :-)

An update will be coming addressing all the points.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists