Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230712114754.500477-1-Naresh.Solanki@9elements.com>
 <20230712114754.500477-4-Naresh.Solanki@9elements.com> <38157958-9723-73bc-f79a-1ac8c80c512e@roeck-us.net>
In-Reply-To: <38157958-9723-73bc-f79a-1ac8c80c512e@roeck-us.net>
From:   Naresh Solanki <naresh.solanki@9elements.com>
Date:   Fri, 14 Jul 2023 13:23:26 +0200
Message-ID: <CABqG17gHj+wAVqHR0o9ospcAMYPTnO0xtcPGPc8GqNb03mO58w@mail.gmail.com>
Subject: Re: [PATCH 4/8] hwmon: (pmbus/mp2975) Simplify VOUT code
To:     Guenter Roeck <linux@roeck-us.net>
Cc:     devicetree@vger.kernel.org, Jean Delvare <jdelvare@suse.com>,
        linux-kernel@vger.kernel.org, linux-hwmon@vger.kernel.org,
        Patrick Rudolph <patrick.rudolph@9elements.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Guenter,

On Wed, 12 Jul 2023 at 17:50, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 7/12/23 04:47, Naresh Solanki wrote:
> > From: Patrick Rudolph <patrick.rudolph@9elements.com>
> >
> > In order to upstream MP2973/MP2971 simplify the code by removing support
> > for various VOUT formats. The MP2973 and MP2971 supports all PMBUS
> > supported formats for VOUT, while the MP2975 only support DIRECT and
> > VID for VOUT.
> >
> > In DIRECT mode all chips report the voltage in 1mV/LSB.
> >
> > Configure the chip to use DIRECT format for VOUT and drop the code
> > conversion code for other formats. The to be added chips MP2973/MP2971
> > will be configured to also report VOUT in DIRECT format.
> >
> > The maximum voltage that can be reported in DIRECT format is 32768mV.
> > This is sufficient as the maximum output voltage for VR12/VR13 is
> > 3040 mV.
> >
> > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> > ---
> >   drivers/hwmon/pmbus/mp2975.c | 54 ++++++------------------------------
> >   1 file changed, 8 insertions(+), 46 deletions(-)
> >
> > diff --git a/drivers/hwmon/pmbus/mp2975.c b/drivers/hwmon/pmbus/mp2975.c
> > index 04778f2dcbdb..ebc9a84b8ad3 100644
> > --- a/drivers/hwmon/pmbus/mp2975.c
> > +++ b/drivers/hwmon/pmbus/mp2975.c
> > @@ -70,7 +70,6 @@ struct mp2975_data {
> >       int vref_off[MP2975_PAGE_NUM];
> >       int vout_max[MP2975_PAGE_NUM];
> >       int vout_ov_fixed[MP2975_PAGE_NUM];
> > -     int vout_format[MP2975_PAGE_NUM];
> >       int curr_sense_gain[MP2975_PAGE_NUM];
> >   };
> >
> > @@ -83,22 +82,6 @@ MODULE_DEVICE_TABLE(i2c, mp2975_id);
> >
> >   #define to_mp2975_data(x)  container_of(x, struct mp2975_data, info)
> >
> > -static int mp2975_read_byte_data(struct i2c_client *client, int page, int reg)
> > -{
> > -     switch (reg) {
> > -     case PMBUS_VOUT_MODE:
> > -             /*
> > -              * Enforce VOUT direct format, since device allows to set the
> > -              * different formats for the different rails. Conversion from
> > -              * VID to direct provided by driver internally, in case it is
> > -              * necessary.
> > -              */
> > -             return PB_VOUT_MODE_DIRECT;
> > -     default:
> > -             return -ENODATA;
> > -     }
> > -}
> > -
> >   static int
> >   mp2975_read_word_helper(struct i2c_client *client, int page, int phase, u8 reg,
> >                       u16 mask)
> > @@ -273,24 +256,6 @@ static int mp2975_read_word_data(struct i2c_client *client, int page,
> >               ret = DIV_ROUND_CLOSEST(data->vref[page] * 10 - 50 *
> >                                       (ret + 1) * data->vout_scale, 10);
> >               break;
> > -     case PMBUS_READ_VOUT:
> > -             ret = mp2975_read_word_helper(client, page, phase, reg,
> > -                                           GENMASK(11, 0));
> > -             if (ret < 0)
> > -                     return ret;
> > -
> > -             /*
> > -              * READ_VOUT can be provided in VID or direct format. The
> > -              * format type is specified by bit 15 of the register
> > -              * MP2975_MFR_DC_LOOP_CTRL. The driver enforces VOUT direct
> > -              * format, since device allows to set the different formats for
> > -              * the different rails and also all VOUT limits registers are
> > -              * provided in a direct format. In case format is VID - convert
> > -              * to direct.
> > -              */
> > -             if (data->vout_format[page] == vid)
> > -                     ret = mp2975_vid2direct(info->vrm_version[page], ret);
> > -             break;
> >       case PMBUS_VIRT_READ_POUT_MAX:
> >               ret = mp2975_read_word_helper(client, page, phase,
> >                                             MP2975_MFR_READ_POUT_PK,
> > @@ -578,20 +543,18 @@ mp2975_vout_max_get(struct i2c_client *client, struct mp2975_data *data,
> >   }
> >
> >   static int
> > -mp2975_identify_vout_format(struct i2c_client *client,
> > -                         struct mp2975_data *data, int page)
> > +mp2975_set_vout_format(struct i2c_client *client,
> > +                    struct mp2975_data *data, int page)
> >   {
> >       int ret;
> >
> >       ret = i2c_smbus_read_word_data(client, MP2975_MFR_DC_LOOP_CTRL);
> >       if (ret < 0)
> >               return ret;
> > -
> > -     if (ret & MP2975_VOUT_FORMAT)
> > -             data->vout_format[page] = vid;
> > -     else
> > -             data->vout_format[page] = direct;
> > -     return 0;
> > +     /* Enable DIRECT VOUT format 1mV/LSB */
> > +     ret &= ~MP2975_VOUT_FORMAT;
> > +     ret = i2c_smbus_write_word_data(client, MP2975_MFR_DC_LOOP_CTRL, ret);
>
> Writing this back is only needed if MP2975_VOUT_FORMAT was not already cleared.
Yes. Will optimize it as:
if (ret & MP2975_VOUT_FORMAT) {
ret &= ~MP2975_VOUT_FORMAT;
ret = i2c_smbus_write_word_data(client, MP2975_MFR_DC_LOOP_CTRL, ret);
}


>
> > +     return ret;
> >   }
> >
> >   static int
> > @@ -650,11 +613,11 @@ mp2975_vout_per_rail_config_get(struct i2c_client *client,
> >                       return ret;
> >
> >               /*
> > -              * Get VOUT format for READ_VOUT command : VID or direct.
> > +              * Set VOUT format for READ_VOUT command : direct.
> >                * Pages on same device can be configured with different
> >                * formats.
>
> Not sure if this comment still makes sense.
Yes. Updated the comment as below:
/* Set VOUT format for READ_VOUT command : direct. */
ret = mp2975_set_vout_format(....

>
> >                */
> > -             ret = mp2975_identify_vout_format(client, data, i);
> > +             ret = mp2975_set_vout_format(client, data, i);
> >               if (ret < 0)
> >                       return ret;
> >
> > @@ -689,7 +652,6 @@ static struct pmbus_driver_info mp2975_info = {
> >               PMBUS_HAVE_IIN | PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT |
> >               PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP | PMBUS_HAVE_POUT |
> >               PMBUS_HAVE_PIN | PMBUS_HAVE_STATUS_INPUT | PMBUS_PHASE_VIRTUAL,
> > -     .read_byte_data = mp2975_read_byte_data,
> >       .read_word_data = mp2975_read_word_data,
> >   };
> >
>

Regards,
Naresh Solanki