lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20210703171227.77e36483@jic23-huawei>
Date:   Sat, 3 Jul 2021 17:12:27 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Andreas Kemnade <andreas@...nade.info>
Cc:     lee.jones@...aro.org, robh+dt@...nel.org, lars@...afoo.de,
        sre@...nel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
        linux-pm@...r.kernel.org, leonard.crestez@....com,
        letux-kernel@...nphoenux.org
Subject: Re: [PATCH 4/4] power: supply: rn5t618: Add voltage_now property

On Sat,  3 Jul 2021 10:42:24 +0200
Andreas Kemnade <andreas@...nade.info> wrote:

> Read voltage_now via IIO and provide the property.
> 
> Signed-off-by: Andreas Kemnade <andreas@...nade.info>

Hi Andreas,

One comment inline, but looks fine to me in general.

> ---
>  drivers/power/supply/rn5t618_power.c | 56 ++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/drivers/power/supply/rn5t618_power.c b/drivers/power/supply/rn5t618_power.c
> index 819061918b2a..b062208c8a91 100644
> --- a/drivers/power/supply/rn5t618_power.c
> +++ b/drivers/power/supply/rn5t618_power.c
> @@ -9,10 +9,12 @@
>  #include <linux/device.h>
>  #include <linux/bitops.h>
>  #include <linux/errno.h>
> +#include <linux/iio/consumer.h>
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
>  #include <linux/mfd/rn5t618.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/power_supply.h>
>  #include <linux/regmap.h>
> @@ -64,6 +66,8 @@ struct rn5t618_power_info {
>  	struct power_supply *battery;
>  	struct power_supply *usb;
>  	struct power_supply *adp;
> +	struct iio_channel *channel_vusb;
> +	struct iio_channel *channel_vadp;
>  	int irq;
>  };
>  
> @@ -77,6 +81,7 @@ static enum power_supply_usb_type rn5t618_usb_types[] = {
>  static enum power_supply_property rn5t618_usb_props[] = {
>  	/* input current limit is not very accurate */
>  	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
>  	POWER_SUPPLY_PROP_STATUS,
>  	POWER_SUPPLY_PROP_USB_TYPE,
>  	POWER_SUPPLY_PROP_ONLINE,
> @@ -85,6 +90,7 @@ static enum power_supply_property rn5t618_usb_props[] = {
>  static enum power_supply_property rn5t618_adp_props[] = {
>  	/* input current limit is not very accurate */
>  	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
>  	POWER_SUPPLY_PROP_STATUS,
>  	POWER_SUPPLY_PROP_ONLINE,
>  };
> @@ -464,6 +470,16 @@ static int rn5t618_adp_get_property(struct power_supply *psy,
>  
>  		val->intval = FROM_CUR_REG(regval);
>  		break;
> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +		if (!info->channel_vadp)
> +			return -ENODATA;
> +
> +		ret = iio_read_channel_processed(info->channel_vadp, &val->intval);
> +		if (ret < 0)
> +			return ret;
> +
> +		val->intval *= 1000;
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -589,6 +605,16 @@ static int rn5t618_usb_get_property(struct power_supply *psy,
>  			val->intval = FROM_CUR_REG(regval);
>  		}
>  		break;
> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +		if (!info->channel_vusb)
> +			return -ENODATA;
> +
> +		ret = iio_read_channel_processed(info->channel_vusb, &val->intval);
> +		if (ret < 0)
> +			return ret;
> +
> +		val->intval *= 1000;
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -711,6 +737,28 @@ static int rn5t618_power_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, info);
>  
> +	info->channel_vusb = devm_iio_channel_get(&pdev->dev, "vusb");
> +	if (IS_ERR(info->channel_vusb)) {
> +		ret = PTR_ERR(info->channel_vusb);
> +		if (ret == -EPROBE_DEFER)
> +			return ret;
> +
> +		dev_warn(&pdev->dev, "could not request vusb iio channel (%d)",
> +			 ret);

I wonder if you should distinguish between -ENODEV to indicate there wasn't
one specified vs something else when wrong.   Might not be worth bothering
as there are not many things that could go wrong (things like small memory
allocations which will never fail..)

> +		info->channel_vusb = NULL;
> +	}
> +
> +	info->channel_vadp = devm_iio_channel_get(&pdev->dev, "vadp");
> +	if (IS_ERR(info->channel_vadp)) {
> +		ret = PTR_ERR(info->channel_vadp);
> +		if (ret == -EPROBE_DEFER)
> +			return ret;
> +
> +		dev_warn(&pdev->dev, "could not request vadp iio channel (%d)",
> +			 ret);
> +		info->channel_vadp = NULL;
> +	}
> +
>  	ret = regmap_read(info->rn5t618->regmap, RN5T618_CONTROL, &v);
>  	if (ret)
>  		return ret;
> @@ -778,9 +826,17 @@ static int rn5t618_power_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct of_device_id rn5t618_power_of_match[] = {
> +	{.compatible = "ricoh,rc5t619-power", },
> +	{.compatible = "ricoh,rn5t618-power", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, rn5t618_power_of_match);
> +
>  static struct platform_driver rn5t618_power_driver = {
>  	.driver = {
>  		.name   = "rn5t618-power",
> +		.of_match_table = of_match_ptr(rn5t618_power_of_match),
>  	},
>  	.probe = rn5t618_power_probe,
>  };

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