> MAX6650/MAX6651 chip is a multi-function device with I2C busses. The
> chip includes fan-speed regulators and monitors, GPIO, and alarm.
>
> This patch is an initial release of a MAX6650/6651 MFD driver that
> supports to enable the chip with its primary I2C bus that will connect
> the hwmon, and then the gpio devices for now.
>
> Signed-off-by: Laszlo Papp <[email protected]>
> ---
> drivers/mfd/Kconfig | 11 +++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/max6651.c | 132 ++++++++++++++++++++++++++++++++++++
> include/linux/mfd/max6651-private.h | 53 +++++++++++++++
> 4 files changed, 197 insertions(+)
> create mode 100644 drivers/mfd/max6651.c
> create mode 100644 include/linux/mfd/max6651-private.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index dd67158..706c4e5 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -321,6 +321,17 @@ config MFD_88PM860X
> select individual components like voltage regulators, RTC and
> battery-charger under the corresponding menus.
>
> +config MFD_MAX6651
> + bool "Maxim Semiconductor MAX6651 Support"
> + depends on I2C=y
> + select MFD_CORE
> + select IRQ_DOMAIN
Why have you selected IRQ_DOMAIN?
> +#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.
> +#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, also consider renaming it to "max665x".
> +};
> +
> +int max6651_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest)
> +{
Probably best to use Regmap instead.
regmap_i2c_read()
> + struct max6651_dev *max6651 = i2c_get_clientdata(i2c);
> + int ret;
Always use 8 char tabs for kernel code.
> + mutex_lock(&max6651->iolock);
> + ret = i2c_smbus_read_byte_data(i2c, reg);
> + mutex_unlock(&max6651->iolock);
> + if (ret < 0)
> + return ret;
> +
> + ret &= 0xff;
> + *dest = ret;
*dest = ret & 0xff is clear enough.
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(max6651_read_reg);
> +
> +int max6651_write_reg(struct i2c_client *i2c, u8 reg, u8 value)
> +{
> + struct max6651_dev *max6651 = i2c_get_clientdata(i2c);
> + int ret;
Same here.
regmap_i2c_write()
> + mutex_lock(&max6651->iolock);
> + ret = i2c_smbus_write_byte_data(i2c, reg, value);
> + mutex_unlock(&max6651->iolock);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(max6651_write_reg);
> +
> +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?
> + max6651 = kzalloc(sizeof(struct max6651_dev), GFP_KERNEL);
Use managed resources devm_*.
s/sizeof(struct max6651_dev)/sizeof(*max6651)/
> + if (max6651 == NULL)
if (!max6651)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(i2c, max6651);
> + max6651->dev = &i2c->dev;
> +
> + mutex_init(&max6651->iolock);
> +
> + 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?
> + goto err_mfd;
> + }
> +
> + return ret;
> +
> +err_mfd:
> + mfd_remove_devices(max6651->dev);
If mfd_add_devices() failed, you don't need to remove them.
> + kfree(max6651);
If you use managed resources you don't need this.
> + return ret;
> +}
> +
> +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.
> + 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?
If so, then you need to change the name of the file.
> + { }
{},
> +};
> +MODULE_DEVICE_TABLE(i2c, max6651_i2c_id);
> +
> +static struct i2c_driver max6651_i2c_driver = {
> + .driver = {
> + .name = "max6651",
> + .owner = THIS_MODULE,
> + },
> + .probe = max6651_i2c_probe,
> + .remove = max6651_i2c_remove,
> + .id_table = max6651_i2c_id,
> +};
Remove from here ------
> +static int __init max6651_i2c_init(void)
> +{
> + return i2c_add_driver(&max6651_i2c_driver);
> +}
> +/* init early so consumer devices can complete system boot */
I don't think this is required.
> +subsys_initcall(max6651_i2c_init);
> +
> +static void __exit max6651_i2c_exit(void)
> +{
> + i2c_del_driver(&max6651_i2c_driver);
> +}
> +module_exit(max6651_i2c_exit);
To here -----
and replace with one line:
module_i2c_driver()
<snip>
> +#ifndef __LINUX_MFD_MAX6651_PRIVATE_H
> +#define __LINUX_MFD_MAX6651_PRIVATE_H
> +
> +#include <linux/i2c.h>
> +#include <linux/export.h>
Why is this in here?
> +#include <linux/irqdomain.h>
And this?
> +struct max6651_dev {
> + struct device *dev;
> + struct mutex iolock;
> +
> + struct i2c_client *i2c;
Is this used?
> + int type;
Or this?
> +};
> +
> +enum max6651_types {
> + TYPE_MAX6650,
> + TYPE_MAX6651,
> +};
What are you using these for?
> +extern int max6651_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest);
> +extern int max6651_write_reg(struct i2c_client *i2c, u8 reg, u8 value);
regmap_i2c_read()
regmap_i2c_write()
> +#endif
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, Jan 8, 2014 at 10:39 PM, Lee Jones <[email protected]> wrote:
>> MAX6650/MAX6651 chip is a multi-function device with I2C busses. The
>> chip includes fan-speed regulators and monitors, GPIO, and alarm.
>>
>> This patch is an initial release of a MAX6650/6651 MFD driver that
>> supports to enable the chip with its primary I2C bus that will connect
>> the hwmon, and then the gpio devices for now.
>>
>> Signed-off-by: Laszlo Papp <[email protected]>
>> ---
>> drivers/mfd/Kconfig | 11 +++
>> drivers/mfd/Makefile | 1 +
>> drivers/mfd/max6651.c | 132 ++++++++++++++++++++++++++++++++++++
>> include/linux/mfd/max6651-private.h | 53 +++++++++++++++
>> 4 files changed, 197 insertions(+)
>> create mode 100644 drivers/mfd/max6651.c
>> create mode 100644 include/linux/mfd/max6651-private.h
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index dd67158..706c4e5 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -321,6 +321,17 @@ config MFD_88PM860X
>> select individual components like voltage regulators, RTC and
>> battery-charger under the corresponding menus.
>>
>> +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.
>> +#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.
>> +#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.
> also consider renaming it to "max665x".
Hmm, I will consider.
>> +};
>> +
>> +int max6651_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest)
>> +{
>
> Probably best to use Regmap instead.
>
> regmap_i2c_read()
Yes, do not worry, this was already done after Linus' initial comment.
Note that, it will be a bit hackish to get it working with 3.2 where
the device regmap convenience wrapper was missing, but I guess
backporting is my problem, not yours...
>> + 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.
>> + mutex_lock(&max6651->iolock);
>> + ret = i2c_smbus_read_byte_data(i2c, reg);
>> + mutex_unlock(&max6651->iolock);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret &= 0xff;
>> + *dest = ret;
>
> *dest = ret & 0xff is clear enough.
I think this function would be removed altogether instead.
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(max6651_read_reg);
>> +
>> +int max6651_write_reg(struct i2c_client *i2c, u8 reg, u8 value)
>> +{
>> + struct max6651_dev *max6651 = i2c_get_clientdata(i2c);
>> + int ret;
>
> Same here.
>
> regmap_i2c_write()
See above.
>> + mutex_lock(&max6651->iolock);
>> + ret = i2c_smbus_write_byte_data(i2c, reg, value);
>> + mutex_unlock(&max6651->iolock);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(max6651_write_reg);
>> +
>> +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.
>> + max6651 = kzalloc(sizeof(struct max6651_dev), GFP_KERNEL);
>
> Use managed resources devm_*.
Yes, that was also done after Linus' suggestion.
> s/sizeof(struct max6651_dev)/sizeof(*max6651)/
>
>> + if (max6651 == NULL)
>
> if (!max6651)
Yes, that was also done after Linus' suggestion, although this is
consistent with the other maxim drivers. I will refactor those
later...
>> + return -ENOMEM;
>> +
>> + i2c_set_clientdata(i2c, max6651);
>> + max6651->dev = &i2c->dev;
>> +
>> + mutex_init(&max6651->iolock);
>> +
>> + 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?
>> + goto err_mfd;
>> + }
>> +
>> + return ret;
>> +
>> +err_mfd:
>> + mfd_remove_devices(max6651->dev);
>
> If mfd_add_devices() failed, you don't need to remove them.
Sure.
>> + 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...
>> + return ret;
>> +}
>> +
>> +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...
>> + 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...
>> +};
>> +MODULE_DEVICE_TABLE(i2c, max6651_i2c_id);
>> +
>> +static struct i2c_driver max6651_i2c_driver = {
>> + .driver = {
>> + .name = "max6651",
>> + .owner = THIS_MODULE,
>> + },
>> + .probe = max6651_i2c_probe,
>> + .remove = max6651_i2c_remove,
>> + .id_table = max6651_i2c_id,
>> +};
>
> Remove from here ------
>
>> +static int __init max6651_i2c_init(void)
>> +{
>> + return i2c_add_driver(&max6651_i2c_driver);
>> +}
>> +/* init early so consumer devices can complete system boot */
>
> I don't think this is required.
>
>> +subsys_initcall(max6651_i2c_init);
>> +
>> +static void __exit max6651_i2c_exit(void)
>> +{
>> + i2c_del_driver(&max6651_i2c_driver);
>> +}
>> +module_exit(max6651_i2c_exit);
>
> To here -----
>
> and replace with one line:
>
> module_i2c_driver()
Yes, this was for consistency again, and modified after Linus' hint.
The other maxim drivers need to be patched later....
>> +#ifndef __LINUX_MFD_MAX6651_PRIVATE_H
>> +#define __LINUX_MFD_MAX6651_PRIVATE_H
>> +
>> +#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.
>> +#include <linux/irqdomain.h>
>
> And this?
Same stuff as above...
>> +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.
>> + int type;
>
> Or this?
Absolutely, this identifies the type, which is necessary for
initializing some corresponding data.
>> +};
>> +
>> +enum max6651_types {
>> + TYPE_MAX6650,
>> + TYPE_MAX6651,
>> +};
>
> What are you using these for?
See above.
>> +extern int max6651_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest);
>> +extern int max6651_write_reg(struct i2c_client *i2c, u8 reg, u8 value);
>
> regmap_i2c_read()
> regmap_i2c_write()
Sure, that was done after Linus' hint.
Thanks for the review...
Cheers ...
> >> +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
On Thu, Jan 9, 2014 at 9:41 AM, Lee Jones <[email protected]> wrote:
>> >> +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.
Sure.
>> >> +#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.
Do not worry, I appreciate your review a lot. :)
>> >> +#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.
I already changed that locally last week when debugging the
platform_match bug about it... This two lines can be taken into a
fourth patch, surely, but is it worth it when the change is already
long for the conversion and two lines do not make much difference.
Anyway, I will surely do it since you would seem to be happier.
>> >> + 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.
Yes, I understand. The problem is that the Linux kernel is not the
only project I am contributing to. That is why I was asking what
people do in similar scenarios. Do they have different editor profiles
for each project? How is it gently solved? What is the best practice?
I am using vim for what it is worth. Although, I can probably ask this
on the list in a different thread.... Will do it.
>> >> +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.
Sure. It might be that my experiment was reading so at some point, and
I did not adhere to the change later. I will remove it as you asked
because I agree.
>> >> + 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"
Right, but in that case, I will go ahead and fix at the place rfom
where I picked this up.
>> >> + 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.
OK, thankie.
>> >> +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...
As above... (thankie). :-)
>> >> + 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.
Yes, we discussed that before and I agree with the "obfuscating" 'x'.
>> >
>> >> + { }
>> >
>> > {},
>>
>> 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. ;)
or a new tool to be more professional ...
>> >> +#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"...
>> >> +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 ...
>> >> + 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 ...
>> >> +};
>> >> +
>> >> +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.
> > 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
On Thu, Jan 9, 2014 at 11:06 AM, Lee Jones <[email protected]> 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.
> >> > 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. :-)
No one is asking you to pull off anything. This is your first
submission and I am reviewing as such.
> An update will be coming addressing all the points.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
>> +int max6651_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest)
>> +{
>
> Probably best to use Regmap instead.
>
> regmap_i2c_read()
>> +int max6651_write_reg(struct i2c_client *i2c, u8 reg, u8 value)
>> +{
>> + struct max6651_dev *max6651 = i2c_get_clientdata(i2c);
>> + int ret;
>
> Same here.
>
> regmap_i2c_write()
Hmm, but what Linus linked is using regmap_read/write(...) instead of
regmap_i2c_read/write(...).
I thought I would need to be using the former. Perhaps, I
misunderstand something?
I do not even find the aforementioend functions used by drivers based
on this LXR result:
http://lxr.free-electrons.com/ident?i=regmap_i2c_write
On Thu, 09 Jan 2014, Laszlo Papp wrote:
> >> +int max6651_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest)
> >> +{
> >
> > Probably best to use Regmap instead.
> >
> > regmap_i2c_read()
>
> >> +int max6651_write_reg(struct i2c_client *i2c, u8 reg, u8 value)
> >> +{
> >> + struct max6651_dev *max6651 = i2c_get_clientdata(i2c);
> >> + int ret;
> >
> > Same here.
> >
> > regmap_i2c_write()
>
> Hmm, but what Linus linked is using regmap_read/write(...) instead of
> regmap_i2c_read/write(...).
>
> I thought I would need to be using the former. Perhaps, I
> misunderstand something?
>
> I do not even find the aforementioend functions used by drivers based
> on this LXR result:
> http://lxr.free-electrons.com/ident?i=regmap_i2c_write
Linus is in a better position to know, use his suggestion.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Fri, Jan 10, 2014 at 10:56 AM, Lee Jones <[email protected]> wrote:
> On Thu, 09 Jan 2014, Laszlo Papp wrote:
>
>> >> +int max6651_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest)
>> >> +{
>> >
>> > Probably best to use Regmap instead.
>> >
>> > regmap_i2c_read()
>>
>> >> +int max6651_write_reg(struct i2c_client *i2c, u8 reg, u8 value)
>> >> +{
>> >> + struct max6651_dev *max6651 = i2c_get_clientdata(i2c);
>> >> + int ret;
>> >
>> > Same here.
>> >
>> > regmap_i2c_write()
>>
>> Hmm, but what Linus linked is using regmap_read/write(...) instead of
>> regmap_i2c_read/write(...).
>>
>> I thought I would need to be using the former. Perhaps, I
>> misunderstand something?
>>
>> I do not even find the aforementioend functions used by drivers based
>> on this LXR result:
>> http://lxr.free-electrons.com/ident?i=regmap_i2c_write
>
> Linus is in a better position to know, use his suggestion.
Just use regmap_read/write to abstract things and hide the
underlying marshalling in regmap.
regmap_i2c_write is some regmap-internal thing.
Yours,
Linus Walleij