Re: [PATCH 2/2] drm/aspeed: Use dt matching for default register values
Hi Joel, > There are minor differences in the values for the threshold value and > the scan line size between families of ASPEED SoC. Additionally the > SCU register for the output control differs between families. > > This adds device tree matching to parameterise these values, allowing > us to add support for the AST2400 now, and in the future the AST2600. Looks good to me. Two super minor things: > --- a/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c > +++ b/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c > @@ -60,7 +60,8 @@ static void aspeed_gfx_enable_controller(struct > aspeed_gfx *priv) > u32 ctrl2 = readl(priv->base + CRT_CTRL2); > > /* SCU2C: set DAC source for display output to Graphics CRT (GFX) */ > - regmap_update_bits(priv->scu, 0x2c, BIT(16), BIT(16)); > + regmap_update_bits(priv->scu, priv->dac_reg, BIT(16), BIT(16)); The comment references SCU2C; but you've implied that this will change... > @@ -228,7 +258,7 @@ static ssize_t dac_mux_store(struct device *dev, > struct device_attribute *attr, > if (val > 3) > return -EINVAL; > > - rc = regmap_update_bits(priv->scu, ASPEED_SCU_MISC_CTRL, 0x3, val > << 16); > + rc = regmap_update_bits(priv->scu, priv->dac_reg, 0x3, val << 16); > if (rc < 0) > return 0; > > @@ -241,7 +271,7 @@ static ssize_t dac_mux_show(struct device *dev, > struct device_attribute *attr, c > u32 reg; > int rc; > > - rc = regmap_read(priv->scu, ASPEED_SCU_MISC_CTRL, ®); > + rc = regmap_read(priv->scu, priv->dac_reg, ®); > if (rc) > return rc; You've removed the only uses of ASPEED_SCU_MISC_CTRL here, maybe drop the #define too? Regardless: Reviewed-by: Jeremy Kerr Cheers, Jeremy ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/aspeed: Look up syscon by phandle
Hi Joel,
Sounds like a good idea! One comment though:
> @@ -111,10 +112,13 @@ static int aspeed_gfx_load(struct drm_device *drm)
> if (IS_ERR(priv->base))
> return PTR_ERR(priv->base);
>
> - priv->scu = syscon_regmap_lookup_by_compatible("aspeed,ast2500-scu");
> + priv->scu = syscon_regmap_lookup_by_phandle(np, "syscon");
> if (IS_ERR(priv->scu)) {
> - dev_err(&pdev->dev, "failed to find SCU regmap\n");
> - return PTR_ERR(priv->scu);
> + priv->scu =
> syscon_regmap_lookup_by_compatible("aspeed,aspeed-scu");
Is this (more generic) compatible value guaranteed to exist alongside
aspeed,ast2500-scu? The scu binding only specifies the model-specific
ones:
Documentation/devicetree/bindings/mfd/aspeed-scu.txt:
Required properties:
- compatible: One of:
"aspeed,ast2400-scu", "syscon", "simple-mfd"
"aspeed,ast2500-scu", "syscon", "simple-mfd"
- the only mention of the new compatible value that I can find is this
thread. Maybe we should retain the existing one to keep the fallback
case working?
Cheers,
Jeremy
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/aspeed: Fix vga_pw sysfs output
Hi Joel, > Before the drm driver had support for this file there was a driver > that exposed the contents of the vga password register to userspace. > It would present the entire register instead of interpreting it. > > The drm implementation chose to mask of the lower bit, without > explaining why. This breaks the existing userspace, which is looking > for 0xa8 in the lower byte. > > Change our implementation to expose the entire register. As a userspace consumer of this: Reviewed-by: Jeremy Kerr Thanks! Jeremy
[PATCH] fbcon: use default if cursor blink interval is not valid
Hi Scot, > Use the normal cursor blink default interval of 200 ms if > ops->cur_blink_jiffies is not in the range specified in commit > bd63364caa8d. Since invalid values are not used, specific system > initialization timings should not cause lockups. This fixes an issue we're seeing with the ast driver on OpenPOWER machines, thanks! Acked-by: Jeremy Kerr Cheers, Jeremy
Re: [PATCH 00/17] Introduce and use generic parity32/64 helper
Hi Kuan-Wei, > Several parts of the kernel contain redundant implementations of parity > calculations for 32-bit and 64-bit values. Introduces generic > parity32() and parity64() helpers in bitops.h, providing a standardized > and optimized implementation. More so than __builtin_parity() ? I'm all for reducing the duplication, but the compiler may well have a better parity approach than the xor-folding implementation here. Looks like we can get this to two instructions on powerpc64, for example. Cheers, Jeremy
Re: [PATCH v3 00/16] Introduce and use generic parity16/32/64 helper
Hi Yuri & Kuan-Wei: > Thank you for sharing your opinion on this fixed parity(). Your > arguments may or may not be important, depending on what existing > users actually need. Unfortunately, Kuan-Wei didn't collect > performance numbers and opinions from those proposed users. For the fsi-i2c side: this isn't a performance-critical path, and any reasonable common approach would likely perform better that the current per-bit implementation. Our common targets for this driver would be arm and powerpc64le. In case it's useful as a reference, using the kernel compilers I have to hand, a __builtin_parity() is a library call on the former, and a two-instruction sequence for the latter. Cheers, Jeremy
Re: [PATCH v3 00/16] Introduce and use generic parity16/32/64 helper
Hi Kuan-Wei, > Thanks for your feedback. No problem! > IIUC, from the fsi-i2c perspective, parity efficiency isn't a major > concern, Yes > but you still prefer optimizing with methods like __builtin_parity(). No, it's not really about optimisation. In the case of this driver, my preference would be more directed to using common code (either in the form of these changes, or __builtin_parity) rather than any performance considerations. The implementation details I gave was more a note about the platforms that are applicable for the driver. Cheers, Jeremy
