| Message ID | 20191213132956.11074-4-glaroque@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support of CEC wakeup on Amlogic G12 and SM1 SoCs | expand |
On 13/12/2019 14:29, Guillaume La Roque wrote: > add register configuration to activate wakeup feature in bl301 > > Tested-by: Kevin Hilman <khilman@baylibre.com> > Signed-off-by: Guillaume La Roque <glaroque@baylibre.com> > --- > drivers/media/platform/meson/ao-cec-g12a.c | 33 ++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/drivers/media/platform/meson/ao-cec-g12a.c b/drivers/media/platform/meson/ao-cec-g12a.c > index 891533060d49..85850b974126 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,18 @@ > > #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) > + > struct meson_ao_cec_g12a_data { > /* Setup the internal CECB_CTRL2 register */ > bool ctrl2_setup; > @@ -177,6 +190,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 +532,13 @@ 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_LOG_ADDR_MASK, > + FIELD_PREP(CECB_LOG_ADDR_MASK, > + logical_addr)); > + > /* 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; > @@ -685,6 +713,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, > Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
Hi Guillaume, On 12/13/19 2:29 PM, Guillaume La Roque wrote: > add register configuration to activate wakeup feature in bl301 > > Tested-by: Kevin Hilman <khilman@baylibre.com> > Signed-off-by: Guillaume La Roque <glaroque@baylibre.com> > --- > drivers/media/platform/meson/ao-cec-g12a.c | 33 ++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/drivers/media/platform/meson/ao-cec-g12a.c b/drivers/media/platform/meson/ao-cec-g12a.c > index 891533060d49..85850b974126 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,18 @@ > > #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) > + > struct meson_ao_cec_g12a_data { > /* Setup the internal CECB_CTRL2 register */ > bool ctrl2_setup; > @@ -177,6 +190,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 +532,13 @@ 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_LOG_ADDR_MASK, > + FIELD_PREP(CECB_LOG_ADDR_MASK, > + logical_addr)); > + > /* 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); What exactly is enabled here? Looking at CECB_FUNC_CFG_ALL it seems to enable automatic standby (I presume when the STANDBY message is received?) and power on (I presume when SET_STREAM_PATH is received?). Do you really want to automatically handle STANDBY that way? What does this do on the hardware level anyway? Isn't this something that should be controlled in userspace? Similar questions for power on: you may not always want to enable this feature since it depends very much on the precise use-case. And which messages it reacts to in order to do a power-on needs to be documented since this differs depending on whether the CEC adapter is used for a TV or for a playback device. This feature may be hardwired for a playback device only, in which case it should probably be disabled if the CEC adapter is configured as a TV. In any case I would like to see some more details about how this works, especially since this is the first implementation of such a feature. I suspect that some userspace API might be needed to get the right level of control of such a feature. Regards, Hans > + } > + > meson_ao_cec_g12a_irq_setup(ao_cec, true); > > return 0; > @@ -685,6 +713,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, >
Hi Hans, On 1/7/20 3:36 PM, Hans Verkuil wrote: > Hi Guillaume, > > On 12/13/19 2:29 PM, Guillaume La Roque wrote: >> add register configuration to activate wakeup feature in bl301 >> >> Tested-by: Kevin Hilman <khilman@baylibre.com> >> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com> >> --- >> drivers/media/platform/meson/ao-cec-g12a.c | 33 ++++++++++++++++++++++ >> 1 file changed, 33 insertions(+) >> >> diff --git a/drivers/media/platform/meson/ao-cec-g12a.c b/drivers/media/platform/meson/ao-cec-g12a.c >> index 891533060d49..85850b974126 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,18 @@ >> >> #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) >> + >> struct meson_ao_cec_g12a_data { >> /* Setup the internal CECB_CTRL2 register */ >> bool ctrl2_setup; >> @@ -177,6 +190,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 +532,13 @@ 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_LOG_ADDR_MASK, >> + FIELD_PREP(CECB_LOG_ADDR_MASK, >> + logical_addr)); >> + >> /* 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); > What exactly is enabled here? Looking at CECB_FUNC_CFG_ALL it seems to > enable automatic standby (I presume when the STANDBY message is received?) > and power on (I presume when SET_STREAM_PATH is received?). this register and flags are used by bl301 part. amlogic implemented a task to check cec event/message. for power on in bl301 it's not only on SET_STREAM_PATH but also on : USER_CONTROL_PRESSED TEXT_VIEW_ON ACTIVE_SOURCE ROUTING_CHANGE when in CECB_FUNC_CFG_REG register we put CECB_FUNC_CFG_CEC_ON and CECB_FUNC_CFG_AUTO_POWER_ON it's not possible to change this > > Do you really want to automatically handle STANDBY that way? What does this > do on the hardware level anyway? Isn't this something that should be > controlled in userspace? in fact i do a new check in bl301 code amlogic do nothing on STANDBY so i will clean code and activate real option supported by bl301 > > Similar questions for power on: you may not always want to enable this feature > since it depends very much on the precise use-case. > > And which messages it reacts to in order to do a power-on needs to be > documented since this differs depending on whether the CEC adapter is > used for a TV or for a playback device. This feature may be hardwired for > a playback device only, in which case it should probably be disabled if > the CEC adapter is configured as a TV. > > In any case I would like to see some more details about how this works, > especially since this is the first implementation of such a feature. > > I suspect that some userspace API might be needed to get the right level > of control of such a feature. i will send v3 next week with some comments and fix ( disable are missing for example) actual usercase is for android TV. when cec was enable android TV want to be wakeup by cec event. > Regards, > > Hans > thanks for your review Regards Guillaume >> + } >> + >> meson_ao_cec_g12a_irq_setup(ao_cec, true); >> >> return 0; >> @@ -685,6 +713,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, >>
On 1/10/20 4:06 PM, guillaume La Roque wrote: > Hi Hans, > > On 1/7/20 3:36 PM, Hans Verkuil wrote: >> Hi Guillaume, >> >> On 12/13/19 2:29 PM, Guillaume La Roque wrote: >>> add register configuration to activate wakeup feature in bl301 >>> >>> Tested-by: Kevin Hilman <khilman@baylibre.com> >>> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com> >>> --- >>> drivers/media/platform/meson/ao-cec-g12a.c | 33 ++++++++++++++++++++++ >>> 1 file changed, 33 insertions(+) >>> >>> diff --git a/drivers/media/platform/meson/ao-cec-g12a.c b/drivers/media/platform/meson/ao-cec-g12a.c >>> index 891533060d49..85850b974126 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,18 @@ >>> >>> #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) >>> + >>> struct meson_ao_cec_g12a_data { >>> /* Setup the internal CECB_CTRL2 register */ >>> bool ctrl2_setup; >>> @@ -177,6 +190,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 +532,13 @@ 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_LOG_ADDR_MASK, >>> + FIELD_PREP(CECB_LOG_ADDR_MASK, >>> + logical_addr)); >>> + >>> /* 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); >> What exactly is enabled here? Looking at CECB_FUNC_CFG_ALL it seems to >> enable automatic standby (I presume when the STANDBY message is received?) >> and power on (I presume when SET_STREAM_PATH is received?). > this register and flags are used by bl301 part. > > amlogic implemented a task to check cec event/message. > > for power on in bl301 it's not only on SET_STREAM_PATH but also on : > > USER_CONTROL_PRESSED > TEXT_VIEW_ON Not IMAGE_VIEW_ON? > ACTIVE_SOURCE > ROUTING_CHANGE > > when in CECB_FUNC_CFG_REG register we put CECB_FUNC_CFG_CEC_ON and CECB_FUNC_CFG_AUTO_POWER_ON > > it's not possible to change this Too bad since ACTIVE_SOURCE and ROUTING_CHANGE should not power on a CEC device. See section 11.5.2 in the HDMI 2.0 Specification for more details, if you have that spec. > >> >> Do you really want to automatically handle STANDBY that way? What does this >> do on the hardware level anyway? Isn't this something that should be >> controlled in userspace? > > in fact i do a new check in bl301 code amlogic do nothing on STANDBY so i will clean code > > and activate real option supported by bl301 > >> >> Similar questions for power on: you may not always want to enable this feature >> since it depends very much on the precise use-case. >> >> And which messages it reacts to in order to do a power-on needs to be >> documented since this differs depending on whether the CEC adapter is >> used for a TV or for a playback device. This feature may be hardwired for >> a playback device only, in which case it should probably be disabled if >> the CEC adapter is configured as a TV. >> >> In any case I would like to see some more details about how this works, >> especially since this is the first implementation of such a feature. >> >> I suspect that some userspace API might be needed to get the right level >> of control of such a feature. > > i will send v3 next week with some comments and fix ( disable are missing for example) > > actual usercase is for android TV. > > when cec was enable android TV want to be wakeup by cec event. > > >> Regards, >> >> Hans >> > > thanks for your review No problem! Hans > > Regards > > Guillaume > >>> + } >>> + >>> meson_ao_cec_g12a_irq_setup(ao_cec, true); >>> >>> return 0; >>> @@ -685,6 +713,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, >>>
On 1/13/20 2:30 PM, Hans Verkuil wrote: > On 1/10/20 4:06 PM, guillaume La Roque wrote: >> Hi Hans, >> >> On 1/7/20 3:36 PM, Hans Verkuil wrote: >>> Hi Guillaume, >>> >>> On 12/13/19 2:29 PM, Guillaume La Roque wrote: >>>> add register configuration to activate wakeup feature in bl301 >>>> >>>> Tested-by: Kevin Hilman <khilman@baylibre.com> >>>> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com> >>>> --- >>>> drivers/media/platform/meson/ao-cec-g12a.c | 33 ++++++++++++++++++++++ >>>> 1 file changed, 33 insertions(+) >>>> >>>> diff --git a/drivers/media/platform/meson/ao-cec-g12a.c b/drivers/media/platform/meson/ao-cec-g12a.c >>>> index 891533060d49..85850b974126 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,18 @@ >>>> >>>> #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) >>>> + >>>> struct meson_ao_cec_g12a_data { >>>> /* Setup the internal CECB_CTRL2 register */ >>>> bool ctrl2_setup; >>>> @@ -177,6 +190,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 +532,13 @@ 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_LOG_ADDR_MASK, >>>> + FIELD_PREP(CECB_LOG_ADDR_MASK, >>>> + logical_addr)); >>>> + >>>> /* 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); >>> What exactly is enabled here? Looking at CECB_FUNC_CFG_ALL it seems to >>> enable automatic standby (I presume when the STANDBY message is received?) >>> and power on (I presume when SET_STREAM_PATH is received?). >> this register and flags are used by bl301 part. >> >> amlogic implemented a task to check cec event/message. >> >> for power on in bl301 it's not only on SET_STREAM_PATH but also on : >> >> USER_CONTROL_PRESSED >> TEXT_VIEW_ON > Not IMAGE_VIEW_ON? sorry i forgot it , yes on IMAGE_VIEW_ON too > >> ACTIVE_SOURCE >> ROUTING_CHANGE >> >> when in CECB_FUNC_CFG_REG register we put CECB_FUNC_CFG_CEC_ON and CECB_FUNC_CFG_AUTO_POWER_ON >> >> it's not possible to change this > Too bad since ACTIVE_SOURCE and ROUTING_CHANGE should not power on a CEC device. > > See section 11.5.2 in the HDMI 2.0 Specification for more details, if you have > that spec. i will return your comment to amlogic but not sure they do change in bl301. > >>> Do you really want to automatically handle STANDBY that way? What does this >>> do on the hardware level anyway? Isn't this something that should be >>> controlled in userspace? >> in fact i do a new check in bl301 code amlogic do nothing on STANDBY so i will clean code >> >> and activate real option supported by bl301 >> >>> Similar questions for power on: you may not always want to enable this feature >>> since it depends very much on the precise use-case. >>> >>> And which messages it reacts to in order to do a power-on needs to be >>> documented since this differs depending on whether the CEC adapter is >>> used for a TV or for a playback device. This feature may be hardwired for >>> a playback device only, in which case it should probably be disabled if >>> the CEC adapter is configured as a TV. >>> >>> In any case I would like to see some more details about how this works, >>> especially since this is the first implementation of such a feature. >>> >>> I suspect that some userspace API might be needed to get the right level >>> of control of such a feature. >> i will send v3 next week with some comments and fix ( disable are missing for example) >> >> actual usercase is for android TV. >> >> when cec was enable android TV want to be wakeup by cec event. >> >> >>> Regards, >>> >>> Hans >>> >> thanks for your review > No problem! > > Hans > >> Regards >> >> Guillaume >> >>>> + } >>>> + >>>> meson_ao_cec_g12a_irq_setup(ao_cec, true); >>>> >>>> return 0; >>>> @@ -685,6 +713,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, >>>> Regards Guillaume
diff --git a/drivers/media/platform/meson/ao-cec-g12a.c b/drivers/media/platform/meson/ao-cec-g12a.c index 891533060d49..85850b974126 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,18 @@ #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) + struct meson_ao_cec_g12a_data { /* Setup the internal CECB_CTRL2 register */ bool ctrl2_setup; @@ -177,6 +190,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 +532,13 @@ 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_LOG_ADDR_MASK, + FIELD_PREP(CECB_LOG_ADDR_MASK, + logical_addr)); + /* 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; @@ -685,6 +713,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,