| 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
| ||
|
Message-ID: <20211120165345.53de0d51@jic23-huawei>
Date: Sat, 20 Nov 2021 16:54:05 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Anand Ashok Dumbre <anand.ashok.dumbre@...inx.com>
Cc: <linux-kernel@...r.kernel.org>, <lars@...afoo.de>,
<linux-iio@...r.kernel.org>, <git@...inx.com>,
<michal.simek@...inx.com>, <gregkh@...uxfoundation.org>,
<rafael@...nel.org>, <linux-acpi@...r.kernel.org>,
<andriy.shevchenko@...ux.intel.com>,
<heikki.krogerus@...ux.intel.com>,
Manish Narani <manish.narani@...inx.com>
Subject: Re: [PATCH v10 3/5] iio: adc: Add Xilinx AMS driver
On Wed, 17 Nov 2021 16:10:26 +0000
Anand Ashok Dumbre <anand.ashok.dumbre@...inx.com> wrote:
> The AMS includes an ADC as well as on-chip sensors that can be used to
> sample external voltages and monitor on-die operating conditions, such
> as temperature and supply voltage levels. The AMS has two SYSMON blocks.
> PL-SYSMON block is capable of monitoring off chip voltage and
> temperature.
>
> PL-SYSMON block has DRP, JTAG and I2C interface to enable monitoring
> from an external master. Out of these interfaces currently only DRP is
> supported. Other block PS-SYSMON is memory mapped to PS.
>
> The AMS can use internal channels to monitor voltage and temperature as
> well as one primary and up to 16 auxiliary channels for measuring
> external voltages.
>
> The voltage and temperature monitoring channels also have event capability
> which allows to generate an interrupt when their value falls below or
> raises above a set threshold.
>
> Co-developed-by: Manish Narani <manish.narani@...inx.com>
> Signed-off-by: Manish Narani <manish.narani@...inx.com>
> Signed-off-by: Anand Ashok Dumbre <anand.ashok.dumbre@...inx.com>
A few minor additions from me to what Andy has noted.
Thanks,
Jonathan
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index d3f53549720c..4a8f1833993b 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -115,4 +115,5 @@ obj-$(CONFIG_VF610_ADC) += vf610_adc.o
> obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
> xilinx-xadc-y := xilinx-xadc-core.o xilinx-xadc-events.o
> obj-$(CONFIG_XILINX_XADC) += xilinx-xadc.o
> +obj-$(CONFIG_XILINX_AMS) += xilinx-ams.o
> obj-$(CONFIG_SD_ADC_MODULATOR) += sd_adc_modulator.o
> diff --git a/drivers/iio/adc/xilinx-ams.c b/drivers/iio/adc/xilinx-ams.c
> new file mode 100644
> index 000000000000..bb3876b51e3e
> --- /dev/null
> +++ b/drivers/iio/adc/xilinx-ams.c
> @@ -0,0 +1,1447 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Xilinx AMS driver
> + *
> + * Copyright (C) 2021 Xilinx, Inc.
> + *
> + * Manish Narani <mnarani@...inx.com>
> + * Rajnikant Bhojani <rajnikant.bhojani@...inx.com>
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/overflow.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
> +
> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
Used?
> +
...
> +/**
> + * struct ams - Driver data for xilinx-ams
> + * @base: physical base address of device
> + * @ps_base: physical base address of PS device
> + * @pl_base: physical base address of PL device
> + * @clk: clocks associated with the device
> + * @dev: pointer to device struct
> + * @lock: to handle multiple user interaction
> + * @intr_lock: to protect interrupt mask values
> + * @alarm_mask: alarm configuration
> + * @masked_alarm: currently masked due to alarm
> + * @intr_mask: interrupt configuration
> + * @ams_unmask_work: re-enables event once the event condition disappears
> + *
> + * This structure contains necessary state for Sysmon driver to operate
> + */
> +struct ams {
> + void __iomem *base;
> + void __iomem *ps_base;
> + void __iomem *pl_base;
> + struct clk *clk;
> + struct device *dev;
> + struct mutex lock;
> + spinlock_t intr_lock;
> + unsigned int alarm_mask;
> + unsigned int masked_alarm;
Hmm. maybe a rename to make these two less confusing?
Perhaps
current_masked_alarm?
> + u64 intr_mask;
> + struct delayed_work ams_unmask_work;
> +};
> +
...
> +
> +static void ams_handle_event(struct iio_dev *indio_dev, u32 event)
> +{
> + const struct iio_chan_spec *chan;
> +
> + chan = ams_event_to_channel(indio_dev, event);
> +
> + if (chan->type == IIO_TEMP) {
> + /*
> + * The temperature channel only supports over-temperature
> + * events.
> + */
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(chan->type, chan->channel,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_RISING),
> + iio_get_time_ns(indio_dev));
> + } else {
> + /*
> + * For other channels we don't know whether it is a upper or
> + * lower threshold event. Userspace will have to check the
> + * channel value if it wants to know.
> + */
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(chan->type, chan->channel,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_EITHER),
> + iio_get_time_ns(indio_dev));
I think alignment is wrong here. iio_get_time_ns() should align with opening bracket as well.
> + }
> +}
> +
...
> +
> +static int ams_parse_firmware(struct iio_dev *indio_dev,
> + struct platform_device *pdev)
> +{
> + struct ams *ams = iio_priv(indio_dev);
> + struct iio_chan_spec *ams_channels, *dev_channels;
> + struct fwnode_handle *child = NULL, *fwnode = dev_fwnode(&pdev->dev);
Where you have values being set, I'd prefer separate line per variable.
Tends to be a little more readable and we need all the help we can get :)
> + size_t dev_chan_size, ams_chan_size, num_chan;
> + int ret, ch_cnt = 0, i, rising_off, falling_off;
> + unsigned int num_channels = 0;
> +
One blank line is almost always enough. Definitely is here.
> +
> + num_chan = ARRAY_SIZE(ams_ps_channels) + ARRAY_SIZE(ams_pl_channels) +
> + ARRAY_SIZE(ams_ctrl_channels);
> +
> + ams_chan_size = array_size(num_chan, sizeof(struct iio_chan_spec));
> + if (ams_chan_size == SIZE_MAX)
> + return -EINVAL;
> +
> + /* Initialize buffer for channel specification */
> + ams_channels = kcalloc(num_chan, sizeof(struct iio_chan_spec), GFP_KERNEL);
> + if (!ams_channels)
> + return -ENOMEM;
> +
> + if (fwnode_device_is_available(fwnode)) {
> + ret = ams_init_module(indio_dev, fwnode, ams_channels);
> + if (ret < 0)
> + goto free_mem;
> +
> + num_channels += ret;
> + }
> +
> + fwnode_for_each_child_node(fwnode, child) {
> + if (fwnode_device_is_available(child)) {
> + ret = ams_init_module(indio_dev, child,
> + ams_channels + num_channels);
> + if (ret < 0) {
> + fwnode_handle_put(child);
> + goto free_mem;
> + }
> +
> + num_channels += ret;
> + }
> + }
> +
> + for (i = 0; i < num_channels; i++) {
> + ams_channels[i].channel = ch_cnt++;
> +
> + if (ams_channels[i].scan_index < AMS_CTRL_SEQ_BASE) {
> + /* set threshold to max and min for each channel */
> + falling_off =
> + ams_get_alarm_offset(ams_channels[i].scan_index,
> + IIO_EV_DIR_FALLING);
> + rising_off =
> + ams_get_alarm_offset(ams_channels[i].scan_index,
> + IIO_EV_DIR_RISING);
> + if (ams_channels[i].scan_index >= AMS_PS_SEQ_MAX) {
> + writel(AMS_ALARM_THR_MIN,
> + ams->pl_base + falling_off);
> + writel(AMS_ALARM_THR_MAX,
> + ams->pl_base + rising_off);
> + } else {
> + writel(AMS_ALARM_THR_MIN,
> + ams->ps_base + falling_off);
> + writel(AMS_ALARM_THR_MAX,
> + ams->ps_base + rising_off);
> + }
> + }
> + }
> +
> + dev_chan_size = array_size((size_t)num_channels, sizeof(struct iio_chan_spec));
> + if (dev_chan_size == SIZE_MAX)
Why not goto free_mem for this error case?
Obviously should never happen, but should handle the error anyway.
> + return -EINVAL;
> +
> + dev_channels = devm_kcalloc(&pdev->dev, (size_t)num_channels,
> + sizeof(struct iio_chan_spec), GFP_KERNEL);
> + if (!dev_channels) {
> + ret = -ENOMEM;
> + goto free_mem;
> + }
> +
> + memcpy(dev_channels, ams_channels,
> + sizeof(*ams_channels) * num_channels);
> + indio_dev->channels = dev_channels;
> + indio_dev->num_channels = num_channels;
> +
> + ret = 0;
> +
> +free_mem:
> + kfree(ams_channels);
> +
> + return ret;
> +}
Powered by blists - more mailing lists