lkml.org 
[lkml]   [2014]   [Feb]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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/

\
 
 \ /
  Last update: 2014-02-14 15:41    [W:0.065 / U:0.172 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site