On 02-06-21, 13:24, Nelson Costa wrote:
> +# Makefile for the PHY drivers.
> +#
> +
> +phy-dw-hdmi-e40x-y := phy-dw-hdmi-e40x-core.o
> +phy-dw-hdmi-e40x-y += phy-dw-hdmi-e405.o
> +phy-dw-hdmi-e40x-y += phy-dw-hdmi-e406.o
why not:
phy-dw-hdmi-e40x-y := phy-dw-hdmi-e40x-core.o phy-dw-hdmi-e405.o phy-dw-hdmi-e406.o ?
> +obj-$(CONFIG_VIDEO_DWC_HDMI_PHY_E40X) += phy-dw-hdmi-e40x.o
> diff --git a/drivers/phy/dwc/phy-dw-hdmi-e405.c b/drivers/phy/dwc/phy-dw-hdmi-e405.c
> new file mode 100644
> index 0000000..5078a86
> --- /dev/null
> +++ b/drivers/phy/dwc/phy-dw-hdmi-e405.c
> @@ -0,0 +1,497 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018 - present Synopsys, Inc. and/or its affiliates.
> + * Synopsys DesignWare HDMI PHY e405
2018 - 2021 ?
> +/* PHY e405 mpll configuration */
> +static const struct dw_phy_mpll_config dw_phy_e405_mpll_cfg[] = {
> + { 0x27, 0x1B94 },
Lowercase please
> + { 0x28, 0x16D2 },
> + { 0x29, 0x12D9 },
> + { 0x2A, 0x3249 },
> + { 0x2B, 0x3653 },
> + { 0x2C, 0x3436 },
> + { 0x2D, 0x124D },
> + { 0x2E, 0x0001 },
> + { 0xCE, 0x0505 },
> + { 0xCF, 0x0505 },
> + { 0xD0, 0x0000 },
> + { 0x00, 0x0000 },
> +};
> +
> +/* PHY e405 equalization functions */
> +static int dw_phy_eq_test(struct dw_phy_dev *dw_dev,
> + u16 *fat_bit_mask, int *min_max_length)
Please align this to preceding line open brace (checkpatch with --strict would warn you about this)
> +{
> + u16 main_fsm_status, val;
> + unsigned int i;
> +
> + for (i = 0; i < DW_PHY_EQ_WAIT_TIME_START; i++) {
> + main_fsm_status = dw_phy_read(dw_dev, DW_PHY_MAINFSM_STATUS1);
> + if (main_fsm_status & DW_PHY_CLOCK_STABLE)
> + break;
> + mdelay(DW_PHY_EQ_SLEEP_TIME_CDR);
> + }
> +
> + if (i == DW_PHY_EQ_WAIT_TIME_START) {
> + dev_dbg(dw_dev->dev, "PHY start conditions not achieved\n");
not error?
> + return -ETIMEDOUT;
> + }
> +
> + if (main_fsm_status & DW_PHY_PLL_RATE_BIT1) {
> + dev_dbg(dw_dev->dev, "invalid pll rate\n");
error?
> + return -EINVAL;
> + }
> +
> + val = dw_phy_read(dw_dev, DW_PHY_CDR_CTRL_CNT) &
> + DW_PHY_HDMI_MHL_MODE_MASK;
can be single line
> +static void dw_phy_eq_init_vars(struct dw_phy_eq_ch *ch)
> +{
> + ch->acc = 0;
> + ch->acq = 0;
> + ch->last_acq = 0;
> + ch->valid_long_setting = 0;
> + ch->valid_short_setting = 0;
memset() ?
> +static bool dw_phy_eq_acquire_early_cnt(struct dw_phy_dev *dw_dev,
> + u16 setting, u16 acq,
> + struct dw_phy_eq_ch *ch0,
> + struct dw_phy_eq_ch *ch1,
> + struct dw_phy_eq_ch *ch2)
> +{
> + u16 lock_vector = 0x1 << setting;
> + unsigned int i;
> +
> + ch0->out_bound_acq = 0;
> + ch1->out_bound_acq = 0;
> + ch2->out_bound_acq = 0;
> + ch0->acq = 0;
> + ch1->acq = 0;
> + ch2->acq = 0;
> +
> + dw_phy_eq_equal_setting(dw_dev, lock_vector);
> + dw_phy_eq_auto_calib(dw_dev);
> +
> + mdelay(DW_PHY_EQ_SLEEP_TIME_CDR);
> + if (!dw_phy_tmds_valid(dw_dev))
> + dev_dbg(dw_dev->dev, "TMDS is NOT valid\n");
> +
> + ch0->read_acq = dw_phy_read(dw_dev, DW_PHY_CH0_EQ_STATUS3);
> + ch1->read_acq = dw_phy_read(dw_dev, DW_PHY_CH1_EQ_STATUS3);
> + ch2->read_acq = dw_phy_read(dw_dev, DW_PHY_CH2_EQ_STATUS3);
> +
> + ch0->acq += ch0->read_acq;
> + ch1->acq += ch1->read_acq;
> + ch2->acq += ch2->read_acq;
> +
> + ch0->upper_bound_acq = ch0->read_acq + DW_PHY_EQ_BOUNDSPREAD;
> + ch0->lower_bound_acq = ch0->read_acq - DW_PHY_EQ_BOUNDSPREAD;
> + ch1->upper_bound_acq = ch1->read_acq + DW_PHY_EQ_BOUNDSPREAD;
> + ch1->lower_bound_acq = ch1->read_acq - DW_PHY_EQ_BOUNDSPREAD;
> + ch2->upper_bound_acq = ch2->read_acq + DW_PHY_EQ_BOUNDSPREAD;
> + ch2->lower_bound_acq = ch2->read_acq - DW_PHY_EQ_BOUNDSPREAD;
> +
> + for (i = 1; i < acq; i++) {
why do we start from 1 here..?
> +static const struct dw_phy_mpll_config dw_phy_e406_mpll_cfg[] = {
> + { 0x27, 0x1C94 },
> + { 0x28, 0x3713 },
> + { 0x29, 0x24DA },
> + { 0x2A, 0x5492 },
> + { 0x2B, 0x4B0D },
> + { 0x2C, 0x4760 },
> + { 0x2D, 0x008C },
> + { 0x2E, 0x0010 },
> + { 0x00, 0x0000 },
lower case here too please
> +static void dw_phy_eq_init_vars(struct dw_phy_eq_ch *ch)
> +{
> + ch->acc = 0;
> + ch->acq = 0;
> + ch->last_acq = 0;
> + ch->valid_long_setting = 0;
> + ch->valid_short_setting = 0;
> + ch->best_setting = DW_PHY_EQ_SHORT_CABLE_SETTING;
> +}
duplicate, it would make sense to create a common lib of such functions
and use them across these files
> +static int dw_phy_set_data(struct dw_phy_dev *dw_dev)
> +{
> + const struct dw_hdmi_phy_data *of_data;
> +
> + of_data = of_device_get_match_data(dw_dev->dev);
> +
> + if (of_data) {
> + dw_dev->phy_data = (struct dw_hdmi_phy_data *)of_data;
> + } else if (dw_dev->config->version == dw_phy_e405_data.version) {
> + dw_dev->phy_data = &dw_phy_e405_data;
> + } else if (dw_dev->config->version == dw_phy_e406_data.version) {
> + dw_dev->phy_data = &dw_phy_e406_data;
Driver supports only of, where will these else cases get triggered?
--
~Vinod
