Hi Guillaume, (I don't know the specifics of this hardware but I have two general comments below) On Thu, Dec 12, 2019 at 4:00 PM Guillaume La Roque <glaroque@xxxxxxxxxxxx> wrote: > +#define CECB_FUNC_CFG_REG 0xA0 > +#define CECB_FUNC_CFG_MASK GENMASK(6, 0) > +#define CECB_FUNC_CFG_CEC_ON 0x01 > +#define CECB_FUNC_CFG_OTP_ON 0x02 > +#define CECB_FUNC_CFG_AUTO_STANDBY 0x04 > +#define CECB_FUNC_CFG_AUTO_POWER_ON 0x08 > +#define CECB_FUNC_CFG_ALL 0x2f > +#define CECB_FUNC_CFG_NONE 0x0 > + > +#define CECB_LOG_ADDR_REG 0xA4 > +#define CECB_LOG_ADDR_MASK GENMASK(22, 16) do these registers have some RTI_* prefix in the datasheet? that would make it easier to spot that these registers belong to AO / RTI (while all other registers belong to the CEC controller) [...] > + if (ao_cec->regmap_ao_sysctrl) > + ret |= regmap_update_bits(ao_cec->regmap_ao_sysctrl, > + CECB_LOG_ADDR_REG, > + CECB_FUNC_CFG_MASK, why do we need to mask CECB_FUNC_CFG_MASK (from register 0xa0) in the CECB_LOG_ADDR_REG register (0xa4)? > + logical_addr << CECB_LOG_ADDR_SHIFT); FIELD_PREP(CECB_FUNC_CFG_MASK, logical_addr) would make it consistent with the rest of the driver then you can also drop the #define CECB_LOG_ADDR_SHIFT Martin
