Re: [PATCH 7/9] serdev: Introduce new bus for serial attached devices

Andy Shevchenko
2017-01-07T14:02:51Z

On Fri, 2017-01-06 at 10:26 -0600, Rob Herring wrote:
> The serdev bus is designed for devices such as Bluetooth, WiFi, GPS
> and NFC connected to UARTs on host processors. Tradionally these have
> been handled with tty line disciplines, rfkill, and userspace glue
> such
> as hciattach. This approach has many drawbacks since it doesn't fit
> into the Linux driver model. Handling of sideband signals, power
> control
> and firmware loading are the main issues.
>
> This creates a serdev bus with controllers (i.e. host serial ports)
> and
> attached devices. Typically, these are point to point connections, but
> some devices have muxing protocols or a h/w mux is conceivable. Any
> muxing is not yet supported with the serdev bus.

> --- /dev/null
> +++ b/drivers/tty/serdev/core.c
> @@ -0,0 +1,388 @@
>

> +
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/idr.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/serdev.h>

Alphabetical order?

> +
> +static bool is_registered;
> +static DEFINE_IDA(ctrl_ida);
> +
> +static void serdev_device_release(struct device *dev)
> +{
> + struct serdev_device *serdev = to_serdev_device(dev);
> + kfree(serdev);
> +}
> +
> +static const struct device_type serdev_device_type = {
> + .release = serdev_device_release,
> +};
> +
> +static void serdev_ctrl_release(struct device *dev)
> +{
> + struct serdev_controller *ctrl = to_serdev_controller(dev);
> + ida_simple_remove(&ctrl_ida, ctrl->nr);
> + kfree(ctrl);
> +}
> +
> +static const struct device_type serdev_ctrl_type = {
> + .release = serdev_ctrl_release,
> +};
> +
> +static int serdev_device_match(struct device *dev, struct
> device_driver *drv)
> +{
> + return of_driver_match_device(dev, drv);
> +}
> +
>

> +int serdev_device_open(struct serdev_device *serdev)
> +{
> + struct serdev_controller *ctrl = serdev->ctrl;
> +
> + if (!ctrl || !ctrl->ops->open)
> + return 0;
> +

> + return serdev->ctrl->ops->open(ctrl);

Perhaps just ctrl->...();

> +}
> +EXPORT_SYMBOL_GPL(serdev_device_open);
> +
> +void serdev_device_close(struct serdev_device *serdev)
> +{
> + struct serdev_controller *ctrl = serdev->ctrl;
> +
> + if (ctrl && ctrl->ops->close)

Perhaps same pattern

if (!ctrl || !ctrl->ops->close)
return;

> + serdev->ctrl->ops->close(ctrl);

Just ctrl->... ?

> +}
> +EXPORT_SYMBOL_GPL(serdev_device_close);
> +
> +int serdev_device_write_buf(struct serdev_device *serdev,
> +     const unsigned char *buf, size_t count)
> +{
> + struct serdev_controller *ctrl = serdev->ctrl;
> +
> + if (!ctrl || !ctrl->ops->write_buf)
> + return 0;
> +
> + return serdev->ctrl->ops->write_buf(ctrl, buf, count);

Just ctrl->... ?

> +}
> +EXPORT_SYMBOL_GPL(serdev_device_write_buf);
> +
> +void serdev_device_write_flush(struct serdev_device *serdev)
> +{
> + struct serdev_controller *ctrl = serdev->ctrl;
> +
> + if (ctrl && ctrl->ops->write_flush)
> + serdev->ctrl->ops->write_flush(ctrl);

Both comments.

> +}
> +EXPORT_SYMBOL_GPL(serdev_device_write_flush);
> +
> +int serdev_device_write_room(struct serdev_device *serdev)
> +{
> + struct serdev_controller *ctrl = serdev->ctrl;
> +
> + if (ctrl && ctrl->ops->write_room)
> + return serdev->ctrl->ops->write_room(ctrl);
> +

Ditto.

> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(serdev_device_write_room);
> +
> +unsigned int serdev_device_set_baudrate(struct serdev_device *serdev,
> unsigned int speed)
> +{
> + struct serdev_controller *ctrl = serdev->ctrl;
> +
> + if (!ctrl || !ctrl->ops->set_baudrate)
> + return 0;
> +
> + return serdev->ctrl->ops->set_baudrate(ctrl, speed);

ctrl->...

> +
> +}
> +EXPORT_SYMBOL_GPL(serdev_device_set_baudrate);
> +
> +void serdev_device_set_flow_control(struct serdev_device *serdev,
> bool enable)
> +{
> + struct serdev_controller *ctrl = serdev->ctrl;
> +
> + if (ctrl && ctrl->ops->set_flow_control)
> + serdev->ctrl->ops->set_flow_control(ctrl, enable);

Both comments.

> +}
> +EXPORT_SYMBOL_GPL(serdev_device_set_flow_control);
> +

> +static int of_serdev_register_devices(struct serdev_controller *ctrl)
> +{
> + struct device_node *node;
> + struct serdev_device *serdev = NULL;
> + int err;
> + bool found = false;
> +
> + for_each_available_child_of_node(ctrl->dev.of_node, node) {
> + if (!of_get_property(node, "compatible", NULL))
> + continue;
> +
> + dev_dbg(&ctrl->dev, "adding child %s\n", node-
> >full_name);
> +
> + serdev = serdev_device_alloc(ctrl);
> + if (!serdev)
> + continue;
> +
> + serdev->dev.of_node = node;
> +
> + err = serdev_device_add(serdev);
> + if (err) {
> + dev_err(&serdev->dev,
> + "failure adding device. status %d\n",
> err);
> + serdev_device_put(serdev);
> + }
>

> + found = true;

Perhaps

} else if (!found)
found = true;

Otherwise if we end up with all devices not being added, called will not
know about it.

> + }
> + if (!found)
> + return -ENODEV;
> +
> + return 0;
> +}
>

+/**
> + * serdev_controller_remove(): remove an serdev controller
> + * @ctrl: controller to remove
> + *
> + * Remove a serdev controller.  Caller is responsible for calling
> + * serdev_controller_put() to discard the allocated controller.
> + */
> +void serdev_controller_remove(struct serdev_controller *ctrl)
> +{
> + int dummy;
> +
>

> + if (!ctrl)
> + return;

By the way, should we take care or caller? What is the best practice
here?

> +#include <linux/types.h>
> +#include <linux/device.h>

>

> +static inline void serdev_controller_write_wakeup(struct
> serdev_controller *ctrl)
> +{
> + if (ctrl->serdev && ctrl->serdev->ops->write_wakeup)
> + ctrl->serdev->ops->write_wakeup(ctrl->serdev);

Same comment about pattern.

> +}
> +
> +static inline int serdev_controller_receive_buf(struct
> serdev_controller *ctrl,
> +       const unsigned char
> *data,
> +       size_t count)
> +{
> + if (ctrl->serdev && ctrl->serdev->ops->receive_buf)
> + return ctrl->serdev->ops->receive_buf(ctrl->serdev,
> data, count);

Ditto.

> +
> + return 0;
> +}

--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy