Re: [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Laszlo Papp <lpapp@kde.org>]

On Thu, Feb 13, 2014 at 9:58 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> 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>
>> ---
>> This patch has been compile tested only and will be tested with real hardware,
>> but early reviews to catch any trivial issues would be welcome.
>>  drivers/hwmon/Kconfig   |   2 +-
>>  drivers/hwmon/max6650.c | 155 ++++++++++++++++++++++++------------------------
>>  2 files changed, 79 insertions(+), 78 deletions(-)
>
> <snip>
>
>>  /*
>>   * Insmod parameters
>> @@ -105,24 +108,23 @@ module_param(clock, int, S_IRUGO);
>>
>>  #define DIV_FROM_REG(reg) (1 << (reg & 7))
>>
>> -static int max6650_probe(struct i2c_client *client,
>> -                      const struct i2c_device_id *id);
>> -static int max6650_init_client(struct i2c_client *client);
>> -static int max6650_remove(struct i2c_client *client);
>> +static int max6650_probe(struct platform_device *pdev);
>> +static int max6650_init_client(struct platform_device *pdev);
>> +static int max6650_remove(struct platform_device *pdev);
>>  static struct max6650_data *max6650_update_device(struct device *dev);
>
> It would be good to remove these forward declarations in the future.
>
> If no one volunteers I'll happily do it.

I personally do not see any problem with the code either way, hence I
would not personally touch what works. :)

But if it is a strong opinionated style restriction in the codebase,
then yeah, someone could do it, except me.

>>  /*
>>   * Driver data (common to all clients)
>>   */
>>
>> -static const struct i2c_device_id max6650_id[] = {
>> +static const struct platform_device_id max6650_id[] = {
>>       { "max6650", 1 },
>>       { "max6651", 4 },
>>       { }
>>  };
>> -MODULE_DEVICE_TABLE(i2c, max6650_id);
>> +MODULE_DEVICE_TABLE(platform, max6650_id);
>
> I thought you were going to do the matching in the MFD driver?
>
> If not, what's the point in the exported 'type' attribute?

I am yet to understand the concept here. You were objecting to those,
so I removed this in MFD. I could add it to that back in this patch as
proposed.

>> -static struct i2c_driver max6650_driver = {
>> +static struct platform_driver max6650_driver = {
>>       .driver = {
>>               .name   = "max6650",
>
> This should be changed as it now supports max665{0,1} right?

This is a strange historical driver. It has always supported both, yet
it was named max6650 weirdly enough as you note.

> <snip>
>
>> -     i2c_smbus_write_byte_data(client, MAX6650_REG_SPEED, data->speed);
>> +     regmap_write(data->iodev->map, MAX6650_REG_SPEED, data->speed);
>
> Ensure all of the regmap stuff is fully tested.

Yes, sure.

> <snip>
>
>> @@ -484,10 +482,12 @@ static umode_t max6650_attrs_visible(struct kobject *kobj, struct attribute *a,
>>                                   int n)
>>  {
>>       struct device *dev = container_of(kobj, struct device, kobj);
>> -     struct i2c_client *client = to_i2c_client(dev);
>> -     u8 alarm_en = i2c_smbus_read_byte_data(client, MAX6650_REG_ALARM_EN);
>> +     struct max6650_data *data = dev_get_drvdata(dev);
>> +     int alarm_en;
>>       struct device_attribute *devattr;
>>
>> +     regmap_read(data->iodev->map, MAX6650_REG_ALARM_EN, &alarm_en);
>> +
>
> Where is this then used?

Nowhere, so it was a sub-optimal situation in the old code. It is just
a direct platform device port of it whatever it was. Perhaps it is the
right time to clean it a bit, I agree.

>> -static int max6650_probe(struct i2c_client *client,
>> -                      const struct i2c_device_id *id)
>> +static int max6650_probe(struct platform_device *pdev)
>>  {
>> +     struct max665x_dev *max665x = dev_get_drvdata(pdev->dev.parent);
>>       struct max6650_data *data;
>> +     const struct platform_device_id *id = platform_get_device_id(pdev);
>
> What's the point in 'type' in the global container?
>
> It's looking as though you're not going to need it to be global after
> all, right?

I would need it for the bit fiddling, too, I think.