Re: Enabling pmbus power control

[Guenter Roeck]

On 4/20/21 12:06 AM, Zev Weiss wrote:
> On Tue, Apr 20, 2021 at 01:00:25AM CDT, Guenter Roeck wrote:
>> On 4/19/21 10:50 PM, Zev Weiss wrote:
>> [ ... ]
>>
>>> I had a glance at the enclosure driver; it looks pretty geared toward SES-like things (drivers/scsi/ses.c being its only usage I can see in the kernel at the moment) and while it could perhaps be pressed into working for this it seems like it would probably drag in a fair amount of boilerplate and result in a somewhat gratuitously confusing driver arrangement (calling the things involved in the cases we're looking at "enclosures" seems like a bit of a stretch).
>>>
>>> As an alternative, would something like the patch below be more along the lines of what you're suggesting?  And if so, would it make sense to generalize it into something like 'pmbus-switch.c' and add a PMBUS_HAVE_POWERSWITCH functionality bit or similar in the pmbus code instead of hardcoding it for only LM25066 support?
>>>
>>>
>>
>> No. Don't access pmbus functions from outside drivers/hwmon/pmbus.
>>
>> I used to be opposed to function export restrictions (aka export namespaces),
>> but you are making a good case that we need to introduce them for pmbus
>> functions.
>>
>> Guenter
> 
> Okay -- I figured that was likely to be frowned upon, but the alternative seemed to be effectively duplicating non-trivial chunks of the pmbus code.  However, upon realizing that the LM25066 doesn't actually require any of the paging work the generic pmbus code provides, I suppose it can actually be done with a simple smbus read & write.  Does this version look better?
> 

It is just getting worse and worse. You are re-implementing regulator
support for the lm25066. The correct solution would be to implement
regulator support in the lm25066 and use it from the secondary driver
(which should be chip independent).

Guenter

> 
> Zev
> 
> 
> From 1662e1c59c498ad6b208e6ab450bd467d71def34 Mon Sep 17 00:00:00 2001
> From: Zev Weiss <zev@xxxxxxxxxxxxxxxxx>
> Date: Wed, 31 Mar 2021 01:58:35 -0500
> Subject: [PATCH] misc: add lm25066-switch driver
> 
> This driver allows an lm25066 to be switched on and off from userspace
> via sysfs.
> 
> Signed-off-by: Zev Weiss <zev@xxxxxxxxxxxxxxxxx>
> ---
>  drivers/misc/Kconfig          |   7 ++
>  drivers/misc/Makefile         |   1 +
>  drivers/misc/lm25066-switch.c | 126 ++++++++++++++++++++++++++++++++++
>  3 files changed, 134 insertions(+)
>  create mode 100644 drivers/misc/lm25066-switch.c
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index f532c59bb59b..384b6022ec15 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -445,6 +445,13 @@ config HISI_HIKEY_USB
>        switching between the dual-role USB-C port and the USB-A host ports
>        using only one USB controller.
>  
> +config LM25066_SWITCH
> +    tristate "LM25066 power switch support"
> +    depends on OF && SENSORS_LM25066
> +    help
> +      This driver augments LM25066 hwmon support with power switch
> +      functionality controllable from userspace via sysfs.
> +
>  source "drivers/misc/c2port/Kconfig"
>  source "drivers/misc/eeprom/Kconfig"
>  source "drivers/misc/cb710/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 99b6f15a3c70..c948510d0cc9 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -56,3 +56,4 @@ obj-$(CONFIG_HABANA_AI)        += habanalabs/
>  obj-$(CONFIG_UACCE)        += uacce/
>  obj-$(CONFIG_XILINX_SDFEC)    += xilinx_sdfec.o
>  obj-$(CONFIG_HISI_HIKEY_USB)    += hisi_hikey_usb.o
> +obj-$(CONFIG_LM25066_SWITCH)    += lm25066-switch.o
> diff --git a/drivers/misc/lm25066-switch.c b/drivers/misc/lm25066-switch.c
> new file mode 100644
> index 000000000000..9adc67c320f9
> --- /dev/null
> +++ b/drivers/misc/lm25066-switch.c
> @@ -0,0 +1,126 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * This module provides a thin wrapper around the lm25066 hwmon driver that
> + * additionally exposes a userspace-controllable on/off power switch via
> + * sysfs.
> + *
> + * Author: Zev Weiss <zweiss@xxxxxxxxxxx>
> + *
> + * Copyright (C) 2021 Equinix Services, Inc.
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/i2c.h>
> +#include <linux/platform_device.h>
> +
> +/*
> + * The relevant PMBus command and data values for controlling the LM25066
> + * power state.  Because it's not a paged device we skip the usual paging
> + * business other PMBus devices might require.
> + */
> +#define CMD_OPERATION 0x01
> +#define OPERATION_ON 0x80
> +#define OPERATION_OFF 0x00
> +
> +static ssize_t switch_show_state(struct device *dev, struct device_attribute *attr,
> +                                 char *buf)
> +{
> +    struct i2c_client *pmic = dev_get_drvdata(dev);
> +    ssize_t ret = i2c_smbus_read_byte_data(pmic, CMD_OPERATION);
> +    if (ret < 0)
> +        return ret;
> +
> +    return sysfs_emit(buf, "%s\n", (ret & OPERATION_ON) ? "on" : "off");
> +}
> +
> +static ssize_t switch_set_state(struct device *dev, struct device_attribute *attr,
> +                                const char *buf, size_t count)
> +{
> +    int status;
> +    u8 value;
> +    struct i2c_client *pmic = dev_get_drvdata(dev);
> +    if (sysfs_streq(buf, "on"))
> +        value = OPERATION_ON;
> +    else if (sysfs_streq(buf, "off"))
> +        value = OPERATION_OFF;
> +    else
> +        return -EINVAL;
> +    status = i2c_smbus_write_byte_data(pmic, CMD_OPERATION, value);
> +    return status ? : count;
> +}
> +
> +static DEVICE_ATTR(state, 0644, switch_show_state, switch_set_state);
> +
> +static struct attribute *attributes[] = {
> +    &dev_attr_state.attr,
> +    NULL,
> +};
> +
> +static const struct attribute_group attr_group = {
> +    .attrs = attributes,
> +};
> +
> +static int lm25066_switch_probe(struct platform_device *pdev)
> +{
> +    int status;
> +    struct device_node *np = pdev->dev.of_node;
> +    struct device_node *pmic_np;
> +    struct i2c_client *pmic;
> +
> +    pmic_np = of_parse_phandle(np, "pmic", 0);
> +    if (!pmic_np) {
> +        dev_err(&pdev->dev, "Cannot parse lm25066-switch pmic\n");
> +        return -ENODEV;
> +    }
> +
> +    if (!of_device_is_compatible(pmic_np, "lm25066")) {
> +        dev_err(&pdev->dev, "lm25066-switch pmic not lm25066 compatible");
> +        status = -ENODEV;
> +        goto out;
> +    }
> +
> +    pmic = of_find_i2c_device_by_node(pmic_np);
> +    if (!pmic) {
> +        status = -EPROBE_DEFER;
> +        goto out;
> +    }
> +
> +    platform_set_drvdata(pdev, pmic);
> +
> +    status = sysfs_create_group(&pdev->dev.kobj, &attr_group);
> +
> +out:
> +    of_node_put(pmic_np);
> +    return status;
> +}
> +
> +static int lm25066_switch_remove(struct platform_device *pdev)
> +{
> +    struct i2c_client *pmic = platform_get_drvdata(pdev);
> +
> +    sysfs_remove_group(&pdev->dev.kobj, &attr_group);
> +    put_device(&pmic->dev);
> +
> +    return 0;
> +}
> +
> +static const struct of_device_id lm25066_switch_table[] = {
> +    { .compatible = "lm25066-switch" },
> +    { },
> +};
> +
> +static struct platform_driver lm25066_switch_driver = {
> +    .driver = {
> +        .name = "lm25066-switch",
> +        .of_match_table = lm25066_switch_table,
> +    },
> +    .probe = lm25066_switch_probe,
> +    .remove = lm25066_switch_remove,
> +};
> +
> +module_platform_driver(lm25066_switch_driver);
> +
> +MODULE_AUTHOR("Zev Weiss <zweiss@xxxxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("LM25066 power-switch driver");