Re: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API

[Satya Priya Kakitapalli (Temp)]

On 6/17/2022 2:27 AM, Lee Jones wrote:
> On Tue, 14 Jun 2022, Satya Priya wrote:
>
> > Use i2c_new_dummy_device() to register pm8008-regulator
> > client present at a different address space, instead of
> > defining a separate DT node. This avoids calling the probe
> > twice for the same chip, once for each client pm8008-infra
> > and pm8008-regulator.
> >
> > As a part of this define pm8008_regmap_init() to do regmap
> > init for both the clients and define pm8008_get_regmap() to
> > pass the regmap to the regulator driver.
> >
> > Signed-off-by: Satya Priya <quic_c_skakit@xxxxxxxxxxx>
> > Reviewed-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> > ---
> > Changes in V15:
> >   - None.
> >
> > Changes in V14:
> >   - None.
> >
> > Changes in V13:
> >   - None.
> >
> >   drivers/mfd/qcom-pm8008.c       | 34 ++++++++++++++++++++++++++++++++--
> >   include/linux/mfd/qcom_pm8008.h |  9 +++++++++
> >   2 files changed, 41 insertions(+), 2 deletions(-)
> >   create mode 100644 include/linux/mfd/qcom_pm8008.h
> >
> > diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> > index 569ffd50..55e2a8e 100644
> > --- a/drivers/mfd/qcom-pm8008.c
> > +++ b/drivers/mfd/qcom-pm8008.c
> > @@ -9,6 +9,7 @@
> >   #include <linux/interrupt.h>
> >   #include <linux/irq.h>
> >   #include <linux/irqdomain.h>
> > +#include <linux/mfd/qcom_pm8008.h>
> >   #include <linux/module.h>
> >   #include <linux/of_device.h>
> >   #include <linux/of_platform.h>
> > @@ -57,6 +58,7 @@ enum {
> >   
> >   struct pm8008_data {
> >   	struct device *dev;
> > +	struct regmap *regulators_regmap;
> >   	int irq;
> >   	struct regmap_irq_chip_data *irq_data;
> >   };
> > @@ -150,6 +152,12 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
> >   	.max_register	= 0xFFFF,
> >   };
> >   
> > +struct regmap *pm8008_get_regmap(const struct pm8008_data *chip)
> > +{
> > +	return chip->regulators_regmap;
> > +}
> > +EXPORT_SYMBOL_GPL(pm8008_get_regmap);
> Seems like abstraction for the sake of abstraction.
>
> Why not do the dereference inside the regulator driver?


To derefer this in the regulator driver, we need to have the pm8008_data 
struct definition in the qcom_pm8008 header file.

I think it doesn't look great to have only that structure in header and 
all other structs and enum in the mfd driver.


> >   static int pm8008_init(struct regmap *regmap)
> >   {
> >   	int rc;
> > @@ -217,11 +225,25 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
> >   	return 0;
> >   }
> >   
> > +static struct regmap *pm8008_regmap_init(struct i2c_client *client,
> > +							struct pm8008_data *chip)
> > +{
> > +	struct regmap *regmap;
> > +
> > +	regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
> > +	if (!regmap)
> > +		return NULL;
> > +
> > +	i2c_set_clientdata(client, chip);
> > +	return regmap;
> > +}
> This function seems superfluous.
>
> It's only called once and it contains a single call.


No, It is being called twice. To avoid repetitive code, I've added this 
subroutine.


> Just pop the call directly into probe.
>
> >   static int pm8008_probe(struct i2c_client *client)
> >   {
> >   	int rc;
> >   	struct pm8008_data *chip;
> >   	struct gpio_desc *reset_gpio;
> > +	struct i2c_client *regulators_client;
> >   	struct regmap *regmap;
> >   
> >   	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> > @@ -229,11 +251,19 @@ static int pm8008_probe(struct i2c_client *client)
> >   		return -ENOMEM;
> >   
> >   	chip->dev = &client->dev;
> > -	regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
> > +	regmap = pm8008_regmap_init(client, chip);
> >   	if (!regmap)
> >   		return -ENODEV;
> >   
> > -	i2c_set_clientdata(client, chip);
> > +	regulators_client = i2c_new_dummy_device(client->adapter, client->addr + 1);
> > +	if (IS_ERR(regulators_client)) {
> > +		dev_err(&client->dev, "can't attach client\n");
> > +		return PTR_ERR(regulators_client);
> > +	}
> > +
> > +	chip->regulators_regmap = pm8008_regmap_init(regulators_client, chip);
> > +	if (!chip->regulators_regmap)
> > +		return -ENODEV;
> >   
> >   	reset_gpio = devm_gpiod_get(chip->dev, "reset", GPIOD_OUT_LOW);
> >   	if (IS_ERR(reset_gpio))
> > diff --git a/include/linux/mfd/qcom_pm8008.h b/include/linux/mfd/qcom_pm8008.h
> > new file mode 100644
> > index 0000000..3814bff
> > --- /dev/null
> > +++ b/include/linux/mfd/qcom_pm8008.h
> > @@ -0,0 +1,9 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +// Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> > +#ifndef __QCOM_PM8008_H__
> > +#define __QCOM_PM8008_H__
> > +
> > +struct pm8008_data;
> > +struct regmap *pm8008_get_regmap(const struct pm8008_data *chip);
> > +
> > +#endif