diff mbox series

pmbus based power regulator

Message ID b1a69448-66bd-6760-21c6-239ef2afecdf@xilinx.com (mailing list archive)
State Not Applicable
Headers show
Series pmbus based power regulator | expand

Commit Message

Michal Simek April 9, 2019, 12:56 p.m. UTC
Hi,

I have one question about hwmon/pmbus. I have tps544b25 on the board. I
have enabled this chip via DT to get probed. Patch below.

I can't see any issue with monitoring but I am curious how to enable
setting up voltage. I expect this should be moved to regulator folder or
better split done as MFD device.

I see there wm831x-hwmon and also wm8350-hwmon but nothing with pmbus
wiring.
Can you please suggest a way how this should be done?

Thanks,
Michal

commit 0aeba3b5a67eee06d3afe67c71e45a47e8e00f8d
Author:     Michal Simek <michal.simek@xilinx.com>
AuthorDate: Mon Mar 4 13:51:05 2019 +0100
Commit:     Michal Simek <michal.simek@xilinx.com>
CommitDate: Thu Mar 28 15:32:03 2019 +0100

    hwmon: pmbus: Add DT wiring for ti,tps544b25

    This should be enough for DT wiring to get generic functionality.

    Signed-off-by: Michal Simek <michal.simek@xilinx.com>

Comments

