| Message ID | 20191212145925.32123-4-glaroque@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support of CEC wakeup on Amlogic G12 and SM1 SoCs | expand |
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@baylibre.com> 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
Hi Martin, thanks for review On 12/12/19 8:57 PM, Martin Blumenstingl wrote: > 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@baylibre.com> 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) as i say register info come from amlogic BSP. nothing in datasheet unfortunately. in amlogic code , this register are called AO_DEBUG_REG0 and AO_DEBUG_REG1 in amlogic BSP... > > [...] >> + 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)? good point, it's an error i will fix > >> + 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 i will > > Martin
diff --git a/drivers/media/platform/meson/ao-cec-g12a.c b/drivers/media/platform/meson/ao-cec-g12a.c index 3b39e875292e..d441b5a62b0c 100644 --- a/drivers/media/platform/meson/ao-cec-g12a.c +++ b/drivers/media/platform/meson/ao-cec-g12a.c @@ -25,6 +25,7 @@ #include <media/cec.h> #include <media/cec-notifier.h> #include <linux/clk-provider.h> +#include <linux/mfd/syscon.h> /* CEC Registers */ @@ -168,6 +169,19 @@ #define CECB_WAKEUPCTRL 0x31 +#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) +#define CECB_LOG_ADDR_SHIFT 16 + struct meson_ao_cec_g12a_data { /* Setup the internal CECB_CTRL2 register */ bool ctrl2_setup; @@ -177,6 +191,7 @@ struct meson_ao_cec_g12a_device { struct platform_device *pdev; struct regmap *regmap; struct regmap *regmap_cec; + struct regmap *regmap_ao_sysctrl; spinlock_t cec_reg_lock; struct cec_notifier *notify; struct cec_adapter *adap; @@ -518,6 +533,12 @@ meson_ao_cec_g12a_set_log_addr(struct cec_adapter *adap, u8 logical_addr) BIT(logical_addr - 8)); } + if (ao_cec->regmap_ao_sysctrl) + ret |= regmap_update_bits(ao_cec->regmap_ao_sysctrl, + CECB_LOG_ADDR_REG, + CECB_FUNC_CFG_MASK, + logical_addr << CECB_LOG_ADDR_SHIFT); + /* Always set Broadcast/Unregistered 15 address */ ret |= regmap_update_bits(ao_cec->regmap_cec, CECB_LADD_HIGH, BIT(CEC_LOG_ADDR_UNREGISTERED - 8), @@ -618,6 +639,13 @@ static int meson_ao_cec_g12a_adap_enable(struct cec_adapter *adap, bool enable) regmap_write(ao_cec->regmap_cec, CECB_CTRL2, FIELD_PREP(CECB_CTRL2_RISE_DEL_MAX, 2)); + if (ao_cec->regmap_ao_sysctrl) { + regmap_update_bits(ao_cec->regmap_ao_sysctrl, + CECB_FUNC_CFG_REG, + CECB_FUNC_CFG_MASK, + CECB_FUNC_CFG_ALL); + } + meson_ao_cec_g12a_irq_setup(ao_cec, true); return 0; @@ -692,6 +720,11 @@ static int meson_ao_cec_g12a_probe(struct platform_device *pdev) goto out_probe_adapter; } + ao_cec->regmap_ao_sysctrl = syscon_regmap_lookup_by_phandle + (pdev->dev.of_node, "amlogic,ao-sysctrl"); + if (IS_ERR(ao_cec->regmap_ao_sysctrl)) + dev_warn(&pdev->dev, "ao-sysctrl syscon regmap lookup failed.\n"); + irq = platform_get_irq(pdev, 0); ret = devm_request_threaded_irq(&pdev->dev, irq, meson_ao_cec_g12a_irq,
add register configuration to activate wakeup feature in bl301 Signed-off-by: Guillaume La Roque <glaroque@baylibre.com> --- drivers/media/platform/meson/ao-cec-g12a.c | 33 ++++++++++++++++++++++ 1 file changed, 33 insertions(+)