lkml.org 
[lkml]   [2014]   [Jan]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 1/3] mfd: MAX6650/6651 support
From
On Thu, Jan 9, 2014 at 11:06 AM, Lee Jones <lee.jones@linaro.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.


\
 
 \ /
  Last update: 2014-01-09 12:41    [W:0.066 / U:0.508 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site