Guenter Roeck April 9, 2019, 1:05 p.m. UTC | #1
On 4/9/19 5:56 AM, Michal Simek wrote:
> Hi,
> 
> I have one question about hwmon/pmbus. I have tps544b25 on the board. I
> have enabled this chip via DT to get probed. Patch below.
> 
> I can't see any issue with monitoring but I am curious how to enable
> setting up voltage. I expect this should be moved to regulator folder or
> better split done as MFD device.
> 
> I see there wm831x-hwmon and also wm8350-hwmon but nothing with pmbus
> wiring.
> Can you please suggest a way how this should be done?
> 
> Thanks,
> Michal
> 
> commit 0aeba3b5a67eee06d3afe67c71e45a47e8e00f8d
> Author:     Michal Simek <michal.simek@xilinx.com>
> AuthorDate: Mon Mar 4 13:51:05 2019 +0100
> Commit:     Michal Simek <michal.simek@xilinx.com>
> CommitDate: Thu Mar 28 15:32:03 2019 +0100
> 
>      hwmon: pmbus: Add DT wiring for ti,tps544b25
> 
>      This should be enough for DT wiring to get generic functionality.
> 
>      Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> 
> diff --git a/drivers/hwmon/pmbus/pmbus.c b/drivers/hwmon/pmbus/pmbus.c
> index 7718e58dbda5..be00cd144a2b 100644
> --- a/drivers/hwmon/pmbus/pmbus.c
> +++ b/drivers/hwmon/pmbus/pmbus.c
> @@ -220,10 +220,17 @@ static int pmbus_probe(struct i2c_client *client,
> 
>   MODULE_DEVICE_TABLE(i2c, pmbus_id);
> 
> +static const struct of_device_id pmbus_of_match[] = {
> +        {.compatible = "ti,tps544b25"},
> +        {}
> +};
> +MODULE_DEVICE_TABLE(of, pmbus_of_match);
> +
>   /* This is the driver that will be inserted */
>   static struct i2c_driver pmbus_driver = {
>          .driver = {
>                     .name = "pmbus",
> +                  .of_match_table = of_match_ptr(pmbus_of_match),
>                     },
>          .probe = pmbus_probe,
>          .remove = pmbus_do_remove,
>
Michal Simek April 9, 2019, 1:07 p.m. UTC | #2
On 09. 04. 19 15:05, Guenter Roeck wrote:
> On 4/9/19 5:56 AM, Michal Simek wrote:
>> Hi,
>>
>> I have one question about hwmon/pmbus. I have tps544b25 on the board. I
>> have enabled this chip via DT to get probed. Patch below.
>>
>> I can't see any issue with monitoring but I am curious how to enable
>> setting up voltage. I expect this should be moved to regulator folder or
>> better split done as MFD device.
>>
>> I see there wm831x-hwmon and also wm8350-hwmon but nothing with pmbus
>> wiring.
>> Can you please suggest a way how this should be done?
>>
>> Thanks,
>> Michal
>>
>> commit 0aeba3b5a67eee06d3afe67c71e45a47e8e00f8d
>> Author:     Michal Simek <michal.simek@xilinx.com>
>> AuthorDate: Mon Mar 4 13:51:05 2019 +0100
>> Commit:     Michal Simek <michal.simek@xilinx.com>
>> CommitDate: Thu Mar 28 15:32:03 2019 +0100
>>
>>      hwmon: pmbus: Add DT wiring for ti,tps544b25
>>
>>      This should be enough for DT wiring to get generic functionality.
>>
>>      Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>
>> diff --git a/drivers/hwmon/pmbus/pmbus.c b/drivers/hwmon/pmbus/pmbus.c
>> index 7718e58dbda5..be00cd144a2b 100644
>> --- a/drivers/hwmon/pmbus/pmbus.c
>> +++ b/drivers/hwmon/pmbus/pmbus.c
>> @@ -220,10 +220,17 @@ static int pmbus_probe(struct i2c_client *client,
>>
>>   MODULE_DEVICE_TABLE(i2c, pmbus_id);
>>
>> +static const struct of_device_id pmbus_of_match[] = {
>> +        {.compatible = "ti,tps544b25"},
>> +        {}
>> +};
>> +MODULE_DEVICE_TABLE(of, pmbus_of_match);
>> +
>>   /* This is the driver that will be inserted */
>>   static struct i2c_driver pmbus_driver = {
>>          .driver = {
>>                     .name = "pmbus",
>> +                  .of_match_table = of_match_ptr(pmbus_of_match),
>>                     },
>>          .probe = pmbus_probe,
>>          .remove = pmbus_do_remove,
>>
> 

no response?

M
Guenter Roeck April 9, 2019, 1:20 p.m. UTC | #3
On 4/9/19 5:56 AM, Michal Simek wrote:
> Hi,
> 
> I have one question about hwmon/pmbus. I have tps544b25 on the board. I
> have enabled this chip via DT to get probed. Patch below.
> 
> I can't see any issue with monitoring but I am curious how to enable
> setting up voltage. I expect this should be moved to regulator folder or
> better split done as MFD device.
> 
> I see there wm831x-hwmon and also wm8350-hwmon but nothing with pmbus
> wiring.
> Can you please suggest a way how this should be done?
> 

Hit the wrong button with my earlier email. Please ignore.

The pmbus core code does support for registering regulators. See
drivers/hwmon/pmbus/ltc2978.c for an example. You would have to write a
front-end driver for the TI chip to pass the necessary parameters to the
pmbus core. We could try to add generic regulator support to pmbus.c, but
I hesitate doing that because regulator support is much more critical than
monitoring code and may require chip specific workarounds.

Sure, we could try to move the pmbus core code to mfd and try to split out
regulator and hwmon code from it. That would require moving the core plus
all front-end drivers. It would be a substantial effort with, as far as I
can see, little benefit.

> Thanks,
> Michal
> 
> commit 0aeba3b5a67eee06d3afe67c71e45a47e8e00f8d
> Author:     Michal Simek <michal.simek@xilinx.com>
> AuthorDate: Mon Mar 4 13:51:05 2019 +0100
> Commit:     Michal Simek <michal.simek@xilinx.com>
> CommitDate: Thu Mar 28 15:32:03 2019 +0100
> 
>      hwmon: pmbus: Add DT wiring for ti,tps544b25
> 
>      This should be enough for DT wiring to get generic functionality.
> 
>      Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> 
> diff --git a/drivers/hwmon/pmbus/pmbus.c b/drivers/hwmon/pmbus/pmbus.c
> index 7718e58dbda5..be00cd144a2b 100644
> --- a/drivers/hwmon/pmbus/pmbus.c
> +++ b/drivers/hwmon/pmbus/pmbus.c
> @@ -220,10 +220,17 @@ static int pmbus_probe(struct i2c_client *client,
> 
>   MODULE_DEVICE_TABLE(i2c, pmbus_id);
> 
> +static const struct of_device_id pmbus_of_match[] = {
> +        {.compatible = "ti,tps544b25"},
> +        {}
> +};
> +MODULE_DEVICE_TABLE(of, pmbus_of_match);
> +
>   /* This is the driver that will be inserted */
>   static struct i2c_driver pmbus_driver = {
>          .driver = {
>                     .name = "pmbus",
> +                  .of_match_table = of_match_ptr(pmbus_of_match),
>                     },
>          .probe = pmbus_probe,
>          .remove = pmbus_do_remove,
> 

tps544b25 is already listed in const struct i2c_device_id pmbus_id,
and the driver should thus instantiate with DT. I don't mind if we add
a complete of_match table to the file, but I don't really see the point
of creating one with just a single entry.

Guenter
Guenter Roeck April 9, 2019, 1:21 p.m. UTC | #4
On 4/9/19 6:07 AM, Michal Simek wrote:
> On 09. 04. 19 15:05, Guenter Roeck wrote:
>> On 4/9/19 5:56 AM, Michal Simek wrote:
>>> Hi,
>>>
>>> I have one question about hwmon/pmbus. I have tps544b25 on the board. I
>>> have enabled this chip via DT to get probed. Patch below.
>>>
>>> I can't see any issue with monitoring but I am curious how to enable
>>> setting up voltage. I expect this should be moved to regulator folder or
>>> better split done as MFD device.
>>>
>>> I see there wm831x-hwmon and also wm8350-hwmon but nothing with pmbus
>>> wiring.
>>> Can you please suggest a way how this should be done?
>>>
>>> Thanks,
>>> Michal
>>>
>>> commit 0aeba3b5a67eee06d3afe67c71e45a47e8e00f8d
>>> Author:     Michal Simek <michal.simek@xilinx.com>
>>> AuthorDate: Mon Mar 4 13:51:05 2019 +0100
>>> Commit:     Michal Simek <michal.simek@xilinx.com>
>>> CommitDate: Thu Mar 28 15:32:03 2019 +0100
>>>
>>>       hwmon: pmbus: Add DT wiring for ti,tps544b25
>>>
>>>       This should be enough for DT wiring to get generic functionality.
>>>
>>>       Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>>
>>> diff --git a/drivers/hwmon/pmbus/pmbus.c b/drivers/hwmon/pmbus/pmbus.c
>>> index 7718e58dbda5..be00cd144a2b 100644
>>> --- a/drivers/hwmon/pmbus/pmbus.c
>>> +++ b/drivers/hwmon/pmbus/pmbus.c
>>> @@ -220,10 +220,17 @@ static int pmbus_probe(struct i2c_client *client,
>>>
>>>    MODULE_DEVICE_TABLE(i2c, pmbus_id);
>>>
>>> +static const struct of_device_id pmbus_of_match[] = {
>>> +        {.compatible = "ti,tps544b25"},
>>> +        {}
>>> +};
>>> +MODULE_DEVICE_TABLE(of, pmbus_of_match);
>>> +
>>>    /* This is the driver that will be inserted */
>>>    static struct i2c_driver pmbus_driver = {
>>>           .driver = {
>>>                      .name = "pmbus",
>>> +                  .of_match_table = of_match_ptr(pmbus_of_match),
>>>                      },
>>>           .probe = pmbus_probe,
>>>           .remove = pmbus_do_remove,
>>>
>>
> 
> no response?
> 

fat finger. Sorry.

Guenter
Michal Simek April 10, 2019, 10:25 a.m. UTC | #5
On 09. 04. 19 15:20, Guenter Roeck wrote:
> On 4/9/19 5:56 AM, Michal Simek wrote:
>> Hi,
>>
>> I have one question about hwmon/pmbus. I have tps544b25 on the board. I
>> have enabled this chip via DT to get probed. Patch below.
>>
>> I can't see any issue with monitoring but I am curious how to enable
>> setting up voltage. I expect this should be moved to regulator folder or
>> better split done as MFD device.
>>
>> I see there wm831x-hwmon and also wm8350-hwmon but nothing with pmbus
>> wiring.
>> Can you please suggest a way how this should be done?
>>
> 
> Hit the wrong button with my earlier email. Please ignore.
> 
> The pmbus core code does support for registering regulators. See
> drivers/hwmon/pmbus/ltc2978.c for an example. You would have to write a
> front-end driver for the TI chip to pass the necessary parameters to the
> pmbus core. We could try to add generic regulator support to pmbus.c, but
> I hesitate doing that because regulator support is much more critical than
> monitoring code and may require chip specific workarounds.
> 
> Sure, we could try to move the pmbus core code to mfd and try to split out
> regulator and hwmon code from it. That would require moving the core plus
> all front-end drivers. It would be a substantial effort with, as far as I
> can see, little benefit.

ok. It means use pmbus functions from pmbus_regulator_ops and add
missing would be the way to go.

const struct regulator_ops pmbus_regulator_ops = {
	.enable = pmbus_regulator_enable,
	.disable = pmbus_regulator_disable,
	.is_enabled = pmbus_regulator_is_enabled,
};

The way how it is connected on the board is that power regulator has
gpio pin for enabling/disabling and also monitoring feature.
And this is just enabling power for different chip.

Thanks,
Michal
diff mbox series

Patch

diff --git a/drivers/hwmon/pmbus/pmbus.c b/drivers/hwmon/pmbus/pmbus.c
index 7718e58dbda5..be00cd144a2b 100644
--- a/drivers/hwmon/pmbus/pmbus.c
+++ b/drivers/hwmon/pmbus/pmbus.c
@@ -220,10 +220,17 @@  static int pmbus_probe(struct i2c_client *client,

 MODULE_DEVICE_TABLE(i2c, pmbus_id);

+static const struct of_device_id pmbus_of_match[] = {
+        {.compatible = "ti,tps544b25"},
+        {}
+};
+MODULE_DEVICE_TABLE(of, pmbus_of_match);
+
 /* This is the driver that will be inserted */
 static struct i2c_driver pmbus_driver = {
        .driver = {
                   .name = "pmbus",
+                  .of_match_table = of_match_ptr(pmbus_of_match),
                   },
        .probe = pmbus_probe,
        .remove = pmbus_do_remove,