RE: [RFC PATCH hwmon-next v1 5/5] hwmon: (pmbus/tps53679) Extend device list supported by driver

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

 




> -----Original Message-----
> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
> Sent: Sunday, January 05, 2020 8:35 PM
> To: Vadim Pasternak <vadimp@xxxxxxxxxxxx>; robh+dt@xxxxxxxxxx;
> vijaykhemka@xxxxxx
> Cc: linux-hwmon@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> Subject: Re: [RFC PATCH hwmon-next v1 5/5] hwmon: (pmbus/tps53679) Extend
> device list supported by driver
> 
> On 1/5/20 8:44 AM, Vadim Pasternak wrote:
> >
> >
> >> -----Original Message-----
> >> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
> >> Sent: Sunday, January 05, 2020 6:04 PM
> >> To: Vadim Pasternak <vadimp@xxxxxxxxxxxx>; robh+dt@xxxxxxxxxx;
> >> vijaykhemka@xxxxxx
> >> Cc: linux-hwmon@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> >> Subject: Re: [RFC PATCH hwmon-next v1 5/5] hwmon: (pmbus/tps53679)
> >> Extend device list supported by driver
> >>
> >> On 1/5/20 2:58 AM, Vadim Pasternak wrote:
> >>> Extends driver with support of the additional devices:
> >>> Texas Instruments Dual channel DCAP+ multiphase controllers:
> >>> TPS53688, SN1906016.
> >>> Infineon Multi-phase Digital VR Controller Sierra devices
> >>> XDPE12286C, XDPE12284C, XDPE12283C, XDPE12254C and XDPE12250C.
> >>>
> >>> Extend Kconfig with added devices.
> >>>
> >>> Signed-off-by: Vadim Pasternak <vadimp@xxxxxxxxxxxx>
> >>> ---
> >>>    drivers/hwmon/pmbus/Kconfig    |  5 +++--
> >>>    drivers/hwmon/pmbus/tps53679.c | 14 ++++++++++++++
> >>>    2 files changed, 17 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/hwmon/pmbus/Kconfig
> >>> b/drivers/hwmon/pmbus/Kconfig index 59859979571d..9e3d197d5322
> >>> 100644
> >>> --- a/drivers/hwmon/pmbus/Kconfig
> >>> +++ b/drivers/hwmon/pmbus/Kconfig
> >>> @@ -200,10 +200,11 @@ config SENSORS_TPS40422
> >>>    	  be called tps40422.
> >>>
> >>>    config SENSORS_TPS53679
> >>> -	tristate "TI TPS53679"
> >>> +	tristate "TI TPS53679, TPS53688, SN1906016, Infineon XDPE122xxx
> >> family"
> >>>    	help
> >>>    	  If you say yes here you get hardware monitoring support for TI
> >>> -	  TPS53679.
> >>> +	  TPS53679, PS53688, SN1906016 and Infineon XDPE12286C,
> >> XDPE12284C,
> >>
> >> TPS53688. For the others, for some I can't even determine if they
> >> exist in the first place (eg SN1906016, XPDE12250C) or how they would
> >> differ from other variants (eg XPDE12284C vs. XPDE12284A).
> >> And why would they all use the same bit map in the VOUT_MODE
> >> register, the same number of PMBus pages (phases), and the same attributes
> in each page ?
> >
> > Hi Guenter,
> >
> > Thank you for reply.
> >
> > On our new system we have device XPDE12284C equipped.
> > I tested this device.
> >
> Sounds good, but did you also make sure that all chips have the same number of
> pages (phases), the same set of commands as the TI chip, and support the same
> bit settings in VOUT_MODE ? It seems a bit unlikely that TI's register definitions
> would make it into an Infineon chip.
> 
> Also, what about the SN1906016 ? I don't find that anywhere, except in one
> place where it is listed as MCU from TI.

I'll drop SN1906016.
Datasheet has a title Dual channel DCAP+ multiphase controllers:
TPS53688, SN1906016.
But maybe it's some custom device (anyway I'll try to check it with TI).

> 
> > Infineon datasheet refers all these device as XDPE122xxC family and it
> > doesn't specify any differences in register map between these devices.
> 
> That is a bit vague, especially when it includes devices which return zero results
> with Google searches.
> 
> "A" vs. "C" may distinguish automotive vs. commercial; the "A" device is listed
> under automotive. If the command set is the same, I don't really want the "c" in
> the id.

Got feedback from Infineon guys.
No need 'C' at the end, as you wrote.
All XDPE12250, XDPE12254, XDPE12283, XDPE12284, XDPE12286 are
treated in the same way:
same pages, same VOUT_MODE, VOUT_READ, etcetera.

> 
> > Tomorrow we'll have guys from Infineon in our lab and I'll verify if
> > there is any difference.
> 
> Tell them that it isn't really helpful to keep their datasheets under wrap.
> Unfortunately, TI started doing the same, which isn't helpful either.

Told them about datasheets availability - got :)

> 
> Thanks,
> Guenter
> 
> > If yes, I'll leave only XPDE12284C.
> >
> >>
> >> Thanks,
> >> Guenter
> >>
> >>> +	  XDPE12283C, XDPE12254C, XDPE12250C devices.
> >>>
> >>>    	  This driver can also be built as a module. If so, the module will
> >>>    	  be called tps53679.
> >>> diff --git a/drivers/hwmon/pmbus/tps53679.c
> >>> b/drivers/hwmon/pmbus/tps53679.c index 7ce2fca4acde..f38eb116735b
> >>> 100644
> >>> --- a/drivers/hwmon/pmbus/tps53679.c
> >>> +++ b/drivers/hwmon/pmbus/tps53679.c
> >>> @@ -89,6 +89,13 @@ static int tps53679_probe(struct i2c_client
> >>> *client,
> >>>
> >>>    static const struct i2c_device_id tps53679_id[] = {
> >>>    	{"tps53679", 0},
> >>> +	{"tps53688", 0},
> >>> +	{"sn1906016", 0},
> >>> +	{"xdpe12283c", 0},
> >>> +	{"xdpe12250c", 0},
> >>> +	{"xdpe12254c", 0},
> >>> +	{"xdpe12284c", 0},
> >>> +	{"xdpe12286c", 0},
> >>
> >> Alphabetic order, please.
> >>
> >>>    	{}
> >>>    };
> >>>
> >>> @@ -96,6 +103,13 @@ MODULE_DEVICE_TABLE(i2c, tps53679_id);
> >>>
> >>>    static const struct of_device_id __maybe_unused tps53679_of_match[] =
> {
> >>>    	{.compatible = "ti,tps53679"},
> >>> +	{.compatible = "ti,tps53688"},
> >>> +	{.compatible = "ti,sn1906016"},
> >>> +	{.compatible = "infineon,xdpe12283c"},
> >>> +	{.compatible = "infineon,xdpe12250c"},
> >>> +	{.compatible = "infineon,xdpe12254c"},
> >>> +	{.compatible = "infineon,xdpe12284c"},
> >>> +	{.compatible = "infineon,xdpe12286c"},
> >>>    	{}
> >>>    };
> >>>    MODULE_DEVICE_TABLE(of, tps53679_of_match);
> >>>
> >





[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