Hi Neil,
Thank you for the patch.
On Tuesday 17 Jan 2017 13:31:31 Neil Armstrong wrote:
> The Synopsys Designware HDMI TX Controller does not enforce register access
> on platforms instanciating it.
> The current driver supports two different types of memory-mapped flat
> register access, but in order to support the Amlogic Meson SoCs integration,
> and provide a more generic way to handle all sorts of register mapping,
> switch the register access to use the regmap infrastructure.
>
> In the case of the registers are not flat memory-mapped or does not conform
s/does/do/
> at the actual driver implementation, a regmap struct can be given in the
s/at the actual/to the current/ ?
> plat_data and be used at probe or bind.
>
> Since the AHB audio driver only uses direct memory access, using regmap only
> allows the I2S audio driver to be registered.
This sounds a bit unclear to me, how about "[...], only allow the I2C audio
driver to be registered if the device is directly memory-mapped." ?
> Signed-off-by: Neil Armstrong <narmstr...@baylibre.com>
> ---
> drivers/gpu/drm/bridge/dw-hdmi.c | 105 +++++++++++++++++++-----------------
> include/drm/bridge/dw_hdmi.h | 1 +
> 2 files changed, 57 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c
> b/drivers/gpu/drm/bridge/dw-hdmi.c index ca9d0ce..13747fe 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -20,6 +20,7 @@
> #include <linux/mutex.h>
> #include <linux/of_device.h>
> #include <linux/spinlock.h>
> +#include <linux/regmap.h>
Could you please keep the headers alphabetically sorted ?
> #include <drm/drm_of.h>
> #include <drm/drmP.h>
> @@ -167,8 +168,7 @@ struct dw_hdmi {
> unsigned int audio_n;
> bool audio_enable;
>
> - void (*write)(struct dw_hdmi *hdmi, u8 val, int offset);
> - u8 (*read)(struct dw_hdmi *hdmi, int offset);
> + struct regmap *regm;
> };
>
> #define HDMI_IH_PHY_STAT0_RX_SENSE \
> @@ -179,42 +179,23 @@ struct dw_hdmi {
> (HDMI_PHY_RX_SENSE0 | HDMI_PHY_RX_SENSE1 | \
> HDMI_PHY_RX_SENSE2 | HDMI_PHY_RX_SENSE3)
>
> -static void dw_hdmi_writel(struct dw_hdmi *hdmi, u8 val, int offset)
> -{
> - writel(val, hdmi->regs + (offset << 2));
> -}
> -
> -static u8 dw_hdmi_readl(struct dw_hdmi *hdmi, int offset)
> -{
> - return readl(hdmi->regs + (offset << 2));
> -}
> -
> -static void dw_hdmi_writeb(struct dw_hdmi *hdmi, u8 val, int offset)
> -{
> - writeb(val, hdmi->regs + offset);
> -}
> -
> -static u8 dw_hdmi_readb(struct dw_hdmi *hdmi, int offset)
> -{
> - return readb(hdmi->regs + offset);
> -}
> -
> static inline void hdmi_writeb(struct dw_hdmi *hdmi, u8 val, int offset)
Not related to this patch, but the value, offset order of arguments to the
write function has been making me cringe since the very first time I read the
code. I wonder if modifying this would be accepted.
> {
> - hdmi->write(hdmi, val, offset);
> + regmap_write(hdmi->regm, offset, val);
> }
>
> static inline u8 hdmi_readb(struct dw_hdmi *hdmi, int offset)
> {
> - return hdmi->read(hdmi, offset);
> + unsigned int val = 0;
> +
> + regmap_read(hdmi->regm, offset, &val);
> +
> + return val;
> }
>
> static void hdmi_modb(struct dw_hdmi *hdmi, u8 data, u8 mask, unsigned reg)
> {
> - u8 val = hdmi_readb(hdmi, reg) & ~mask;
> -
> - val |= data & mask;
> - hdmi_writeb(hdmi, val, reg);
> + regmap_update_bits(hdmi->regm, reg, mask, data);
> }
>
> static void hdmi_mask_writeb(struct dw_hdmi *hdmi, u8 data, unsigned int
> reg, @@ -1949,6 +1930,21 @@ static int dw_hdmi_detect_phy(struct dw_hdmi
> *hdmi) return 0;
> }
>
> +static struct regmap_config hdmi_regmap_8bit_config = {
> + .reg_bits = 32,
> + .val_bits = 8,
> + .reg_stride = 1,
> + .max_register = HDMI_I2CM_FS_SCL_LCNT_0_ADDR,
> +};
> +
> +static struct regmap_config hdmi_regmap_32bit_config = {
> + .reg_bits = 8,
> + .pad_bits = 24,
> + .val_bits = 32,
> + .reg_stride = 4,
> + .max_register = HDMI_I2CM_FS_SCL_LCNT_0_ADDR,
> +};
I believe you can make these const.
> static struct dw_hdmi *
> __dw_hdmi_probe(struct platform_device *pdev,
> const struct dw_hdmi_plat_data *plat_data)
> @@ -1958,7 +1954,8 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi)
> struct platform_device_info pdevinfo;
> struct device_node *ddc_node;
> struct dw_hdmi *hdmi;
> - struct resource *iores;
> + struct regmap_config *reg_config = &hdmi_regmap_8bit_config;
No need to assign a value at declaration time (unless the compiler is not
smart enough and complains, or is smarter than me and finds a problem where I
don't).
> + struct resource *iores = NULL;
> int irq;
> int ret;
> u32 val = 1;
> @@ -1982,20 +1979,21 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi)
> mutex_init(&hdmi->audio_mutex);
> spin_lock_init(&hdmi->audio_lock);
>
> - of_property_read_u32(np, "reg-io-width", &val);
> -
> - switch (val) {
> - case 4:
> - hdmi->write = dw_hdmi_writel;
> - hdmi->read = dw_hdmi_readl;
> - break;
> - case 1:
> - hdmi->write = dw_hdmi_writeb;
> - hdmi->read = dw_hdmi_readb;
> - break;
> - default:
> - dev_err(dev, "reg-io-width must be 1 or 4\n");
> - return ERR_PTR(-EINVAL);
> + if (plat_data->regm)
> + hdmi->regm = plat_data->regm;
You need curly braces around this statement.
> + else {
> + of_property_read_u32(np, "reg-io-width", &val);
> + switch (val) {
> + case 4:
> + reg_config = &hdmi_regmap_32bit_config;
> + break;
> + case 1:
> + reg_config = &hdmi_regmap_8bit_config;
> + break;
> + default:
> + dev_err(dev, "reg-io-width must be 1 or 4\n");
> + return ERR_PTR(-EINVAL);
> + }
> }
>
> ddc_node = of_parse_phandle(np, "ddc-i2c-bus", 0);
> @@ -2011,11 +2009,20 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi)
> dev_dbg(hdmi->dev, "no ddc property found\n");
> }
>
> - iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - hdmi->regs = devm_ioremap_resource(dev, iores);
> - if (IS_ERR(hdmi->regs)) {
> - ret = PTR_ERR(hdmi->regs);
> - goto err_res;
> + if (!plat_data->regm) {
> + iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + hdmi->regs = devm_ioremap_resource(dev, iores);
> + if (IS_ERR(hdmi->regs)) {
> + ret = PTR_ERR(hdmi->regs);
> + goto err_res;
> + }
> +
> + hdmi->regm = devm_regmap_init_mmio(dev, hdmi->regs,
reg_config);
> + if (IS_ERR(hdmi->regm)) {
> + dev_err(dev, "Failed to configure regmap\n");
> + ret = PTR_ERR(hdmi->regm);
> + goto err_res;
> + }
> }
>
> hdmi->isfr_clk = devm_clk_get(hdmi->dev, "isfr");
> @@ -2123,7 +2130,7 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi)
> config0 = hdmi_readb(hdmi, HDMI_CONFIG0_ID);
> config3 = hdmi_readb(hdmi, HDMI_CONFIG3_ID);
>
> - if (config3 & HDMI_CONFIG3_AHBAUDDMA) {
> + if (iores && config3 & HDMI_CONFIG3_AHBAUDDMA) {
You test !plat->regm above, and iores here. How about standardizing that ? If
you test for !plat->regm here, you won't have to initialize iores to NULL.
Apart from these small issues the patch looks good to me.
> struct dw_hdmi_audio_data audio;
>
> audio.phys = iores->start;
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index 735a8ab..163842d 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -60,6 +60,7 @@ struct dw_hdmi_plat_data {
> unsigned long mpixelclock);
> enum drm_mode_status (*mode_valid)(struct drm_connector *connector,
> struct drm_display_mode *mode);
> + struct regmap *regm;
> };
>
> int dw_hdmi_probe(struct platform_device *pdev,
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
