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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 9 Jan 2014 09:41:47 +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 > >> +config MFD_MAX6651 > >> + bool "Maxim Semiconductor MAX6651 Support" > >> + depends on I2C=y > >> + select MFD_CORE > >> + select IRQ_DOMAIN > > > > Why have you selected IRQ_DOMAIN? > > Initial consistency with other corresponding drivers, but I should > have dropped it once I dropped the irq handling to be as simple as > possible initially. IRQ_DOMAINs are only relevant for IRQ Controllers. > >> +#include <linux/device.h> > >> +#include <linux/delay.h> > >> +#include <linux/input.h> > >> +#include <linux/interrupt.h> > >> +#include <linux/mfd/core.h> > >> +#include <linux/slab.h> > >> +#include <linux/module.h> > >> +#include <linux/i2c.h> > > > > Are you sure all these are used? I'm pretty sure some of them are > > not. Only add headers if you require them. Try not to copy and paste > > stuff you don't need. > > Yes, this was meant to be the "final clean up step". I aimed > functionality and design first. In future please only send your best, most cleaned-up code. Sub-standard codes desearves nothing but a sub-standard review. > >> +#include <linux/mfd/max6651-private.h> > >> + > >> +static struct mfd_cell max6651_devs[] = { > >> + { .name = "max6651-gpio", }, > >> + { .name = "max6650", }, > > > > It would be nice to have a comment here to indicate that this is a > > hwmon driver. If you're planning to add support for the MAX6651 to > > this existing driver, > > Actually, it is already renamed to max6650-hwmon in the next patch of > this series. This won't work, as you haven't changed the name in the platform_driver struct. And rightly so, as it has nothing to do with converting the driver over to a platform one. Pull the part that changes the name into another patch. > >> + struct max6651_dev *max6651 = i2c_get_clientdata(i2c); > >> + int ret; > > > > Always use 8 char tabs for kernel code. > > As discussed, style stuff is not fixed for a design review. I am still > intereted in having an automated fix-up like astyle in other projects? > What is the recommended way? I really would not like to waste too much > time with style clean up. If you send any more lazy patches where it's clear that no attempt has been made to adhere to the documentation I've provided you with, I won't review. 'Please', no more half-ar$ed patches RFC or otherwise. There is no automated way to get styling right, but the first step is to set your editor's config for 8 char tabbing at a bare minimum. <snip> > >> +static int max6651_i2c_probe(struct i2c_client *i2c, > >> + const struct i2c_device_id *id) > >> +{ > >> + struct max6651_dev *max6651; > >> + int ret = 0; > > > > Why are you initialising ret? > > Habit for striving for good pratice, I think. It may have also been > consitency. That being said, I already removed it when I took a look > at the other driver based on Linus' suggestion. I was trying to be > consistent with other maxim drivers. > > The linux kernel drivers are inconsistent in general at large, > unfortunately. It is hard to pick up the "right one" for consistency. > I will do whatever asked as it really does not make any difference for > me. The kernel should be mostly standard with this kind of stuff. If the variable 'could possibly' be read before it is written to, then initialise it, failing that, don't worry. > >> + ret = mfd_add_devices(max6651->dev, -1, max6651_devs, > >> + ARRAY_SIZE(max6651_devs), > >> + NULL, 0, NULL); > >> + > >> + if (ret < 0) { > >> + dev_err(max6651->dev, "cannot add mfd cells\n"); > > > > Are you trying to add cells or register devices? > > I would not know the difference in this context. Care to elaborate? Providing a cell structure is just a tool. A means to an end if you will. The real goal here is to register child devices. "failed to register child devices\n" > >> + kfree(max6651); > > > > If you use managed resources you don't need this. > > I am not sure what exactly you mean by managed resource here. I only > used the malloc above as far as I can tell. Perhaps, the called > function has some magic behind. I would need to double check... Yes, devm_* (managed resources) contains magic so you don't have you free your own memory. You can remove the goto altogether. > >> +static int max6651_i2c_remove(struct i2c_client *i2c) > >> +{ > >> + struct max6651_dev *max6651 = i2c_get_clientdata(i2c); > >> + > >> + mfd_remove_devices(max6651->dev); > > > > In this case you would normally need to kfree() here, but if you use > > managed resources you won't have to. > > As above... As above... > >> + return 0; > >> +} > >> + > >> +static const struct i2c_device_id max6651_i2c_id[] = { > >> + { "max6650", TYPE_MAX6650 }, > >> + { "max6651", TYPE_MAX6651 }, > > > > So were're registering the max6650 from here too? > > Absolutely, that is the idea. > > > If so, then you need to change the name of the file. > > > >> + { } > > > > {}, > > Yep, tiring style stuff... 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. ;) <snip> > >> +#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. > >> +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? > >> + int type; > > > > Or this? > > Absolutely, this identifies the type, which is necessary for > initializing some corresponding data. Can you show me where? > >> +}; > >> + > >> +enum max6651_types { > >> + TYPE_MAX6650, > >> + TYPE_MAX6651, > >> +}; > > > > What are you using these for? > > See above. Can you show me where you are using them? -- 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/
Powered by blists - more mailing lists