On Wednesday 18 April 2012 13:24:26 Wolfram Sang wrote:
<snip>
> > @@ -254,7 +265,13 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
> >
> > /* Configure register endianess access */
> > reg = dw_readl(dev, DW_IC_COMP_TYPE);
> > if (reg == ___constant_swab32(DW_IC_COMP_TYPE_VALUE)) {
> >
> > - dev->swab = 1;
> > + dev->accessor_flags |= ACCESS_SWAP;
> > + reg = DW_IC_COMP_TYPE_VALUE;
>
> May I ask you to use proper if/elseif/else blocks instead of overwriting
> reg? I know you didn't come up with the mechanism, yet it looks too
> fragile to be extended IMO.
Okay, will do.
> > + }
> > +
> > + /* Configure register access mode 16bit */
> > + if (reg == (DW_IC_COMP_TYPE_VALUE & 0x0000ffff)) {
>
> Does it make sense to check reg + 2 for the upper part of the signature?
Not from my point of view. Its very unlikely that this driver will be used on
another controller. Its configured (via its address in platform-data or DT)
for exactly this controller. And matching 16bit of this 32bit register seems
enough for me. I would even have no problem to remove this check completely.
Thanks,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
