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 <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
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
signature.asc
Description: OpenPGP digital signature
_______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
