Hi
Am 28.07.20 um 20:04 schrieb Sam Ravnborg:
Hi Thomas.
A few comments related to the code - but not to this patch and not to this patch-set. But I noticed so here goes.
You're right on these points. But it's worth a separate patchset. I really just move code around here.
Best regards Thomas
Sam
On Tue, Jul 28, 2020 at 09:44:13AM +0200, Thomas Zimmermann wrote:
The I2C support feels slammed down to the end of ast_mode.c. Improve this by moving the code before it's callers, remove the declarations, rename the callbacks to match I2C's get/set sda/scl convention, and prefix all functions with ast_. No functional changes have been made.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/ast/ast_mode.c | 249 +++++++++++++++++---------------- 1 file changed, 125 insertions(+), 124 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 154cd877d9d1..19f1dfc8e9e0 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -46,9 +46,6 @@ #include "ast_drv.h" #include "ast_tables.h"
-static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev); -static void ast_i2c_destroy(struct ast_i2c_chan *i2c);
static inline void ast_load_palette_index(struct ast_private *ast, u8 index, u8 red, u8 green, u8 blue) @@ -514,6 +511,131 @@ static void ast_set_start_address_crt1(struct ast_private *ast,
}
+/*
- I2C
- */
+static int ast_i2c_getscl(void *i2c_priv) +{
- struct ast_i2c_chan *i2c = i2c_priv;
- struct ast_private *ast = to_ast_private(i2c->dev);
- uint32_t val, val2, count, pass;
s/uint32_t/u32
- count = 0;
- pass = 0;
- val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;
So val is a bool - but anyway an int is used.
- do {
val2 = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;Likewise for val2.
if (val == val2) {pass++;} else {pass = 0;val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;}- } while ((pass < 5) && (count++ < 0x10000));
- return val & 1 ? 1 : 0;
bool to int conversion could do the trick here.
+}
+static int ast_i2c_getsda(void *i2c_priv) +{
- struct ast_i2c_chan *i2c = i2c_priv;
- struct ast_private *ast = to_ast_private(i2c->dev);
- uint32_t val, val2, count, pass;
- count = 0;
- pass = 0;
- val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;
- do {
val2 = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;if (val == val2) {pass++;} else {pass = 0;val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;}- } while ((pass < 5) && (count++ < 0x10000));
- return val & 1 ? 1 : 0;
+}
+static void ast_i2c_setscl(void *i2c_priv, int clock) +{
- struct ast_i2c_chan *i2c = i2c_priv;
- struct ast_private *ast = to_ast_private(i2c->dev);
- int i;
- u8 ujcrb7, jtemp;
And now u8 is used for registers. Maybe because ast_get_index_reg_mask() returns uint8_t.
So for consistentcy do the uint8_t => u8 etc. conversion first.
- for (i = 0; i < 0x10000; i++) {
ujcrb7 = ((clock & 0x01) ? 0 : 1);ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0xf4, ujcrb7);jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x01);if (ujcrb7 == jtemp)break;- }
+}
+static void ast_i2c_setsda(void *i2c_priv, int data) +{
- struct ast_i2c_chan *i2c = i2c_priv;
- struct ast_private *ast = to_ast_private(i2c->dev);
- int i;
- u8 ujcrb7, jtemp;
- for (i = 0; i < 0x10000; i++) {
ujcrb7 = ((data & 0x01) ? 0 : 1) << 2;ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0xf1, ujcrb7);jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x04);if (ujcrb7 == jtemp)break;- }
+}
+static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev) +{
- struct ast_i2c_chan *i2c;
- int ret;
- i2c = kzalloc(sizeof(struct ast_i2c_chan), GFP_KERNEL);
- if (!i2c)
return NULL;- i2c->adapter.owner = THIS_MODULE;
- i2c->adapter.class = I2C_CLASS_DDC;
- i2c->adapter.dev.parent = &dev->pdev->dev;
- i2c->dev = dev;
- i2c_set_adapdata(&i2c->adapter, i2c);
If ast_private * was passed here and not i2c. Then the implementation of ast_i2c_* could be a tad simpler - no upclassing needed. And then you could drop the drm_device field. (And would need to invent another way to check if i2c is initialized).
- snprintf(i2c->adapter.name, sizeof(i2c->adapter.name),
"AST i2c bit bus");- i2c->adapter.algo_data = &i2c->bit;
- i2c->bit.udelay = 20;
- i2c->bit.timeout = 2;
- i2c->bit.data = i2c;
- i2c->bit.setsda = ast_i2c_setsda;
- i2c->bit.setscl = ast_i2c_setscl;
- i2c->bit.getsda = ast_i2c_getsda;
- i2c->bit.getscl = ast_i2c_getscl;
- ret = i2c_bit_add_bus(&i2c->adapter);
- if (ret) {
drm_err(dev, "Failed to register bit i2c\n");goto out_free;- }
- return i2c;
+out_free:
- kfree(i2c);
- return NULL;
+}
+static void ast_i2c_destroy(struct ast_i2c_chan *i2c) +{
- if (!i2c)
return;- i2c_del_adapter(&i2c->adapter);
- kfree(i2c);
+}
/*
- Primary plane
*/ @@ -1146,124 +1268,3 @@ int ast_mode_config_init(struct ast_private *ast)
return 0; }
-static int get_clock(void *i2c_priv) -{
- struct ast_i2c_chan *i2c = i2c_priv;
- struct ast_private *ast = to_ast_private(i2c->dev);
- uint32_t val, val2, count, pass;
- count = 0;
- pass = 0;
- val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;
- do {
val2 = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;if (val == val2) {pass++;} else {pass = 0;val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;}- } while ((pass < 5) && (count++ < 0x10000));
- return val & 1 ? 1 : 0;
-}
-static int get_data(void *i2c_priv) -{
- struct ast_i2c_chan *i2c = i2c_priv;
- struct ast_private *ast = to_ast_private(i2c->dev);
- uint32_t val, val2, count, pass;
- count = 0;
- pass = 0;
- val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;
- do {
val2 = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;if (val == val2) {pass++;} else {pass = 0;val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;}- } while ((pass < 5) && (count++ < 0x10000));
- return val & 1 ? 1 : 0;
-}
-static void set_clock(void *i2c_priv, int clock) -{
- struct ast_i2c_chan *i2c = i2c_priv;
- struct ast_private *ast = to_ast_private(i2c->dev);
- int i;
- u8 ujcrb7, jtemp;
- for (i = 0; i < 0x10000; i++) {
ujcrb7 = ((clock & 0x01) ? 0 : 1);ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0xf4, ujcrb7);jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x01);if (ujcrb7 == jtemp)break;- }
-}
-static void set_data(void *i2c_priv, int data) -{
- struct ast_i2c_chan *i2c = i2c_priv;
- struct ast_private *ast = to_ast_private(i2c->dev);
- int i;
- u8 ujcrb7, jtemp;
- for (i = 0; i < 0x10000; i++) {
ujcrb7 = ((data & 0x01) ? 0 : 1) << 2;ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0xf1, ujcrb7);jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x04);if (ujcrb7 == jtemp)break;- }
-}
-static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev) -{
- struct ast_i2c_chan *i2c;
- int ret;
- i2c = kzalloc(sizeof(struct ast_i2c_chan), GFP_KERNEL);
- if (!i2c)
return NULL;- i2c->adapter.owner = THIS_MODULE;
- i2c->adapter.class = I2C_CLASS_DDC;
- i2c->adapter.dev.parent = &dev->pdev->dev;
- i2c->dev = dev;
- i2c_set_adapdata(&i2c->adapter, i2c);
- snprintf(i2c->adapter.name, sizeof(i2c->adapter.name),
"AST i2c bit bus");- i2c->adapter.algo_data = &i2c->bit;
- i2c->bit.udelay = 20;
- i2c->bit.timeout = 2;
- i2c->bit.data = i2c;
- i2c->bit.setsda = set_data;
- i2c->bit.setscl = set_clock;
- i2c->bit.getsda = get_data;
- i2c->bit.getscl = get_clock;
- ret = i2c_bit_add_bus(&i2c->adapter);
- if (ret) {
drm_err(dev, "Failed to register bit i2c\n");goto out_free;- }
- return i2c;
-out_free:
- kfree(i2c);
- return NULL;
-}
-static void ast_i2c_destroy(struct ast_i2c_chan *i2c) -{
- if (!i2c)
return;- i2c_del_adapter(&i2c->adapter);
- kfree(i2c);
-}
2.27.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel