Re: [PATCH 3/6] mfd: mp2629: Add support for mps mp2733 battery charger

[saravanan sekar <sravanhome@gmail.com>]

Hello Andy,
Thanks for your time to review, I try fix all the review comments

On 14/06/22 18:05, Andy Shevchenko wrote:
> On Tue, Jun 14, 2022 at 5:17 PM Saravanan Sekar <sravanhome@gmail.com> wrote:
>>
>> mp2733 is updated version of mp2629 battery charge management
>> device for single-cell Li-ion or Li-polymer battery. Additionally
>> supports usb fast-charge and higher range of input voltage.
> 
> ...
> 
>> +#include <linux/of_device.h>
> 
> What the original code misses is the mod_devicetable.h, and also see below.
> 
> ...
> 
>> +static const struct of_device_id mp2629_of_match[] = {
>> +       { .compatible = "mps,mp2629", .data = (void *)CHIP_ID_MP2629 },
>> +       { .compatible = "mps,mp2733", .data = (void *)CHIP_ID_MP2733 },
>> +       { }
>> +};
>> +MODULE_DEVICE_TABLE(of, mp2629_of_match);
> 
> No need to move, see below.
> 
> ...
> 
>> +static int mp2629_probe(struct i2c_client *client,
>> +                       const struct i2c_device_id *id)
> 
> Why out of a sudden you moved from ->probe_new() to ->probe()?
> 
I was experiment to pass i2c_device_id table to differentiate, the used 
compatible. I will switch back to probe_new.

>> +       enum mp2xx_chip_id chip_id;
>> +       const struct of_device_id *of_id;
>>          int ret;
>>
>> +       if (client->dev.of_node) {
>> +               of_id = of_match_device(mp2629_of_match, &client->dev);
>> +               if (!of_id) {
>> +                       dev_err(&client->dev, "Failed to match device\n");
>> +                       return -ENODEV;
>> +               }
>> +               chip_id = (enum mp2xx_chip_id)of_id->data;
>> +       }
> 
> This all is a single LoC only + property.h:
> 
> #include <linux/property.h>
> 
>       enum mp2xx_chip_id chip_id;
> 
>       chip_id = (uintptr_t)device_get_match_data(&client->dev);
> 
sure.


Thanks,
Saravanan