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;
Sender: Guenter Roeck <groeck7@gmail.com>
Message-ID: <38157958-9723-73bc-f79a-1ac8c80c512e@roeck-us.net>
Date:   Wed, 12 Jul 2023 08:49:58 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Content-Language: en-US
To:     Naresh Solanki <naresh.solanki@9elements.com>,
        devicetree@vger.kernel.org, Jean Delvare <jdelvare@suse.com>
Cc:     linux-kernel@vger.kernel.org, linux-hwmon@vger.kernel.org,
        Patrick Rudolph <patrick.rudolph@9elements.com>
References: <20230712114754.500477-1-Naresh.Solanki@9elements.com>
 <20230712114754.500477-4-Naresh.Solanki@9elements.com>
From:   Guenter Roeck <linux@roeck-us.net>
Subject: Re: [PATCH 4/8] hwmon: (pmbus/mp2975) Simplify VOUT code
In-Reply-To: <20230712114754.500477-4-Naresh.Solanki@9elements.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.

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

>   		 */
> -		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,
>   };
>