Add AXI PWM generator driver to mainline - MMIO regmap version #2395

threexc · 2024-01-11T13:58:59Z

This is a continuation of the work in #2363, with the following changes:

use of MMIO regmap and sanity checking of the clk rate as discussed in Add AXI PWM generator driver to mainline #2363
use of dev_err_probe in the probe and setup functions
Some cleanup (mostly with checkpatch and review from @dlech)

I've copied the previous reviewer list and added @pdp7. As before, the axi-pwmgen driver is related to the SPI offload upstreaming that @dlech has been doing in #2341 as both are required to upstream drivers like the ad400x.

Checkpatch still throws a warning for a handful of the error messages due to line length, but I have left them intact so they're searchable.

drivers/pwm/pwm-axi-pwmgen.c

Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml

drivers/pwm/Kconfig

drivers/pwm/pwm-axi-pwmgen.c

threexc · 2024-01-11T19:41:27Z

Pushed another minor change to fix the binding example.

threexc · 2024-01-12T12:49:39Z

With both reviews I think this is good to go now, but I'll leave it open until next week in case there's any more feedback.

nunojsa

Some more comments that I just realized now... With that fixed, I think we are good to go

drivers/pwm/Kconfig

drivers/pwm/pwm-axi-pwmgen.c

nunojsa

With my comment addressed...

drivers/pwm/Kconfig

threexc · 2024-01-16T20:12:48Z

drivers/pwm/pwm-axi-pwmgen.c

+#define AXI_PWMGEN_CHX_PERIOD(ch)	(AXI_PWMGEN_CH_PERIOD_BASE + (12 * (ch)))
+#define AXI_PWMGEN_CHX_DUTY(ch)		(AXI_PWMGEN_CH_DUTY_BASE + (12 * (ch)))
+#define AXI_PWMGEN_CHX_OFFSET(ch)	(AXI_PWMGEN_CH_OFFSET_BASE + (12 * (ch)))
+#define AXI_PWMGEN_TEST_DATA		0x5A0F0081


@nunojsa does this AXI_PWMGEN_TEST_DATA value have any significance? Upstream is wondering and I got here after it was added :)

Hmm no idea... My feeling is that it's just a random value to write in the scratch pad register and read it back. Don't think it has any special meaning (at least I can't find anything on the wiki page about it).

drivers/pwm/pwm-axi-pwmgen.c

dlech · 2024-01-17T21:30:31Z

drivers/pwm/Kconfig

+	tristate "Analog Devices AXI PWM generator"
+	depends on HAS_IOMEM
+	select REGMAP_MMIO
+	help


Re: Uwe's comment

Assuming you won't find the device on all machines, can you please add a
reasonable dependency to not annoy users?

It looks like most AXI drivers don't have anything like this in Kconfig, but AXI DMAC (DMA) has:

depends on MICROBLAZE || NIOS2 || ARCH_ZYNQ || ARCH_ZYNQMP || ARCH_INTEL_SOCFPGA || COMPILE_TEST

So maybe that would work?

A more future-proof way to do this would be to add some kind of ARCH_HAS_COMPATIBLE_FPGA option and have the various arches select ARCH_HAS_COMPATIBLE_FPGA and then depend on that option here. (COMPATIBLE_FPGA should be renamed to something that actually makes sense)

Not sure what kind of precedence there is for doing something like that.

That's a cool idea. I'll keep that in mind as a possible future improvement.

I think like this is pretty much what exists today. But it yes, would be a cool idea. Not sure how easy to have it in a sane way

drivers/pwm/pwm-axi-pwmgen.c

dlech · 2024-01-17T21:52:07Z

drivers/pwm/pwm-axi-pwmgen.c

+		return ret;
+
+	/* Enable the core */
+	return regmap_update_bits(regmap, AXI_PWMGEN_REG_CONFIG, AXI_PWMGEN_RESET, 0);


Does this reset the chip if it is already enabled? If so, we might want to leave it out like the other resets that were removed. Or we might be able to read the bit and only reset/enable if not already enabled.

Maybe I'm misunderstanding what's happening, but it looks like this is writing a 0 into the reset, which is already 0 by default?

0x0004 0x0010 REG_RSTN Reset and load values [1] LOAD_CONFIG WO 0x0 Loads the new values written in the config registers. [0] RESET RW 0x0 Reset, default is (0x0).

The formatting doesn't carry over well...

It isn't exactly clear to me from the docs what the intention of how this is supposed to work. But I think that writing 1 would put the IP core into reset and writing 0 takes it out of reset. So writing 0 when already out of reset is probably fine (has no side-effects). @acostina could probably clarify if I am correct or not.

In any case, it would be good to test that it doesn't have any unintended side effects (e.g unbind the driver while PWM is running and bind it again and see if it glitches).

It isn't exactly clear to me from the docs what the intention of how this is supposed to work. But I think that writing 1 would put the IP core into reset and writing 0 takes it out of reset.

Likely that... Well, we can also attach a scope (if there's some test point where to attach it) and be sure about it :)

drivers/pwm/pwm-axi-pwmgen.c

threexc · 2024-01-18T17:37:12Z

I've pushed an update to the branch covering almost everything, except for the question for @acostina and the question about Kconfig dependency (which I want to look more into).

The code handling the sysfs API uses "child" and "parent" to refer to the devices corresponding to a struct pwm or a struct pwm_chip respectively. Other parts of the pwm core use "parent" to refer to the parent device of a struct pwm_chip. So rename "child" to "pwm_dev" and "parent" to "pwmchip_dev" which better explains the semantic. Also two functions are changed to match the new names: child_to_pwm_export() -> pwmexport_from_dev() child_to_pwm_device() -> pwm_from_dev() (which have the additional advantage to start with "pwm" which gives the right scope). Additionally introduce a wrapper for dev_get_drvdata() to convert a pwmchip_dev to the respective pwm_chip. Link: https://lore.kernel.org/r/9cc05aceeae2f06ecb850bccb15ba821e768c183.1710670958.git.u.kleine-koenig@pengutronix.de Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

With the upcoming restructuring having all in a single file simplifies things a bit. The relevant and somewhat visible changes are: - Some dropped prototypes from include/linux/pwm.h that were only necessary that core.c has a declaration of the symbols defined in sysfs.c. The respective functions are static now. - The pwm class now also exists if CONFIG_SYSFS isn't enabled. Having CONFIG_SYSFS is not very relevant today, but even without it the class and device stuff still provides lifetime tracking. - Both files had an initcall, these are merged into a single one now. Instead of a big #ifdef block for CONFIG_DEBUG_FS, a single if (IS_ENABLED(CONFIG_DEBUG_FS)) is used now. This increases compile coverage a bit and is a tad nicer on the eyes. Link: https://lore.kernel.org/r/9e2d39a5280d7dda5bfc6682a8aef510148635b2.1710670958.git.u.kleine-koenig@pengutronix.de Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

It's required to not free the memory underlying a requested PWM while a consumer still has a reference to it. While currently a pwm_chip doesn't live long enough in all cases, linking the struct pwm to the pwm_chip results in the right lifetime as soon as the pwmchip is living long enough. This happens with the following commits. Note this is a breaking change for all pwm drivers that don't use pwmchip_alloc(). Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org> # for struct_size() and __counted_by() Link: https://lore.kernel.org/r/7e9e958841f049026c0023b309cc9deecf0ab61d.1710670958.git.u.kleine-koenig@pengutronix.de Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

This replaces the formerly dynamically allocated struct device. This allows to additionally use it to track the lifetime of the struct pwm_chip. Otherwise the new struct device provides the same sysfs API as was provided by the dynamic device before. Link: https://lore.kernel.org/r/35c65ea7f6de789a568ff39d7b6b4ce80de4b7dc.1710670958.git.u.kleine-koenig@pengutronix.de Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

After assigning chip = pwm->chip; the compiler is free to assume that pwm is non-NULL and so can optimize out the check for pwm against NULL. While it's probably a programming error to pass a NULL pointer to pwm_put() this shouldn't be dropped without careful consideration and wasn't intended. So assign chip only after the NULL check. Reported-by: David Lechner <dlechner@baylibre.com> Link: https://lore.kernel.org/r/66a6f562-1fdd-4e45-995a-e7995432aa0c@baylibre.com Fixes: 4c56b14 ("pwm: Add a struct device to struct pwm_chip") Link: https://lore.kernel.org/r/20240329101648.544155-2-u.kleine-koenig@pengutronix.de Reviewed-by: David Lechner <dlechner@baylibre.com> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Now that a pwm_chip has a dedicated struct device, pwmchip_set_drvdata() and pwmchip_get_drvdata() can be made thin wrappers around dev_set_drvdata() and dev_get_drvdata() respectively and the previously needed pointer can be dropped from struct pwm_chip. Link: https://lore.kernel.org/r/a5e05bd2d83421a26fdef6a87d69253c0f98becf.1710670958.git.u.kleine-koenig@pengutronix.de Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Giving an indication about the problem if probing a device fails is a nice move. Do that for the stm32 pwm driver. Reviewed-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> Link: https://lore.kernel.org/r/20240315145443.982807-2-u.kleine-koenig@pengutronix.de Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

While mathematically it's ok to calculate the number of cyles for the duty cycle as: duty_cycles = period_cycles * duty_ns / period_ns this doesn't always give the right result when doing integer math. This is best demonstrated using an example: With the input clock running at 208877930 Hz a request for duty_cycle = 383 ns and period = 49996 ns results in period_cycles = clkrate * period_ns / NSEC_PER_SEC = 10443.06098828 Now calculating duty_cycles with the above formula gives: duty_cycles = 10443.06098828 * 383 / 49996 = 80.00024719 However with period_cycle truncated to an integer results in: duty_cycles = 10443 * 383 / 49996 = 79.99977998239859 So while a value of (a little more than) 80 would be the right result, only 79 is used here. The problem here is that 14443 is a rounded result that should better not be used to do further math. So to fix that use the exact formular similar to how period_cycles is calculated. Link: https://lore.kernel.org/r/7628ecd8a7538aa5a7397f0fc4199a077168e8a6.1710711976.git.u.kleine-koenig@pengutronix.de Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

stm32_pwm_config() took the duty_cycle and period values with the type int, however stm32_pwm_apply() passed u64 values there. Expand the function parameters to u64 to not discard relevant bits and adapt the calculations to the wider type. To ensure the calculations won't overflow, check in .probe() the input clk doesn't run faster than 1 GHz. Link: https://lore.kernel.org/r/06b4a650a608d0887d934c1b2b8919e0f78e4db2.1710711976.git.u.kleine-koenig@pengutronix.de Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Instead of looping over increasing values for the prescaler and testing if it's big enough, calculate the value using a single division. Link: https://lore.kernel.org/r/498a44b313a6c0a84ccddd03cd67aadaaaf7daf2.1710711976.git.u.kleine-koenig@pengutronix.de Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

&pdev->dev is used several times in bcm2835_pwm_probe(). Introduce a local variable to simplify all usages. Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com> Link: https://lore.kernel.org/r/3f302472e30e21c7ef5624a1d0a2890d9fdf3c7f.1710078146.git.u.kleine-koenig@pengutronix.de Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Since commit b0cde62 ("clk: Add a devm variant of clk_rate_exclusive_get()") the clk subsystem provides devm_clk_rate_exclusive_get(). Replace the open coded implementation by the new function. Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com> Link: https://lore.kernel.org/r/8e1a5151a7bcd455996c873bb3d13ab86def3490.1710078146.git.u.kleine-koenig@pengutronix.de Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Introduce a new compatible support in the Amlogic PWM driver. The PWM HW is actually the same for all SoCs supported so far. A specific compatible is needed only because the clock sources of the PWMs are hard-coded in the driver. It is better to have the clock source described in DT but this changes the bindings so a new compatible must be introduced. When all supported platform have migrated to the new compatible, support for the legacy ones may be removed from the driver. The addition of this new compatible makes the old ones obsolete, as described in the DT documentation. Adding a callback to setup the clock will also make it easier to add support for the new PWM HW found in a1, s4, c3 and t7 SoC families. Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> Link: https://lore.kernel.org/r/20240221151154.26452-6-jbrunet@baylibre.com Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Add a compatible string for MediaTek Genio 350 MT8365's display PWM block: this is the same as MT8183. Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> Acked-by: Rob Herring (Arm) <robh@kernel.org> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Link: https://lore.kernel.org/r/20231023-display-support-v3-11-53388f3ed34b@baylibre.com Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Drop checking state argument for NULL pointer in meson_pwm_get_state() due to it is called only from pwm core with always valid arguments. Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com> Signed-off-by: George Stark <gnstark@salutedevices.com> Link: https://lore.kernel.org/r/20240425171253.2752877-2-gnstark@salutedevices.com Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

clk_round_rate() can return not only zero if requested frequency can not be provided but also negative error code so add check for it too. Also change type of variable holding clk_round_rate() result from unsigned long to long. It's safe due to clk_round_rate() returns long. Fixes: 329db10 ("pwm: meson: make full use of common clock framework") Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com> Signed-off-by: George Stark <gnstark@salutedevices.com> Link: https://lore.kernel.org/r/20240425171253.2752877-3-gnstark@salutedevices.com Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

While calculating frequency for the given period u64 numbers are multiplied before division what can lead to overflow in theory so use secure mul_u64_u64_div_u64() which handles overflow correctly. Fixes: 329db10 ("pwm: meson: make full use of common clock framework") Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Signed-off-by: George Stark <gnstark@salutedevices.com> Link: https://lore.kernel.org/r/20240425171253.2752877-4-gnstark@salutedevices.com Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

pwm-cells property is already required by pwm.yaml schema. Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn> Acked-by: Conor Dooley <conor.dooley@microchip.com> Link: https://lore.kernel.org/r/85d7dc606d60a7d0d1557b98ac3a600c660b7421.1714450308.git.zhoubinbin@loongson.cn Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

pwm-cells property is already required by pwm.yaml schema. Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn> Acked-by: Conor Dooley <conor.dooley@microchip.com> Link: https://lore.kernel.org/r/231df058d9cb35cfcf4bcdf4385f4ad8cb21a046.1714450308.git.zhoubinbin@loongson.cn Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

pwm-cells property is already required by pwm.yaml schema. Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn> Acked-by: Conor Dooley <conor.dooley@microchip.com> Link: https://lore.kernel.org/r/06765e0dd9a842dc51ff9c9cea93f26b8792e44b.1714450308.git.zhoubinbin@loongson.cn Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

pwm-cells property is already required by pwm.yaml schema. Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn> Acked-by: Conor Dooley <conor.dooley@microchip.com> Link: https://lore.kernel.org/r/0a0c13d6d716a52540e165e4835c0c406cc5d8ec.1714450308.git.zhoubinbin@loongson.cn Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

pwm-cells property is already required by pwm.yaml schema. Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn> Acked-by: Conor Dooley <conor.dooley@microchip.com> Link: https://lore.kernel.org/r/480bcdc37958a9d99f4b61c26a13b844a1e416df.1714450308.git.zhoubinbin@loongson.cn Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

pwm-cells property is already required by pwm.yaml schema. Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn> Acked-by: Conor Dooley <conor.dooley@microchip.com> Link: https://lore.kernel.org/r/68eaecf11db9bec7bdb90dbf38507332628a0434.1714450308.git.zhoubinbin@loongson.cn Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

…river_data to 0 The driver doesn't use the driver_data member of struct i2c_device_id, so don't explicitly initialize this member. This prepares putting driver_data in an anonymous union which requires either no initialization or named designators. But it's also a nice cleanup on its own. While add it, also remove the trailing commas after the sentinel entry. Link: https://lore.kernel.org/r/20240508130618.2148631-2-u.kleine-koenig@pengutronix.de Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Add Analog Devices AXI PWM generator. Link: https://wiki.analog.com/resources/fpga/docs/axi_pwm_gen Signed-off-by: Drew Fustini <dfustini@baylibre.com> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Co-developed-by: Trevor Gamblin <tgamblin@baylibre.com> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>

Add support for the Analog Devices AXI PWM Generator. This device is an FPGA-implemented peripheral used as PWM signal generator and can be interfaced with AXI4. The register map of this peripheral makes it possible to configure the period and duty cycle of the output signal. Link: https://wiki.analog.com/resources/fpga/docs/axi_pwm_gen Co-developed-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com> Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com> Co-developed-by: David Lechner <dlechner@baylibre.com> Signed-off-by: David Lechner <dlechner@baylibre.com> Signed-off-by: Drew Fustini <dfustini@baylibre.com> Acked-by: Nuno Sa <nuno.sa@analog.com> Co-developed-by: Trevor Gamblin <tgamblin@baylibre.com> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>

dlech · 2024-05-15T16:06:07Z

We can probably close this one since discussions are happening on the upstream mailing lists now instead of here.

threexc · 2024-05-15T16:30:40Z

Good point, closing.

threexc requested review from pdp7, dlech, mhennerich, amiclaus, nunojsa and IuliaCMoldovan January 11, 2024 13:58

nunojsa requested changes Jan 11, 2024

View reviewed changes

threexc force-pushed the tgamblin-axi-pwmgen branch from 8622d8e to 191ca12 Compare January 11, 2024 14:53

threexc requested a review from nunojsa January 11, 2024 14:58

dlech reviewed Jan 11, 2024

View reviewed changes

threexc force-pushed the tgamblin-axi-pwmgen branch 2 times, most recently from 8ae1f8f to 424e37b Compare January 11, 2024 17:52

dlech approved these changes Jan 11, 2024

View reviewed changes

threexc force-pushed the tgamblin-axi-pwmgen branch from 424e37b to 328a1c6 Compare January 11, 2024 19:41

pamolloy mentioned this pull request Jan 12, 2024

Add AXI PWM generator driver to mainline #2363

Closed

nunojsa reviewed Jan 12, 2024

View reviewed changes

drivers/pwm/Kconfig Outdated Show resolved Hide resolved

drivers/pwm/pwm-axi-pwmgen.c Outdated Show resolved Hide resolved

threexc force-pushed the tgamblin-axi-pwmgen branch from 328a1c6 to e8163e0 Compare January 12, 2024 14:00

nunojsa approved these changes Jan 12, 2024

View reviewed changes

drivers/pwm/Kconfig Outdated Show resolved Hide resolved

threexc force-pushed the tgamblin-axi-pwmgen branch from e8163e0 to 8e3626f Compare January 12, 2024 18:53

threexc commented Jan 16, 2024

View reviewed changes

threexc force-pushed the tgamblin-axi-pwmgen branch from 8e3626f to 726a7f4 Compare January 17, 2024 18:53

dlech changed the base branch from spi/for-6.8 to pwm/for-next January 17, 2024 20:01

threexc force-pushed the tgamblin-axi-pwmgen branch from 5a6fabb to 726a7f4 Compare January 17, 2024 20:04

dlech requested changes Jan 17, 2024

View reviewed changes

threexc force-pushed the tgamblin-axi-pwmgen branch from 726a7f4 to 9f7f183 Compare January 18, 2024 17:34

threexc requested a review from dlech January 18, 2024 17:39

threexc force-pushed the tgamblin-axi-pwmgen branch from 9f7f183 to a9c57f1 Compare January 18, 2024 18:49

ukleinek and others added 26 commits April 26, 2024 21:29

threexc force-pushed the tgamblin-axi-pwmgen branch from 9922b5b to 7115b50 Compare May 14, 2024 13:44

threexc closed this May 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AXI PWM generator driver to mainline - MMIO regmap version #2395

Add AXI PWM generator driver to mainline - MMIO regmap version #2395

threexc commented Jan 11, 2024 •

edited

threexc commented Jan 11, 2024

threexc commented Jan 12, 2024

nunojsa left a comment

nunojsa left a comment

threexc Jan 16, 2024

nunojsa Jan 17, 2024

dlech Jan 17, 2024

dlech Jan 17, 2024

threexc Jan 18, 2024

nunojsa Jan 19, 2024

dlech Jan 17, 2024

threexc Jan 18, 2024 •

edited

threexc Jan 18, 2024

dlech Jan 18, 2024

nunojsa Jan 19, 2024

threexc commented Jan 18, 2024

dlech commented May 15, 2024

threexc commented May 15, 2024

Add AXI PWM generator driver to mainline - MMIO regmap version #2395

Add AXI PWM generator driver to mainline - MMIO regmap version #2395

Conversation

threexc commented Jan 11, 2024 • edited

threexc commented Jan 11, 2024

threexc commented Jan 12, 2024

nunojsa left a comment

Choose a reason for hiding this comment

nunojsa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

threexc Jan 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

threexc commented Jan 18, 2024

dlech commented May 15, 2024

threexc commented May 15, 2024

threexc commented Jan 11, 2024 •

edited

threexc Jan 18, 2024 •

edited