Hello Zelong,
On Thu, Mar 02, 2023 at 02:34:00PM +0800, zelong dong wrote:
> From: Zelong Dong <zelong.dong@xxxxxxxxxxx>
>
> Meson IR Controller supports hardware decoder in Meson-8B and later
> SoC. So far, protocol NEC/RC-6/XMP could be decoded in hardware.
> DTS property 'amlogic,ir-support-hw-decode' can enable this feature.
>
> Signed-off-by: Zelong Dong <zelong.dong@xxxxxxxxxxx>
> ---
> drivers/media/rc/meson-ir.c | 713 ++++++++++++++++++++++++++++++++----
> 1 file changed, 632 insertions(+), 81 deletions(-)
>
> diff --git a/drivers/media/rc/meson-ir.c b/drivers/media/rc/meson-ir.c
> index 4b769111f78e..1bfdce1c1864 100644
> --- a/drivers/media/rc/meson-ir.c
> +++ b/drivers/media/rc/meson-ir.c
> @@ -14,6 +14,7 @@
> #include <linux/platform_device.h>
> #include <linux/spinlock.h>
> #include <linux/bitfield.h>
> +#include <linux/regmap.h>
>
> #include <media/rc-core.h>
>
> @@ -21,87 +22,598 @@
>
> /* valid on all Meson platforms */
> #define IR_DEC_LDR_ACTIVE 0x00
> + #define IR_DEC_LDR_ACTIVE_MAX GENMASK(28, 16)
> + #define IR_DEC_LDR_ACTIVE_MIN GENMASK(12, 0)
Extra tabs before #define statement. The same problem is located in
the whole patchset.
[...]
> +#define MESON_IR_TIMINGS(proto, r_cnt, r_chk, r_comp, b1_e, hc, cnt_tick, ori, \
> + flt, len, f_max, la_max, la_min, li_max, li_min, \
> + rl_max, rl_min, b0_max, b0_min, b1_max, b1_min, \
> + d2_max, d2_min, d3_max, d3_min) \
> + { \
> + .hw_protocol = proto, \
> + .repeat_counter_enable = r_cnt, \
> + .repeat_check_enable = r_chk, \
> + .repeat_compare_enable = r_comp, \
> + .bit1_match_enable = b1_e, \
> + .hold_code_enable = hc, \
> + .count_tick_mode = cnt_tick, \
> + .bit_order = ori, \
> + .filter_cnt = flt, \
> + .code_length = len, \
> + .frame_time_max = f_max, \
> + .leader_active_max = la_max, \
> + .leader_active_min = la_min, \
> + .leader_idle_max = li_max, \
> + .leader_idle_min = li_min, \
> + .repeat_leader_max = rl_max, \
> + .repeat_leader_min = rl_min, \
> + .bit0_max = b0_max, \
> + .bit0_min = b0_min, \
> + .bit1_max = b1_max, \
> + .bit1_min = b1_min, \
> + .duration2_max = d2_max, \
> + .duration2_min = d2_min, \
> + .duration3_max = d3_max, \
> + .duration3_min = d3_min, \
> + } \
Extra tabs for back slash alignment
> +
> +/**
> + * struct meson_ir_param - describe IR Protocol parameter
> + * @hw_protocol: select IR Protocol from IR Controller.
> + * @repeat_counter_enable: enable frame-to-frame time counter, it should work
> + * with @repeat_compare_enable to detect the repeat frame.
> + * @repeat_check_enable: enable repeat time check for repeat detection.
> + * @repeat_compare_enable: enable to compare frame for repeat frame detection.
> + * Some IR Protocol send the same data as repeat frame. In this case,
> + * it should work with @repeat_counter_enable to detect the repeat frame.
> + * @bit_order: bit order, LSB or MSB.
> + * @bit1_match_enable: enable to check bit 1.
> + * @hold_code_enable: hold frame code in register IR_DEC_FRAME1, the new one
> + * frame code will not be store in IR_DEC_FRAME1. until IR_DEC_FRAME1
> + * has been read.
> + * @count_tick_mode: increasing time unit of frame-to-frame time counter.
> + * 0 = 100us, 1 = 10us.
> + * @filter_cnt: input filter, to filter burr
> + * @code_length: length (N-1) of frame's data part.
> + * @frame_time_max: max time for whole frame. Unit: MESON_HW_TRATE
> + * @leader_active_max: max time for NEC/RC6 leader active part. Unit: MESON_HW_TRATE.
> + * @leader_active_min: min time for NEC/RC6 leader active part. Unit: MESON_HW_TRATE.
> + * @leader_idle_max: max time for NEC/RC6 leader idle part. Unit: MESON_HW_TRATE.
> + * @leader_idle_min: min time for NEC/RC6 leader idle part. Unit: MESON_HW_TRATE.
> + * @repeat_leader_max: max time for NEC repeat leader idle part. Unit: MESON_HW_TRATE.
> + * @repeat_leader_min: min time for NEC repeat leader idle part. Unit: MESON_HW_TRATE.
> + * @bit0_max: max time for NEC Logic '0', half of RC6 trailer bit, XMP Logic '00'
> + * @bit0_min: min time for NEC Logic '0', half of RC6 trailer bit, XMP Logic '00'
> + * @bit1_max: max time for NEC Logic '1', whole of RC6 trailer bit, XMP Logic '01'
> + * @bit1_min: min time for NEC Logic '1', whole of RC6 trailer bit, XMP Logic '01'
> + * @duration2_max: max time for half of RC6 normal bit, XMP Logic '10'.
> + * @duration2_min: min time for half of RC6 normal bit, XMP Logic '10'.
> + * @duration3_max: max time for whole of RC6 normal bit, XMP Logic '11'.
> + * @duration3_min: min time for whole of RC6 normal bit, XMP Logic '11'.
> + */
>
Did you run checkpatch and kernel-doc checker for above struct
documentation?
> -#define REG0_RATE_MASK GENMASK(11, 0)
> +struct meson_ir_param {
> + u8 hw_protocol;
> + bool repeat_counter_enable;
> + bool repeat_check_enable;
> + bool repeat_compare_enable;
> + bool bit_order;
> + bool bit1_match_enable;
> + bool hold_code_enable;
> + bool count_tick_mode;
> + u8 filter_cnt;
> + u8 code_length;
> + u16 frame_time_max;
> + u16 leader_active_max;
> + u16 leader_active_min;
> + u16 leader_idle_max;
> + u16 leader_idle_min;
> + u16 repeat_leader_max;
> + u16 repeat_leader_min;
> + u16 bit0_max;
> + u16 bit0_min;
> + u16 bit1_max;
> + u16 bit1_min;
> + u16 duration2_max;
> + u16 duration2_min;
> + u16 duration3_max;
> + u16 duration3_min;
> +};
Why do you need tab alignment between type and variable?
[...]
> +#ifdef CONFIG_PM
> +static int meson_ir_resume(struct device *dev)
> +{
> + struct meson_ir *ir = dev_get_drvdata(dev);
> +
> + if (ir->support_hw_dec)
> + meson_ir_change_protocol(ir->rc, &ir->rc->enabled_protocols);
> + else
> + meson_ir_sw_decoder_init(ir->rc);
> +
> + return 0;
> +}
> +
> +static int meson_ir_suspend(struct device *dev)
> +{
> + struct meson_ir *ir = dev_get_drvdata(dev);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&ir->lock, flags);
> + regmap_update_bits(ir->reg, IR_DEC_REG1, IR_DEC_REG1_ENABLE, 0);
> + spin_unlock_irqrestore(&ir->lock, flags);
> +
> + return 0;
> +}
> +#endif
> +
> static const struct of_device_id meson_ir_match[] = {
> { .compatible = "amlogic,meson6-ir" },
> { .compatible = "amlogic,meson8b-ir" },
> { .compatible = "amlogic,meson-gxbb-ir" },
> + { .compatible = "amlogic,meson-s4-ir" },
> { },
> };
> MODULE_DEVICE_TABLE(of, meson_ir_match);
>
> +#ifdef CONFIG_PM
> +static const struct dev_pm_ops meson_ir_pm_ops = {
> + .suspend_late = meson_ir_suspend,
> + .resume_early = meson_ir_resume,
> +};
> +#endif
> +
> static struct platform_driver meson_ir_driver = {
> .probe = meson_ir_probe,
> .remove = meson_ir_remove,
> @@ -231,6 +779,9 @@ static struct platform_driver meson_ir_driver = {
> .driver = {
> .name = DRIVER_NAME,
> .of_match_table = meson_ir_match,
> +#ifdef CONFIG_PM
> + .pm = &meson_ir_pm_ops,
> +#endif
You can use pm_ptr() and DEFINE_SIMPLE_DEV_PM_OPS instead of checking of
CONFIG_PM definition.
[...]
--
Thank you,
Dmitry
