Re: [PATCH 1/3] mfd: MAX6650/6651 support

[Lee Jones <lee.jones@linaro.org>]

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

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

> >> >> +};
> >> >> +
> >> >> +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.
-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog