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.
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 <tzimmerm...@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
