* Re: [PATCH v5 7/7] iio: accel: adxl345: Add spi-3wire option
From: Lothar Rubusch @ 2024-04-01 16:06 UTC (permalink / raw)
To: Jonathan Cameron
Cc: lars, Michael.Hennerich, robh+dt, krzysztof.kozlowski+dt,
conor+dt, linux-iio, devicetree, linux-kernel, eraretuya
In-Reply-To: <20240330152401.034aedad@jic23-huawei>
On Sat, Mar 30, 2024 at 4:24 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 29 Mar 2024 01:33:01 +0100
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > On Thu, Mar 28, 2024 at 2:39 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Wed, 27 Mar 2024 22:03:20 +0000
> > > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > >
> > > > Add a setup function implementation to the spi module to enable spi-3wire
> > > > as option when specified in the device-tree.
> > > >
> > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > > ---
> > > > drivers/iio/accel/adxl345.h | 2 ++
> > > > drivers/iio/accel/adxl345_spi.c | 12 +++++++++++-
> > > > 2 files changed, 13 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> > > > index 4ea9341d4..e6bc3591c 100644
> > > > --- a/drivers/iio/accel/adxl345.h
> > > > +++ b/drivers/iio/accel/adxl345.h
> > > > @@ -30,6 +30,8 @@
> > > > #define ADXL345_POWER_CTL_MEASURE BIT(3)
> > > > #define ADXL345_POWER_CTL_STANDBY 0x00
> > > >
> > > > +#define ADXL345_DATA_FORMAT_SPI_3WIRE BIT(6) /* 3-wire SPI mode */
> > > > +
> > > > #define ADXL345_DATA_FORMAT_RANGE GENMASK(1, 0) /* Set the g range */
> > > > #define ADXL345_DATA_FORMAT_JUSTIFY BIT(2) /* Left-justified (MSB) mode */
> > > > #define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */
> > > > diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
> > > > index 1c0513bd3..f145d5c1d 100644
> > > > --- a/drivers/iio/accel/adxl345_spi.c
> > > > +++ b/drivers/iio/accel/adxl345_spi.c
> > > > @@ -20,6 +20,16 @@ static const struct regmap_config adxl345_spi_regmap_config = {
> > > > .read_flag_mask = BIT(7) | BIT(6),
> > > > };
> > > >
> > > > +static int adxl345_spi_setup(struct device *dev, struct regmap *regmap)
> > > > +{
> > > > + struct spi_device *spi = container_of(dev, struct spi_device, dev);
> > > > +
> > > > + if (spi->mode & SPI_3WIRE)
> > > > + return regmap_write(regmap, ADXL345_REG_DATA_FORMAT,
> > > > + ADXL345_DATA_FORMAT_SPI_3WIRE);
> > > Your earlier patch carefully (I think) left one or two fields alone, then
> > > this write just comes in and changes them. In particular INT_INVERT.
> > >
> > Why do you refer here to INT_INVERT? In this code above I try to set
> > SPI to 3 lines. Since this is a SPI configuration, i.e. bus specific,
> > it happens in adxl345_spi.c. Passing this function to the bus
> > independent adxl345_core.c file allows to configure the bus first.
> > Therefore, I'm using the update function in core masking out the SPI
> > filag.
>
> Ah. Ok. It was only intended to mask out the SPI3-wire bit, not the
> other bits that you also masked out. I thought intent was to leave
> them untouched for some reason. Given they don't matter in the driver
> at the moment (no interrupt support) then no problem.
>
> >
> > My reason why I try to keep INT_INVERT out is different. There is
> > another driver for the same part in the kernel:
> > ./drivers/input/misc/adxl34x.c - This is a input driver, using the
> > interrupts of the adxl345 for the input device implementation. I
> > assumed, that in the iio implementation there won't be interrupt
> > handling for the adx345, since it is not an input device. Does this
> > make sense?
>
> No. You can't use these two drivers at the same time. They will almost
> certainly trample over each other in actually reading channels etc.
>
> Their is some legacy of old drivers in input from a long time back.
> Given this driver clearly doesn't have full functionality yet in IIO there
> and the different userspace ABI, we've just left the input driver alone.
>
Going by the git history gave this impression, too. But it was still a
bit confusing to me.
The IIO driver so far does not handle any of the interrupt features.
The older driver also seems to support more of the chip's features.
Would it make sense to continue implement/port what's missing -
feature by feature - for the IIO driver in order to make the input
driver obsolete (one day)?
> >
> > > If it doesn't makes sense to write it there, either write that bit
> > > every time here, or leave it alone every time. Not decide on whether
> > > to write the bit based on SPI_3WIRE or not. As far as I know they
> > > are unconnected features.
> > >
> > I think I did not understand. Could you please specify a bit more?
> > When spi-3wire is configured in the DT it has to be parsed and handled
> > in the bus specific part, i.e. the adxl345_spi.c Therefore I configure
> > SPI_3WIRE there. I don't want to place SPI specific code in the core
> > file.
>
> My confusion was that you were deliberately not touching the other unused
> flags. In reality you are touching the but only if you enable 3wire.
> I would write them register to 0 in the !3wire case so all other values
> are the same in both paths.
>
> >
> > > > + return 0;
> > > > +}
> > > > +
> > > > static int adxl345_spi_probe(struct spi_device *spi)
> > > > {
> > > > struct regmap *regmap;
> > > > @@ -33,7 +43,7 @@ static int adxl345_spi_probe(struct spi_device *spi)
> > > > if (IS_ERR(regmap))
> > > > return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing regmap\n");
> > > >
> > > > - return adxl345_core_probe(&spi->dev, regmap, NULL);
> > > > + return adxl345_core_probe(&spi->dev, regmap, adxl345_spi_setup);
> > > > }
> > > >
> > > > static const struct adxl345_chip_info adxl345_spi_info = {
> > >
>
^ permalink raw reply
* Re: [PATCH v6 RESEND 0/4] arm64: qcom: add AIM300 AIoT board support
From: Trilok Soni @ 2024-04-01 15:54 UTC (permalink / raw)
To: Tengfei Fan, andersson, konrad.dybcio, robh,
krzysztof.kozlowski+dt, conor+dt
Cc: keescook, tony.luck, gpiccoli, linux-arm-msm, devicetree,
linux-kernel, linux-hardening, kernel
In-Reply-To: <20240401093843.2591147-1-quic_tengfan@quicinc.com>
On 4/1/2024 2:38 AM, Tengfei Fan wrote:
> Here is a diagram of AIM300 AIoT Carrie Board and SoM
> +--------------------------------------------------+
> | AIM300 AIOT Carrie Board |
spellcheck
s/Carrie/Carrier ?
--
---Trilok Soni
^ permalink raw reply
* Re: [PATCH v4 1/4] remoteproc: Add TEE support
From: Mathieu Poirier @ 2024-04-01 15:54 UTC (permalink / raw)
To: Arnaud POULIQUEN
Cc: Bjorn Andersson, Jens Wiklander, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-stm32, linux-arm-kernel, linux-remoteproc,
linux-kernel, op-tee, devicetree
In-Reply-To: <a4a1f938-d185-46d7-9f57-af7bf3a65e9c@foss.st.com>
On Fri, Mar 29, 2024 at 09:58:11AM +0100, Arnaud POULIQUEN wrote:
> Hello Mathieu,
>
> On 3/27/24 18:07, Mathieu Poirier wrote:
> > On Tue, Mar 26, 2024 at 08:18:23PM +0100, Arnaud POULIQUEN wrote:
> >> Hello Mathieu,
> >>
> >> On 3/25/24 17:46, Mathieu Poirier wrote:
> >>> On Fri, Mar 08, 2024 at 03:47:05PM +0100, Arnaud Pouliquen wrote:
> >>>> Add a remoteproc TEE (Trusted Execution Environment) driver
> >>>> that will be probed by the TEE bus. If the associated Trusted
> >>>> application is supported on secure part this device offers a client
> >>>
> >>> Device or driver? I thought I touched on that before.
> >>
> >> Right, I changed the first instance and missed this one
> >>
> >>>
> >>>> interface to load a firmware in the secure part.
> >>>> This firmware could be authenticated by the secure trusted application.
> >>>>
> >>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >>>> ---
> >>>> Updates from V3:
> >>>> - rework TEE_REMOTEPROC description in Kconfig
> >>>> - fix some namings
> >>>> - add tee_rproc_parse_fw to support rproc_ops::parse_fw
> >>>> - add proc::tee_interface;
> >>>> - add rproc struct as parameter of the tee_rproc_register() function
> >>>> ---
> >>>> drivers/remoteproc/Kconfig | 10 +
> >>>> drivers/remoteproc/Makefile | 1 +
> >>>> drivers/remoteproc/tee_remoteproc.c | 434 ++++++++++++++++++++++++++++
> >>>> include/linux/remoteproc.h | 4 +
> >>>> include/linux/tee_remoteproc.h | 112 +++++++
> >>>> 5 files changed, 561 insertions(+)
> >>>> create mode 100644 drivers/remoteproc/tee_remoteproc.c
> >>>> create mode 100644 include/linux/tee_remoteproc.h
> >>>>
> >>>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> >>>> index 48845dc8fa85..2cf1431b2b59 100644
> >>>> --- a/drivers/remoteproc/Kconfig
> >>>> +++ b/drivers/remoteproc/Kconfig
> >>>> @@ -365,6 +365,16 @@ config XLNX_R5_REMOTEPROC
> >>>>
> >>>> It's safe to say N if not interested in using RPU r5f cores.
> >>>>
> >>>> +
> >>>> +config TEE_REMOTEPROC
> >>>> + tristate "remoteproc support by a TEE application"
> >>>
> >>> s/remoteproc/Remoteproc
> >>>
> >>>> + depends on OPTEE
> >>>> + help
> >>>> + Support a remote processor with a TEE application. The Trusted
> >>>> + Execution Context is responsible for loading the trusted firmware
> >>>> + image and managing the remote processor's lifecycle.
> >>>> + This can be either built-in or a loadable module.
> >>>> +
> >>>> endif # REMOTEPROC
> >>>>
> >>>> endmenu
> >>>> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> >>>> index 91314a9b43ce..fa8daebce277 100644
> >>>> --- a/drivers/remoteproc/Makefile
> >>>> +++ b/drivers/remoteproc/Makefile
> >>>> @@ -36,6 +36,7 @@ obj-$(CONFIG_RCAR_REMOTEPROC) += rcar_rproc.o
> >>>> obj-$(CONFIG_ST_REMOTEPROC) += st_remoteproc.o
> >>>> obj-$(CONFIG_ST_SLIM_REMOTEPROC) += st_slim_rproc.o
> >>>> obj-$(CONFIG_STM32_RPROC) += stm32_rproc.o
> >>>> +obj-$(CONFIG_TEE_REMOTEPROC) += tee_remoteproc.o
> >>>> obj-$(CONFIG_TI_K3_DSP_REMOTEPROC) += ti_k3_dsp_remoteproc.o
> >>>> obj-$(CONFIG_TI_K3_R5_REMOTEPROC) += ti_k3_r5_remoteproc.o
> >>>> obj-$(CONFIG_XLNX_R5_REMOTEPROC) += xlnx_r5_remoteproc.o
> >>>> diff --git a/drivers/remoteproc/tee_remoteproc.c b/drivers/remoteproc/tee_remoteproc.c
> >>>> new file mode 100644
> >>>> index 000000000000..c855210e52e3
> >>>> --- /dev/null
> >>>> +++ b/drivers/remoteproc/tee_remoteproc.c
> >>>> @@ -0,0 +1,434 @@
> >>>> +// SPDX-License-Identifier: GPL-2.0-or-later
> >>>> +/*
> >>>> + * Copyright (C) STMicroelectronics 2024 - All Rights Reserved
> >>>> + * Author: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >>>> + */
> >>>> +
> >>>> +#include <linux/firmware.h>
> >>>> +#include <linux/io.h>
> >>>> +#include <linux/module.h>
> >>>> +#include <linux/remoteproc.h>
> >>>> +#include <linux/slab.h>
> >>>> +#include <linux/tee_drv.h>
> >>>> +#include <linux/tee_remoteproc.h>
> >>>> +
> >>>> +#include "remoteproc_internal.h"
> >>>> +
> >>>> +#define MAX_TEE_PARAM_ARRY_MEMBER 4
> >>>> +
> >>>> +/*
> >>>> + * Authentication of the firmware and load in the remote processor memory
> >>>> + *
> >>>> + * [in] params[0].value.a: unique 32bit identifier of the remote processor
> >>>> + * [in] params[1].memref: buffer containing the image of the buffer
> >>>> + */
> >>>> +#define TA_RPROC_FW_CMD_LOAD_FW 1
> >>>> +
> >>>> +/*
> >>>> + * Start the remote processor
> >>>> + *
> >>>> + * [in] params[0].value.a: unique 32bit identifier of the remote processor
> >>>> + */
> >>>> +#define TA_RPROC_FW_CMD_START_FW 2
> >>>> +
> >>>> +/*
> >>>> + * Stop the remote processor
> >>>> + *
> >>>> + * [in] params[0].value.a: unique 32bit identifier of the remote processor
> >>>> + */
> >>>> +#define TA_RPROC_FW_CMD_STOP_FW 3
> >>>> +
> >>>> +/*
> >>>> + * Return the address of the resource table, or 0 if not found
> >>>> + * No check is done to verify that the address returned is accessible by
> >>>> + * the non secure context. If the resource table is loaded in a protected
> >>>> + * memory the access by the non secure context will lead to a data abort.
> >>>> + *
> >>>> + * [in] params[0].value.a: unique 32bit identifier of the remote processor
> >>>> + * [out] params[1].value.a: 32bit LSB resource table memory address
> >>>> + * [out] params[1].value.b: 32bit MSB resource table memory address
> >>>> + * [out] params[2].value.a: 32bit LSB resource table memory size
> >>>> + * [out] params[2].value.b: 32bit MSB resource table memory size
> >>>> + */
> >>>> +#define TA_RPROC_FW_CMD_GET_RSC_TABLE 4
> >>>> +
> >>>> +/*
> >>>> + * Return the address of the core dump
> >>>> + *
> >>>> + * [in] params[0].value.a: unique 32bit identifier of the remote processor
> >>>> + * [out] params[1].memref: address of the core dump image if exist,
> >>>> + * else return Null
> >>>> + */
> >>>> +#define TA_RPROC_FW_CMD_GET_COREDUMP 5
> >>>> +
> >>>> +struct tee_rproc_context {
> >>>> + struct list_head sessions;
> >>>> + struct tee_context *tee_ctx;
> >>>> + struct device *dev;
> >>>> +};
> >>>> +
> >>>> +static struct tee_rproc_context *tee_rproc_ctx;
> >>>> +
> >>>> +static void tee_rproc_prepare_args(struct tee_rproc *trproc, int cmd,
> >>>> + struct tee_ioctl_invoke_arg *arg,
> >>>> + struct tee_param *param,
> >>>> + unsigned int num_params)
> >>>> +{
> >>>> + memset(arg, 0, sizeof(*arg));
> >>>> + memset(param, 0, MAX_TEE_PARAM_ARRY_MEMBER * sizeof(*param));
> >>>> +
> >>>> + arg->func = cmd;
> >>>> + arg->session = trproc->session_id;
> >>>> + arg->num_params = num_params + 1;
> >>>> +
> >>>> + param[0] = (struct tee_param) {
> >>>> + .attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> >>>> + .u.value.a = trproc->rproc_id,
> >>>> + };
> >>>> +}
> >>>> +
> >>>> +int tee_rproc_load_fw(struct rproc *rproc, const struct firmware *fw)
> >>>> +{
> >>>> + struct tee_ioctl_invoke_arg arg;
> >>>> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
> >>>> + struct tee_rproc *trproc = rproc->tee_interface;
> >>>> + struct tee_shm *fw_shm;
> >>>> + int ret;
> >>>
> >>> Declarations in reverse ascending order here and everywhere in the driver.
> >>> Sometimes it is done properly, sometimes it isn't.
> >>>
> >>>> +
> >>>> + if (!trproc)
> >>>> + return -EINVAL;
> >>>> +
> >>>> + fw_shm = tee_shm_register_kernel_buf(tee_rproc_ctx->tee_ctx, (void *)fw->data, fw->size);
> >>>> + if (IS_ERR(fw_shm))
> >>>> + return PTR_ERR(fw_shm);
> >>>> +
> >>>> + tee_rproc_prepare_args(trproc, TA_RPROC_FW_CMD_LOAD_FW, &arg, param, 1);
> >>>> +
> >>>> + /* Provide the address of the firmware image */
> >>>> + param[1] = (struct tee_param) {
> >>>> + .attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT,
> >>>> + .u.memref = {
> >>>> + .shm = fw_shm,
> >>>> + .size = fw->size,
> >>>> + .shm_offs = 0,
> >>>> + },
> >>>> + };
> >>>> +
> >>>> + ret = tee_client_invoke_func(tee_rproc_ctx->tee_ctx, &arg, param);
> >>>> + if (ret < 0 || arg.ret != 0) {
> >>>> + dev_err(tee_rproc_ctx->dev,
> >>>> + "TA_RPROC_FW_CMD_LOAD_FW invoke failed TEE err: %x, ret:%x\n",
> >>>> + arg.ret, ret);
> >>>> + if (!ret)
> >>>> + ret = -EIO;
> >>>> + }
> >>>> +
> >>>> + tee_shm_free(fw_shm);
> >>>> +
> >>>> + return ret;
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(tee_rproc_load_fw);
> >>>> +
> >>>> +struct resource_table *tee_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz)
> >>>> +{
> >>>> + struct tee_ioctl_invoke_arg arg;
> >>>> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
> >>>> + struct tee_rproc *trproc = rproc->tee_interface;
> >>>> + struct resource_table *rsc_table;
> >>>> + int ret;
> >>>> +
> >>>> + if (!trproc)
> >>>> + return ERR_PTR(-EINVAL);
> >>>> +
> >>>> + tee_rproc_prepare_args(trproc, TA_RPROC_FW_CMD_GET_RSC_TABLE, &arg, param, 2);
> >>>> +
> >>>> + param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;
> >>>> + param[2].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;
> >>>> +
> >>>> + ret = tee_client_invoke_func(tee_rproc_ctx->tee_ctx, &arg, param);
> >>>> + if (ret < 0 || arg.ret != 0) {
> >>>> + dev_err(tee_rproc_ctx->dev,
> >>>> + "TA_RPROC_FW_CMD_GET_RSC_TABLE invoke failed TEE err: %x, ret:%x\n",
> >>>> + arg.ret, ret);
> >>>> + return ERR_PTR(-EIO);
> >>>> + }
> >>>> +
> >>>> + *table_sz = param[2].u.value.a;
> >>>> +
> >>>> + /* If the size is null no resource table defined in the image */
> >>>> + if (!*table_sz)
> >>>> + return NULL;
> >>>> +
> >>>> + /* Store the resource table address that would be updated by the remote core. */
> >>>> + rsc_table = ioremap_wc(param[1].u.value.a, *table_sz);
> >>>> + if (IS_ERR_OR_NULL(rsc_table)) {
> >>>> + dev_err(tee_rproc_ctx->dev, "Unable to map memory region: %lld+%zx\n",
> >>>> + param[1].u.value.a, *table_sz);
> >>>> + return ERR_PTR(-ENOMEM);
> >>>> + }
> >>>> +
> >>>> + return rsc_table;
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(tee_rproc_get_loaded_rsc_table);
> >>>> +
> >>>> +int tee_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
> >>>> +{
> >>>> + struct tee_rproc *trproc = rproc->tee_interface;
> >>>> + struct resource_table *rsc_table;
> >>>> + size_t table_sz;
> >>>> + int ret;
> >>>> +
> >>>> + ret = tee_rproc_load_fw(rproc, fw);
> >>>> + if (ret)
> >>>> + return ret;
> >>>> +
> >>>> + rsc_table = tee_rproc_get_loaded_rsc_table(rproc, &table_sz);
> >>>> + if (IS_ERR(rsc_table))
> >>>> + return PTR_ERR(rsc_table);
> >>>> +
> >>>> + /* Create a copy of the resource table to have same behaviour than the elf loader. */
> >>>> + rproc->cached_table = kmemdup(rsc_table, table_sz, GFP_KERNEL);
> >>>> + if (!rproc->cached_table)
> >>>> + return -ENOMEM;
> >>>
> >>> Why not ->table_ptr and setting ->cached_table to NULL?
> >>
> >> It was my plan preparing this version. But during implementarion it looks
> >> to me that having exactly same behavior than the ELF loader would be
> >> simpler to maintain the remoteproc avoiding to update in the remoteproc core
> >> to manage for the cached resource table (see also my comment below abourt recovery)
> >> That why I propose this implementation
> >>
> >> That said what you proposal should also work (with some updates in
> >> remoteproc_core for the management of the cached table).
> >>
> >
> > Yes
> >
> >> So please just comfirm your preference.
> >>
> >
> > Definitely keep ->cached_table to NULL.
> >
> >>>
> >>>> +
> >>>> + rproc->table_ptr = rproc->cached_table;
> >>>> + rproc->table_sz = table_sz;
> >>>> + trproc->rsc_table = rsc_table;
> >>>
> >>> I really don't see why this is needed, please remove and use rproc->table_ptr
> >>> instead.
> >>
> >> I need to store it for the iounmap in tee_rproc_remove.
> >
> > iounmap(entry->rproc->rsc_table);
> >
> > What am I missing?
>
> rproc->rsc_table is a field that can be updated by remoteproc_core.
> How can we garanty in tee_remoteproc that it still points to the mapped resource
> table?
By making sure the core doesn't touch rproc->rsc_table when
rproc->tee_interface is valid.
> As the remoteproc_tee maps the pointer, it seems reliable that it stores it for
> unmap.
>
Doing so creates a shadow value that is confusing and really hard to maintain
going forward.
> Seems also that I also missed to handle the case where rproc_fw_boot() fails[3]
> (not done yet).
>
> [3]https://elixir.bootlin.com/linux/latest/source/drivers/remoteproc/remoteproc_core.c#L1442
>
>
> >
> >>
> >>>
> >>>> +
> >>>> + return 0;
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(tee_rproc_parse_fw);
> >>>> +
> >>>> +struct resource_table *tee_rproc_find_loaded_rsc_table(struct rproc *rproc,
> >>>> + const struct firmware *fw)
> >>>> +{
> >>>> + struct tee_rproc *trproc = rproc->tee_interface;
> >>>> + struct resource_table *rsc_table;
> >>>> + size_t table_sz;
> >>>> +
> >>>> + if (!trproc)
> >>>> + return ERR_PTR(-EINVAL);
> >>>> +
> >>>> + /* Check if the resourse table has already been obtained in tee_rproc_parse_fw() */
> >>>> + if (trproc->rsc_table)
> >>>> + return trproc->rsc_table;
> >>>
> >>> Again, why not simply use rproc->rsc_table? This function should only return
> >>> the resource table that was set in tee_rproc_parse_fw().
> >>
> >> In case of recovery rproc->_rsc_table point to the cached table [1]
> >
> > In [1], on line 1554, add a check for rproc->tee_interface and if it is valid
> > call rproc_find_loaded_rsc_table().
> >
> >> and we need to reapply the configuration in rproc_start() called during the
> >> recovery[2]
> >
> > 1) Rename rproc_set_rsc_table() to rproc_set_rsc_table_on_attach()
> > 2) Introduce a new function called rproc_set_rsc_table_on_start()
> > 3) Move code from [2], line 1278 to 1292, to that new function. In the new
> > function, add a check on rproc->tee_interface. If it is valid then call
> > rproc_find_loaded_rsc_table().
> > 4) in rproc_start(), replace lines 1278 to 1292 with a call to
> > rproc_set_rsc_table_on_start().
>
>
> I will try this
>
Ok
> Thanks!
> Arnaud
>
> >
> >> [1]https://elixir.bootlin.com/linux/latest/source/drivers/remoteproc/remoteproc_core.c#L1586
> >> [2]https://elixir.bootlin.com/linux/latest/source/drivers/remoteproc/remoteproc_core.c#L1287
> >>
> >>>
> >>>> +
> >>>> + rsc_table = tee_rproc_get_loaded_rsc_table(rproc, &table_sz);
> >>>> + if (IS_ERR(rsc_table))
> >>>> + return rsc_table;
> >>>> +
> >>>> + rproc->table_sz = table_sz;
> >>>> + trproc->rsc_table = rsc_table;
> >>>> +
> >>>> + return rsc_table;
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(tee_rproc_find_loaded_rsc_table);
> >>>> +
> >>>> +int tee_rproc_start(struct rproc *rproc)
> >>>> +{
> >>>> + struct tee_ioctl_invoke_arg arg;
> >>>> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
> >>>> + struct tee_rproc *trproc = rproc->tee_interface;
> >>>> + int ret;
> >>>> +
> >>>> + if (!trproc)
> >>>> + return -EINVAL;
> >>>> +
> >>>> + tee_rproc_prepare_args(trproc, TA_RPROC_FW_CMD_START_FW, &arg, param, 0);
> >>>> +
> >>>> + ret = tee_client_invoke_func(tee_rproc_ctx->tee_ctx, &arg, param);
> >>>> + if (ret < 0 || arg.ret != 0) {
> >>>> + dev_err(tee_rproc_ctx->dev,
> >>>> + "TA_RPROC_FW_CMD_START_FW invoke failed TEE err: %x, ret:%x\n",
> >>>> + arg.ret, ret);
> >>>> + if (!ret)
> >>>> + ret = -EIO;
> >>>> + }
> >>>> +
> >>>> + return ret;
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(tee_rproc_start);
> >>>> +
> >>>> +int tee_rproc_stop(struct rproc *rproc)
> >>>> +{
> >>>> + struct tee_ioctl_invoke_arg arg;
> >>>> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
> >>>> + struct tee_rproc *trproc = rproc->tee_interface;
> >>>> + int ret;
> >>>> +
> >>>> + if (!trproc)
> >>>> + return -EINVAL;
> >>>> +
> >>>> + tee_rproc_prepare_args(trproc, TA_RPROC_FW_CMD_STOP_FW, &arg, param, 0);
> >>>> +
> >>>> + ret = tee_client_invoke_func(tee_rproc_ctx->tee_ctx, &arg, param);
> >>>> + if (ret < 0 || arg.ret != 0) {
> >>>> + dev_err(tee_rproc_ctx->dev,
> >>>> + "TA_RPROC_FW_CMD_STOP_FW invoke failed TEE err: %x, ret:%x\n",
> >>>> + arg.ret, ret);
> >>>> + if (!ret)
> >>>> + ret = -EIO;
> >>>> + }
> >>>> +
> >>>> + if (!rproc->table_ptr)
> >>>> + return ret;
> >>>> +
> >>>> + iounmap(trproc->rsc_table);
> >>>> + trproc->rsc_table = NULL;
> >>>> + rproc->table_ptr = NULL;
> >>>> +
> >>>> + return 0;
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(tee_rproc_stop);
> >>>> +
> >>>> +static const struct tee_client_device_id stm32_tee_rproc_id_table[] = {
> >>>> + {UUID_INIT(0x80a4c275, 0x0a47, 0x4905,
> >>>> + 0x82, 0x85, 0x14, 0x86, 0xa9, 0x77, 0x1a, 0x08)},
> >>>> + {}
> >>>> +};
> >>>> +
> >>>> +struct tee_rproc *tee_rproc_register(struct device *dev, struct rproc *rproc, unsigned int rproc_id)
> >>>> +{
> >>>> + struct tee_client_device *tee_device;
> >>>> + struct tee_ioctl_open_session_arg sess_arg;
> >>>> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
> >>>> + struct tee_rproc *trproc;
> >>>> + int ret;
> >>>> +
> >>>> + /*
> >>>> + * Test if the device has been probed by the TEE bus. In case of failure, we ignore the
> >>>> + * reason. The bus could be not yet probed or the service not available in the secure
> >>>> + * firmware.The assumption in such a case is that the TEE remoteproc is not probed.
> >>>> + */
> >>>> + if (!tee_rproc_ctx)
> >>>> + return ERR_PTR(-EPROBE_DEFER);
> >>>> +
> >>>> + trproc = devm_kzalloc(dev, sizeof(*trproc), GFP_KERNEL);
> >>>> + if (!trproc)
> >>>> + return ERR_PTR(-ENOMEM);
> >>>> +
> >>>> + tee_device = to_tee_client_device(tee_rproc_ctx->dev);
> >>>> + memset(&sess_arg, 0, sizeof(sess_arg));
> >>>> +
> >>>> + memcpy(sess_arg.uuid, tee_device->id.uuid.b, TEE_IOCTL_UUID_LEN);
> >>>> +
> >>>> + sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
> >>>> + sess_arg.num_params = 1;
> >>>> +
> >>>> + param[0] = (struct tee_param) {
> >>>> + .attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> >>>> + .u.value.a = rproc_id,
> >>>> + };
> >>>> +
> >>>> + ret = tee_client_open_session(tee_rproc_ctx->tee_ctx, &sess_arg, param);
> >>>> + if (ret < 0 || sess_arg.ret != 0) {
> >>>> + dev_err(dev, "tee_client_open_session failed, err: %x\n", sess_arg.ret);
> >>>> + return ERR_PTR(-EINVAL);
> >>>> + }
> >>>> +
> >>>> + trproc->parent = dev;
> >>>> + trproc->rproc_id = rproc_id;
> >>>> + trproc->session_id = sess_arg.session;
> >>>> +
> >>>> + trproc->rproc = rproc;
> >>>> + rproc->tee_interface = trproc;
> >>>> +
> >>>> + list_add_tail(&trproc->node, &tee_rproc_ctx->sessions);
> >>>> +
> >>>> + return trproc;
> >>>
> >>> Once this function was called by a client, what prevents a user from unloading
> >>> the tee_remoteproc module and breaking everything?
> >>
> >> Good point! seems better toremove the module build capability
> >>
> >
> > I was thinking more along the lines of try_module_get() and module_put() to
> > avoid bloating the core.
> >
> >> Thanks,
> >> Arnaud
> >>
> >>>
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(tee_rproc_register);
> >>>> +
> >>>> +int tee_rproc_unregister(struct tee_rproc *trproc)
> >>>> +{
> >>>
> >>> If you pass a struct_rproc instead of a struct tee_rproc there is no need for
> >>> tee_rproc::rproc, which is only ever used in this function.
> >>>
> >>>
> >>>> + struct rproc *rproc = trproc->rproc;
> >>>> + int ret;
> >>>> +
> >>>> + ret = tee_client_close_session(tee_rproc_ctx->tee_ctx, trproc->session_id);
> >>>> + if (ret < 0)
> >>>> + dev_err(trproc->parent, "tee_client_close_session failed, err: %x\n", ret);
> >>>> +
> >>>> + list_del(&trproc->node);
> >>>> + rproc->tee_interface = NULL;
> >>>> +
> >>>> + return ret;
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(tee_rproc_unregister);
> >>>> +
> >>>> +static int tee_rproc_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
> >>>> +{
> >>>> + /* Today we support only the OP-TEE, could be extend to other tees */
> >>>> + return (ver->impl_id == TEE_IMPL_ID_OPTEE);
> >>>> +}
> >>>> +
> >>>> +static int tee_rproc_probe(struct device *dev)
> >>>> +{
> >>>> + struct tee_context *tee_ctx;
> >>>> + int ret;
> >>>> +
> >>>> + /* Open context with TEE driver */
> >>>> + tee_ctx = tee_client_open_context(NULL, tee_rproc_ctx_match, NULL, NULL);
> >>>> + if (IS_ERR(tee_ctx))
> >>>> + return PTR_ERR(tee_ctx);
> >>>> +
> >>>> + tee_rproc_ctx = devm_kzalloc(dev, sizeof(*tee_ctx), GFP_KERNEL);
> >>>> + if (!tee_rproc_ctx) {
> >>>> + ret = -ENOMEM;
> >>>> + goto err;
> >>>> + }
> >>>> +
> >>>> + tee_rproc_ctx->dev = dev;
> >>>> + tee_rproc_ctx->tee_ctx = tee_ctx;
> >>>> + INIT_LIST_HEAD(&tee_rproc_ctx->sessions);
> >>>> +
> >>>> + return 0;
> >>>> +err:
> >>>> + tee_client_close_context(tee_ctx);
> >>>> +
> >>>> + return ret;
> >>>> +}
> >>>> +
> >>>> +static int tee_rproc_remove(struct device *dev)
> >>>> +{
> >>>> + struct tee_rproc *entry, *tmp;
> >>>> +
> >>>> + list_for_each_entry_safe(entry, tmp, &tee_rproc_ctx->sessions, node) {
> >>>> + tee_client_close_session(tee_rproc_ctx->tee_ctx, entry->session_id);
> >>>> + list_del(&entry->node);
> >>>> + if (entry->rsc_table)
> >>>> + iounmap(entry->rsc_table);
> >>>> + kfree(entry);
> >>>> + }
> >>>> +
> >>>> + tee_client_close_context(tee_rproc_ctx->tee_ctx);
> >>>> +
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> +MODULE_DEVICE_TABLE(tee, stm32_tee_rproc_id_table);
> >>>> +
> >>>> +static struct tee_client_driver tee_rproc_fw_driver = {
> >>>> + .id_table = stm32_tee_rproc_id_table,
> >>>> + .driver = {
> >>>> + .name = KBUILD_MODNAME,
> >>>> + .bus = &tee_bus_type,
> >>>> + .probe = tee_rproc_probe,
> >>>> + .remove = tee_rproc_remove,
> >>>> + },
> >>>> +};
> >>>> +
> >>>> +static int __init tee_rproc_fw_mod_init(void)
> >>>> +{
> >>>> + return driver_register(&tee_rproc_fw_driver.driver);
> >>>> +}
> >>>> +
> >>>> +static void __exit tee_rproc_fw_mod_exit(void)
> >>>> +{
> >>>> + driver_unregister(&tee_rproc_fw_driver.driver);
> >>>> +}
> >>>> +
> >>>> +module_init(tee_rproc_fw_mod_init);
> >>>> +module_exit(tee_rproc_fw_mod_exit);
> >>>> +
> >>>> +MODULE_DESCRIPTION(" TEE remote processor control driver");
> >>>> +MODULE_LICENSE("GPL");
> >>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> >>>> index b4795698d8c2..8b678009e481 100644
> >>>> --- a/include/linux/remoteproc.h
> >>>> +++ b/include/linux/remoteproc.h
> >>>> @@ -503,6 +503,8 @@ enum rproc_features {
> >>>> RPROC_MAX_FEATURES,
> >>>> };
> >>>>
> >>>> +struct tee_rproc;
> >>>> +
> >>>> /**
> >>>> * struct rproc - represents a physical remote processor device
> >>>> * @node: list node of this rproc object
> >>>> @@ -545,6 +547,7 @@ enum rproc_features {
> >>>> * @cdev: character device of the rproc
> >>>> * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
> >>>> * @features: indicate remoteproc features
> >>>> + * @tee_interface: pointer to the remoteproc tee context
> >>>> */
> >>>> struct rproc {
> >>>> struct list_head node;
> >>>> @@ -586,6 +589,7 @@ struct rproc {
> >>>> struct cdev cdev;
> >>>> bool cdev_put_on_release;
> >>>> DECLARE_BITMAP(features, RPROC_MAX_FEATURES);
> >>>> + struct tee_rproc *tee_interface;
> >>>> };
> >>>>
> >>>> /**
> >>>> diff --git a/include/linux/tee_remoteproc.h b/include/linux/tee_remoteproc.h
> >>>> new file mode 100644
> >>>> index 000000000000..571e47190d02
> >>>> --- /dev/null
> >>>> +++ b/include/linux/tee_remoteproc.h
> >>>> @@ -0,0 +1,112 @@
> >>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >>>> +/*
> >>>> + * Copyright(c) 2024 STMicroelectronics - All Rights Reserved
> >>>> + */
> >>>> +
> >>>> +#ifndef TEE_REMOTEPROC_H
> >>>> +#define TEE_REMOTEPROC_H
> >>>> +
> >>>> +#include <linux/tee_drv.h>
> >>>> +#include <linux/firmware.h>
> >>>> +#include <linux/remoteproc.h>
> >>>> +
> >>>> +struct rproc;
> >>>> +
> >>>> +/**
> >>>> + * struct tee_rproc - TEE remoteproc structure
> >>>> + * @node: Reference in list
> >>>> + * @rproc: Remoteproc reference
> >>>> + * @parent: Parent device
> >>>> + * @rproc_id: Identifier of the target firmware
> >>>> + * @session_id: TEE session identifier
> >>>> + * @rsc_table: Resource table virtual address.
> >>>> + */
> >>>> +struct tee_rproc {
> >>>> + struct list_head node;
> >>>> + struct rproc *rproc;
> >>>> + struct device *parent;
> >>>> + u32 rproc_id;
> >>>> + u32 session_id;
> >>>> + struct resource_table *rsc_table;
> >>>> +};
> >>>> +
> >>>> +#if IS_REACHABLE(CONFIG_TEE_REMOTEPROC)
> >>>> +
> >>>> +struct tee_rproc *tee_rproc_register(struct device *dev, struct rproc *rproc,
> >>>> + unsigned int rproc_id);
> >>>> +int tee_rproc_unregister(struct tee_rproc *trproc);
> >>>> +int tee_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw);
> >>>> +int tee_rproc_load_fw(struct rproc *rproc, const struct firmware *fw);
> >>>> +struct resource_table *tee_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz);
> >>>> +struct resource_table *tee_rproc_find_loaded_rsc_table(struct rproc *rproc,
> >>>> + const struct firmware *fw);
> >>>> +int tee_rproc_start(struct rproc *rproc);
> >>>> +int tee_rproc_stop(struct rproc *rproc);
> >>>> +
> >>>> +#else
> >>>> +
> >>>> +static inline struct tee_rproc *tee_rproc_register(struct device *dev, struct rproc *rproc,
> >>>> + unsigned int rproc_id)
> >>>> +{
> >>>> + return ERR_PTR(-ENODEV);
> >>>> +}
> >>>> +
> >>>> +static int tee_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
> >>>> +{
> >>>> + /* This shouldn't be possible */
> >>>> + WARN_ON(1);
> >>>> +
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> +static inline int tee_rproc_unregister(struct tee_rproc *trproc)
> >>>> +{
> >>>> + /* This shouldn't be possible */
> >>>> + WARN_ON(1);
> >>>> +
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> +static inline int tee_rproc_load_fw(struct rproc *rproc, const struct firmware *fw)
> >>>> +{
> >>>> + /* This shouldn't be possible */
> >>>> + WARN_ON(1);
> >>>> +
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> +static inline int tee_rproc_start(struct rproc *rproc)
> >>>> +{
> >>>> + /* This shouldn't be possible */
> >>>> + WARN_ON(1);
> >>>> +
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> +static inline int tee_rproc_stop(struct rproc *rproc)
> >>>> +{
> >>>> + /* This shouldn't be possible */
> >>>> + WARN_ON(1);
> >>>> +
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> +static inline struct resource_table *
> >>>> +tee_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz)
> >>>> +{
> >>>> + /* This shouldn't be possible */
> >>>> + WARN_ON(1);
> >>>> +
> >>>> + return NULL;
> >>>> +}
> >>>> +
> >>>> +static inline struct resource_table *
> >>>> +tee_rproc_find_loaded_rsc_table(struct rproc *rproc, const struct firmware *fw)
> >>>> +{
> >>>> + /* This shouldn't be possible */
> >>>> + WARN_ON(1);
> >>>> +
> >>>> + return NULL;
> >>>> +}
> >>>> +#endif /* CONFIG_TEE_REMOTEPROC */
> >>>> +#endif /* TEE_REMOTEPROC_H */
> >>>> --
> >>>> 2.25.1
> >>>>
^ permalink raw reply
* Re: [PATCH v4 4/4] remoteproc: stm32: Add support of an OP-TEE TA to load the firmware
From: Mathieu Poirier @ 2024-04-01 15:46 UTC (permalink / raw)
To: Arnaud POULIQUEN
Cc: Bjorn Andersson, Jens Wiklander, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-stm32, linux-arm-kernel, linux-remoteproc,
linux-kernel, op-tee, devicetree
In-Reply-To: <2cd23e93-1a3a-4128-b947-35fe2b04ccab@foss.st.com>
On Fri, Mar 29, 2024 at 11:57:43AM +0100, Arnaud POULIQUEN wrote:
>
>
> On 3/27/24 18:14, Mathieu Poirier wrote:
> > On Tue, Mar 26, 2024 at 08:31:33PM +0100, Arnaud POULIQUEN wrote:
> >>
> >>
> >> On 3/25/24 17:51, Mathieu Poirier wrote:
> >>> On Fri, Mar 08, 2024 at 03:47:08PM +0100, Arnaud Pouliquen wrote:
> >>>> The new TEE remoteproc device is used to manage remote firmware in a
> >>>> secure, trusted context. The 'st,stm32mp1-m4-tee' compatibility is
> >>>> introduced to delegate the loading of the firmware to the trusted
> >>>> execution context. In such cases, the firmware should be signed and
> >>>> adhere to the image format defined by the TEE.
> >>>>
> >>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >>>> ---
> >>>> Updates from V3:
> >>>> - remove support of the attach use case. Will be addressed in a separate
> >>>> thread,
> >>>> - add st_rproc_tee_ops::parse_fw ops,
> >>>> - inverse call of devm_rproc_alloc()and tee_rproc_register() to manage cross
> >>>> reference between the rproc struct and the tee_rproc struct in tee_rproc.c.
> >>>> ---
> >>>> drivers/remoteproc/stm32_rproc.c | 60 +++++++++++++++++++++++++++++---
> >>>> 1 file changed, 56 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
> >>>> index 8cd838df4e92..13df33c78aa2 100644
> >>>> --- a/drivers/remoteproc/stm32_rproc.c
> >>>> +++ b/drivers/remoteproc/stm32_rproc.c
> >>>> @@ -20,6 +20,7 @@
> >>>> #include <linux/remoteproc.h>
> >>>> #include <linux/reset.h>
> >>>> #include <linux/slab.h>
> >>>> +#include <linux/tee_remoteproc.h>
> >>>> #include <linux/workqueue.h>
> >>>>
> >>>> #include "remoteproc_internal.h"
> >>>> @@ -49,6 +50,9 @@
> >>>> #define M4_STATE_STANDBY 4
> >>>> #define M4_STATE_CRASH 5
> >>>>
> >>>> +/* Remote processor unique identifier aligned with the Trusted Execution Environment definitions */
> >>>
> >>> Why is this the case? At least from the kernel side it is possible to call
> >>> tee_rproc_register() with any kind of value, why is there a need to be any
> >>> kind of alignment with the TEE?
> >>
> >>
> >> The use of the proc_id is to identify a processor in case of multi co-processors.
> >>
> >
> > That is well understood.
> >
> >> For instance we can have a system with A DSP and a modem. We would use the same
> >> TEE service, but
> >
> > That too.
> >
> >> the TEE driver will probably be different, same for the signature key.
> >
> > What TEE driver are we talking about here?
>
> In OP-TEE remoteproc frameork is divided in 2 or 3 layers:
>
> - the remoteproc Trusted Application (TA) [1] which is platform agnostic
> - The remoteproc Pseudo Trusted Application (PTA) [2] which is platform
> dependent and can rely on the proc ID to retrieve the context.
> - the remoteproc driver (optional for some platforms) [3], which is in charge
> of DT parsing and platform configuration.
>
That part makes sense.
> Here TEE driver can be interpreted by remote PTA and/or platform driver.
>
I have to guess PTA means "Platform Trusted Application" but I have no
guarantee, adding to the level of (already high) confusion brought on by this
patchset.
> [1]
> https://elixir.bootlin.com/op-tee/latest/source/ta/remoteproc/src/remoteproc_core.c
> [2]
> https://elixir.bootlin.com/op-tee/latest/source/core/pta/stm32mp/remoteproc_pta.c
> [3]
> https://elixir.bootlin.com/op-tee/latest/source/core/drivers/remoteproc/stm32_remoteproc.c
>
> >
> >> In such case the proc ID allows to identify the the processor you want to address.
> >>
> >
> > That too is well understood, but there is no alignment needed with the TEE, i.e
> > the TEE application is not expecting a value of '0'. We could set
> > STM32_MP1_M4_PROC_ID to 0xDEADBEEF and things would work. This driver won't go
> > anywhere for as long as it is not the case.
>
>
> Here I suppose that you do not challenge the rproc_ID use in general, but for
> the stm32mp1 platform as we have only one remote processor. I'm right?
That is correct - I understand the need for an rproc_ID. The problem is with
the comment that states that '0' is aligned with the TEE definitions, which in
my head means hard coded value and a big red flag. What it should say is
"aligned with the TEE device tree definition".
>
> In OP-TEE the check is done here:
> https://elixir.bootlin.com/op-tee/latest/source/core/drivers/remoteproc/stm32_remoteproc.c#L64
>
> If driver does not register the proc ID an error is returned indicating that the
> feature is not supported.
>
> In case of stm32mp1 yes we could consider it as useless as we have only one
> remote proc.
>
> Nevertheless I can not guaranty that a customer will not add an external
> companion processor that uses OP-TEE to authenticate the associated firmware. As
> the trusted Application is the unique entry point. he will need the proc_id to
> identify the target at PTA level.
>
> So from my point of view having a proc ID on stm32MP1 (and on stm32mp2 that will
> reuse same driver) aligned between Linux and OP-TEE is useful.
I agree, for as long as it is not hard coded. The way remote processors are
discovered in the DT is perfectly acceptable, i.e the first remote processor is
for application X, the second for application Y...
>
> That said using a TEE_REMOTEPROC_DEFAULT_ID is something that could be
> more generic (for linux and OP-TEE). This ID could be reuse in the stm32mp
> driver and platform drivers with an unique internal remote processor.
>
I can't find the definition of TEE_REMOTEPROC_DEFAULT_ID anywhere, something
that doesn't help the confusion I referred to above.
> It that solution would be ok for you?
>
> Regards,
> Arnaud
>
>
> >
> >>
> >>>
> >>>> +#define STM32_MP1_M4_PROC_ID 0
> >>>> +
> >>>> struct stm32_syscon {
> >>>> struct regmap *map;
> >>>> u32 reg;
> >>>> @@ -257,6 +261,19 @@ static int stm32_rproc_release(struct rproc *rproc)
> >>>> return 0;
> >>>> }
> >>>>
> >>>> +static int stm32_rproc_tee_stop(struct rproc *rproc)
> >>>> +{
> >>>> + int err;
> >>>> +
> >>>> + stm32_rproc_request_shutdown(rproc);
> >>>> +
> >>>> + err = tee_rproc_stop(rproc);
> >>>> + if (err)
> >>>> + return err;
> >>>> +
> >>>> + return stm32_rproc_release(rproc);
> >>>> +}
> >>>> +
> >>>> static int stm32_rproc_prepare(struct rproc *rproc)
> >>>> {
> >>>> struct device *dev = rproc->dev.parent;
> >>>> @@ -693,8 +710,19 @@ static const struct rproc_ops st_rproc_ops = {
> >>>> .get_boot_addr = rproc_elf_get_boot_addr,
> >>>> };
> >>>>
> >>>> +static const struct rproc_ops st_rproc_tee_ops = {
> >>>> + .prepare = stm32_rproc_prepare,
> >>>> + .start = tee_rproc_start,
> >>>> + .stop = stm32_rproc_tee_stop,
> >>>> + .kick = stm32_rproc_kick,
> >>>> + .load = tee_rproc_load_fw,
> >>>> + .parse_fw = tee_rproc_parse_fw,
> >>>> + .find_loaded_rsc_table = tee_rproc_find_loaded_rsc_table,
> >>>> +};
> >>>> +
> >>>> static const struct of_device_id stm32_rproc_match[] = {
> >>>> - { .compatible = "st,stm32mp1-m4" },
> >>>> + {.compatible = "st,stm32mp1-m4",},
> >>>> + {.compatible = "st,stm32mp1-m4-tee",},
> >>>> {},
> >>>> };
> >>>> MODULE_DEVICE_TABLE(of, stm32_rproc_match);
> >>>> @@ -853,6 +881,7 @@ static int stm32_rproc_probe(struct platform_device *pdev)
> >>>> struct device *dev = &pdev->dev;
> >>>> struct stm32_rproc *ddata;
> >>>> struct device_node *np = dev->of_node;
> >>>> + struct tee_rproc *trproc = NULL;
> >>>> struct rproc *rproc;
> >>>> unsigned int state;
> >>>> int ret;
> >>>> @@ -861,9 +890,26 @@ static int stm32_rproc_probe(struct platform_device *pdev)
> >>>> if (ret)
> >>>> return ret;
> >>>>
> >>>> - rproc = devm_rproc_alloc(dev, np->name, &st_rproc_ops, NULL, sizeof(*ddata));
> >>>> - if (!rproc)
> >>>> - return -ENOMEM;
> >>>> + if (of_device_is_compatible(np, "st,stm32mp1-m4-tee")) {
> >>>> + /*
> >>>> + * Delegate the firmware management to the secure context.
> >>>> + * The firmware loaded has to be signed.
> >>>> + */
> >>>> + rproc = devm_rproc_alloc(dev, np->name, &st_rproc_tee_ops, NULL, sizeof(*ddata));
> >>>> + if (!rproc)
> >>>> + return -ENOMEM;
> >>>> +
> >>>> + trproc = tee_rproc_register(dev, rproc, STM32_MP1_M4_PROC_ID);
> >>>> + if (IS_ERR(trproc)) {
> >>>> + dev_err_probe(dev, PTR_ERR(trproc),
> >>>> + "signed firmware not supported by TEE\n");
> >>>> + return PTR_ERR(trproc);
> >>>> + }
> >>>> + } else {
> >>>> + rproc = devm_rproc_alloc(dev, np->name, &st_rproc_ops, NULL, sizeof(*ddata));
> >>>> + if (!rproc)
> >>>> + return -ENOMEM;
> >>>> + }
> >>>>
> >>>> ddata = rproc->priv;
> >>>>
> >>>> @@ -915,6 +961,9 @@ static int stm32_rproc_probe(struct platform_device *pdev)
> >>>> dev_pm_clear_wake_irq(dev);
> >>>> device_init_wakeup(dev, false);
> >>>> }
> >>>> + if (trproc)
> >>>
> >>> if (rproc->tee_interface)
> >>>
> >>>
> >>> I am done reviewing this set.
> >>
> >> Thank for your review!
> >> Arnaud
> >>
> >>>
> >>> Thanks,
> >>> Mathieu
> >>>
> >>>> + tee_rproc_unregister(trproc);
> >>>> +
> >>>> return ret;
> >>>> }
> >>>>
> >>>> @@ -935,6 +984,9 @@ static void stm32_rproc_remove(struct platform_device *pdev)
> >>>> dev_pm_clear_wake_irq(dev);
> >>>> device_init_wakeup(dev, false);
> >>>> }
> >>>> + if (rproc->tee_interface)
> >>>> + tee_rproc_unregister(rproc->tee_interface);
> >>>> +
> >>>> }
> >>>>
> >>>> static int stm32_rproc_suspend(struct device *dev)
> >>>> --
> >>>> 2.25.1
> >>>>
^ permalink raw reply
* Re: [PATCH v6 7/7] iio: accel: adxl345: Add spi-3wire option
From: Lothar Rubusch @ 2024-04-01 15:44 UTC (permalink / raw)
To: Jonathan Cameron
Cc: lars, Michael.Hennerich, robh+dt, krzysztof.kozlowski+dt,
conor+dt, linux-iio, devicetree, linux-kernel, eraretuya
In-Reply-To: <20240330152949.7e10ebcc@jic23-huawei>
On Sat, Mar 30, 2024 at 4:30 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 29 Mar 2024 00:40:30 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Add a setup function implementation to the spi module to enable spi-3wire
> > when specified in the device-tree.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > ---
> > drivers/iio/accel/adxl345.h | 1 +
> > drivers/iio/accel/adxl345_spi.c | 12 +++++++++++-
> > 2 files changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> > index e859c01d4..3d5c8719d 100644
> > --- a/drivers/iio/accel/adxl345.h
> > +++ b/drivers/iio/accel/adxl345.h
> > @@ -31,6 +31,7 @@
> > #define ADXL345_DATA_FORMAT_RANGE GENMASK(1, 0) /* Set the g range */
> > #define ADXL345_DATA_FORMAT_JUSTIFY BIT(2) /* Left-justified (MSB) mode */
> > #define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */
> > +#define ADXL345_DATA_FORMAT_SPI_3WIRE BIT(6) /* 3-wire SPI mode */
> > #define ADXL345_DATA_FORMAT_SELF_TEST BIT(7) /* Enable a self test */
> >
> > #define ADXL345_DATA_FORMAT_2G 0
> > diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
> > index 1c0513bd3..f145d5c1d 100644
> > --- a/drivers/iio/accel/adxl345_spi.c
> > +++ b/drivers/iio/accel/adxl345_spi.c
> > @@ -20,6 +20,16 @@ static const struct regmap_config adxl345_spi_regmap_config = {
> > .read_flag_mask = BIT(7) | BIT(6),
> > };
> >
> > +static int adxl345_spi_setup(struct device *dev, struct regmap *regmap)
> > +{
> > + struct spi_device *spi = container_of(dev, struct spi_device, dev);
> > +
> > + if (spi->mode & SPI_3WIRE)
> > + return regmap_write(regmap, ADXL345_REG_DATA_FORMAT,
> > + ADXL345_DATA_FORMAT_SPI_3WIRE);
> My only remaining comment on this patch set is to add equivalent of
> else
> return regmap_write(regmap, ADXL345_REG_DATA_FORMAT, 0);
>
> If the hardware had some sort of software reset, that was used,
> this wouldn't be needed as the status of those other bits would be known.
> If we leave them alone in the non 3wire path we may in the future have
> subtle issues because some other code left this in an odd state and
> we clear those other bits only for 3wire mode.
>
I see your point. Thinking over it, I came to the following: Given the
spi-3wire case, if I did a regmap_write(spi-3wire), else I did
regmap_write(0), in the i2c case I still passed NULL as setup()
function. So there would still be just a regmap_update() only in the
core module. Furthermore I see three cases: spi_setup() passed w/
3wire, spi_setu() passed w/o 3wire or NULL passed. This means there is
the same issue and more complexity. Hence, I will not do this. I think
I found something else.
What do you think about the following approach: If there is a
spi-3wire set in the device-tree, I pass the setup() function, else I
pass NULL. Then in the core module, if the setup() function is valid,
I do a regmap_update(), else the first option will be set with
regmap_write(). This makes up only two cases: setup() passed, or not -
and in either case the first call will be a regmap_write(). Thus all
bits are initialized to a defined state. I will update the patchset
later today, that you can see.
Happy Easter!
Lothar
> Jonathan
>
> > + return 0;
> > +}
> > +
> > static int adxl345_spi_probe(struct spi_device *spi)
> > {
> > struct regmap *regmap;
> > @@ -33,7 +43,7 @@ static int adxl345_spi_probe(struct spi_device *spi)
> > if (IS_ERR(regmap))
> > return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing regmap\n");
> >
> > - return adxl345_core_probe(&spi->dev, regmap, NULL);
> > + return adxl345_core_probe(&spi->dev, regmap, adxl345_spi_setup);
> > }
> >
> > static const struct adxl345_chip_info adxl345_spi_info = {
>
^ permalink raw reply
* [PATCH v2] media: dt-bindings: ovti,ov2680: Document more properties
From: Fabio Estevam @ 2024-04-01 15:43 UTC (permalink / raw)
To: sakari.ailus
Cc: rmfrfs, laurent.pinchart, hansg, robh, krzysztof.kozlowski+dt,
conor+dt, linux-media, devicetree, Fabio Estevam
From: Fabio Estevam <festevam@denx.de>
OV2680 has a single data lane MIPI interface.
Document the clock-lanes and data-lanes properties to avoid
the following dt-schema warning:
imx7s-warp.dtb: camera@36: port:endpoint: Unevaluated properties are not allowed ('clock-lanes', 'data-lanes' were unexpected)
from schema $id: http://devicetree.org/schemas/media/i2c/ovti,ov2680.yaml#
While at it, also document the link-frequencies property as recommended
by the following document:
https://www.kernel.org/doc/html/v6.9-rc1/driver-api/media/camera-sensor.html#handling-clocks
Signed-off-by: Fabio Estevam <festevam@denx.de>
---
Changes since v1:
- Keep the existing 'additionalProperties: false'. (Krzysztof)
- Also document link-frequencies.
.../bindings/media/i2c/ovti,ov2680.yaml | 20 ++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov2680.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov2680.yaml
index cf456f8d9ddc..a1cb08283818 100644
--- a/Documentation/devicetree/bindings/media/i2c/ovti,ov2680.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov2680.yaml
@@ -50,9 +50,24 @@ properties:
Definition of the regulator used as digital power supply.
port:
- $ref: /schemas/graph.yaml#/properties/port
description:
A node containing an output port node.
+ $ref: /schemas/graph.yaml#/$defs/port-base
+ additionalProperties: false
+
+ properties:
+ endpoint:
+ $ref: /schemas/media/video-interfaces.yaml#
+ unevaluatedProperties: false
+
+ properties:
+ clock-lanes:
+ const: 0
+
+ data-lanes:
+ const: 1
+
+ link-frequencies: true
required:
- compatible
@@ -89,6 +104,9 @@ examples:
port {
ov2680_to_mipi: endpoint {
remote-endpoint = <&mipi_from_sensor>;
+ clock-lanes = <0>;
+ data-lanes = <1>;
+ link-frequencies = /bits/ 64 <330000000>;
};
};
};
--
2.34.1
^ permalink raw reply related
* [PATCH RFT 10/10] arm64: dts: microchip: sparx5_pcb135: drop duplicated NOR flash
From: Krzysztof Kozlowski @ 2024-04-01 15:37 UTC (permalink / raw)
To: Conor Dooley, Nicolas Ferre, Claudiu Beznea, Rob Herring,
Krzysztof Kozlowski, Lars Povlsen, Steen Hegelund, Daniel Machon,
UNGLinuxDriver, David S. Miller, Bjarni Jonasson,
linux-arm-kernel, devicetree, linux-kernel
Cc: Krzysztof Kozlowski
In-Reply-To: <20240401153740.123978-1-krzk@kernel.org>
Since beginning the DTS extended the SPI0 in two places adding two SPI
muxes, each with same SPI NOR flash. Both used exactly the same
chip-selects, so this was clearly buggy code. Without checking in
datasheet, assume device has only one SPI NOR flash, so code was
duplicated.
Fixes dtc W=1 warnings:
sparx5_pcb135_board.dtsi:92.10-96.4: Warning (unique_unit_address_if_enabled): /axi@600000000/spi@600104000/flash@0: duplicate unit-address (also used in node /axi@600000000/spi@600104000/spi@0)
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
Not tested on hardware
---
.../boot/dts/microchip/sparx5_pcb135_board.dtsi | 16 ----------------
1 file changed, 16 deletions(-)
diff --git a/arch/arm64/boot/dts/microchip/sparx5_pcb135_board.dtsi b/arch/arm64/boot/dts/microchip/sparx5_pcb135_board.dtsi
index 20016efb3656..d64e642e3873 100644
--- a/arch/arm64/boot/dts/microchip/sparx5_pcb135_board.dtsi
+++ b/arch/arm64/boot/dts/microchip/sparx5_pcb135_board.dtsi
@@ -96,22 +96,6 @@ flash@0 {
};
};
-&spi0 {
- status = "okay";
- spi@0 {
- compatible = "spi-mux";
- mux-controls = <&mux>;
- #address-cells = <1>;
- #size-cells = <0>;
- reg = <0>; /* CS0 */
- flash@9 {
- compatible = "jedec,spi-nor";
- spi-max-frequency = <8000000>;
- reg = <0x9>; /* SPI */
- };
- };
-};
-
&sgpio1 {
status = "okay";
microchip,sgpio-port-ranges = <24 31>;
--
2.34.1
^ permalink raw reply related
* [PATCH RFT 09/10] arm64: dts: microchip: sparx5_pcb134: drop duplicated NOR flash
From: Krzysztof Kozlowski @ 2024-04-01 15:37 UTC (permalink / raw)
To: Conor Dooley, Nicolas Ferre, Claudiu Beznea, Rob Herring,
Krzysztof Kozlowski, Lars Povlsen, Steen Hegelund, Daniel Machon,
UNGLinuxDriver, David S. Miller, Bjarni Jonasson,
linux-arm-kernel, devicetree, linux-kernel
Cc: Krzysztof Kozlowski
In-Reply-To: <20240401153740.123978-1-krzk@kernel.org>
Since beginning the DTS extended the SPI0 in two places adding two SPI
muxes, each with same SPI NOR flash. Both used exactly the same
chip-selects, so this was clearly buggy code. Without checking in
datasheet, assume device has only one SPI NOR flash, so code was
duplicated.
Fixes dtc W=1 warnings:
sparx5_pcb134_board.dtsi:277.10-281.4: Warning (unique_unit_address_if_enabled): /axi@600000000/spi@600104000/flash@0: duplicate unit-address (also used in node /axi@600000000/spi@600104000/spi@0)
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
Not tested on hardware
---
.../boot/dts/microchip/sparx5_pcb134_board.dtsi | 16 ----------------
1 file changed, 16 deletions(-)
diff --git a/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi b/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi
index f165a409bc1d..dc7b59dfcb40 100644
--- a/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi
+++ b/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi
@@ -281,22 +281,6 @@ flash@0 {
};
};
-&spi0 {
- status = "okay";
- spi@0 {
- compatible = "spi-mux";
- mux-controls = <&mux>;
- #address-cells = <1>;
- #size-cells = <0>;
- reg = <0>; /* CS0 */
- flash@9 {
- compatible = "jedec,spi-nor";
- spi-max-frequency = <8000000>;
- reg = <0x9>; /* SPI */
- };
- };
-};
-
&sgpio0 {
status = "okay";
microchip,sgpio-port-ranges = <8 15>;
--
2.34.1
^ permalink raw reply related
* [PATCH 08/10] arm64: dts: microchip: sparx5_pcb135: drop LED unit addresses
From: Krzysztof Kozlowski @ 2024-04-01 15:37 UTC (permalink / raw)
To: Conor Dooley, Nicolas Ferre, Claudiu Beznea, Rob Herring,
Krzysztof Kozlowski, Lars Povlsen, Steen Hegelund, Daniel Machon,
UNGLinuxDriver, David S. Miller, Bjarni Jonasson,
linux-arm-kernel, devicetree, linux-kernel
Cc: Krzysztof Kozlowski
In-Reply-To: <20240401153740.123978-1-krzk@kernel.org>
GPIO leds should not have unit addresses (no "reg" property), as
reported by dtc W=1 warnings:
sparx5_pcb135_board.dtsi:18.9-22.5: Warning (unit_address_vs_reg): /leds/led@0: node has a unit name, but no reg or ranges property
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
.../boot/dts/microchip/sparx5_pcb135_board.dtsi | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/arch/arm64/boot/dts/microchip/sparx5_pcb135_board.dtsi b/arch/arm64/boot/dts/microchip/sparx5_pcb135_board.dtsi
index 860975ffe0a1..20016efb3656 100644
--- a/arch/arm64/boot/dts/microchip/sparx5_pcb135_board.dtsi
+++ b/arch/arm64/boot/dts/microchip/sparx5_pcb135_board.dtsi
@@ -15,42 +15,42 @@ gpio-restart {
leds {
compatible = "gpio-leds";
- led@0 {
+ led-0 {
label = "eth60:yellow";
gpios = <&sgpio_out1 28 0 GPIO_ACTIVE_LOW>;
default-state = "off";
};
- led@1 {
+ led-1 {
label = "eth60:green";
gpios = <&sgpio_out1 28 1 GPIO_ACTIVE_LOW>;
default-state = "off";
};
- led@2 {
+ led-2 {
label = "eth61:yellow";
gpios = <&sgpio_out1 29 0 GPIO_ACTIVE_LOW>;
default-state = "off";
};
- led@3 {
+ led-3 {
label = "eth61:green";
gpios = <&sgpio_out1 29 1 GPIO_ACTIVE_LOW>;
default-state = "off";
};
- led@4 {
+ led-4 {
label = "eth62:yellow";
gpios = <&sgpio_out1 30 0 GPIO_ACTIVE_LOW>;
default-state = "off";
};
- led@5 {
+ led-5 {
label = "eth62:green";
gpios = <&sgpio_out1 30 1 GPIO_ACTIVE_LOW>;
default-state = "off";
};
- led@6 {
+ led-6 {
label = "eth63:yellow";
gpios = <&sgpio_out1 31 0 GPIO_ACTIVE_LOW>;
default-state = "off";
};
- led@7 {
+ led-7 {
label = "eth63:green";
gpios = <&sgpio_out1 31 1 GPIO_ACTIVE_LOW>;
default-state = "off";
--
2.34.1
^ permalink raw reply related
* [PATCH 07/10] arm64: dts: microchip: sparx5_pcb134: drop LED unit addresses
From: Krzysztof Kozlowski @ 2024-04-01 15:37 UTC (permalink / raw)
To: Conor Dooley, Nicolas Ferre, Claudiu Beznea, Rob Herring,
Krzysztof Kozlowski, Lars Povlsen, Steen Hegelund, Daniel Machon,
UNGLinuxDriver, David S. Miller, Bjarni Jonasson,
linux-arm-kernel, devicetree, linux-kernel
Cc: Krzysztof Kozlowski
In-Reply-To: <20240401153740.123978-1-krzk@kernel.org>
GPIO leds should not have unit addresses (no "reg" property), as
reported by dtc W=1 warnings:
sparx5_pcb134_board.dtsi:18.9-21.5: Warning (unit_address_vs_reg): /leds/led@0: node has a unit name, but no reg or ranges property
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
.../dts/microchip/sparx5_pcb134_board.dtsi | 96 +++++++++----------
1 file changed, 48 insertions(+), 48 deletions(-)
diff --git a/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi b/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi
index cafec6ef0d0f..f165a409bc1d 100644
--- a/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi
+++ b/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi
@@ -15,234 +15,234 @@ gpio-restart {
leds {
compatible = "gpio-leds";
- led@0 {
+ led-0 {
label = "twr0:green";
gpios = <&sgpio_out0 8 0 GPIO_ACTIVE_LOW>;
};
- led@1 {
+ led-1 {
label = "twr0:yellow";
gpios = <&sgpio_out0 8 1 GPIO_ACTIVE_LOW>;
};
- led@2 {
+ led-2 {
label = "twr1:green";
gpios = <&sgpio_out0 9 0 GPIO_ACTIVE_LOW>;
};
- led@3 {
+ led-3 {
label = "twr1:yellow";
gpios = <&sgpio_out0 9 1 GPIO_ACTIVE_LOW>;
};
- led@4 {
+ led-4 {
label = "twr2:green";
gpios = <&sgpio_out0 10 0 GPIO_ACTIVE_LOW>;
};
- led@5 {
+ led-5 {
label = "twr2:yellow";
gpios = <&sgpio_out0 10 1 GPIO_ACTIVE_LOW>;
};
- led@6 {
+ led-6 {
label = "twr3:green";
gpios = <&sgpio_out0 11 0 GPIO_ACTIVE_LOW>;
};
- led@7 {
+ led-7 {
label = "twr3:yellow";
gpios = <&sgpio_out0 11 1 GPIO_ACTIVE_LOW>;
};
- led@8 {
+ led-8 {
label = "eth12:green";
gpios = <&sgpio_out0 12 0 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@9 {
+ led-9 {
label = "eth12:yellow";
gpios = <&sgpio_out0 12 1 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@10 {
+ led-10 {
label = "eth13:green";
gpios = <&sgpio_out0 13 0 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@11 {
+ led-11 {
label = "eth13:yellow";
gpios = <&sgpio_out0 13 1 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@12 {
+ led-12 {
label = "eth14:green";
gpios = <&sgpio_out0 14 0 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@13 {
+ led-13 {
label = "eth14:yellow";
gpios = <&sgpio_out0 14 1 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@14 {
+ led-14 {
label = "eth15:green";
gpios = <&sgpio_out0 15 0 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@15 {
+ led-15 {
label = "eth15:yellow";
gpios = <&sgpio_out0 15 1 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@16 {
+ led-16 {
label = "eth48:green";
gpios = <&sgpio_out1 16 0 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@17 {
+ led-17 {
label = "eth48:yellow";
gpios = <&sgpio_out1 16 1 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@18 {
+ led-18 {
label = "eth49:green";
gpios = <&sgpio_out1 17 0 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@19 {
+ led-19 {
label = "eth49:yellow";
gpios = <&sgpio_out1 17 1 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@20 {
+ led-20 {
label = "eth50:green";
gpios = <&sgpio_out1 18 0 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@21 {
+ led-21 {
label = "eth50:yellow";
gpios = <&sgpio_out1 18 1 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@22 {
+ led-22 {
label = "eth51:green";
gpios = <&sgpio_out1 19 0 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@23 {
+ led-23 {
label = "eth51:yellow";
gpios = <&sgpio_out1 19 1 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@24 {
+ led-24 {
label = "eth52:green";
gpios = <&sgpio_out1 20 0 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@25 {
+ led-25 {
label = "eth52:yellow";
gpios = <&sgpio_out1 20 1 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@26 {
+ led-26 {
label = "eth53:green";
gpios = <&sgpio_out1 21 0 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@27 {
+ led-27 {
label = "eth53:yellow";
gpios = <&sgpio_out1 21 1 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@28 {
+ led-28 {
label = "eth54:green";
gpios = <&sgpio_out1 22 0 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@29 {
+ led-29 {
label = "eth54:yellow";
gpios = <&sgpio_out1 22 1 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@30 {
+ led-30 {
label = "eth55:green";
gpios = <&sgpio_out1 23 0 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@31 {
+ led-31 {
label = "eth55:yellow";
gpios = <&sgpio_out1 23 1 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@32 {
+ led-32 {
label = "eth56:green";
gpios = <&sgpio_out1 24 0 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@33 {
+ led-33 {
label = "eth56:yellow";
gpios = <&sgpio_out1 24 1 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@34 {
+ led-34 {
label = "eth57:green";
gpios = <&sgpio_out1 25 0 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@35 {
+ led-35 {
label = "eth57:yellow";
gpios = <&sgpio_out1 25 1 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@36 {
+ led-36 {
label = "eth58:green";
gpios = <&sgpio_out1 26 0 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@37 {
+ led-37 {
label = "eth58:yellow";
gpios = <&sgpio_out1 26 1 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@38 {
+ led-38 {
label = "eth59:green";
gpios = <&sgpio_out1 27 0 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@39 {
+ led-39 {
label = "eth59:yellow";
gpios = <&sgpio_out1 27 1 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@40 {
+ led-40 {
label = "eth60:green";
gpios = <&sgpio_out1 28 0 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@41 {
+ led-41 {
label = "eth60:yellow";
gpios = <&sgpio_out1 28 1 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@42 {
+ led-42 {
label = "eth61:green";
gpios = <&sgpio_out1 29 0 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@43 {
+ led-43 {
label = "eth61:yellow";
gpios = <&sgpio_out1 29 1 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@44 {
+ led-44 {
label = "eth62:green";
gpios = <&sgpio_out1 30 0 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@45 {
+ led-45 {
label = "eth62:yellow";
gpios = <&sgpio_out1 30 1 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@46 {
+ led-46 {
label = "eth63:green";
gpios = <&sgpio_out1 31 0 GPIO_ACTIVE_HIGH>;
default-state = "off";
};
- led@47 {
+ led-47 {
label = "eth63:yellow";
gpios = <&sgpio_out1 31 1 GPIO_ACTIVE_HIGH>;
default-state = "off";
--
2.34.1
^ permalink raw reply related
* [PATCH 06/10] arm64: dts: microchip: sparx5_pcb135: align I2C mux node name with bindings
From: Krzysztof Kozlowski @ 2024-04-01 15:37 UTC (permalink / raw)
To: Conor Dooley, Nicolas Ferre, Claudiu Beznea, Rob Herring,
Krzysztof Kozlowski, Lars Povlsen, Steen Hegelund, Daniel Machon,
UNGLinuxDriver, David S. Miller, Bjarni Jonasson,
linux-arm-kernel, devicetree, linux-kernel
Cc: Krzysztof Kozlowski
In-Reply-To: <20240401153740.123978-1-krzk@kernel.org>
DT schema expects node names to match certain. This fixes dtbs_check
warnings like:
sparx5_pcb135_emmc.dtb: i2c0-imux@0: $nodename:0: 'i2c0-imux@0' does not match '^(i2c-?)?mux'
and dtc W=1 warnings:
sparx5_pcb135_board.dtsi:132.25-137.4: Warning (simple_bus_reg): /axi@600000000/i2c0-imux@0: missing or empty reg/ranges property
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
arch/arm64/boot/dts/microchip/sparx5_pcb135_board.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/microchip/sparx5_pcb135_board.dtsi b/arch/arm64/boot/dts/microchip/sparx5_pcb135_board.dtsi
index bf51a6e11cf1..860975ffe0a1 100644
--- a/arch/arm64/boot/dts/microchip/sparx5_pcb135_board.dtsi
+++ b/arch/arm64/boot/dts/microchip/sparx5_pcb135_board.dtsi
@@ -129,7 +129,7 @@ &sgpio2 {
};
&axi {
- i2c0_imux: i2c0-imux@0 {
+ i2c0_imux: i2c-mux {
compatible = "i2c-mux-pinctrl";
#address-cells = <1>;
#size-cells = <0>;
--
2.34.1
^ permalink raw reply related
* [PATCH 05/10] arm64: dts: microchip: sparx5_pcb134: align I2C mux node name with bindings
From: Krzysztof Kozlowski @ 2024-04-01 15:37 UTC (permalink / raw)
To: Conor Dooley, Nicolas Ferre, Claudiu Beznea, Rob Herring,
Krzysztof Kozlowski, Lars Povlsen, Steen Hegelund, Daniel Machon,
UNGLinuxDriver, David S. Miller, Bjarni Jonasson,
linux-arm-kernel, devicetree, linux-kernel
Cc: Krzysztof Kozlowski
In-Reply-To: <20240401153740.123978-1-krzk@kernel.org>
DT schema expects node names to match certain. This fixes dtbs_check
warnings like:
sparx5_pcb134_emmc.dtb: i2c0-emux@0: $nodename:0: 'i2c0-emux@0' does not match '^(i2c-?)?mux'
and dtc W=1 warnings:
sparx5_pcb134_board.dtsi:398.25-403.4: Warning (unique_unit_address_if_enabled): /axi@600000000/i2c0-imux@0: duplicate unit-address (also used in node /axi@600000000/i2c0-emux@0)
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi b/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi
index e816e6e9d62d..cafec6ef0d0f 100644
--- a/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi
+++ b/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi
@@ -395,13 +395,13 @@ i2cmux_11: i2cmux-11-pins {
};
&axi {
- i2c0_imux: i2c0-imux@0 {
+ i2c0_imux: i2c-mux-0 {
compatible = "i2c-mux-pinctrl";
#address-cells = <1>;
#size-cells = <0>;
i2c-parent = <&i2c0>;
};
- i2c0_emux: i2c0-emux@0 {
+ i2c0_emux: i2c-mux-1 {
compatible = "i2c-mux-gpio";
#address-cells = <1>;
#size-cells = <0>;
--
2.34.1
^ permalink raw reply related
* [PATCH 04/10] arm64: dts: microchip: sparx5_pcb135: add missing I2C mux unit addresses
From: Krzysztof Kozlowski @ 2024-04-01 15:37 UTC (permalink / raw)
To: Conor Dooley, Nicolas Ferre, Claudiu Beznea, Rob Herring,
Krzysztof Kozlowski, Lars Povlsen, Steen Hegelund, Daniel Machon,
UNGLinuxDriver, David S. Miller, Bjarni Jonasson,
linux-arm-kernel, devicetree, linux-kernel
Cc: Krzysztof Kozlowski
In-Reply-To: <20240401153740.123978-1-krzk@kernel.org>
The children of I2C mux should be named "i2c", according to DT schema
and bindings, and they should have unit address.
This fixes dtbs_check warnings like:
sparx5_pcb135.dtb: i2c0-imux@0: Unevaluated properties are not allowed ('#address-cells', '#size-cells', 'i2c_sfp1', 'i2c_sfp2', 'i2c_sfp3', 'i2c_sfp4' were unexpected)
and dtc W=1 warnings:
sparx5_pcb135_board.dtsi:172.23-180.4: Warning (simple_bus_reg): /axi@600000000/sfp-eth60: missing or empty reg/ranges property
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
arch/arm64/boot/dts/microchip/sparx5_pcb135_board.dtsi | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/boot/dts/microchip/sparx5_pcb135_board.dtsi b/arch/arm64/boot/dts/microchip/sparx5_pcb135_board.dtsi
index 82ce007d9959..bf51a6e11cf1 100644
--- a/arch/arm64/boot/dts/microchip/sparx5_pcb135_board.dtsi
+++ b/arch/arm64/boot/dts/microchip/sparx5_pcb135_board.dtsi
@@ -146,22 +146,22 @@ &i2c0_imux {
pinctrl-2 = <&i2cmux_s31>;
pinctrl-3 = <&i2cmux_s32>;
pinctrl-4 = <&i2cmux_pins_i>;
- i2c_sfp1: i2c_sfp1 {
+ i2c_sfp1: i2c@0 {
reg = <0x0>;
#address-cells = <1>;
#size-cells = <0>;
};
- i2c_sfp2: i2c_sfp2 {
+ i2c_sfp2: i2c@1 {
reg = <0x1>;
#address-cells = <1>;
#size-cells = <0>;
};
- i2c_sfp3: i2c_sfp3 {
+ i2c_sfp3: i2c@2 {
reg = <0x2>;
#address-cells = <1>;
#size-cells = <0>;
};
- i2c_sfp4: i2c_sfp4 {
+ i2c_sfp4: i2c@3 {
reg = <0x3>;
#address-cells = <1>;
#size-cells = <0>;
--
2.34.1
^ permalink raw reply related
* [PATCH 03/10] arm64: dts: microchip: sparx5_pcb134: add missing I2C mux unit addresses
From: Krzysztof Kozlowski @ 2024-04-01 15:37 UTC (permalink / raw)
To: Conor Dooley, Nicolas Ferre, Claudiu Beznea, Rob Herring,
Krzysztof Kozlowski, Lars Povlsen, Steen Hegelund, Daniel Machon,
UNGLinuxDriver, David S. Miller, Bjarni Jonasson,
linux-arm-kernel, devicetree, linux-kernel
Cc: Krzysztof Kozlowski
In-Reply-To: <20240401153740.123978-1-krzk@kernel.org>
The children of I2C mux should be named "i2c", according to DT schema
and bindings, and they should have unit address.
This fixes dtbs_check warnings like:
sparx5_pcb134_emmc.dtb: i2c0-imux@0: Unevaluated properties are not allowed ('#address-cells', '#size-cells', 'i2c_sfp1', ...
and dtc W=1 warnings:
sparx5_pcb134_board.dtsi:548.23-555.4: Warning (simple_bus_reg): /axi@600000000/sfp-eth12: missing or empty reg/ranges property
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
.../dts/microchip/sparx5_pcb134_board.dtsi | 40 +++++++++----------
1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi b/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi
index f3e226de5e5e..e816e6e9d62d 100644
--- a/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi
+++ b/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi
@@ -427,62 +427,62 @@ &i2c0_imux {
pinctrl-10 = <&i2cmux_10>;
pinctrl-11 = <&i2cmux_11>;
pinctrl-12 = <&i2cmux_pins_i>;
- i2c_sfp1: i2c_sfp1 {
+ i2c_sfp1: i2c@0 {
reg = <0x0>;
#address-cells = <1>;
#size-cells = <0>;
};
- i2c_sfp2: i2c_sfp2 {
+ i2c_sfp2: i2c@1 {
reg = <0x1>;
#address-cells = <1>;
#size-cells = <0>;
};
- i2c_sfp3: i2c_sfp3 {
+ i2c_sfp3: i2c@2 {
reg = <0x2>;
#address-cells = <1>;
#size-cells = <0>;
};
- i2c_sfp4: i2c_sfp4 {
+ i2c_sfp4: i2c@3 {
reg = <0x3>;
#address-cells = <1>;
#size-cells = <0>;
};
- i2c_sfp5: i2c_sfp5 {
+ i2c_sfp5: i2c@4 {
reg = <0x4>;
#address-cells = <1>;
#size-cells = <0>;
};
- i2c_sfp6: i2c_sfp6 {
+ i2c_sfp6: i2c@5 {
reg = <0x5>;
#address-cells = <1>;
#size-cells = <0>;
};
- i2c_sfp7: i2c_sfp7 {
+ i2c_sfp7: i2c@6 {
reg = <0x6>;
#address-cells = <1>;
#size-cells = <0>;
};
- i2c_sfp8: i2c_sfp8 {
+ i2c_sfp8: i2c@7 {
reg = <0x7>;
#address-cells = <1>;
#size-cells = <0>;
};
- i2c_sfp9: i2c_sfp9 {
+ i2c_sfp9: i2c@8 {
reg = <0x8>;
#address-cells = <1>;
#size-cells = <0>;
};
- i2c_sfp10: i2c_sfp10 {
+ i2c_sfp10: i2c@9 {
reg = <0x9>;
#address-cells = <1>;
#size-cells = <0>;
};
- i2c_sfp11: i2c_sfp11 {
+ i2c_sfp11: i2c@a {
reg = <0xa>;
#address-cells = <1>;
#size-cells = <0>;
};
- i2c_sfp12: i2c_sfp12 {
+ i2c_sfp12: i2c@b {
reg = <0xb>;
#address-cells = <1>;
#size-cells = <0>;
@@ -495,42 +495,42 @@ &gpio 60 GPIO_ACTIVE_HIGH
&gpio 61 GPIO_ACTIVE_HIGH
&gpio 54 GPIO_ACTIVE_HIGH>;
idle-state = <0x8>;
- i2c_sfp13: i2c_sfp13 {
+ i2c_sfp13: i2c@0 {
reg = <0x0>;
#address-cells = <1>;
#size-cells = <0>;
};
- i2c_sfp14: i2c_sfp14 {
+ i2c_sfp14: i2c@1 {
reg = <0x1>;
#address-cells = <1>;
#size-cells = <0>;
};
- i2c_sfp15: i2c_sfp15 {
+ i2c_sfp15: i2c@2 {
reg = <0x2>;
#address-cells = <1>;
#size-cells = <0>;
};
- i2c_sfp16: i2c_sfp16 {
+ i2c_sfp16: i2c@3 {
reg = <0x3>;
#address-cells = <1>;
#size-cells = <0>;
};
- i2c_sfp17: i2c_sfp17 {
+ i2c_sfp17: i2c@4 {
reg = <0x4>;
#address-cells = <1>;
#size-cells = <0>;
};
- i2c_sfp18: i2c_sfp18 {
+ i2c_sfp18: i2c@5 {
reg = <0x5>;
#address-cells = <1>;
#size-cells = <0>;
};
- i2c_sfp19: i2c_sfp19 {
+ i2c_sfp19: i2c@6 {
reg = <0x6>;
#address-cells = <1>;
#size-cells = <0>;
};
- i2c_sfp20: i2c_sfp20 {
+ i2c_sfp20: i2c@7 {
reg = <0x7>;
#address-cells = <1>;
#size-cells = <0>;
--
2.34.1
^ permalink raw reply related
* [PATCH 02/10] arm64: dts: microchip: sparx5: correct serdes unit address
From: Krzysztof Kozlowski @ 2024-04-01 15:37 UTC (permalink / raw)
To: Conor Dooley, Nicolas Ferre, Claudiu Beznea, Rob Herring,
Krzysztof Kozlowski, Lars Povlsen, Steen Hegelund, Daniel Machon,
UNGLinuxDriver, David S. Miller, Bjarni Jonasson,
linux-arm-kernel, devicetree, linux-kernel
Cc: Krzysztof Kozlowski
In-Reply-To: <20240401153740.123978-1-krzk@kernel.org>
Unit address should match "reg" property, as reported by dtc W=1
warnings:
sparx5.dtsi:463.27-468.5: Warning (simple_bus_reg): /axi@600000000/serdes@10808000: simple-bus unit address format error, expected "610808000"
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
arch/arm64/boot/dts/microchip/sparx5.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/microchip/sparx5.dtsi b/arch/arm64/boot/dts/microchip/sparx5.dtsi
index 5d820da8c69d..c3029e0abacc 100644
--- a/arch/arm64/boot/dts/microchip/sparx5.dtsi
+++ b/arch/arm64/boot/dts/microchip/sparx5.dtsi
@@ -460,7 +460,7 @@ mdio3: mdio@61101031c {
reg = <0x6 0x1101031c 0x24>;
};
- serdes: serdes@10808000 {
+ serdes: serdes@610808000 {
compatible = "microchip,sparx5-serdes";
#phy-cells = <1>;
clocks = <&sys_clk>;
--
2.34.1
^ permalink raw reply related
* [PATCH RFT 01/10] arm64: dts: microchip: sparx5: fix mdio reg
From: Krzysztof Kozlowski @ 2024-04-01 15:37 UTC (permalink / raw)
To: Conor Dooley, Nicolas Ferre, Claudiu Beznea, Rob Herring,
Krzysztof Kozlowski, Lars Povlsen, Steen Hegelund, Daniel Machon,
UNGLinuxDriver, David S. Miller, Bjarni Jonasson,
linux-arm-kernel, devicetree, linux-kernel
Cc: Krzysztof Kozlowski
Correct the reg address of mdio node to match unit address. Assume the
reg is not correct and unit address was correct, because there is
alerady node using the existing reg 0x110102d4.
sparx5.dtsi:443.25-451.5: Warning (simple_bus_reg): /axi@600000000/mdio@6110102f8: simple-bus unit address format error, expected "6110102d4"
Fixes: d0f482bb06f9 ("arm64: dts: sparx5: Add the Sparx5 switch node")
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
Not tested on hardware
---
arch/arm64/boot/dts/microchip/sparx5.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/microchip/sparx5.dtsi b/arch/arm64/boot/dts/microchip/sparx5.dtsi
index 24075cd91420..5d820da8c69d 100644
--- a/arch/arm64/boot/dts/microchip/sparx5.dtsi
+++ b/arch/arm64/boot/dts/microchip/sparx5.dtsi
@@ -447,7 +447,7 @@ mdio2: mdio@6110102f8 {
pinctrl-names = "default";
#address-cells = <1>;
#size-cells = <0>;
- reg = <0x6 0x110102d4 0x24>;
+ reg = <0x6 0x110102f8 0x24>;
};
mdio3: mdio@61101031c {
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v3] dt-bindings: mfd: twl: Convert trivial subdevices to json-schema
From: Rob Herring @ 2024-04-01 15:17 UTC (permalink / raw)
To: Andreas Kemnade
Cc: krzysztof.kozlowski+dt, linux, devicetree, wim, linux-input,
dmitry.torokhov, linux-rtc, lee, conor+dt, linux-watchdog, sre,
linux-kernel, alexandre.belloni
In-Reply-To: <20240401081831.456828-1-andreas@kemnade.info>
On Mon, 01 Apr 2024 10:18:31 +0200, Andreas Kemnade wrote:
> Convert subdevices with just an interrupt and compatbile to
> json-schema and wire up already converted subdevices.
> RTC is available in all variants, so allow it unconditionally.
> GPADC binding for TWL603X uses two different compatibles, so
> specify just the compatible and do not include it.
>
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> Acked-by: Guenter Roeck <linux@roeck-us.net>
> ---
> Changes in v3:
> - added Ack
> (apparantly many recipients did not receive the V2 patch,
> so there is a need for a resend)
>
> Changes in v2:
> - style cleanup
> - absolute paths
> - unevalutedProperties instead of additionalProperties
> due to not accepting things in if: clauses without it
>
> .../bindings/input/twl4030-pwrbutton.txt | 21 ------
> .../devicetree/bindings/mfd/ti,twl.yaml | 72 ++++++++++++++++++-
> .../devicetree/bindings/rtc/twl-rtc.txt | 11 ---
> .../bindings/watchdog/twl4030-wdt.txt | 10 ---
> 4 files changed, 71 insertions(+), 43 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/input/twl4030-pwrbutton.txt
> delete mode 100644 Documentation/devicetree/bindings/rtc/twl-rtc.txt
> delete mode 100644 Documentation/devicetree/bindings/watchdog/twl4030-wdt.txt
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH] ASoC: dt-bindings: mt2701-wm8960: Convert to dtschema
From: Rob Herring @ 2024-04-01 15:14 UTC (permalink / raw)
To: Kartik Agarwala
Cc: lgirdwood, broonie, krzysztof.kozlowski+dt, conor+dt,
matthias.bgg, angelogioacchino.delregno, linux-sound, devicetree,
linux-kernel, linux-arm-kernel, linux-mediatek
In-Reply-To: <20240401043505.40972-1-agarwala.kartik@gmail.com>
On Mon, Apr 01, 2024 at 10:05:05AM +0530, Kartik Agarwala wrote:
> Convert mt2701-wm890 bindings from text to dtschema. This is used by MediaTek mt77623a/n SoC.
Wrap lines at 75.
>
> Signed-off-by: Kartik Agarwala <agarwala.kartik@gmail.com>
> ---
> .../sound/mediatek,mt2701-wm8960.yaml | 59 +++++++++++++++++++
> .../bindings/sound/mt2701-wm8960.txt | 24 --------
> 2 files changed, 59 insertions(+), 24 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/sound/mediatek,mt2701-wm8960.yaml
> delete mode 100644 Documentation/devicetree/bindings/sound/mt2701-wm8960.txt
>
> diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt2701-wm8960.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt2701-wm8960.yaml
> new file mode 100644
> index 000000000..771f14a59
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/mediatek,mt2701-wm8960.yaml
> @@ -0,0 +1,59 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/mediatek,mt2701-wm8960.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek MT2701 with WM8960 CODEC
> +
> +maintainers:
> + - Kartik Agarwala <agarwala.kartik@gmail.com>
> +
> +properties:
> + compatible:
> + const: mediatek,mt2701-wm8960-machine
> +
> + mediatek,platform:
> + $ref: /schemas/types.yaml#/definitions/phandle
> + description: The phandle of MT2701 ASoC platform.
> +
> + audio-routing:
> + $ref: /schemas/types.yaml#/definitions/non-unique-string-array
> + description: |
Don't need '|'.
> + A list of the connections between audio components. Each entry is a
> + pair of strings, the first being the connection's sink, the second
> + being the connection's source.
> +
> + mediatek,audio-codec:
> + $ref: /schemas/types.yaml#/definitions/phandle
> + description: The phandle of the WM8960 audio codec.
> +
> + pinctrl-names:
> + const: default
> +
> + pinctrl-0: true
You can drop pinctrl properties. Those are implicitly supported.
> +
> +unevaluatedProperties: false
> +
> +required:
> + - compatible
> + - mediatek,platform
> + - audio-routing
> + - mediatek,audio-codec
> + - pinctrl-names
> + - pinctrl-0
> +
> +examples:
> + - |
> + sound {
> + compatible = "mediatek,mt2701-wm8960-machine";
> + mediatek,platform = <&afe>;
> + audio-routing =
> + "Headphone", "HP_L",
> + "Headphone", "HP_R",
> + "LINPUT1", "AMIC",
> + "RINPUT1", "AMIC";
> + mediatek,audio-codec = <&wm8960>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&aud_pins_default>;
> + };
^ permalink raw reply
* Re: [PATCH 1/3] dt-bindings: remoteproc: qcom,msm8996-mss-pil: allow glink-edge on msm8996
From: Rob Herring @ 2024-04-01 15:12 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Conor Dooley, Mathieu Poirier, linux-arm-msm, devicetree,
Konrad Dybcio, Krzysztof Kozlowski, Bjorn Andersson, Sibi Sankar,
linux-kernel, linux-remoteproc
In-Reply-To: <20240401-msm8996-remoteproc-v1-1-f02ab47fc728@linaro.org>
On Mon, 01 Apr 2024 00:10:42 +0300, Dmitry Baryshkov wrote:
> MSM8996 has limited glink support, allow glink-edge node on MSM8996
> platform.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> Documentation/devicetree/bindings/remoteproc/qcom,msm8996-mss-pil.yaml | 1 -
> 1 file changed, 1 deletion(-)
>
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* [PATCH 5/6] iio: adc: ad7173: Remove index from temp channel
From: Dumitru Ceclan via B4 Relay @ 2024-04-01 15:32 UTC (permalink / raw)
To: Ceclan Dumitru
Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
linux-iio, devicetree, linux-kernel, Dumitru Ceclan
In-Reply-To: <20240401-ad4111-v1-0-34618a9cc502@analog.com>
From: Dumitru Ceclan <dumitru.ceclan@analog.com>
Temperature channel is unique per device, index is not needed.
Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
drivers/iio/adc/ad7173.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index bf5a5b384fe2..9526585e6929 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -797,7 +797,6 @@ static const struct iio_info ad7173_info = {
static const struct iio_chan_spec ad7173_channel_template = {
.type = IIO_VOLTAGE,
- .indexed = 1,
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
BIT(IIO_CHAN_INFO_SCALE),
.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
--
2.43.0
^ permalink raw reply related
* [PATCH 6/6] iio: adc: ad7173: Add support for AD411x devices
From: Dumitru Ceclan via B4 Relay @ 2024-04-01 15:32 UTC (permalink / raw)
To: Ceclan Dumitru
Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
linux-iio, devicetree, linux-kernel, Dumitru Ceclan
In-Reply-To: <20240401-ad4111-v1-0-34618a9cc502@analog.com>
From: Dumitru Ceclan <dumitru.ceclan@analog.com>
Add support for AD4111/AD4112/AD4114/AD4115/AD4116.
The AD411X family encompasses a series of low power, low noise, 24-bit,
sigma-delta analog-to-digital converters that offer a versatile range of
specifications.
This family of ADCs integrates an analog front end suitable for processing
both fully differential and single-ended, bipolar voltage inputs
addressing a wide array of industrial and instrumentation requirements.
- All ADCs have inputs with a precision voltage divider with a division
ratio of 10.
- AD4116 has 5 low level inputs without a voltage divider.
- AD4111 and AD4112 support current inputs (0 mA to 20 mA) using a 50ohm
shunt resistor.
Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
drivers/iio/adc/ad7173.c | 224 ++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 210 insertions(+), 14 deletions(-)
diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 9526585e6929..ac32bd7dbd1e 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -1,8 +1,9 @@
// SPDX-License-Identifier: GPL-2.0+
/*
- * AD717x family SPI ADC driver
+ * AD717x and AD411x family SPI ADC driver
*
* Supported devices:
+ * AD4111/AD4112/AD4114/AD4115/AD4116
* AD7172-2/AD7172-4/AD7173-8/AD7175-2
* AD7175-8/AD7176-2/AD7177-2
*
@@ -72,6 +73,11 @@
#define AD7175_2_ID 0x0cd0
#define AD7172_4_ID 0x2050
#define AD7173_ID 0x30d0
+#define AD4111_ID 0x30d0
+#define AD4112_ID 0x30d0
+#define AD4114_ID 0x30d0
+#define AD4116_ID 0x34d0
+#define AD4115_ID 0x38d0
#define AD7175_8_ID 0x3cd0
#define AD7177_ID 0x4fd0
#define AD7173_ID_MASK GENMASK(15, 4)
@@ -120,11 +126,20 @@
#define AD7173_VOLTAGE_INT_REF_uV 2500000
#define AD7173_TEMP_SENSIIVITY_uV_per_C 477
#define AD7177_ODR_START_VALUE 0x07
+#define AD4111_SHUNT_RESISTOR_OHM 50
+#define AD4111_DIVIDER_RATIO 10
+#define AD411X_VCOM_INPUT 0X10
+#define AD4111_CURRENT_CHAN_CUTOFF 16
#define AD7173_FILTER_ODR0_MASK GENMASK(5, 0)
#define AD7173_MAX_CONFIGS 8
enum ad7173_ids {
+ ID_AD4111,
+ ID_AD4112,
+ ID_AD4114,
+ ID_AD4115,
+ ID_AD4116,
ID_AD7172_2,
ID_AD7172_4,
ID_AD7173_8,
@@ -134,16 +149,26 @@ enum ad7173_ids {
ID_AD7177_2,
};
+enum ad4111_current_channels {
+ AD4111_CURRENT_IN0P_IN0N,
+ AD4111_CURRENT_IN1P_IN1N,
+ AD4111_CURRENT_IN2P_IN2N,
+ AD4111_CURRENT_IN3P_IN3N,
+};
+
struct ad7173_device_info {
const unsigned int *sinc5_data_rates;
unsigned int num_sinc5_data_rates;
unsigned int odr_start_value;
+ unsigned int num_inputs_with_divider;
unsigned int num_channels;
unsigned int num_configs;
unsigned int num_inputs;
unsigned int clock;
unsigned int id;
char *name;
+ bool has_current_inputs;
+ bool has_vcom;
bool has_temp;
bool has_input_buf;
bool has_int_ref;
@@ -189,6 +214,24 @@ struct ad7173_state {
#endif
};
+static unsigned int ad4115_sinc5_data_rates[] = {
+ 24845000, 24845000, 20725000, 20725000, /* 0-3 */
+ 15564000, 13841000, 10390000, 10390000, /* 4-7 */
+ 4994000, 2499000, 1000000, 500000, /* 8-11 */
+ 395500, 200000, 100000, 59890, /* 12-15 */
+ 49920, 20000, 16660, 10000, /* 16-19 */
+ 5000, 2500, 2500, /* 20-22 */
+};
+
+static unsigned int ad4116_sinc5_data_rates[] = {
+ 12422360, 12422360, 12422360, 12422360, /* 0-3 */
+ 10362690, 10362690, 7782100, 6290530, /* 4-7 */
+ 5194800, 2496900, 1007600, 499900, /* 8-11 */
+ 390600, 200300, 100000, 59750, /* 12-15 */
+ 49840, 20000, 16650, 10000, /* 16-19 */
+ 5000, 2500, 1250, /* 20-22 */
+};
+
static const unsigned int ad7173_sinc5_data_rates[] = {
6211000, 6211000, 6211000, 6211000, 6211000, 6211000, 5181000, 4444000, /* 0-7 */
3115000, 2597000, 1007000, 503800, 381000, 200300, 100500, 59520, /* 8-15 */
@@ -204,7 +247,91 @@ static const unsigned int ad7175_sinc5_data_rates[] = {
5000, /* 20 */
};
+static unsigned int ad4111_current_channel_config[] = {
+ [AD4111_CURRENT_IN0P_IN0N] = 0x1E8,
+ [AD4111_CURRENT_IN1P_IN1N] = 0x1C9,
+ [AD4111_CURRENT_IN2P_IN2N] = 0x1AA,
+ [AD4111_CURRENT_IN3P_IN3N] = 0x18B,
+};
+
static const struct ad7173_device_info ad7173_device_info[] = {
+ [ID_AD4111] = {
+ .id = AD4111_ID,
+ .num_inputs_with_divider = 8,
+ .num_channels = 16,
+ .num_configs = 8,
+ .num_inputs = 8,
+ .num_gpios = 2,
+ .has_temp = true,
+ .has_vcom = true,
+ .has_input_buf = true,
+ .has_current_inputs = true,
+ .has_int_ref = true,
+ .clock = 2 * HZ_PER_MHZ,
+ .sinc5_data_rates = ad7173_sinc5_data_rates,
+ .num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+ },
+ [ID_AD4112] = {
+ .id = AD4112_ID,
+ .num_inputs_with_divider = 8,
+ .num_channels = 16,
+ .num_configs = 8,
+ .num_inputs = 8,
+ .num_gpios = 2,
+ .has_vcom = true,
+ .has_temp = true,
+ .has_input_buf = true,
+ .has_current_inputs = true,
+ .has_int_ref = true,
+ .clock = 2 * HZ_PER_MHZ,
+ .sinc5_data_rates = ad7173_sinc5_data_rates,
+ .num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+ },
+ [ID_AD4114] = {
+ .id = AD4114_ID,
+ .num_inputs_with_divider = 16,
+ .num_channels = 16,
+ .num_configs = 8,
+ .num_inputs = 16,
+ .num_gpios = 4,
+ .has_vcom = true,
+ .has_temp = true,
+ .has_input_buf = true,
+ .has_int_ref = true,
+ .clock = 2 * HZ_PER_MHZ,
+ .sinc5_data_rates = ad7173_sinc5_data_rates,
+ .num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+ },
+ [ID_AD4115] = {
+ .id = AD4115_ID,
+ .num_inputs_with_divider = 16,
+ .num_channels = 16,
+ .num_configs = 8,
+ .num_inputs = 16,
+ .num_gpios = 4,
+ .has_vcom = true,
+ .has_temp = true,
+ .has_input_buf = true,
+ .has_int_ref = true,
+ .clock = 8 * HZ_PER_MHZ,
+ .sinc5_data_rates = ad4115_sinc5_data_rates,
+ .num_sinc5_data_rates = ARRAY_SIZE(ad4115_sinc5_data_rates),
+ },
+ [ID_AD4116] = {
+ .id = AD4116_ID,
+ .num_inputs_with_divider = 11,
+ .num_channels = 16,
+ .num_configs = 8,
+ .num_inputs = 16,
+ .num_gpios = 4,
+ .has_vcom = true,
+ .has_temp = true,
+ .has_input_buf = true,
+ .has_int_ref = true,
+ .clock = 4 * HZ_PER_MHZ,
+ .sinc5_data_rates = ad4116_sinc5_data_rates,
+ .num_sinc5_data_rates = ARRAY_SIZE(ad4116_sinc5_data_rates),
+ },
[ID_AD7172_2] = {
.name = "ad7172-2",
.id = AD7172_2_ID,
@@ -670,18 +797,34 @@ static int ad7173_read_raw(struct iio_dev *indio_dev,
return IIO_VAL_INT;
case IIO_CHAN_INFO_SCALE:
- if (chan->type == IIO_TEMP) {
+
+ switch (chan->type) {
+ case IIO_TEMP:
temp = AD7173_VOLTAGE_INT_REF_uV * MILLI;
temp /= AD7173_TEMP_SENSIIVITY_uV_per_C;
*val = temp;
*val2 = chan->scan_type.realbits;
- } else {
+ break;
+ case IIO_VOLTAGE:
*val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel);
*val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar);
+
+ if (chan->channel < st->info->num_inputs_with_divider)
+ *val *= AD4111_DIVIDER_RATIO;
+ break;
+ case IIO_CURRENT:
+ *val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel);
+ *val /= AD4111_SHUNT_RESISTOR_OHM;
+ *val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar);
+ break;
+ default:
+ return -EINVAL;
}
return IIO_VAL_FRACTIONAL_LOG2;
case IIO_CHAN_INFO_OFFSET:
- if (chan->type == IIO_TEMP) {
+
+ switch (chan->type) {
+ case IIO_TEMP:
/* 0 Kelvin -> raw sample */
temp = -ABSOLUTE_ZERO_MILLICELSIUS;
temp *= AD7173_TEMP_SENSIIVITY_uV_per_C;
@@ -690,8 +833,13 @@ static int ad7173_read_raw(struct iio_dev *indio_dev,
AD7173_VOLTAGE_INT_REF_uV *
MILLI);
*val = -temp;
- } else {
+ break;
+ case IIO_VOLTAGE:
+ case IIO_CURRENT:
*val = -BIT(chan->scan_type.realbits - 1);
+ break;
+ default:
+ return -EINVAL;
}
return IIO_VAL_INT;
case IIO_CHAN_INFO_SAMP_FREQ:
@@ -909,6 +1057,24 @@ static int ad7173_register_clk_provider(struct iio_dev *indio_dev)
&st->int_clk_hw);
}
+static int ad4111_validate_current_ain(struct ad7173_state *st,
+ unsigned int ain[2])
+{
+ struct device *dev = &st->sd.spi->dev;
+
+ if (!st->info->has_current_inputs)
+ return dev_err_probe(dev, -EINVAL,
+ "Reg values equal to or higher than %d are restricted to models with current channels.\n",
+ AD4111_CURRENT_CHAN_CUTOFF);
+
+ if (ain[1] != 0 && ain[0] >= ARRAY_SIZE(ad4111_current_channel_config))
+ return dev_err_probe(dev, -EINVAL,
+ "For current channel diff-channels must be <[0-%d],0>\n",
+ ARRAY_SIZE(ad4111_current_channel_config) - 1);
+
+ return 0;
+}
+
static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
unsigned int ain[2])
{
@@ -951,7 +1117,7 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
struct device *dev = indio_dev->dev.parent;
struct iio_chan_spec *chan_arr, *chan;
unsigned int ain[2], chan_index = 0;
- int ref_sel, ret, num_channels;
+ int ref_sel, ret, reg, num_channels;
num_channels = device_get_child_node_count(dev);
@@ -1004,10 +1170,20 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
if (ret)
return ret;
- ret = ad7173_validate_voltage_ain_inputs(st, ain);
+ ret = fwnode_property_read_u32(child, "reg", ®);
if (ret)
return ret;
+ if (reg >= AD4111_CURRENT_CHAN_CUTOFF) {
+ ret = ad4111_validate_current_ain(st, ain);
+ if (ret)
+ return ret;
+ } else {
+ ret = ad7173_validate_voltage_ain_inputs(st, ain);
+ if (ret)
+ return ret;
+ }
+
ret = fwnode_property_match_property_string(child,
"adi,reference-select",
ad7173_ref_sel_str,
@@ -1028,15 +1204,22 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
*chan = ad7173_channel_template;
chan->address = chan_index;
chan->scan_index = chan_index;
- chan->channel = ain[0];
- chan->channel2 = ain[1];
- chan->differential = true;
- chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
+ if (reg >= AD4111_CURRENT_CHAN_CUTOFF) {
+ chan->type = IIO_CURRENT;
+ chan->channel = ain[0];
+ chan_st_priv->ain = ad4111_current_channel_config[ain[0]];
+ } else {
+ chan->channel = ain[0];
+ chan->channel2 = ain[1];
+ chan->differential = true;
+
+ chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
+ chan_st_priv->cfg.input_buf = st->info->has_input_buf;
+ }
+
chan_st_priv->chan_reg = chan_index;
- chan_st_priv->cfg.input_buf = st->info->has_input_buf;
chan_st_priv->cfg.odr = 0;
-
chan_st_priv->cfg.bipolar = fwnode_property_read_bool(child, "bipolar");
if (chan_st_priv->cfg.bipolar)
chan->info_mask_separate |= BIT(IIO_CHAN_INFO_OFFSET);
@@ -1167,6 +1350,14 @@ static int ad7173_probe(struct spi_device *spi)
}
static const struct of_device_id ad7173_of_match[] = {
+ { .compatible = "ad4111",
+ .data = &ad7173_device_info[ID_AD4111]},
+ { .compatible = "ad4112",
+ .data = &ad7173_device_info[ID_AD4112]},
+ { .compatible = "ad4114",
+ .data = &ad7173_device_info[ID_AD4114]},
+ { .compatible = "ad4115",
+ .data = &ad7173_device_info[ID_AD4115]},
{ .compatible = "adi,ad7172-2",
.data = &ad7173_device_info[ID_AD7172_2]},
{ .compatible = "adi,ad7172-4",
@@ -1186,6 +1377,11 @@ static const struct of_device_id ad7173_of_match[] = {
MODULE_DEVICE_TABLE(of, ad7173_of_match);
static const struct spi_device_id ad7173_id_table[] = {
+ { "ad4111", (kernel_ulong_t)&ad7173_device_info[ID_AD4111]},
+ { "ad4112", (kernel_ulong_t)&ad7173_device_info[ID_AD4112]},
+ { "ad4114", (kernel_ulong_t)&ad7173_device_info[ID_AD4114]},
+ { "ad4115", (kernel_ulong_t)&ad7173_device_info[ID_AD4115]},
+ { "ad4116", (kernel_ulong_t)&ad7173_device_info[ID_AD4116]},
{ "ad7172-2", (kernel_ulong_t)&ad7173_device_info[ID_AD7172_2]},
{ "ad7172-4", (kernel_ulong_t)&ad7173_device_info[ID_AD7172_4]},
{ "ad7173-8", (kernel_ulong_t)&ad7173_device_info[ID_AD7173_8]},
@@ -1210,5 +1406,5 @@ module_spi_driver(ad7173_driver);
MODULE_IMPORT_NS(IIO_AD_SIGMA_DELTA);
MODULE_AUTHOR("Lars-Peter Clausen <lars@metafo.de>");
MODULE_AUTHOR("Dumitru Ceclan <dumitru.ceclan@analog.com>");
-MODULE_DESCRIPTION("Analog Devices AD7172/AD7173/AD7175/AD7176 ADC driver");
+MODULE_DESCRIPTION("Analog Devices AD717x and AD411x ADC driver");
MODULE_LICENSE("GPL");
--
2.43.0
^ permalink raw reply related
* [PATCH 4/6] iio: adc: ad7173: refactor ain and vref selection
From: Dumitru Ceclan via B4 Relay @ 2024-04-01 15:32 UTC (permalink / raw)
To: Ceclan Dumitru
Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
linux-iio, devicetree, linux-kernel, Dumitru Ceclan
In-Reply-To: <20240401-ad4111-v1-0-34618a9cc502@analog.com>
From: Dumitru Ceclan <dumitru.ceclan@analog.com>
Move validation of analog inputs and reference voltage selection to
separate functions.
Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
drivers/iio/adc/ad7173.c | 59 +++++++++++++++++++++++++++++++++---------------
1 file changed, 41 insertions(+), 18 deletions(-)
diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 699bc6970790..bf5a5b384fe2 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -910,6 +910,41 @@ static int ad7173_register_clk_provider(struct iio_dev *indio_dev)
&st->int_clk_hw);
}
+static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
+ unsigned int ain[2])
+{
+ struct device *dev = &st->sd.spi->dev;
+
+ if (ain[0] >= st->info->num_inputs ||
+ ain[1] >= st->info->num_inputs)
+ return dev_err_probe(dev, -EINVAL,
+ "Input pin number out of range for pair (%d %d).\n",
+ ain[0], ain[1]);
+
+ return 0;
+}
+
+static int ad7173_validate_reference(struct ad7173_state *st, int ref_sel)
+{
+ struct device *dev = &st->sd.spi->dev;
+ int ret;
+
+ if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF && !st->info->has_int_ref)
+ return dev_err_probe(dev, -EINVAL,
+ "Internal reference is not available on current model.\n");
+
+ if (ref_sel == AD7173_SETUP_REF_SEL_EXT_REF2 && !st->info->has_ref2)
+ return dev_err_probe(dev, -EINVAL,
+ "External reference 2 is not available on current model.\n");
+
+ ret = ad7173_get_ref_voltage_milli(st, ref_sel);
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
+ "Cannot use reference %u\n", ref_sel);
+
+ return 0;
+}
+
static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
{
struct ad7173_channel *chans_st_arr, *chan_st_priv;
@@ -970,11 +1005,9 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
if (ret)
return ret;
- if (ain[0] >= st->info->num_inputs ||
- ain[1] >= st->info->num_inputs)
- return dev_err_probe(dev, -EINVAL,
- "Input pin number out of range for pair (%d %d).\n",
- ain[0], ain[1]);
+ ret = ad7173_validate_voltage_ain_inputs(st, ain);
+ if (ret)
+ return ret;
ret = fwnode_property_match_property_string(child,
"adi,reference-select",
@@ -985,19 +1018,9 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
else
ref_sel = ret;
- if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF &&
- !st->info->has_int_ref)
- return dev_err_probe(dev, -EINVAL,
- "Internal reference is not available on current model.\n");
-
- if (ref_sel == AD7173_SETUP_REF_SEL_EXT_REF2 && !st->info->has_ref2)
- return dev_err_probe(dev, -EINVAL,
- "External reference 2 is not available on current model.\n");
-
- ret = ad7173_get_ref_voltage_milli(st, ref_sel);
- if (ret < 0)
- return dev_err_probe(dev, ret,
- "Cannot use reference %u\n", ref_sel);
+ ret = ad7173_validate_reference(st, ref_sel);
+ if (ret)
+ return ret;
if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF)
st->adc_mode |= AD7173_ADC_MODE_REF_EN;
--
2.43.0
^ permalink raw reply related
* [PATCH 3/6] iio: adc: ad7173: refactor channel configuration parsing
From: Dumitru Ceclan via B4 Relay @ 2024-04-01 15:32 UTC (permalink / raw)
To: Ceclan Dumitru
Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
linux-iio, devicetree, linux-kernel, Dumitru Ceclan
In-Reply-To: <20240401-ad4111-v1-0-34618a9cc502@analog.com>
From: Dumitru Ceclan <dumitru.ceclan@analog.com>
Move configurations regarding number of channels from
*_fw_parse_device_config to *_fw_parse_channel_config.
Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
drivers/iio/adc/ad7173.c | 29 +++++++++++++++++------------
1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 8a95b1391826..699bc6970790 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -917,7 +917,23 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
struct device *dev = indio_dev->dev.parent;
struct iio_chan_spec *chan_arr, *chan;
unsigned int ain[2], chan_index = 0;
- int ref_sel, ret;
+ int ref_sel, ret, num_channels;
+
+ num_channels = device_get_child_node_count(dev);
+
+ if (st->info->has_temp)
+ num_channels++;
+
+ if (num_channels == 0)
+ return dev_err_probe(dev, -ENODATA, "No channels specified\n");
+
+ if (num_channels > st->info->num_channels)
+ return dev_err_probe(dev, -EINVAL,
+ "Too many channels specified. Maximum is %d, not including temperature channel if supported.\n",
+ st->info->num_channels);
+
+ indio_dev->num_channels = num_channels;
+ st->num_channels = num_channels;
chan_arr = devm_kcalloc(dev, sizeof(*indio_dev->channels),
st->num_channels, GFP_KERNEL);
@@ -1012,7 +1028,6 @@ static int ad7173_fw_parse_device_config(struct iio_dev *indio_dev)
{
struct ad7173_state *st = iio_priv(indio_dev);
struct device *dev = indio_dev->dev.parent;
- unsigned int num_channels;
int ret;
st->regulators[0].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_EXT_REF];
@@ -1071,16 +1086,6 @@ static int ad7173_fw_parse_device_config(struct iio_dev *indio_dev)
ad7173_sigma_delta_info.irq_line = ret;
- num_channels = device_get_child_node_count(dev);
-
- if (st->info->has_temp)
- num_channels++;
-
- if (num_channels == 0)
- return dev_err_probe(dev, -ENODATA, "No channels specified\n");
- indio_dev->num_channels = num_channels;
- st->num_channels = num_channels;
-
return ad7173_fw_parse_channel_config(indio_dev);
}
--
2.43.0
^ permalink raw reply related
* [PATCH 2/6] iio: adc: ad7173: fix buffers enablement for ad7176-2
From: Dumitru Ceclan via B4 Relay @ 2024-04-01 15:32 UTC (permalink / raw)
To: Ceclan Dumitru
Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
linux-iio, devicetree, linux-kernel, Dumitru Ceclan
In-Reply-To: <20240401-ad4111-v1-0-34618a9cc502@analog.com>
From: Dumitru Ceclan <dumitru.ceclan@analog.com>
AD7176-2 does not feature input buffers, enable buffers only on
supported models.
Fixes: cff259bf7274 ("iio: adc: ad7173: fix buffers enablement for ad7176-2")
Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
drivers/iio/adc/ad7173.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index f6d29abe1d04..8a95b1391826 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -145,6 +145,7 @@ struct ad7173_device_info {
unsigned int id;
char *name;
bool has_temp;
+ bool has_input_buf;
bool has_int_ref;
bool has_ref2;
u8 num_gpios;
@@ -212,6 +213,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
.num_configs = 4,
.num_gpios = 2,
.has_temp = true,
+ .has_input_buf = true,
.has_int_ref = true,
.clock = 2 * HZ_PER_MHZ,
.sinc5_data_rates = ad7173_sinc5_data_rates,
@@ -224,6 +226,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
.num_configs = 8,
.num_gpios = 4,
.has_temp = false,
+ .has_input_buf = true,
.has_ref2 = true,
.clock = 2 * HZ_PER_MHZ,
.sinc5_data_rates = ad7173_sinc5_data_rates,
@@ -237,6 +240,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
.num_configs = 8,
.num_gpios = 4,
.has_temp = true,
+ .has_input_buf = true,
.has_int_ref = true,
.has_ref2 = true,
.clock = 2 * HZ_PER_MHZ,
@@ -251,6 +255,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
.num_configs = 4,
.num_gpios = 2,
.has_temp = true,
+ .has_input_buf = true,
.has_int_ref = true,
.clock = 16 * HZ_PER_MHZ,
.sinc5_data_rates = ad7175_sinc5_data_rates,
@@ -263,6 +268,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
.num_configs = 8,
.num_gpios = 4,
.has_temp = true,
+ .has_input_buf = true,
.has_int_ref = true,
.has_ref2 = true,
.clock = 16 * HZ_PER_MHZ,
@@ -289,6 +295,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
.num_configs = 4,
.num_gpios = 2,
.has_temp = true,
+ .has_input_buf = true,
.has_int_ref = true,
.clock = 16 * HZ_PER_MHZ,
.odr_start_value = AD7177_ODR_START_VALUE,
@@ -932,7 +939,7 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
AD7173_CH_ADDRESS(chan_arr[chan_index].channel,
chan_arr[chan_index].channel2);
chan_st_priv->cfg.bipolar = false;
- chan_st_priv->cfg.input_buf = true;
+ chan_st_priv->cfg.input_buf = st->info->has_input_buf;
chan_st_priv->cfg.ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
st->adc_mode |= AD7173_ADC_MODE_REF_EN;
@@ -989,7 +996,7 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
chan_st_priv->chan_reg = chan_index;
- chan_st_priv->cfg.input_buf = true;
+ chan_st_priv->cfg.input_buf = st->info->has_input_buf;
chan_st_priv->cfg.odr = 0;
chan_st_priv->cfg.bipolar = fwnode_property_read_bool(child, "bipolar");
--
2.43.0
^ permalink raw reply related
* [PATCH 1/6] dt-bindings: adc: ad7173: add support for ad411x
From: Dumitru Ceclan via B4 Relay @ 2024-04-01 15:32 UTC (permalink / raw)
To: Ceclan Dumitru
Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
linux-iio, devicetree, linux-kernel, Dumitru Ceclan
In-Reply-To: <20240401-ad4111-v1-0-34618a9cc502@analog.com>
From: Dumitru Ceclan <dumitru.ceclan@analog.com>
Add support for: AD4111, AD4112, AD4114, AD4115, AD4116.
AD411x family ADCs support a VCOM pin, dedicated for single-ended usage.
AD4111/AD4112 support current channels, usage is implemented by
specifying channel reg values bigger than 15.
Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
.../devicetree/bindings/iio/adc/adi,ad7173.yaml | 59 +++++++++++++++++++++-
1 file changed, 57 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
index ea6cfcd0aff4..bba2de0a52f3 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
@@ -19,7 +19,18 @@ description: |
primarily for measurement of signals close to DC but also delivers
outstanding performance with input bandwidths out to ~10kHz.
+ Analog Devices AD411x ADC's:
+ The AD411X family encompasses a series of low power, low noise, 24-bit,
+ sigma-delta analog-to-digital converters that offer a versatile range of
+ specifications. They integrate an analog front end suitable for processing
+ fully differential/single-ended and bipolar voltage inputs.
+
Datasheets for supported chips:
+ https://www.analog.com/media/en/technical-documentation/data-sheets/AD4111.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/AD4112.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/AD4114.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/AD4115.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/AD4116.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-4.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
@@ -31,6 +42,11 @@ description: |
properties:
compatible:
enum:
+ - adi,ad4111
+ - adi,ad4112
+ - adi,ad4114
+ - adi,ad4115
+ - adi,ad4116
- adi,ad7172-2
- adi,ad7172-4
- adi,ad7173-8
@@ -125,10 +141,19 @@ patternProperties:
properties:
reg:
+ description:
+ Reg values 16-19 are only permitted for ad4111/ad4112 current channels.
minimum: 0
- maximum: 15
+ maximum: 19
diff-channels:
+ description:
+ For using current channels specify only the positive channel.
+ (IIN2+, IIN2−) -> diff-channels = <2 0>
+
+ Family AD411x supports a dedicated VCOM voltage input.
+ To select it set the second channel to 16.
+ (VIN2, VCOM) -> diff-channels = <2 16>
items:
minimum: 0
maximum: 31
@@ -166,7 +191,6 @@ allOf:
- $ref: /schemas/spi/spi-peripheral-props.yaml#
# Only ad7172-4, ad7173-8 and ad7175-8 support vref2
- # Other models have [0-3] channel registers
- if:
properties:
compatible:
@@ -187,6 +211,37 @@ allOf:
- vref
- refout-avss
- avdd
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - adi,ad4114
+ - adi,ad4115
+ - adi,ad4116
+ - adi,ad7173-8
+ - adi,ad7175-8
+ then:
+ patternProperties:
+ "^channel@[0-9a-f]$":
+ properties:
+ reg:
+ maximum: 15
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - adi,ad7172-2
+ - adi,ad7175-2
+ - adi,ad7176-2
+ - adi,ad7177-2
+ then:
+ patternProperties:
+ "^channel@[0-9a-f]$":
+ properties:
reg:
maximum: 3
--
2.43.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox