Re: [PATCH v5] hwmon: (pmbus) Add support for MPS MP2949A

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 29 Jun 2021 at 10:20, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> On 6/28/21 6:26 PM, Ainux Wang wrote:
> > On Thu, 24 Jun 2021 at 21:17, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> >>
> >> On Wed, Jun 23, 2021 at 08:54:26AM +0800, ainux.wang@xxxxxxxxx wrote:
> >>> From: "Ainux.Wang" <ainux.wang@xxxxxxxxx>
> >>>
> >>> Add support for MP2949A device from Monolithic Power Systems, Inc. (MPS).
> >>> This is a triple-loop, digital, multi-phase controller.
> >>> This device:
> >>> - Supports up to three power rail.
> >>> - Provides 6 pulse-width modulations (PWMs), and can be configured up
> >>>    to 6-phase operation for Rail A , up to 2-phase operation for Rail B
> >>>    and up to 1-phase operation for Rail C.
> >>> - The PMBus registers are distributed into three pages: Page 0, Page 1,
> >>>    Page 2. Page 0 contains the registers for Rail A and most of the common
> >>>    settings for all of the rails. Page 1 contains register information for
> >>>    Rail B. Page 2 contains register information for Rail C.
> >>> - The MP2949A supports both 5mV VID step and 10mv VID step for IMVP8 and
> >>>    IMVP9.
> >>>
> >>> Signed-off-by: Ainux.Wang <ainux.wang@xxxxxxxxx>
> >>> ---
> >>> v5:
> >>> - Moved change log to right here.
> >>> v4:
> >>> - Removed mp2949a_read_byte_data().
> >>
> >> Your other question left me confused. I had previously asked to provide
> >> a rationale for filtering out the PMBUS_VOUT_MODE command, and I had
> >> stated that "the chip does not support it" is not a valid reason. However,
> >> "the chip does not support it but does not report an error when reading
> >> it" _is_ a valid reason. So what happens when the PMBus core reads
> >> PMBUS_VOUT_MODE ? Does the chip return an error, or some random data ?
> >>
> >> Thanks,
> >> Guenter
> >>
> > Hi, Guenter
> >
> > I have not been clear about the cause of this problem that
> > "the chip does not support it is not a valid reason".
> >
> > However, i have added some "printk", i found the chip will return some
> > random data,
> > when the PMbus core reads PMBUS_STATUS and PMBUS_VOUT_MODE.
> >
> > So, I do not known what should i do, when the PMbus core reads PMBUS_STATUS.
> > and i have knwon that use "This chip do not support the VOUT_MODE command,
> > the chip does not support it but return some random data when reading
> > " instead of
> > "
> >                 /*
> >                  * This chip do not support the VOUT_MODE command.
> >                  * There is not VOUT_MODE command in MP2949A datasheet P29~P31.
> >                  * So this is EINVAL in here.
> >                 */
> > "
> > Now, there is only one question left, what should the PMbus do, the
> > chip return random data,
> > when it reads PMBUS_STATUS by i2c_smbus_read_word_data() or
> > i2c_smbus_read_byte_data()?
> > Can the PMbus core use  pmbus_read_status_byte() and
> > pmbus_read_status_word() instead of
> > i2c_smbus_read_byte_data() and i2c_smbus_read_word_data() ?
> >
>
> The driver should implement a read_byte_data() function and either
> simulate PMBUS_VOUT_MODE and let it return whatever makes sense for
> the driver, or have it return -EINVAL.
>
> For the status register, handling is a bit more difficult. For this, we will
> need an introductory patch. That patch needs to change pmbus_init_common()
> to call pmbus_read_status_word() and pmbus_read_status_byte() instead
> of i2c_smbus_read_word_data() and i2c_smbus_read_byte_data() (with page set
> to -1).
> The driver patch then needs to implement a read_word_data() function and
> have it simulate the PMBUS_STATUS_WORD command. Or, alternatively,
> have the read_word_data() function return -EINVAL for PMBUS_STATUS_WORD,
> and simulate PMBUS_STATUS_BYTE in the read_byte_data callback.
>
> Thanks,
> Guenter
Hi, Guenter

Very nice, that is what i mean, we have the same idea, I am going to
revise pmbus_init_common(),
pmbus_check_register and so on.

Best regards,
Ainux Wang.



[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux