[U-Boot] [PATCH 17/33] pch: Add ioctl support

[Bin Meng]

Hi Simon,

On Tue, Jan 22, 2019 at 9:14 AM Simon Glass <sjg at chromium.org> wrote:
>
> At present the PCH has 4 operations and these are reasonably widely used
> in the drivers. But sometimes we want to add rarely used operations, and
> each of these currently adds to the size of the PCH operations table.
>
> Add an ioctl() method which can be easily expanded without any more impact
> on the operations table.
>
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
>
>  drivers/pch/pch-uclass.c  | 10 ++++++++
>  drivers/pch/sandbox_pch.c | 17 ++++++++++++++
>  include/pch.h             | 48 ++++++++++++++++++++++++++++++++++++++-
>  test/dm/pch.c             | 19 ++++++++++++++++
>  4 files changed, 93 insertions(+), 1 deletion(-)
>

Reviewed-by: Bin Meng <bmeng.cn at gmail.com>

But please see comments below.

> diff --git a/drivers/pch/pch-uclass.c b/drivers/pch/pch-uclass.c
> index 831b283d7b..caf8b72803 100644
> --- a/drivers/pch/pch-uclass.c
> +++ b/drivers/pch/pch-uclass.c
> @@ -51,6 +51,16 @@ int pch_get_io_base(struct udevice *dev, u32 *iobasep)
>         return ops->get_io_base(dev, iobasep);
>  }
>
> +int pch_ioctl(struct udevice *dev, ulong req, void *data, int size)
> +{
> +       struct pch_ops *ops = pch_get_ops(dev);
> +
> +       if (!ops->ioctl)
> +               return -ENOSYS;
> +
> +       return ops->ioctl(dev, req, data, size);
> +}
> +
>  UCLASS_DRIVER(pch) = {
>         .id             = UCLASS_PCH,
>         .name           = "pch",
> diff --git a/drivers/pch/sandbox_pch.c b/drivers/pch/sandbox_pch.c
> index 2209bff8d4..43688368ed 100644
> --- a/drivers/pch/sandbox_pch.c
> +++ b/drivers/pch/sandbox_pch.c
> @@ -48,11 +48,28 @@ static int sandbox_pch_get_io_base(struct udevice *dev, u32 *iobasep)
>         return 0;
>  }
>
> +int sandbox_pch_ioctl(struct udevice *dev, enum pch_req_t req, void *data,
> +                     int size)
> +{
> +       switch (req) {
> +       case PCH_REQ_TEST1:
> +               return -ENOSYS;
> +       case PCH_REQ_TEST2:
> +               return *(char *)data;
> +       case PCH_REQ_TEST3:
> +               *(char *)data = 'x';
> +               return 1;
> +       default:
> +               return -ENOSYS;
> +       }
> +}
> +
>  static const struct pch_ops sandbox_pch_ops = {
>         .get_spi_base   = sandbox_pch_get_spi_base,
>         .set_spi_protect = sandbox_pch_set_spi_protect,
>         .get_gpio_base  = sandbox_pch_get_gpio_base,
>         .get_io_base = sandbox_pch_get_io_base,
> +       .ioctl          = sandbox_pch_ioctl,
>  };
>
>  static const struct udevice_id sandbox_pch_ids[] = {
> diff --git a/include/pch.h b/include/pch.h
> index 73994b8343..c9995702c1 100644
> --- a/include/pch.h
> +++ b/include/pch.h
> @@ -11,7 +11,20 @@
>
>  #define BIOS_CTRL_BIOSWE       BIT(0)
>
> -/* Operations for the Platform Controller Hub */
> +/* All the supported PCH ioctls */
> +enum pch_req_t {
> +       PCH_REQ_TEST1,          /* Test requests for sandbox driver */
> +       PCH_REQ_TEST2,
> +       PCH_REQ_TEST3,
> +
> +       PCH_REQ_COUNT,          /* Number of ioctrls supported */
> +};
> +
> +/**
> + * struct pch_ops - Operations for the Platform Controller Hub
> + *
> + * Consider using ioctl() to add rarely used or driver-specific operations.
> + */
>  struct pch_ops {
>         /**
>          * get_spi_base() - get the address of SPI base
> @@ -49,6 +62,23 @@ struct pch_ops {
>          * @return 0 if OK, -ve on error (e.g. there is no IO base)
>          */
>         int (*get_io_base)(struct udevice *dev, u32 *iobasep);
> +
> +       /**
> +        * ioctl() - perform misc read/write operations
> +        *
> +        * This is a catch-all operation intended to avoid adding lots of
> +        * methods to this uclass, of which few are commonly used. Uncommon
> +        * operations that pertain only to a few devices in this uclass should
> +        * use this method instead of adding new methods.
> +        *
> +        * @dev:        PCH device to check
> +        * @req:        PCH request ID
> +        * @data:       Input/output data
> +        * @size:       Size of input data (and maximum size of output data)
> +        * @return size of output data or 0 on sucesss, -ve on error

>From the sandbox ioctl implementation, it never returns 0, so this is
incorrect comment.

> +        */
> +       int (*ioctl)(struct udevice *dev, enum pch_req_t req, void *data,
> +                    int size);
>  };
>
>  #define pch_get_ops(dev)        ((struct pch_ops *)(dev)->driver->ops)
> @@ -90,4 +120,20 @@ int pch_get_gpio_base(struct udevice *dev, u32 *gbasep);
>   */
>  int pch_get_io_base(struct udevice *dev, u32 *iobasep);
>
> +/**
> + * pch_ioctl() - perform misc read/write operations
> + *
> + * This is a catch-all operation intended to avoid adding lots of
> + * methods to this uclass, of which few are commonly used. Uncommon
> + * operations that pertain only to a few devices in this uclass should
> + * use this method instead of adding new methods.
> + *
> + * @dev:       PCH device to check
> + * @req:       PCH request ID
> + * @data:      Input/output data
> + * @size:      Size of input data (and maximum size of output data)
> + * @return size of output data or 0 on sucesss, -ve on error

Ditto.

> + */
> +int pch_ioctl(struct udevice *dev, ulong req, void *data, int size);
> +
>  #endif

[snip]

Regards,
Bin