Re: [PATCH drm/hisilicon v2 1/2] drm/hisilicon: Support i2c driver algorithms for bit-shift adapters

[Thomas Zimmermann]

Hi,

again a few nits below.

Am 22.09.20 um 09:03 schrieb Tian Tao:
> Adding driver implementation to support i2c driver algorithms for
> bit-shift adapters, so hibmc will using the interface provided by
> drm to read edid.
> 
> Signed-off-by: Tian Tao <tiantao6@xxxxxxxxxxxxx>

I gave this an R-b, which you should have added. The code is still the
same, so it counts.

> ---
>  drivers/gpu/drm/hisilicon/hibmc/Makefile        |  2 +-
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 25 ++++++-
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c | 99 +++++++++++++++++++++++++
>  3 files changed, 124 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> index f991327..684ef79 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
> +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> @@ -1,4 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> -hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_ttm.o
> +hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_ttm.o hibmc_drm_i2c.o
>  
>  obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc-drm.o
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> index 197485e..704f477 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> @@ -14,11 +14,23 @@
>  #ifndef HIBMC_DRM_DRV_H
>  #define HIBMC_DRM_DRV_H
>  
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c-algo-bit.h>
> +#include <linux/i2c.h>
> +
> +#include <drm/drm_edid.h>
>  #include <drm/drm_fb_helper.h>
>  #include <drm/drm_framebuffer.h>
>  
>  struct drm_device;
>  
> +struct hibmc_connector {
> +	struct drm_connector base;
> +
> +	struct i2c_adapter adapter;
> +	struct i2c_algo_bit_data bit_data;
> +};
> +
>  struct hibmc_drm_private {
>  	/* hw */
>  	void __iomem   *mmio;
> @@ -31,10 +43,20 @@ struct hibmc_drm_private {
>  	struct drm_plane primary_plane;
>  	struct drm_crtc crtc;
>  	struct drm_encoder encoder;
> -	struct drm_connector connector;
> +	struct hibmc_connector connector;
>  	bool mode_config_initialized;
>  };
>  
> +static inline struct hibmc_connector *to_hibmc_connector(struct drm_connector *connector)
> +{
> +	return container_of(connector, struct hibmc_connector, base);
> +}
> +
> +static inline struct hibmc_drm_private *to_hibmc_drm_private(struct hibmc_connector *connector)
> +{
> +	return container_of(connector, struct hibmc_drm_private, connector);
> +}

That's not how it was supposed to be. What I meant was, that the
function should take struct drm_device and return struct
hibmc_drm_private from dev->dev_private. Sorry about the confusion.

> +
>  void hibmc_set_power_mode(struct hibmc_drm_private *priv,
>  			  unsigned int power_mode);
>  void hibmc_set_current_gate(struct hibmc_drm_private *priv,
> @@ -47,6 +69,7 @@ int hibmc_mm_init(struct hibmc_drm_private *hibmc);
>  void hibmc_mm_fini(struct hibmc_drm_private *hibmc);
>  int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
>  		      struct drm_mode_create_dumb *args);
> +int hibmc_ddc_create(struct drm_device *drm_dev, struct hibmc_connector *connector);
>  
>  extern const struct drm_mode_config_funcs hibmc_mode_funcs;
>  
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
> new file mode 100644
> index 0000000..5e4674b
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/* Hisilicon Hibmc SoC drm driver
> + *
> + * Based on the bochs drm driver.
> + *
> + * Copyright (c) 2016 Huawei Limited.
> + *
> + * Author:
> + *      Tian Tao <tiantao6@xxxxxxxxxxxxx>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/pci.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_probe_helper.h>
> +
> +#include "hibmc_drm_drv.h"
> +
> +#define GPIO_DATA		0x0802A0
> +#define GPIO_DATA_DIRECTION	0x0802A4
> +
> +#define GPIO_SCL_MASK 0x1
> +#define GPIO_SDA_MASK  0x2

The indention of these two lines is still weird. The constants are not
aligned. I'd also suggest to use the BIT() macro to set the constants.

Best regards
Thomas

> +
> +static void hibmc_set_i2c_signal(void *data, u32 mask, int value)
> +{
> +	struct hibmc_connector *hibmc_connector = data;
> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(hibmc_connector);
> +	u32 tmp_dir = readl(priv->mmio + GPIO_DATA_DIRECTION);
> +
> +	if (value) {
> +		tmp_dir &= ~mask;
> +		writel(tmp_dir, priv->mmio + GPIO_DATA_DIRECTION);
> +	} else {
> +		u32 tmp_data = readl(priv->mmio + GPIO_DATA);
> +
> +		tmp_data &= ~mask;
> +		writel(tmp_data, priv->mmio + GPIO_DATA);
> +
> +		tmp_dir |= mask;
> +		writel(tmp_dir, priv->mmio + GPIO_DATA_DIRECTION);
> +	}
> +}
> +
> +static int hibmc_get_i2c_signal(void *data, u32 mask)
> +{
> +	struct hibmc_connector *hibmc_connector = data;
> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(hibmc_connector);
> +	u32 tmp_dir = readl(priv->mmio + GPIO_DATA_DIRECTION);
> +
> +	if ((tmp_dir & mask) != mask) {
> +		tmp_dir &= ~mask;
> +		writel(tmp_dir, priv->mmio + GPIO_DATA_DIRECTION);
> +	}
> +
> +	return (readl(priv->mmio + GPIO_DATA) & mask) ? 1 : 0;
> +}
> +
> +static void hibmc_ddc_setsda(void *data, int state)
> +{
> +	hibmc_set_i2c_signal(data, GPIO_SDA_MASK, state);
> +}
> +
> +static void hibmc_ddc_setscl(void *data, int state)
> +{
> +	hibmc_set_i2c_signal(data, GPIO_SCL_MASK, state);
> +}
> +
> +static int hibmc_ddc_getsda(void *data)
> +{
> +	return hibmc_get_i2c_signal(data, GPIO_SDA_MASK);
> +}
> +
> +static int hibmc_ddc_getscl(void *data)
> +{
> +	return hibmc_get_i2c_signal(data, GPIO_SCL_MASK);
> +}
> +
> +int hibmc_ddc_create(struct drm_device * drm_dev,
> +		     struct hibmc_connector *connector)
> +{
> +	connector->adapter.owner = THIS_MODULE;
> +	connector->adapter.class = I2C_CLASS_DDC;
> +	snprintf(connector->adapter.name, I2C_NAME_SIZE, "HIS i2c bit bus");
> +	connector->adapter.dev.parent = &drm_dev->pdev->dev;
> +	i2c_set_adapdata(&connector->adapter, connector);
> +	connector->adapter.algo_data = &connector->bit_data;
> +
> +	connector->bit_data.udelay = 20;
> +	connector->bit_data.timeout = usecs_to_jiffies(2000);
> +	connector->bit_data.data = connector;
> +	connector->bit_data.setsda = hibmc_ddc_setsda;
> +	connector->bit_data.setscl = hibmc_ddc_setscl;
> +	connector->bit_data.getsda = hibmc_ddc_getsda;
> +	connector->bit_data.getscl = hibmc_ddc_getscl;
> +
> +	return i2c_bit_add_bus(&connector->adapter);
> +}
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

Attachment: signature.asc
Description: OpenPGP digital signature