Re: [PATCH] regulator: pv880x0: Simplify probe()

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Biju Das <biju.das.jz@bp.renesas.com>
Cc: Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH] regulator: pv880x0: Simplify probe()
Date: Mon, 28 Aug 2023 12:19:11 +0300	[thread overview]
Message-ID: <ZOxmj4A5gx2Vra3L@smile.fi.intel.com> (raw)
In-Reply-To: <20230826114919.73897-1-biju.das.jz@bp.renesas.com>

On Sat, Aug 26, 2023 at 12:49:19PM +0100, Biju Das wrote:
> Replace pv88080_types->pv88080_compatible_regmap in OF/ID tables and
> simplify the probe() by replacing of_match_node() and ID lookup for
> retrieving match data by i2c_get_match_data(). After this there is
> no user of enum pv88080_types. So drop it.

...

>  #ifdef CONFIG_OF
>  static const struct of_device_id pv88080_dt_ids[] = {
> -	{ .compatible = "pvs,pv88080",    .data = (void *)TYPE_PV88080_AA },
> -	{ .compatible = "pvs,pv88080-aa", .data = (void *)TYPE_PV88080_AA },
> -	{ .compatible = "pvs,pv88080-ba", .data = (void *)TYPE_PV88080_BA },
> +	{ .compatible = "pvs,pv88080",    .data = &pv88080_aa_regs },
> +	{ .compatible = "pvs,pv88080-aa", .data = &pv88080_aa_regs  },
> +	{ .compatible = "pvs,pv88080-ba", .data = &pv88080_ba_regs },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, pv88080_dt_ids);
>  #endif

With this patch it makes sense to get rid of ugly ifdeffery, correct header
inclusions (if needed) and Kconfig.

See similar patches
$ git log --no-merges --grep "Make use of device properties"

> +static const struct i2c_device_id pv88080_i2c_id[] = {
> +	{ "pv88080",    (kernel_ulong_t)&pv88080_aa_regs },
> +	{ "pv88080-aa", (kernel_ulong_t)&pv88080_aa_regs },
> +	{ "pv88080-ba", (kernel_ulong_t)&pv88080_ba_regs },

> +	{},

Please, remove trailing comma in the terminator entry. Same you can do for
other ID tables and in other patches you prepared.

> +};

...

> -static const struct i2c_device_id pv88080_i2c_id[] = {
> -	{ "pv88080",    TYPE_PV88080_AA },
> -	{ "pv88080-aa", TYPE_PV88080_AA },
> -	{ "pv88080-ba", TYPE_PV88080_BA },
> -	{},
> -};
> -MODULE_DEVICE_TABLE(i2c, pv88080_i2c_id);
> -

Why do you move this one up? Shouldn't it be other way around, i.e. moving the
other closer to its user, namely here?

>  static struct i2c_driver pv88080_regulator_driver = {

-- 
With Best Regards,
Andy Shevchenko