Hello Lee,
Thanks a lot for your feedback.
On 11/18/2014 03:18 PM, Lee Jones wrote:
>>
>> +config MFD_CROS_EC_DEV
>
> _DEV as a post-fix doesn't really describe the driver.
>
the _DEV is meant to denote (user-space device interface) but is true that
a better name can be used, I'll see what I can came with.
>> + tristate "ChromeOS Embedded Controller userspace device interface"
>> + depends on MFD_CROS_EC
>> +
>
> Superfluous '\n'.
>
Ok
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -12,6 +12,7 @@ obj-$(CONFIG_MFD_BCM590XX) += bcm590xx.o
>> obj-$(CONFIG_MFD_CROS_EC) += cros_ec.o
>> obj-$(CONFIG_MFD_CROS_EC_I2C) += cros_ec_i2c.o
>> obj-$(CONFIG_MFD_CROS_EC_SPI) += cros_ec_spi.o
>> +obj-$(CONFIG_MFD_CROS_EC_DEV) += cros_ec_dev.o
>
> Alphabetical?
>
Ok
>> +
>> +#define pr_fmt(fmt) "cros_ec_dev: " fmt
>
> This doesn't appear to even be used?
>
It is used by pr_* family of functions, e.g: from include/linux/printk.h
#define pr_err(fmt, ...) \
printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
>> +#include <linux/compat.h>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/fs.h>
>> +#include <linux/mfd/cros_ec.h>
>> +#include <linux/mfd/cros_ec_commands.h>
>> +#include <linux/mfd/cros_ec_dev.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/printk.h>
>
> What do you need this for?
>
the printk.h header? to use the pr_* functions but I'll make sure that only
the needed headers are included.
>> +#include <linux/types.h>
>> +#include <linux/uaccess.h>
>> +
>> +/* Device variables */
>> +#define CROS_CLASS_NAME "chromeos"
>
> Any reason why you can't use the name directly?
>
I prefer macros if possible since they cost nothing and give you an indirection
level if you want to change it later. Any reason to not use a define directive?
>> +static struct cros_ec_device *ec;
>
> Can't you put this in a struct somewhere?
>
Yes, I'll remove the global variable and put it in struct file .private_data
>> +static struct class *cros_class;
>
> Why this this global instead of using the one in 'struct
> cros_ec_device'?
>
>> +static int ec_major;
>
> Same here. Only use globals if you're forced to.
>
Ok, I'll look how to refactor to remove these globals too.
>> +
>> +
>
> Superfluous '\n'.
>
Ok
>> +
>> + ret = cros_ec_cmd_xfer(ec, &msg);
>> + if (ret < 0)
>> + return ret;
>> + if (msg.result != EC_RES_SUCCESS) {
>> + snprintf(str, maxlen,
>> + "%s\nUnknown EC version: EC returned %d\n",
>> + CROS_EC_DEV_VERSION, msg.result);
>> + return 0;
>> + }
>> + if (resp.current_image >= ARRAY_SIZE(current_image_name))
>> + resp.current_image = 3; /* invalid */
>
> Add a '\n' here.
>
Ok
>> + if (*offset != 0)
>> + return 0;
>> +
>> + ret = ec_get_version(ec, msg, sizeof(msg));
>> + if (ret)
>> + return ret;
>
> New line here. Not sure why you are bunching up these segments.
>
Ok
>> + count = min(length, strlen(msg));
>> +
>> + if (copy_to_user(buffer, msg, count))
>> + return -EFAULT;
>> +
>> + *offset += count;
>
> You know offset is 0 here, so you can just *offset = count.
>
True
>> + return count;
>> +}
>> +
>> +
>
> Please remove these double line spaces throughout.
>
Ok
>> +/* Ioctls */
>> +static long ec_device_ioctl_xcmd(void __user *argp)
>> +{
>> + long ret;
>> + struct cros_ec_command s_cmd;
>> + uint8_t *user_indata;
>> + uint8_t buf[EC_PROTO2_MAX_PARAM_SIZE];
>> +
>> + if (copy_from_user(&s_cmd, argp, sizeof(s_cmd)))
>> + return -EFAULT;
>> + if (s_cmd.outsize &&
>> + copy_from_user(&buf, (void __user *)s_cmd.outdata, sizeof(buf)))
>> + return -EFAULT;
>> +
>> + user_indata = s_cmd.indata;
>> + s_cmd.indata = buf;
>> + s_cmd.outdata = buf;
>> + ret = cros_ec_cmd_xfer(ec, &s_cmd);
>> + s_cmd.indata = user_indata;
>
> For conforming if you swap these round, place a '\n' after this line
> and move the check for 'ret' to just after the call to
> cros_ec_cmd_xfer().
>
Ok
>> +static long ec_device_compat_ioctl(struct file *filp, unsigned int cmd,
>> + unsigned long arg)
>> +{
>> + void __user
>> + *argp = (void __user *)arg;
>
> Why is this expressed over multiple lines?
>
No reason at all, I'll change it.
>> + switch (cmd) {
>> + case CROS_EC_DEV_COMPAT_IOCXCMD:
>> + return ec_device_compat_ioctl_xcmd(argp);
>> + case CROS_EC_DEV_COMPAT_IOCRDMEM:
>> + return ec_device_compat_ioctl_readmem(argp);
>> + }
>
> '\n'
>
Ok
>> + return -ENOTTY;
>> +}
>> +#endif /* CONFIG_COMPAT */
>> +
>> +
>
> Please remove.
>
Ok
>> +static int ec_device_probe(struct platform_device *pdev)
>> +{
>> + int retval = -ENOTTY;
>> +
>> + ec = dev_get_drvdata(pdev->dev.parent);
>
> Stick this as the first line in the function.
>
Ok
>> + cros_class = class_create(THIS_MODULE, CROS_CLASS_NAME);
>> + if (IS_ERR(cros_class)) {
>> + pr_err("failed to register device class\n");
>
> Why are you using pr_err() over dev_err()?
>
You are right, I'll use dev_err() instead.
>> + retval = PTR_ERR(cros_class);
>> + goto failed_class;
>
> Remove this goto and the corresponding label and just return from here.
>
Ok
>> +
>> +static struct platform_driver cros_ec_dev_driver = {
>> + .driver = {
>> + .name = "cros-ec-dev",
>> + .owner = THIS_MODULE,
>
> Remove this line.
>
Right, I've seen some cleanups efforts to remove the owner from drivers
but forgot when reviewing this driver for posting.
>> + },
>> + .probe = ec_device_probe,
>> + .remove = ec_device_remove,
>> +};
>
> Where is this device registered from?
>
>> +module_platform_driver(cros_ec_dev_driver);
This preprocessor macro is expanded to (from include/linux/device.h):
module_platform_driver() -> module_driver()
#define module_driver(__driver, __register, __unregister, ...) \
static int __init __driver##_init(void) \
{ \
return __register(&(__driver) , ##__VA_ARGS__); \
} \
Again, thanks a lot for your detailed review.
Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
