From mboxrd@z Thu Jan 1 00:00:00 1970 From: laurent.pinchart@ideasonboard.com (Laurent Pinchart) Date: Tue, 17 Jan 2017 16:39:32 +0200 Subject: [RFC/RFT PATCH 1/4] drm/bridge: dw-hdmi: Switch to regmap for register access In-Reply-To: <1484656294-6140-2-git-send-email-narmstrong@baylibre.com> References: <1484656294-6140-1-git-send-email-narmstrong@baylibre.com> <1484656294-6140-2-git-send-email-narmstrong@baylibre.com> Message-ID: <5238949.NqUKITq39o@avalon> To: linus-amlogic@lists.infradead.org List-Id: linus-amlogic.lists.infradead.org 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 > --- > 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 > #include > #include > +#include Could you please keep the headers alphabetically sorted ? > #include > #include > @@ -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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [RFC/RFT PATCH 1/4] drm/bridge: dw-hdmi: Switch to regmap for register access Date: Tue, 17 Jan 2017 16:39:32 +0200 Message-ID: <5238949.NqUKITq39o@avalon> References: <1484656294-6140-1-git-send-email-narmstrong@baylibre.com> <1484656294-6140-2-git-send-email-narmstrong@baylibre.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from galahad.ideasonboard.com (galahad.ideasonboard.com [IPv6:2001:4b98:dc2:45:216:3eff:febb:480d]) by gabe.freedesktop.org (Postfix) with ESMTPS id E35AA6E68A for ; Tue, 17 Jan 2017 14:39:16 +0000 (UTC) In-Reply-To: <1484656294-6140-2-git-send-email-narmstrong@baylibre.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Neil Armstrong Cc: Jose.Abreu@synopsys.com, laurent.pinchart+renesas@ideasonboard.com, kieran.bingham@ideasonboard.com, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, linux-amlogic@lists.infradead.org List-Id: dri-devel@lists.freedesktop.org SGkgTmVpbCwKClRoYW5rIHlvdSBmb3IgdGhlIHBhdGNoLgoKT24gVHVlc2RheSAxNyBKYW4gMjAx NyAxMzozMTozMSBOZWlsIEFybXN0cm9uZyB3cm90ZToKPiBUaGUgU3lub3BzeXMgRGVzaWdud2Fy ZSBIRE1JIFRYIENvbnRyb2xsZXIgZG9lcyBub3QgZW5mb3JjZSByZWdpc3RlciBhY2Nlc3MKPiBv biBwbGF0Zm9ybXMgaW5zdGFuY2lhdGluZyBpdC4KPiBUaGUgY3VycmVudCBkcml2ZXIgc3VwcG9y dHMgdHdvIGRpZmZlcmVudCB0eXBlcyBvZiBtZW1vcnktbWFwcGVkIGZsYXQKPiByZWdpc3RlciBh Y2Nlc3MsIGJ1dCBpbiBvcmRlciB0byBzdXBwb3J0IHRoZSBBbWxvZ2ljIE1lc29uIFNvQ3MgaW50 ZWdyYXRpb24sCj4gYW5kIHByb3ZpZGUgYSBtb3JlIGdlbmVyaWMgd2F5IHRvIGhhbmRsZSBhbGwg c29ydHMgb2YgcmVnaXN0ZXIgbWFwcGluZywKPiBzd2l0Y2ggdGhlIHJlZ2lzdGVyIGFjY2VzcyB0 byB1c2UgdGhlIHJlZ21hcCBpbmZyYXN0cnVjdHVyZS4KPiAKPiBJbiB0aGUgY2FzZSBvZiB0aGUg cmVnaXN0ZXJzIGFyZSBub3QgZmxhdCBtZW1vcnktbWFwcGVkIG9yIGRvZXMgbm90IGNvbmZvcm0K CnMvZG9lcy9kby8KCj4gYXQgdGhlIGFjdHVhbCBkcml2ZXIgaW1wbGVtZW50YXRpb24sIGEgcmVn bWFwIHN0cnVjdCBjYW4gYmUgZ2l2ZW4gaW4gdGhlCgpzL2F0IHRoZSBhY3R1YWwvdG8gdGhlIGN1 cnJlbnQvID8KCj4gcGxhdF9kYXRhIGFuZCBiZSB1c2VkIGF0IHByb2JlIG9yIGJpbmQuCj4gCj4g U2luY2UgdGhlIEFIQiBhdWRpbyBkcml2ZXIgb25seSB1c2VzIGRpcmVjdCBtZW1vcnkgYWNjZXNz LCB1c2luZyByZWdtYXAgb25seQo+IGFsbG93cyB0aGUgSTJTIGF1ZGlvIGRyaXZlciB0byBiZSBy ZWdpc3RlcmVkLgoKVGhpcyBzb3VuZHMgYSBiaXQgdW5jbGVhciB0byBtZSwgaG93IGFib3V0ICJb Li4uXSwgb25seSBhbGxvdyB0aGUgSTJDIGF1ZGlvIApkcml2ZXIgdG8gYmUgcmVnaXN0ZXJlZCBp ZiB0aGUgZGV2aWNlIGlzIGRpcmVjdGx5IG1lbW9yeS1tYXBwZWQuIiA/Cgo+IFNpZ25lZC1vZmYt Ynk6IE5laWwgQXJtc3Ryb25nIDxuYXJtc3Ryb25nQGJheWxpYnJlLmNvbT4KPiAtLS0KPiAgZHJp dmVycy9ncHUvZHJtL2JyaWRnZS9kdy1oZG1pLmMgfCAxMDUgKysrKysrKysrKysrKysrKysrKy0t LS0tLS0tLS0tLS0tLS0tCj4gIGluY2x1ZGUvZHJtL2JyaWRnZS9kd19oZG1pLmggICAgIHwgICAx ICsKPiAgMiBmaWxlcyBjaGFuZ2VkLCA1NyBpbnNlcnRpb25zKCspLCA0OSBkZWxldGlvbnMoLSkK PiAKPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL2JyaWRnZS9kdy1oZG1pLmMKPiBiL2Ry aXZlcnMvZ3B1L2RybS9icmlkZ2UvZHctaGRtaS5jIGluZGV4IGNhOWQwY2UuLjEzNzQ3ZmUgMTAw NjQ0Cj4gLS0tIGEvZHJpdmVycy9ncHUvZHJtL2JyaWRnZS9kdy1oZG1pLmMKPiArKysgYi9kcml2 ZXJzL2dwdS9kcm0vYnJpZGdlL2R3LWhkbWkuYwo+IEBAIC0yMCw2ICsyMCw3IEBACj4gICNpbmNs dWRlIDxsaW51eC9tdXRleC5oPgo+ICAjaW5jbHVkZSA8bGludXgvb2ZfZGV2aWNlLmg+Cj4gICNp bmNsdWRlIDxsaW51eC9zcGlubG9jay5oPgo+ICsjaW5jbHVkZSA8bGludXgvcmVnbWFwLmg+CgpD b3VsZCB5b3UgcGxlYXNlIGtlZXAgdGhlIGhlYWRlcnMgYWxwaGFiZXRpY2FsbHkgc29ydGVkID8K Cj4gICNpbmNsdWRlIDxkcm0vZHJtX29mLmg+Cj4gICNpbmNsdWRlIDxkcm0vZHJtUC5oPgo+IEBA IC0xNjcsOCArMTY4LDcgQEAgc3RydWN0IGR3X2hkbWkgewo+ICAJdW5zaWduZWQgaW50IGF1ZGlv X247Cj4gIAlib29sIGF1ZGlvX2VuYWJsZTsKPiAKPiAtCXZvaWQgKCp3cml0ZSkoc3RydWN0IGR3 X2hkbWkgKmhkbWksIHU4IHZhbCwgaW50IG9mZnNldCk7Cj4gLQl1OCAoKnJlYWQpKHN0cnVjdCBk d19oZG1pICpoZG1pLCBpbnQgb2Zmc2V0KTsKPiArCXN0cnVjdCByZWdtYXAgKnJlZ207Cj4gIH07 Cj4gCj4gICNkZWZpbmUgSERNSV9JSF9QSFlfU1RBVDBfUlhfU0VOU0UgXAo+IEBAIC0xNzksNDIg KzE3OSwyMyBAQCBzdHJ1Y3QgZHdfaGRtaSB7Cj4gIAkoSERNSV9QSFlfUlhfU0VOU0UwIHwgSERN SV9QSFlfUlhfU0VOU0UxIHwgXAo+ICAJIEhETUlfUEhZX1JYX1NFTlNFMiB8IEhETUlfUEhZX1JY X1NFTlNFMykKPiAKPiAtc3RhdGljIHZvaWQgZHdfaGRtaV93cml0ZWwoc3RydWN0IGR3X2hkbWkg KmhkbWksIHU4IHZhbCwgaW50IG9mZnNldCkKPiAtewo+IC0Jd3JpdGVsKHZhbCwgaGRtaS0+cmVn cyArIChvZmZzZXQgPDwgMikpOwo+IC19Cj4gLQo+IC1zdGF0aWMgdTggZHdfaGRtaV9yZWFkbChz dHJ1Y3QgZHdfaGRtaSAqaGRtaSwgaW50IG9mZnNldCkKPiAtewo+IC0JcmV0dXJuIHJlYWRsKGhk bWktPnJlZ3MgKyAob2Zmc2V0IDw8IDIpKTsKPiAtfQo+IC0KPiAtc3RhdGljIHZvaWQgZHdfaGRt aV93cml0ZWIoc3RydWN0IGR3X2hkbWkgKmhkbWksIHU4IHZhbCwgaW50IG9mZnNldCkKPiAtewo+ IC0Jd3JpdGViKHZhbCwgaGRtaS0+cmVncyArIG9mZnNldCk7Cj4gLX0KPiAtCj4gLXN0YXRpYyB1 OCBkd19oZG1pX3JlYWRiKHN0cnVjdCBkd19oZG1pICpoZG1pLCBpbnQgb2Zmc2V0KQo+IC17Cj4g LQlyZXR1cm4gcmVhZGIoaGRtaS0+cmVncyArIG9mZnNldCk7Cj4gLX0KPiAtCj4gIHN0YXRpYyBp bmxpbmUgdm9pZCBoZG1pX3dyaXRlYihzdHJ1Y3QgZHdfaGRtaSAqaGRtaSwgdTggdmFsLCBpbnQg b2Zmc2V0KQoKTm90IHJlbGF0ZWQgdG8gdGhpcyBwYXRjaCwgYnV0IHRoZSB2YWx1ZSwgb2Zmc2V0 IG9yZGVyIG9mIGFyZ3VtZW50cyB0byB0aGUgCndyaXRlIGZ1bmN0aW9uIGhhcyBiZWVuIG1ha2lu ZyBtZSBjcmluZ2Ugc2luY2UgdGhlIHZlcnkgZmlyc3QgdGltZSBJIHJlYWQgdGhlIApjb2RlLiBJ IHdvbmRlciBpZiBtb2RpZnlpbmcgdGhpcyB3b3VsZCBiZSBhY2NlcHRlZC4KCj4gIHsKPiAtCWhk bWktPndyaXRlKGhkbWksIHZhbCwgb2Zmc2V0KTsKPiArCXJlZ21hcF93cml0ZShoZG1pLT5yZWdt LCBvZmZzZXQsIHZhbCk7Cj4gIH0KPiAKPiAgc3RhdGljIGlubGluZSB1OCBoZG1pX3JlYWRiKHN0 cnVjdCBkd19oZG1pICpoZG1pLCBpbnQgb2Zmc2V0KQo+ICB7Cj4gLQlyZXR1cm4gaGRtaS0+cmVh ZChoZG1pLCBvZmZzZXQpOwo+ICsJdW5zaWduZWQgaW50IHZhbCA9IDA7Cj4gKwo+ICsJcmVnbWFw X3JlYWQoaGRtaS0+cmVnbSwgb2Zmc2V0LCAmdmFsKTsKPiArCj4gKwlyZXR1cm4gdmFsOwo+ICB9 Cj4gCj4gIHN0YXRpYyB2b2lkIGhkbWlfbW9kYihzdHJ1Y3QgZHdfaGRtaSAqaGRtaSwgdTggZGF0 YSwgdTggbWFzaywgdW5zaWduZWQgcmVnKQo+IHsKPiAtCXU4IHZhbCA9IGhkbWlfcmVhZGIoaGRt aSwgcmVnKSAmIH5tYXNrOwo+IC0KPiAtCXZhbCB8PSBkYXRhICYgbWFzazsKPiAtCWhkbWlfd3Jp dGViKGhkbWksIHZhbCwgcmVnKTsKPiArCXJlZ21hcF91cGRhdGVfYml0cyhoZG1pLT5yZWdtLCBy ZWcsIG1hc2ssIGRhdGEpOwo+ICB9Cj4gCj4gIHN0YXRpYyB2b2lkIGhkbWlfbWFza193cml0ZWIo c3RydWN0IGR3X2hkbWkgKmhkbWksIHU4IGRhdGEsIHVuc2lnbmVkIGludAo+IHJlZywgQEAgLTE5 NDksNiArMTkzMCwyMSBAQCBzdGF0aWMgaW50IGR3X2hkbWlfZGV0ZWN0X3BoeShzdHJ1Y3QgZHdf aGRtaQo+ICpoZG1pKSByZXR1cm4gMDsKPiAgfQo+IAo+ICtzdGF0aWMgc3RydWN0IHJlZ21hcF9j b25maWcgaGRtaV9yZWdtYXBfOGJpdF9jb25maWcgPSB7Cj4gKwkucmVnX2JpdHMJPSAzMiwKPiAr CS52YWxfYml0cwk9IDgsCj4gKwkucmVnX3N0cmlkZQk9IDEsCj4gKwkubWF4X3JlZ2lzdGVyCT0g SERNSV9JMkNNX0ZTX1NDTF9MQ05UXzBfQUREUiwKPiArfTsKPiArCj4gK3N0YXRpYyBzdHJ1Y3Qg cmVnbWFwX2NvbmZpZyBoZG1pX3JlZ21hcF8zMmJpdF9jb25maWcgPSB7Cj4gKwkucmVnX2JpdHMJ PSA4LAo+ICsJLnBhZF9iaXRzCT0gMjQsCj4gKwkudmFsX2JpdHMJPSAzMiwKPiArCS5yZWdfc3Ry aWRlCT0gNCwKPiArCS5tYXhfcmVnaXN0ZXIJPSBIRE1JX0kyQ01fRlNfU0NMX0xDTlRfMF9BRERS LAo+ICt9OwoKSSBiZWxpZXZlIHlvdSBjYW4gbWFrZSB0aGVzZSBjb25zdC4KCj4gIHN0YXRpYyBz dHJ1Y3QgZHdfaGRtaSAqCj4gIF9fZHdfaGRtaV9wcm9iZShzdHJ1Y3QgcGxhdGZvcm1fZGV2aWNl ICpwZGV2LAo+ICAJCWNvbnN0IHN0cnVjdCBkd19oZG1pX3BsYXRfZGF0YSAqcGxhdF9kYXRhKQo+ IEBAIC0xOTU4LDcgKzE5NTQsOCBAQCBzdGF0aWMgaW50IGR3X2hkbWlfZGV0ZWN0X3BoeShzdHJ1 Y3QgZHdfaGRtaSAqaGRtaSkKPiAgCXN0cnVjdCBwbGF0Zm9ybV9kZXZpY2VfaW5mbyBwZGV2aW5m bzsKPiAgCXN0cnVjdCBkZXZpY2Vfbm9kZSAqZGRjX25vZGU7Cj4gIAlzdHJ1Y3QgZHdfaGRtaSAq aGRtaTsKPiAtCXN0cnVjdCByZXNvdXJjZSAqaW9yZXM7Cj4gKwlzdHJ1Y3QgcmVnbWFwX2NvbmZp ZyAqcmVnX2NvbmZpZyA9ICZoZG1pX3JlZ21hcF84Yml0X2NvbmZpZzsKCk5vIG5lZWQgdG8gYXNz aWduIGEgdmFsdWUgYXQgZGVjbGFyYXRpb24gdGltZSAodW5sZXNzIHRoZSBjb21waWxlciBpcyBu b3QgCnNtYXJ0IGVub3VnaCBhbmQgY29tcGxhaW5zLCBvciBpcyBzbWFydGVyIHRoYW4gbWUgYW5k IGZpbmRzIGEgcHJvYmxlbSB3aGVyZSBJIApkb24ndCkuCgo+ICsJc3RydWN0IHJlc291cmNlICpp b3JlcyA9IE5VTEw7Cj4gIAlpbnQgaXJxOwo+ICAJaW50IHJldDsKPiAgCXUzMiB2YWwgPSAxOwo+ IEBAIC0xOTgyLDIwICsxOTc5LDIxIEBAIHN0YXRpYyBpbnQgZHdfaGRtaV9kZXRlY3RfcGh5KHN0 cnVjdCBkd19oZG1pICpoZG1pKQo+ICAJbXV0ZXhfaW5pdCgmaGRtaS0+YXVkaW9fbXV0ZXgpOwo+ ICAJc3Bpbl9sb2NrX2luaXQoJmhkbWktPmF1ZGlvX2xvY2spOwo+IAo+IC0Jb2ZfcHJvcGVydHlf cmVhZF91MzIobnAsICJyZWctaW8td2lkdGgiLCAmdmFsKTsKPiAtCj4gLQlzd2l0Y2ggKHZhbCkg ewo+IC0JY2FzZSA0Ogo+IC0JCWhkbWktPndyaXRlID0gZHdfaGRtaV93cml0ZWw7Cj4gLQkJaGRt aS0+cmVhZCA9IGR3X2hkbWlfcmVhZGw7Cj4gLQkJYnJlYWs7Cj4gLQljYXNlIDE6Cj4gLQkJaGRt aS0+d3JpdGUgPSBkd19oZG1pX3dyaXRlYjsKPiAtCQloZG1pLT5yZWFkID0gZHdfaGRtaV9yZWFk YjsKPiAtCQlicmVhazsKPiAtCWRlZmF1bHQ6Cj4gLQkJZGV2X2VycihkZXYsICJyZWctaW8td2lk dGggbXVzdCBiZSAxIG9yIDRcbiIpOwo+IC0JCXJldHVybiBFUlJfUFRSKC1FSU5WQUwpOwo+ICsJ aWYgKHBsYXRfZGF0YS0+cmVnbSkKPiArCQloZG1pLT5yZWdtID0gcGxhdF9kYXRhLT5yZWdtOwoK WW91IG5lZWQgY3VybHkgYnJhY2VzIGFyb3VuZCB0aGlzIHN0YXRlbWVudC4KCj4gKwllbHNlIHsK PiArCQlvZl9wcm9wZXJ0eV9yZWFkX3UzMihucCwgInJlZy1pby13aWR0aCIsICZ2YWwpOwo+ICsJ CXN3aXRjaCAodmFsKSB7Cj4gKwkJY2FzZSA0Ogo+ICsJCQlyZWdfY29uZmlnID0gJmhkbWlfcmVn bWFwXzMyYml0X2NvbmZpZzsKPiArCQkJYnJlYWs7Cj4gKwkJY2FzZSAxOgo+ICsJCQlyZWdfY29u ZmlnID0gJmhkbWlfcmVnbWFwXzhiaXRfY29uZmlnOwo+ICsJCQlicmVhazsKPiArCQlkZWZhdWx0 Ogo+ICsJCQlkZXZfZXJyKGRldiwgInJlZy1pby13aWR0aCBtdXN0IGJlIDEgb3IgNFxuIik7Cj4g KwkJCXJldHVybiBFUlJfUFRSKC1FSU5WQUwpOwo+ICsJCX0KPiAgCX0KPiAKPiAgCWRkY19ub2Rl ID0gb2ZfcGFyc2VfcGhhbmRsZShucCwgImRkYy1pMmMtYnVzIiwgMCk7Cj4gQEAgLTIwMTEsMTEg KzIwMDksMjAgQEAgc3RhdGljIGludCBkd19oZG1pX2RldGVjdF9waHkoc3RydWN0IGR3X2hkbWkg KmhkbWkpCj4gIAkJZGV2X2RiZyhoZG1pLT5kZXYsICJubyBkZGMgcHJvcGVydHkgZm91bmRcbiIp Owo+ICAJfQo+IAo+IC0JaW9yZXMgPSBwbGF0Zm9ybV9nZXRfcmVzb3VyY2UocGRldiwgSU9SRVNP VVJDRV9NRU0sIDApOwo+IC0JaGRtaS0+cmVncyA9IGRldm1faW9yZW1hcF9yZXNvdXJjZShkZXYs IGlvcmVzKTsKPiAtCWlmIChJU19FUlIoaGRtaS0+cmVncykpIHsKPiAtCQlyZXQgPSBQVFJfRVJS KGhkbWktPnJlZ3MpOwo+IC0JCWdvdG8gZXJyX3JlczsKPiArCWlmICghcGxhdF9kYXRhLT5yZWdt KSB7Cj4gKwkJaW9yZXMgPSBwbGF0Zm9ybV9nZXRfcmVzb3VyY2UocGRldiwgSU9SRVNPVVJDRV9N RU0sIDApOwo+ICsJCWhkbWktPnJlZ3MgPSBkZXZtX2lvcmVtYXBfcmVzb3VyY2UoZGV2LCBpb3Jl cyk7Cj4gKwkJaWYgKElTX0VSUihoZG1pLT5yZWdzKSkgewo+ICsJCQlyZXQgPSBQVFJfRVJSKGhk bWktPnJlZ3MpOwo+ICsJCQlnb3RvIGVycl9yZXM7Cj4gKwkJfQo+ICsKPiArCQloZG1pLT5yZWdt ID0gZGV2bV9yZWdtYXBfaW5pdF9tbWlvKGRldiwgaGRtaS0+cmVncywgCnJlZ19jb25maWcpOwo+ ICsJCWlmIChJU19FUlIoaGRtaS0+cmVnbSkpIHsKPiArCQkJZGV2X2VycihkZXYsICJGYWlsZWQg dG8gY29uZmlndXJlIHJlZ21hcFxuIik7Cj4gKwkJCXJldCA9IFBUUl9FUlIoaGRtaS0+cmVnbSk7 Cj4gKwkJCWdvdG8gZXJyX3JlczsKPiArCQl9Cj4gIAl9Cj4gCj4gIAloZG1pLT5pc2ZyX2NsayA9 IGRldm1fY2xrX2dldChoZG1pLT5kZXYsICJpc2ZyIik7Cj4gQEAgLTIxMjMsNyArMjEzMCw3IEBA IHN0YXRpYyBpbnQgZHdfaGRtaV9kZXRlY3RfcGh5KHN0cnVjdCBkd19oZG1pICpoZG1pKQo+ICAJ Y29uZmlnMCA9IGhkbWlfcmVhZGIoaGRtaSwgSERNSV9DT05GSUcwX0lEKTsKPiAgCWNvbmZpZzMg PSBoZG1pX3JlYWRiKGhkbWksIEhETUlfQ09ORklHM19JRCk7Cj4gCj4gLQlpZiAoY29uZmlnMyAm IEhETUlfQ09ORklHM19BSEJBVURETUEpIHsKPiArCWlmIChpb3JlcyAmJiBjb25maWczICYgSERN SV9DT05GSUczX0FIQkFVRERNQSkgewoKWW91IHRlc3QgIXBsYXQtPnJlZ20gYWJvdmUsIGFuZCBp b3JlcyBoZXJlLiBIb3cgYWJvdXQgc3RhbmRhcmRpemluZyB0aGF0ID8gSWYgCnlvdSB0ZXN0IGZv ciAhcGxhdC0+cmVnbSBoZXJlLCB5b3Ugd29uJ3QgaGF2ZSB0byBpbml0aWFsaXplIGlvcmVzIHRv IE5VTEwuCgpBcGFydCBmcm9tIHRoZXNlIHNtYWxsIGlzc3VlcyB0aGUgcGF0Y2ggbG9va3MgZ29v ZCB0byBtZS4KCj4gIAkJc3RydWN0IGR3X2hkbWlfYXVkaW9fZGF0YSBhdWRpbzsKPiAKPiAgCQlh dWRpby5waHlzID0gaW9yZXMtPnN0YXJ0Owo+IGRpZmYgLS1naXQgYS9pbmNsdWRlL2RybS9icmlk Z2UvZHdfaGRtaS5oIGIvaW5jbHVkZS9kcm0vYnJpZGdlL2R3X2hkbWkuaAo+IGluZGV4IDczNWE4 YWIuLjE2Mzg0MmQgMTAwNjQ0Cj4gLS0tIGEvaW5jbHVkZS9kcm0vYnJpZGdlL2R3X2hkbWkuaAo+ ICsrKyBiL2luY2x1ZGUvZHJtL2JyaWRnZS9kd19oZG1pLmgKPiBAQCAtNjAsNiArNjAsNyBAQCBz dHJ1Y3QgZHdfaGRtaV9wbGF0X2RhdGEgewo+ICAJCQkgICAgIHVuc2lnbmVkIGxvbmcgbXBpeGVs Y2xvY2spOwo+ICAJZW51bSBkcm1fbW9kZV9zdGF0dXMgKCptb2RlX3ZhbGlkKShzdHJ1Y3QgZHJt X2Nvbm5lY3RvciAqY29ubmVjdG9yLAo+ICAJCQkJCSAgIHN0cnVjdCBkcm1fZGlzcGxheV9tb2Rl ICptb2RlKTsKPiArCXN0cnVjdCByZWdtYXAgKnJlZ207Cj4gIH07Cj4gCj4gIGludCBkd19oZG1p X3Byb2JlKHN0cnVjdCBwbGF0Zm9ybV9kZXZpY2UgKnBkZXYsCgotLSAKUmVnYXJkcywKCkxhdXJl bnQgUGluY2hhcnQKCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9y ZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZl bAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751297AbdAQOkc (ORCPT ); Tue, 17 Jan 2017 09:40:32 -0500 Received: from galahad.ideasonboard.com ([185.26.127.97]:34769 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751159AbdAQOjS (ORCPT ); Tue, 17 Jan 2017 09:39:18 -0500 From: Laurent Pinchart To: Neil Armstrong Cc: dri-devel@lists.freedesktop.org, laurent.pinchart+renesas@ideasonboard.com, Jose.Abreu@synopsys.com, kieran.bingham@ideasonboard.com, linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [RFC/RFT PATCH 1/4] drm/bridge: dw-hdmi: Switch to regmap for register access Date: Tue, 17 Jan 2017 16:39:32 +0200 Message-ID: <5238949.NqUKITq39o@avalon> User-Agent: KMail/4.14.10 (Linux/4.8.6-gentoo; KDE/4.14.24; x86_64; ; ) In-Reply-To: <1484656294-6140-2-git-send-email-narmstrong@baylibre.com> References: <1484656294-6140-1-git-send-email-narmstrong@baylibre.com> <1484656294-6140-2-git-send-email-narmstrong@baylibre.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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 > #include > #include > +#include Could you please keep the headers alphabetically sorted ? > #include > #include > @@ -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