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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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