On Mon, 24 Feb 2025 19:01:45 +0100
Fabrice Gasnier <fabrice.gasnier@xxxxxxxxxxx> wrote:
> From: Olivier Moysan <olivier.moysan@xxxxxxxxxxx>
>
> Add support for STM32MP25 SoC. Use newly introduced compatible to handle
> this new HW variant. Add new trigger definitions that can be used by the
> stm32 analog-to-digital converter. Use compatible data to identify them.
>
> Signed-off-by: Olivier Moysan <olivier.moysan@xxxxxxxxxxx>
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@xxxxxxxxxxx>
Hi. I'm not really following why you can't use devm calls for the
trigger probe path and hence why you need the explicit remove.
Feels like a lot of infrastructure and I can't see why we need it.
Jonathan
> @@ -54,25 +82,49 @@ bool is_stm32_lptim_trigger(struct iio_trigger *trig)
> }
> EXPORT_SYMBOL(is_stm32_lptim_trigger);
>
> -static int stm32_lptim_setup_trig(struct stm32_lptim_trigger *priv)
> +static void stm32_lptim_unregister_triggers(struct stm32_lptim_trigger *priv)
> {
> - struct iio_trigger *trig;
> + struct iio_trigger *tr;
>
> - trig = devm_iio_trigger_alloc(priv->dev, "%s", priv->trg);
> - if (!trig)
> - return -ENOMEM;
> + list_for_each_entry(tr, &priv->tr_list, alloc_list)
> + iio_trigger_unregister(tr);
> +}
> +
> +static int stm32_lptim_register_triggers(struct stm32_lptim_trigger *priv)
> +{
> + const char * const *cur = priv->triggers;
> + int ret;
>
> - trig->dev.parent = priv->dev->parent;
> - trig->ops = &stm32_lptim_trigger_ops;
> - iio_trigger_set_drvdata(trig, priv);
> + INIT_LIST_HEAD(&priv->tr_list);
>
> - return devm_iio_trigger_register(priv->dev, trig);
> + while (cur && *cur) {
> + struct iio_trigger *trig;
> +
> + trig = devm_iio_trigger_alloc(priv->dev, "%s", *cur);
> + if (!trig)
> + return -ENOMEM;
> +
> + trig->dev.parent = priv->dev->parent;
> + trig->ops = &stm32_lptim_trigger_ops;
> + iio_trigger_set_drvdata(trig, priv);
> +
> + ret = iio_trigger_register(trig);
I'm not really following why you can't use devm_iio_trigger_register() here
and avoid your own tracking with the list below.
> + if (ret)
> + return ret;
> +
> + list_add_tail(&trig->alloc_list, &priv->tr_list);
> + cur++;
> + }
> +
> + return 0;
> }
>
> static int stm32_lptim_trigger_probe(struct platform_device *pdev)
> {
> struct stm32_lptim_trigger *priv;
> + struct stm32_lptim_cfg const *lptim_cfg;
> u32 index;
> + int ret;
>
> priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> if (!priv)
> @@ -81,23 +133,42 @@ static int stm32_lptim_trigger_probe(struct platform_device *pdev)
> if (device_property_read_u32(&pdev->dev, "reg", &index))
> return -EINVAL;
>
> - if (index >= ARRAY_SIZE(stm32_lptim_triggers))
> + lptim_cfg = device_get_match_data(&pdev->dev);
> +
> + if (index >= lptim_cfg->nb_triggers)
> return -EINVAL;
>
> priv->dev = &pdev->dev;
> - priv->trg = stm32_lptim_triggers[index];
> + priv->triggers = lptim_cfg->triggers[index];
> +
> + ret = stm32_lptim_register_triggers(priv);
> + if (ret) {
> + stm32_lptim_unregister_triggers(priv);
> + return ret;
> + }
> +
> + platform_set_drvdata(pdev, priv);
> +
> + return 0;
> +}
> +
> +static void stm32_lptim_trigger_remove(struct platform_device *pdev)
> +{
> + struct stm32_lptim_trigger *priv = platform_get_drvdata(pdev);
>
> - return stm32_lptim_setup_trig(priv);
> + stm32_lptim_unregister_triggers(priv);
Why not a devm_add_action_or_reset?
or for that matter a devm_iio_trigger_register() in the first place.
> }
>
> static const struct of_device_id stm32_lptim_trig_of_match[] = {
> - { .compatible = "st,stm32-lptimer-trigger", },
> + { .compatible = "st,stm32-lptimer-trigger", .data = (void *)&stm32mp15_lptim_cfg },
> + { .compatible = "st,stm32mp25-lptimer-trigger", .data = (void *)&stm32mp25_lptim_cfg},
Why cast away a const then pass it to a const void *?
That is I don't think the casts are needed.
> {},
> };
> MODULE_DEVICE_TABLE(of, stm32_lptim_trig_of_match);
>
> static struct platform_driver stm32_lptim_trigger_driver = {
> .probe = stm32_lptim_trigger_probe,
> + .remove = stm32_lptim_trigger_remove,
> .driver = {
> .name = "stm32-lptimer-trigger",
> .of_match_table = stm32_lptim_trig_of_match,
> diff --git a/include/linux/iio/timer/stm32-lptim-trigger.h b/include/linux/iio/timer/stm32-lptim-trigger.h
> index a34dcf6a6001..ce3cf0addb2e 100644
> --- a/include/linux/iio/timer/stm32-lptim-trigger.h
> +++ b/include/linux/iio/timer/stm32-lptim-trigger.h
> @@ -14,6 +14,15 @@
> #define LPTIM1_OUT "lptim1_out"
> #define LPTIM2_OUT "lptim2_out"
> #define LPTIM3_OUT "lptim3_out"
> +#define LPTIM4_OUT "lptim4_out"
> +#define LPTIM5_OUT "lptim5_out"
> +
> +#define LPTIM1_CH1 "lptim1_ch1"
> +#define LPTIM1_CH2 "lptim1_ch2"
> +#define LPTIM2_CH1 "lptim2_ch1"
> +#define LPTIM2_CH2 "lptim2_ch2"
> +#define LPTIM3_CH1 "lptim3_ch1"
> +#define LPTIM4_CH1 "lptim4_ch1"
>
> #if IS_REACHABLE(CONFIG_IIO_STM32_LPTIMER_TRIGGER)
> bool is_stm32_lptim_trigger(struct iio_trigger *trig);
