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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 9 Jan 2014 11:06:35 +0000
From:	Lee Jones <lee.jones@...aro.org>
To:	Laszlo Papp <lpapp@....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

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