Hi Christophe,
On Mon, 21 Aug 2023 08:02:10 +0200 Christophe JAILLET christophe.jaillet@wanadoo.fr wrote:
Le 18/08/2023 à 18:39, Christophe Leroy a écrit :
From: Herve Codina herve.codina@bootlin.com
A framer is a component in charge of an E1/T1 line interface. Connected usually to a TDM bus, it converts TDM frames to/from E1/T1 frames. It also provides information related to the E1/T1 line.
The framer framework provides a set of APIs for the framer drivers (framer provider) to create/destroy a framer and APIs for the framer users (framer consumer) to obtain a reference to the framer, and use the framer.
This basic implementation provides a framer abstraction for:
- power on/off the framer
- get the framer status (line state)
- be notified on framer status changes
- get/set the framer configuration
Signed-off-by: Herve Codina herve.codina@bootlin.com Reviewed-by: Christophe Leroy christophe.leroy@csgroup.eu Signed-off-by: Christophe Leroy christophe.leroy@csgroup.eu
Hi,
should there be a V5, some nits below.
...
+int framer_power_off(struct framer *framer) +{
- int ret;
- mutex_lock(&framer->mutex);
- if (framer->power_count == 1 && framer->ops->power_off) {
ret = framer->ops->power_off(framer);~~Useless extra space
Will be remove in the next iteration.
if (ret < 0) {dev_err(&framer->dev, "framer poweroff failed --> %d\n", ret);mutex_unlock(&framer->mutex);return ret;}- }
- --framer->power_count;
- mutex_unlock(&framer->mutex);
- framer_pm_runtime_put(framer);
- if (framer->pwr)
regulator_disable(framer->pwr);- return 0;
+}
...
+struct framer *framer_create(struct device *dev, struct device_node *node,
const struct framer_ops *ops)+{
- int ret;
- int id;
- struct framer *framer;
- if (WARN_ON(!dev))
return ERR_PTR(-EINVAL);- /* get_status() is mandatory if the provider ask for polling status */
- if (WARN_ON((ops->flags & FRAMER_FLAG_POLL_STATUS) && !ops->get_status))
return ERR_PTR(-EINVAL);- framer = kzalloc(sizeof(*framer), GFP_KERNEL);
- if (!framer)
return ERR_PTR(-ENOMEM);- id = ida_simple_get(&framer_ida, 0, 0, GFP_KERNEL);
ida_alloc()? (ida_simple_get() is deprecated)
Indeed, ida_alloc() and ida_free() will be used in the next iteration.
- if (id < 0) {
dev_err(dev, "unable to get id\n");ret = id;goto free_framer;- }
- device_initialize(&framer->dev);
- mutex_init(&framer->mutex);
- INIT_WORK(&framer->notify_status_work, framer_notify_status_work);
- INIT_DELAYED_WORK(&framer->polling_work, framer_polling_work);
- BLOCKING_INIT_NOTIFIER_HEAD(&framer->notifier_list);
- framer->dev.class = framer_class;
- framer->dev.parent = dev;
- framer->dev.of_node = node ? node : dev->of_node;
- framer->id = id;
- framer->ops = ops;
- ret = dev_set_name(&framer->dev, "framer-%s.%d", dev_name(dev), id);
- if (ret)
goto put_dev;- /* framer-supply */
- framer->pwr = regulator_get_optional(&framer->dev, "framer");
- if (IS_ERR(framer->pwr)) {
ret = PTR_ERR(framer->pwr);if (ret == -EPROBE_DEFER)goto put_dev;framer->pwr = NULL;- }
- ret = device_add(&framer->dev);
- if (ret)
goto put_dev;- if (pm_runtime_enabled(dev)) {
pm_runtime_enable(&framer->dev);pm_runtime_no_callbacks(&framer->dev);- }
- return framer;
+put_dev:
- put_device(&framer->dev); /* calls framer_release() which frees resources */
- return ERR_PTR(ret);
+free_framer:
- kfree(framer);
- return ERR_PTR(ret);
+}
...
+void framer_provider_of_unregister(struct framer_provider *framer_provider) +{
- mutex_lock(&framer_provider_mutex);
- list_del(&framer_provider->list);
- of_node_put(framer_provider->dev->of_node);
- kfree(framer_provider);
- mutex_unlock(&framer_provider_mutex);
If it make sense, of_node_put() and kfree() could maybe be out of the mutex, in order to match how things are done in __framer_provider_of_register().
Yes, it makes sense. Both of_node_put() and kfree() will be moved out of the mutex.
+}
...
+static void framer_release(struct device *dev) +{
- struct framer *framer;
- framer = dev_to_framer(dev);
- regulator_put(framer->pwr);
- ida_simple_remove(&framer_ida, framer->id);
ida_free()? (ida_simple_remove() is deprecated)
Yes
- kfree(framer);
+}
...
Thanks for the review, Hervé