| Message ID | 20200728074425.2749-2-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/ast: Convert to managed initialization | expand |
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 <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
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 >
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; + + 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 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; + + 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); + 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); -}
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(-)