lkml.org 
[lkml]   [2022]   [Jun]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API
    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@quicinc.com>
    > Reviewed-by: Stephen Boyd <swboyd@chromium.org>
    > ---
    > 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?

    > 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.

    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

    --
    Lee Jones [李琼斯]
    Principal Technical Lead - Developer Services
    Linaro.org │ Open source software for Arm SoCs
    Follow Linaro: Facebook | Twitter | Blog

    \
     
     \ /
      Last update: 2022-06-16 23:00    [W:3.143 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site