lkml.org 
[lkml]   [2014]   [Jan]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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