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

From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Saravanan Sekar <sravanhome@gmail.com>
Cc: Sebastian Reichel <sre@kernel.org>,
	Lee Jones <lee.jones@linaro.org>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Jonathan Cameron <jic23@kernel.org>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Linux PM <linux-pm@vger.kernel.org>,
	devicetree <devicetree@vger.kernel.org>,
	linux-iio <linux-iio@vger.kernel.org>
Subject: Re: [PATCH 3/6] mfd: mp2629: Add support for mps mp2733 battery charger
Date: Tue, 14 Jun 2022 18:05:52 +0200	[thread overview]
Message-ID: <CAHp75VcaU-KkCGZ0tczM6JKaVdGC6icGt0pbpC5sTf+0+ePamQ@mail.gmail.com> (raw)
In-Reply-To: <20220614151722.2194936-3-sravanhome@gmail.com>

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()?

> +       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);

-- 
With Best Regards,
Andy Shevchenko