Messages in this thread | | | Date | Fri, 14 Feb 2014 12:00:23 +0000 | From | Lee Jones <> | Subject | Re: [PATCH 2/2] hwmon: (max6650) Convert to be a platform driver |
| |
> The MFD driver has now been added, so this driver is now being adopted to be a > subdevice driver on top of it. This means, the i2c driver usage is being > converted to platform driver usage all around. > > Signed-off-by: Laszlo Papp <lpapp@kde.org> > --- > drivers/hwmon/Kconfig | 2 +- > drivers/hwmon/max6650.c | 125 ++++++++++++++++++++++++------------------------ > 2 files changed, 63 insertions(+), 64 deletions(-) > <snip>
> @@ -39,6 +39,9 @@ > #include <linux/hwmon.h> > #include <linux/hwmon-sysfs.h> > #include <linux/err.h> > +#include <linux/platform_device.h> > +
Not really any need for this.
> +#include <linux/mfd/max665x-private.h>
<snip>
> struct max6650_data { > struct i2c_client *client; > const struct attribute_group *groups[3]; > + struct max665x_dev *iodev;
I find the name iodev a bit confusing, why not max665x_dev?
<snip>
> - data->speed = i2c_smbus_read_byte_data(client, > - MAX6650_REG_SPEED); > - data->config = i2c_smbus_read_byte_data(client, > - MAX6650_REG_CONFIG); > + regmap_read(map, MAX6650_REG_SPEED, &data->speed); > + regmap_read(map, MAX6650_REG_CONFIG, &data->config);
I'd like Mark to look over your Regmap implementation if possible.
> if (config < 0) { > - dev_err(dev, "Error reading config, aborting.\n"); > + dev_err(&pdev->dev, "Error reading config, aborting.\n");
Rather than make all these changes, just do:
struct device *dev = &pdev->dev;
... at the top.
<snip>
> +static int max6650_probe(struct platform_device *pdev) > { > - struct device *dev = &client->dev; > - struct max6650_data *data; > + struct max6650_data *data = platform_get_drvdata(pdev);
Don't do this here, it's messy. Declare it here and initialise it later.
> + const struct platform_device_id *id = platform_get_device_id(pdev);
Same here.
> struct device *hwmon_dev; > int err; > > - data = devm_kzalloc(dev, sizeof(struct max6650_data), GFP_KERNEL); > + data = devm_kzalloc(&pdev->dev, sizeof(struct max6650_data), > + GFP_KERNEL);
Eh? didn't you already initialise 'data'?
<snip>
> - hwmon_dev = devm_hwmon_device_register_with_groups(dev, > - client->name, data, > - data->groups); > + hwmon_dev = devm_hwmon_device_register_with_groups(&pdev->dev, > + data->client->name, > + data, data->groups);
Leave it as 'dev'.
> -static const struct i2c_device_id max6650_id[] = { > +static const struct platform_device_id max6650_id[] = { > { "max6650", 1 }, > { "max6651", 4 },
I'm still pretty keen to have these magic numbers #defined.
Not yet though, let's get this sorted first.
> +static struct platform_driver max6650_driver = { > .driver = { > .name = "max6650",
Guenter, Jean, Can this be changed now it's a platform device?
Laszio, Next thing to do is to get this working and runtime test it. No more mid-term reviews until it's fully functional.
-- 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/
| |