Hi Eric,
Thanks for starting to upstream this Analogix Slimport driver!
As Greg says, please move this driver to its intended directory, I presume:
/drivers/gpu/drm/bridge
And when you submit, use get_maintainer.pl to add the proper reviewers
and lists.
At present, you have no DRM folks, nor dri-devel, so you probably
won't receive any additional feedback on this version, since the
relevant folks have not seen your emails.
Some more very detailed feedback inline...
On Sep 7, 2015 05:15, "Enric Balletbo i Serra" <eballetbo@xxxxxxxxx> wrote:
>
> The ANX7814 is an ultra-low power Full-HD (1080p60) SlimPort transmitter
> designed for portable devices.
>
> This driver adds initial support and supports HDMI to DP pass-through mode.
>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
> ---
> drivers/staging/Kconfig | 2 +
> drivers/staging/Makefile | 1 +
> drivers/staging/slimport/Kconfig | 7 +
> drivers/staging/slimport/Makefile | 4 +
> drivers/staging/slimport/slimport.c | 301 +++
> drivers/staging/slimport/slimport.h | 49 +
> drivers/staging/slimport/slimport_tx_drv.c | 3293 ++++++++++++++++++++++++++++
> drivers/staging/slimport/slimport_tx_drv.h | 254 +++
> drivers/staging/slimport/slimport_tx_reg.h | 786 +++++++
> 9 files changed, 4697 insertions(+)
> create mode 100644 drivers/staging/slimport/Kconfig
> create mode 100644 drivers/staging/slimport/Makefile
> create mode 100644 drivers/staging/slimport/slimport.c
> create mode 100644 drivers/staging/slimport/slimport.h
> create mode 100644 drivers/staging/slimport/slimport_tx_drv.c
> create mode 100644 drivers/staging/slimport/slimport_tx_drv.h
> create mode 100644 drivers/staging/slimport/slimport_tx_reg.h
>
> diff --git a/drivers/staging/Kconfig b/drivers/staging/Kconfig
> index e29293c..24ccd7c 100644
> --- a/drivers/staging/Kconfig
> +++ b/drivers/staging/Kconfig
> @@ -110,4 +110,6 @@ source "drivers/staging/wilc1000/Kconfig"
>
> source "drivers/staging/most/Kconfig"
>
> +source "drivers/staging/slimport/Kconfig"
> +
> endif # STAGING
> diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile
> index 50824dd..942e886 100644
> --- a/drivers/staging/Makefile
> +++ b/drivers/staging/Makefile
> @@ -47,3 +47,4 @@ obj-$(CONFIG_FB_TFT) += fbtft/
> obj-$(CONFIG_FSL_MC_BUS) += fsl-mc/
> obj-$(CONFIG_WILC1000) += wilc1000/
> obj-$(CONFIG_MOST) += most/
> +obj-$(CONFIG_SLIMPORT_ANX78XX) += slimport/
> diff --git a/drivers/staging/slimport/Kconfig b/drivers/staging/slimport/Kconfig
> new file mode 100644
> index 0000000..f5233ef
> --- /dev/null
> +++ b/drivers/staging/slimport/Kconfig
> @@ -0,0 +1,7 @@
> +config SLIMPORT_ANX78XX
I think the "SLIMPORT_" here is a bit overkill, and just adds to
confusion over what name to use where. I'd recommend just
CONFIG_ANX78XX.
Likewise, for consistency, the rename the files as "anx78xx*" instead
of "slimport*".
> + tristate "Analogix Slimport transmitter ANX78XX support"
> + help
> + Slimport Transmitter is a HD video transmitter chip
> + over micro-USB connector for smartphone device.
> +
> +
> diff --git a/drivers/staging/slimport/Makefile b/drivers/staging/slimport/Makefile
> new file mode 100644
> index 0000000..9bb6ce2
> --- /dev/null
> +++ b/drivers/staging/slimport/Makefile
> @@ -0,0 +1,4 @@
> +obj-${CONFIG_SLIMPORT_ANX78XX} := anx78xx.o
> +
> +anx78xx-y := slimport.o
> +anx78xx-y += slimport_tx_drv.o
> diff --git a/drivers/staging/slimport/slimport.c b/drivers/staging/slimport/slimport.c
> new file mode 100644
> index 0000000..95c5114
> --- /dev/null
> +++ b/drivers/staging/slimport/slimport.c
> @@ -0,0 +1,301 @@
> +/*
> + * Copyright(c) 2015, Analogix Semiconductor. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/err.h>
> +#include <linux/async.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of_platform.h>
> +#include <linux/delay.h>
> +
> +#include "slimport.h"
> +#include "slimport_tx_drv.h"
> +
> +/* HDCP switch for external block */
> +/* external_block_en = 1: enable, 0: disable */
This comment is a bit superfluous.
> +int external_block_en = 1;
> +
> +struct i2c_client *anx7814_client;
A single global client isn't going to work. It is easy to imagine a
machine with more than one anx78xx, requiring multiple instances of
the driver and hence multiple i2c clients.
Also, you'll want to be a bit more consistent with naming; is the
driver for anx78xx? Or anx7814 only?
> +
> +int sp_read_reg_byte(uint8_t slave_addr, uint8_t offset)
> +{
> + int ret = 0;
> + struct device *dev = &anx7814_client->dev;
The use of global anx7814_client has ramifications throughout this
whole driver. direct
Please re-write all of these transaction functions (and their callers)
should to take a context struct pointer as their first parameter.
> +
> + anx7814_client->addr = (slave_addr >> 1);
> + ret = i2c_smbus_read_byte_data(anx7814_client, offset);
> + if (ret < 0) {
> + dev_err(dev, "%s: failed to read i2c addr=%x\n", LOG_TAG,
> + slave_addr);
> + return ret;
> + }
> + return 0;
Don't you need to return the actual byte that was read (ret)?
> +}
> +
> +int sp_read_reg(uint8_t slave_addr, uint8_t offset, uint8_t *buf)
> +{
> + int ret = 0;
> + struct device *dev = &anx7814_client->dev;
> +
> + anx7814_client->addr = (slave_addr >> 1);
> + ret = i2c_smbus_read_byte_data(anx7814_client, offset);
> + if (ret < 0) {
> + dev_err(dev, "%s: failed to read i2c addr=%x\n", LOG_TAG,
> + slave_addr);
> + return ret;
> + }
> + *buf = (uint8_t) ret;
> +
> + return 0;
> +}
> +
> +int sp_write_reg(uint8_t slave_addr, uint8_t offset, uint8_t value)
> +{
> + int ret = 0;
> + struct device *dev = &anx7814_client->dev;
> +
> + anx7814_client->addr = (slave_addr >> 1);
> + ret = i2c_smbus_write_byte_data(anx7814_client, offset, value);
> + if (ret < 0) {
> + dev_err(dev, "%s: failed to write i2c addr=%x\n", LOG_TAG,
> + slave_addr);
> + }
> + return ret;
> +}
> +
> +void sp_tx_hardware_poweron(struct anx7814_data *data)
nit: another personal preference, I prefer to use the specific device
as the local parameter name, rather than the generic "data".
So it would look something like:
void anx7814_tx_hardware_poweron(struct anx7814 *anx7814)
> +{
> + struct device *dev = &data->client->dev;
> + struct anx7814_platform_data *pdata = data->pdata;
> +
> + gpiod_set_value_cansleep(pdata->gpiod_reset, 0);
> + usleep_range(1000, 2000);
> +
> + gpiod_set_value_cansleep(pdata->gpiod_pd, 0);
> + usleep_range(1000, 2000);
> +
> + gpiod_set_value_cansleep(pdata->gpiod_reset, 1);
> +
> + dev_info(dev, "%s: anx7814 power on\n", LOG_TAG);
This kind of verbose logging should be dev_dbg(
> +}
> +
> +void sp_tx_hardware_powerdown(struct anx7814_data *data)
> +{
> + struct device *dev = &data->client->dev;
> + struct anx7814_platform_data *pdata = data->pdata;
> +
> + gpiod_set_value_cansleep(pdata->gpiod_reset, 0);
> + usleep_range(1000, 2000);
> +
> + gpiod_set_value_cansleep(pdata->gpiod_pd, 1);
> + usleep_range(1000, 2000);
> +
> + dev_info(dev, "%s: anx7814 power down\n", LOG_TAG);
> +}
> +
> +static int anx7814_init_gpio(struct anx7814_data *data)
> +{
> + struct device *dev = &data->client->dev;
> + struct anx7814_platform_data *pdata = data->pdata;
> + int ret;
> +
> + /* gpio for chip power down */
> + pdata->gpiod_pd = devm_gpiod_get(dev, "pd", GPIOD_OUT_HIGH);
> + if (IS_ERR(pdata->gpiod_pd)) {
> + dev_err(dev, "unable to claim pd gpio\n");
> + ret = PTR_ERR(pdata->gpiod_pd);
> + return ret;
> + }
> +
> + /* gpio for chip reset */
> + pdata->gpiod_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> + if (IS_ERR(pdata->gpiod_reset)) {
> + dev_err(dev, "unable to claim reset gpio\n");
> + ret = PTR_ERR(pdata->gpiod_reset);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int anx7814_system_init(struct anx7814_data *data)
> +{
> + struct device *dev = &data->client->dev;
> +
> + int ret = 0;
> +
> + ret = slimport_chip_detect(data);
> + if (ret == 0) {
> + sp_tx_hardware_powerdown(data);
> + dev_err(dev, "failed to detect anx7814\n");
> + return -ENODEV;
> + }
> +
> + sp_tx_variable_init();
> + return 0;
> +}
> +
> +static void anx7814_work_func(struct work_struct *work)
> +{
> + struct anx7814_data *data = container_of(work, struct anx7814_data,
> + work.work);
> + int workqueu_timer = 0;
spelling: workqueue_timer
> +
> + if (sp_tx_cur_states() >= STATE_PLAY_BACK)
> + workqueu_timer = 500;
> + else
> + workqueu_timer = 100;
> + mutex_lock(&data->lock);
> + slimport_main_process(data);
> + mutex_unlock(&data->lock);
> + queue_delayed_work(data->workqueue, &data->work,
> + msecs_to_jiffies(workqueu_timer));
The use of a periodic workqueue and a state machine like this seems
like the wrong approach.
The driver should be event driven, and any worker should do whatever
work is required until it completes, rather than arbitrarily chopping
up the tasks into ~100 ms chunks.
> +}
> +
> +static int anx7814_i2c_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct anx7814_data *data;
> + struct anx7814_platform_data *pdata;
> + int ret = 0;
In general, I prefer not to see 'ret' variables initialized up here to
a default value. Rather, only set the ret value as needed.
This let's the compiler catch if there were any 'return' paths that
did not explicitly initialize ret.
> +
> + if (!i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_I2C_BLOCK)) {
> + dev_err(&client->dev, "i2c bus does not support the device\n");
> + return -ENODEV;
> + }
> +
> + data = devm_kzalloc(&client->dev,
> + sizeof(struct anx7814_data),
> + GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + pdata = devm_kzalloc(&client->dev,
> + sizeof(struct anx7814_platform_data),
> + GFP_KERNEL);
> + if (!pdata)
> + return -ENOMEM;
If you are always explicitly allocating both anyway, might as well
just embed a "struct anx7814_platform_data" directly in "struct
anx7814_data".
> +
> + data->pdata = pdata;
> +
> + data->client = client;
> + anx7814_client = client;
> +
> + mutex_init(&data->lock);
> +
> + ret = anx7814_init_gpio(data);
> + if (ret) {
> + dev_err(&client->dev, "failed to initialize gpios\n");
> + return ret;
> + }
> +
> + INIT_DELAYED_WORK(&data->work, anx7814_work_func);
> +
> + data->workqueue = create_singlethread_workqueue("anx7814_work");
> + if (data->workqueue == NULL) {
> + dev_err(&client->dev, "failed to create work queue\n");
> + return -ENOMEM;
> + }
> +
> + ret = anx7814_system_init(data);
> + if (ret) {
> + dev_err(&client->dev, "failed to initialize anx7814\n");
> + goto cleanup;
> + }
> +
> + i2c_set_clientdata(client, data);
> +
> + /* enable driver */
> + queue_delayed_work(data->workqueue, &data->work, 0);
> +
> + return 0;
> +
> +cleanup:
> + destroy_workqueue(data->workqueue);
> + return ret;
> +}
> +
> +static int anx7814_i2c_remove(struct i2c_client *client)
> +{
> + struct anx7814_data *data = i2c_get_clientdata(client);
> +
> + destroy_workqueue(data->workqueue);
> +
> + return 0;
> +}
> +
> +static int anx7814_i2c_suspend(struct device *dev)
> +{
> + struct i2c_client *client = container_of(dev, struct i2c_client, dev);
> + struct anx7814_data *data = i2c_get_clientdata(client);
> +
> + dev_info(&client->dev, "suspend\n");
dev_dbg (or just remove this, since it is a bit redundant with verbose
suspend logging).
> + cancel_delayed_work_sync(&data->work);
> + flush_workqueue(data->workqueue);
> + sp_tx_hardware_powerdown(data);
> + sp_tx_clean_state_machine();
> +
> + return 0;
> +}
> +
> +static int anx7814_i2c_resume(struct device *dev)
> +{
> + struct i2c_client *client = container_of(dev, struct i2c_client, dev);
> + struct anx7814_data *anx7814 = i2c_get_clientdata(client);
> +
> + dev_info(&client->dev, "resume\n");
> + queue_delayed_work(anx7814->workqueue, &anx7814->work, 0);
> +
> + return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(anx7814_i2c_pm_ops,
> + anx7814_i2c_suspend, anx7814_i2c_resume);
> +
> +static const struct i2c_device_id anx7814_id[] = {
> + {"anx7814", 0},
> + { /* sentinel */ }
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, anx7814_id);
> +
> +static const struct of_device_id anx_match_table[] = {
> + {.compatible = "analogix,anx7814",},
> + { /* sentinel */ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, anx_match_table);
> +
> +static struct i2c_driver anx7814_driver = {
> + .driver = {
> + .name = "anx7814",
> + .pm = &anx7814_i2c_pm_ops,
> + .of_match_table = of_match_ptr(anx_match_table),
> + },
> + .probe = anx7814_i2c_probe,
> + .remove = anx7814_i2c_remove,
> + .id_table = anx7814_id,
> +};
> +
> +module_i2c_driver(anx7814_driver);
> +
> +MODULE_DESCRIPTION("Slimport transmitter ANX7814 driver");
> +MODULE_AUTHOR("Junhua Xia <jxia@xxxxxxxxxxxxxxxx>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_VERSION("1.1");
> diff --git a/drivers/staging/slimport/slimport.h b/drivers/staging/slimport/slimport.h
> new file mode 100644
> index 0000000..614d746
> --- /dev/null
> +++ b/drivers/staging/slimport/slimport.h
> @@ -0,0 +1,49 @@
> +/*
> + * Copyright(c) 2015, Analogix Semiconductor. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#ifndef _SLIMPORT_H
> +#define _SLIMPORT_H
> +
> +#include <linux/i2c.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/workqueue.h>
> +#include <linux/gpio/consumer.h>
> +
> +#define AUX_ERR 1
> +#define AUX_OK 0
> +
> +#define LOG_TAG "SlimPort ANX78XX"
> +
> +struct anx7814_platform_data {
> + struct gpio_desc *gpiod_pd;
> + struct gpio_desc *gpiod_reset;
> + spinlock_t lock;
> +};
> +
> +struct anx7814_data {
Nit: my personal preference would be to drop the "_data":
struct anx7814 (or struct anx78xx, to match the driver name).
> + struct i2c_client *client;
> + struct anx7814_platform_data *pdata;
> + struct delayed_work work;
> + struct workqueue_struct *workqueue;
> + struct mutex lock;
> +};
> +
> +int sp_read_reg(uint8_t slave_addr, uint8_t offset, uint8_t *buf);
> +int sp_write_reg(uint8_t slave_addr, uint8_t offset, uint8_t value);
> +
> +void sp_tx_hardware_poweron(struct anx7814_data *data);
> +void sp_tx_hardware_powerdown(struct anx7814_data *data);
> +
> +#endif
> diff --git a/drivers/staging/slimport/slimport_tx_drv.c b/drivers/staging/slimport/slimport_tx_drv.c
> new file mode 100644
> index 0000000..6fad502
> --- /dev/null
> +++ b/drivers/staging/slimport/slimport_tx_drv.c
> @@ -0,0 +1,3293 @@
> +/*
> + * Copyright(c) 2015, Analogix Semiconductor. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/types.h>
> +
> +#include "slimport.h"
> +#include "slimport_tx_drv.h"
> +
> +#define XTAL_CLK_M10 pxtal_data[XTAL_27M].xtal_clk_m10
> +#define XTAL_CLK pxtal_data[XTAL_27M].xtal_clk
> +
> +static u8 sp_tx_test_bw;
> +static bool sp_tx_test_lt;
> +static bool sp_tx_test_edid;
> +
> +static u8 g_changed_bandwidth;
> +static u8 g_hdmi_dvi_status;
> +
> +static u8 g_need_clean_status;
> +
> +static u8 ds_vid_stb_cntr;
> +static u8 hdcp_fail_count;
> +
> +u8 g_edid_break;
> +u8 g_edid_checksum;
> +u8 edid_blocks[256];
> +static u8 g_read_edid_flag;
> +
> +static struct packet_avi sp_tx_packet_avi;
> +static struct packet_spd sp_tx_packet_spd;
> +static struct packet_mpeg sp_tx_packet_mpeg;
> +static struct audio_info_frame sp_tx_audioinfoframe;
> +
> +enum sp_tx_state sp_tx_system_state;
> +
> +enum audio_output_status sp_tx_ao_state;
> +enum video_output_status sp_tx_vo_state;
> +enum sink_connection_status sp_tx_sc_state;
> +enum sp_tx_lt_status sp_tx_lt_state;
> +enum sp_tx_state sp_tx_system_state_bak;
> +enum hdcp_status hcdp_state;
> +
> +const uint chipid_list[] = {
> + 0x7818,
> + 0x7816,
> + 0x7814,
> + 0x7812,
> + 0x7810,
> + 0x7806,
> + 0x7802
> +};
> +
> +struct common_int common_int_status;
> +struct hdmi_rx_int hdmi_rx_int_status;
> +
> +static u8 down_sample_en;
> +
> +static unsigned char sp_i2c_read_byte(unsigned char dev, unsigned char offset);
> +static void hdmi_rx_new_vsi_int(void);
... yeah, this driver needs a lot of work. All of the above global
state variables need to go away.
> +
> +#define reg_bit_ctl(addr, offset, data, enable) \
> + do { \
> + u8 c; \
> + sp_read_reg(addr, offset, &c); \
> + if (enable) { \
> + if ((c & data) != data) { \
> + c |= data; \
> + sp_write_reg(addr, offset, c); \
> + } \
> + } else { \
> + if ((c & data) == data) { \
> + c &= ~data; \
> + sp_write_reg(addr, offset, c); \
> + } \
> + } \
> + } while (0)
> +
> +#define sp_tx_video_mute(enable) \
> + reg_bit_ctl(TX_P2, VID_CTRL1, VIDEO_MUTE, enable)
> +
> +#define hdmi_rx_mute_audio(enable) \
> + reg_bit_ctl(RX_P0, RX_MUTE_CTRL, AUD_MUTE, enable)
> +
> +#define hdmi_rx_mute_video(enable) \
> + reg_bit_ctl(RX_P0, RX_MUTE_CTRL, VID_MUTE, enable)
> +
> +#define sp_tx_addronly_set(enable) \
> + reg_bit_ctl(TX_P0, AUX_CTRL2, ADDR_ONLY_BIT, enable)
> +
> +#define sp_tx_set_link_bw(bw) \
> + sp_write_reg(TX_P0, SP_TX_LINK_BW_SET_REG, bw)
> +
> +#define sp_tx_get_link_bw() \
> + sp_i2c_read_byte(TX_P0, SP_TX_LINK_BW_SET_REG)
> +
> +#define sp_tx_get_pll_lock_status() \
> + ((sp_i2c_read_byte(TX_P0, TX_DEBUG1) & DEBUG_PLL_LOCK) != 0 ? 1 : 0)
> +
> +#define gen_m_clk_with_downspeading() \
> + sp_write_reg_or(TX_P0, SP_TX_M_CALCU_CTRL, M_GEN_CLK_SEL)
> +
> +#define gen_m_clk_without_downspeading \
> + sp_write_reg_and(TX_P0, SP_TX_M_CALCU_CTRL, (~M_GEN_CLK_SEL))
> +
> +#define hdmi_rx_set_hpd(enable) do { \
> + if ((bool)enable) \
> + sp_write_reg_or(TX_P2, SP_TX_VID_CTRL3_REG, HPD_OUT); \
> + else \
> + sp_write_reg_and(TX_P2, SP_TX_VID_CTRL3_REG, ~HPD_OUT); \
> + } while (0)
> +
> +#define hdmi_rx_set_termination(enable) do { \
> + if ((bool)enable) \
> + sp_write_reg_and(RX_P0, HDMI_RX_TMDS_CTRL_REG7, ~TERM_PD); \
> + else \
> + sp_write_reg_or(RX_P0, HDMI_RX_TMDS_CTRL_REG7, TERM_PD); \
> + } while (0)
> +
> +#define sp_tx_clean_hdcp_status() do { \
> + sp_write_reg(TX_P0, TX_HDCP_CTRL0, 0x03); \
> + sp_write_reg_or(TX_P0, TX_HDCP_CTRL0, RE_AUTH); \
> + usleep_range(2000, 4000); \
> + pr_info("%s %s : sp_tx_clean_hdcp_status\n", LOG_TAG, __func__); \
> + } while (0)
> +
> +#define reg_hardware_reset() do { \
> + sp_write_reg_or(TX_P2, SP_TX_RST_CTRL_REG, HW_RST); \
> + sp_tx_clean_state_machine(); \
> + sp_tx_set_sys_state(STATE_SP_INITIALIZED); \
> + msleep(500); \
> + } while (0)
> +
> +#define write_dpcd_addr(addrh, addrm, addrl) \
> + do { \
> + u8 temp; \
> + if (sp_i2c_read_byte(TX_P0, AUX_ADDR_7_0) != (u8)addrl) \
> + sp_write_reg(TX_P0, AUX_ADDR_7_0, (u8)addrl); \
> + if (sp_i2c_read_byte(TX_P0, AUX_ADDR_15_8) \
> + != (u8)addrm) \
> + sp_write_reg(TX_P0, \
> + AUX_ADDR_15_8, (u8)addrm); \
> + sp_read_reg(TX_P0, AUX_ADDR_19_16, &temp); \
> + if ((u8)(temp & 0x0F) != ((u8)addrh & 0x0F)) \
> + sp_write_reg(TX_P0, AUX_ADDR_19_16, \
> + (temp & 0xF0) | ((u8)addrh)); \
> + } while (0)
> +
> +#define sp_tx_set_sys_state(ss) \
> + do { \
> + pr_info("%s %s : set: clean_status: %x,\n ", \
> + LOG_TAG, __func__, (uint)g_need_clean_status); \
> + if ((sp_tx_system_state >= STATE_LINK_TRAINING) \
> + && (ss < STATE_LINK_TRAINING)) \
> + sp_write_reg_or(TX_P0, SP_TX_ANALOG_PD_REG, CH0_PD); \
> + sp_tx_system_state_bak = sp_tx_system_state; \
> + sp_tx_system_state = (u8)ss; \
> + g_need_clean_status = 1; \
> + print_sys_state(sp_tx_system_state); \
> + } while (0)
> +
> +#define goto_next_system_state() \
> + do { \
> + pr_info("%s %s : next: clean_status: %x,\n ", \
> + LOG_TAG, __func__, (uint)g_need_clean_status); \
> + sp_tx_system_state_bak = sp_tx_system_state; \
> + sp_tx_system_state++;\
> + print_sys_state(sp_tx_system_state); \
> + } while (0)
> +
> +#define redo_cur_system_state() \
> + do { \
> + pr_info("%s %s : redo: clean_status: %x,\n ", \
> + LOG_TAG, __func__, (uint)g_need_clean_status); \
> + g_need_clean_status = 1; \
> + sp_tx_system_state_bak = sp_tx_system_state; \
> + print_sys_state(sp_tx_system_state); \
> + } while (0)
> +
> +#define system_state_change_with_case(status) \
> + do { \
> + if (sp_tx_system_state >= status) { \
> + pr_info("%s %s : change_case: clean_status: %xm,\n ", \
> + LOG_TAG, __func__, (uint)g_need_clean_status); \
> + if ((sp_tx_system_state >= STATE_LINK_TRAINING) \
> + && (status < STATE_LINK_TRAINING)) \
> + sp_write_reg_or(TX_P0, \
> + SP_TX_ANALOG_PD_REG, CH0_PD); \
> + g_need_clean_status = 1; \
> + sp_tx_system_state_bak = sp_tx_system_state; \
> + sp_tx_system_state = (u8)status; \
> + print_sys_state(sp_tx_system_state); \
> + } \
> + } while (0)
> +
> +#define sp_write_reg_or(address, offset, mask) \
> + sp_write_reg(address, offset, \
> + ((unsigned char)sp_i2c_read_byte(address, offset) | (mask)))
> +
> +#define sp_write_reg_and(address, offset, mask) \
> + sp_write_reg(address, offset, \
> + ((unsigned char)sp_i2c_read_byte(address, offset) & (mask)))
> +
> +#define sp_write_reg_and_or(address, offset, and_mask, or_mask) \
> + sp_write_reg(address, offset, \
> + (((unsigned char)sp_i2c_read_byte(address, offset)) \
> + & and_mask) | (or_mask))
> +
> +#define sp_write_reg_or_and(address, offset, or_mask, and_mask) \
> + sp_write_reg(address, offset, \
> + (((unsigned char)sp_i2c_read_byte(address, offset)) \
> + | or_mask) & (and_mask))
... and all of the above "function-like macros" should be converted to
type-safe static functions, where applicable.
> +
> +static inline void sp_tx_link_phy_initialization(void)
> +{
> + sp_write_reg(TX_P2, SP_TX_ANALOG_CTRL0, 0x02);
> + sp_write_reg(TX_P1, SP_TX_LT_CTRL_REG0, 0x01);
> + sp_write_reg(TX_P1, SP_TX_LT_CTRL_REG10, 0x00);
> + sp_write_reg(TX_P1, SP_TX_LT_CTRL_REG1, 0x03);
> + sp_write_reg(TX_P1, SP_TX_LT_CTRL_REG11, 0x00);
> + sp_write_reg(TX_P1, SP_TX_LT_CTRL_REG2, 0x07);
> + sp_write_reg(TX_P1, SP_TX_LT_CTRL_REG12, 0x00);
> + sp_write_reg(TX_P1, SP_TX_LT_CTRL_REG3, 0x7f);
> + sp_write_reg(TX_P1, SP_TX_LT_CTRL_REG13, 0x00);
> + sp_write_reg(TX_P1, SP_TX_LT_CTRL_REG4, 0x71);
> + sp_write_reg(TX_P1, SP_TX_LT_CTRL_REG14, 0x0c);
> + sp_write_reg(TX_P1, SP_TX_LT_CTRL_REG5, 0x6b);
> + sp_write_reg(TX_P1, SP_TX_LT_CTRL_REG15, 0x42);
> + sp_write_reg(TX_P1, SP_TX_LT_CTRL_REG6, 0x7f);
> + sp_write_reg(TX_P1, SP_TX_LT_CTRL_REG16, 0x1e);
> + sp_write_reg(TX_P1, SP_TX_LT_CTRL_REG7, 0x73);
> + sp_write_reg(TX_P1, SP_TX_LT_CTRL_REG17, 0x3e);
> + sp_write_reg(TX_P1, SP_TX_LT_CTRL_REG8, 0x7f);
> + sp_write_reg(TX_P1, SP_TX_LT_CTRL_REG18, 0x72);
> + sp_write_reg(TX_P1, SP_TX_LT_CTRL_REG9, 0x7F);
> + sp_write_reg(TX_P1, SP_TX_LT_CTRL_REG19, 0x7e);
> +}
> +
> +static unsigned char sp_i2c_read_byte(unsigned char dev, unsigned char offset)
> +{
> + unsigned char temp;
> +
> + sp_read_reg(dev, offset, &temp);
> + return temp;
> +}
> +
> +static void hardware_power_ctl(struct anx7814_data *data, u8 enable)
> +{
> + if (enable == 0)
> + sp_tx_hardware_powerdown(data);
> + else
> + sp_tx_hardware_poweron(data);
> +}
> +
> +void wait_aux_op_finish(u8 *err_flag)
> +{
> + u8 cnt;
> + u8 c;
> +
> + *err_flag = 0;
> + cnt = 150;
> + while (sp_i2c_read_byte(TX_P0, AUX_CTRL2) & AUX_OP_EN) {
> + usleep_range(2000, 4000);
> + if ((cnt--) == 0) {
> + pr_info("%s %s :aux operate failed!\n",
> + LOG_TAG, __func__);
dev_err() for all of these error prints.
> + *err_flag = 1;
> + break;
> + }
> + }
> +
> + sp_read_reg(TX_P0, SP_TX_AUX_STATUS, &c);
> + if (c & 0x0F) {
> + pr_info("%s %s : wait aux operation status %.2x\n",
> + LOG_TAG, __func__, (uint)c);
> + *err_flag = 1;
> + }
> +}
> +
> +void print_sys_state(u8 ss)
> +{
> + switch (ss) {
> + case STATE_WAITTING_CABLE_PLUG:
> + pr_info("%s %s : -STATE_WAITTING_CABLE_PLUG-\n",
> + LOG_TAG, __func__);
dev_dbg() for all of these state transitions.
> + break;
> + case STATE_SP_INITIALIZED:
> + pr_info("%s %s : -STATE_SP_INITIALIZED-\n", LOG_TAG, __func__);
> + break;
> + case STATE_SINK_CONNECTION:
> + pr_info("%s %s : -STATE_SINK_CONNECTION-\n", LOG_TAG, __func__);
> + break;
> + case STATE_PARSE_EDID:
> + pr_info("%s %s : -STATE_PARSE_EDID-\n", LOG_TAG, __func__);
> + break;
> + case STATE_LINK_TRAINING:
> + pr_info("%s %s : -STATE_LINK_TRAINING-\n", LOG_TAG, __func__);
> + break;
> + case STATE_VIDEO_OUTPUT:
> + pr_info("%s %s : -STATE_VIDEO_OUTPUT-\n", LOG_TAG, __func__);
> + break;
> + case STATE_HDCP_AUTH:
> + pr_info("%s %s : -STATE_HDCP_AUTH-\n", LOG_TAG, __func__);
> + break;
> + case STATE_AUDIO_OUTPUT:
> + pr_info("%s %s : -STATE_AUDIO_OUTPUT-\n", LOG_TAG, __func__);
> + break;
> + case STATE_PLAY_BACK:
> + pr_info("%s %s : -STATE_PLAY_BACK-\n", LOG_TAG, __func__);
> + break;
> + default:
> + pr_err("%s %s : system state is error1\n", LOG_TAG, __func__);
> + break;
> + }
> +}
> +
> +void sp_tx_rst_aux(void)
> +{
> + sp_write_reg_or(TX_P2, RST_CTRL2, AUX_RST);
> + sp_write_reg_and(TX_P2, RST_CTRL2, ~AUX_RST);
> +}
> +
> +u8 sp_tx_aux_dpcdread_bytes(u8 addrh, u8 addrm,
> + u8 addrl, u8 ccount, u8 *pbuf)
> +{
> + u8 c, c1, i;
> + u8 bok;
> +
> + sp_write_reg(TX_P0, BUF_DATA_COUNT, 0x80);
> + c = ((ccount - 1) << 4) | 0x09;
> + sp_write_reg(TX_P0, AUX_CTRL, c);
> + write_dpcd_addr(addrh, addrm, addrl);
> + sp_write_reg_or(TX_P0, AUX_CTRL2, AUX_OP_EN);
> + usleep_range(2000, 4000);
> +
> + wait_aux_op_finish(&bok);
> + if (bok == AUX_ERR) {
> + pr_err("%s %s : aux read failed\n", LOG_TAG, __func__);
> + sp_read_reg(TX_P2, SP_TX_INT_STATUS1, &c);
> + sp_read_reg(TX_P0, TX_DEBUG1, &c1);
> + if (c1 & POLLING_EN) {
> + if (c & POLLING_ERR)
> + sp_tx_rst_aux();
> + } else
> + sp_tx_rst_aux();
> + return AUX_ERR;
> + }
> +
> + for (i = 0; i < ccount; i++) {
> + sp_read_reg(TX_P0, BUF_DATA_0 + i, &c);
> + *(pbuf + i) = c;
> + if (i >= MAX_BUF_CNT)
> + break;
> + }
> + return AUX_OK;
> +}
> +
> +u8 sp_tx_aux_dpcdwrite_bytes(u8 addrh, u8 addrm,
> + u8 addrl, u8 ccount, u8 *pbuf)
> +{
> + u8 c, i, ret;
> +
> + c = ((ccount - 1) << 4) | 0x08;
> + sp_write_reg(TX_P0, AUX_CTRL, c);
> + write_dpcd_addr(addrh, addrm, addrl);
> + for (i = 0; i < ccount; i++) {
> + c = *pbuf;
> + pbuf++;
> + sp_write_reg(TX_P0, BUF_DATA_0 + i, c);
> +
> + if (i >= 15)
> + break;
> + }
> + sp_write_reg_or(TX_P0, AUX_CTRL2, AUX_OP_EN);
> + wait_aux_op_finish(&ret);
> + return ret;
> +}
> +
> +u8 sp_tx_aux_dpcdwrite_byte(u8 addrh, u8 addrm,
> + u8 addrl, u8 data1)
> +{
> + u8 ret;
> +
> + sp_write_reg(TX_P0, AUX_CTRL, 0x08);
> + write_dpcd_addr(addrh, addrm, addrl);
> + sp_write_reg(TX_P0, BUF_DATA_0, data1);
> + sp_write_reg_or(TX_P0, AUX_CTRL2, AUX_OP_EN);
> + wait_aux_op_finish(&ret);
> + return ret;
> +}
> +
> +void slimport_block_power_ctrl(enum sp_tx_power_block sp_tx_pd_block,
> + u8 power)
> +{
> + if (power == SP_POWER_ON)
> + sp_write_reg_and(TX_P2, SP_POWERD_CTRL_REG, (~sp_tx_pd_block));
> + else
> + sp_write_reg_or(TX_P2, SP_POWERD_CTRL_REG, (sp_tx_pd_block));
> + pr_info("%s %s : sp_tx_power_on: %.2x\n",
> + LOG_TAG, __func__, (uint)sp_tx_pd_block);
> +}
> +
> +void vbus_power_ctrl(unsigned char on)
I'd just split this into two functions:
anx78xx_vbus_power_on() & anx78xx_vbus_power_off()
> +{
> + u8 i;
> +
> + if (on == 0) {
> + sp_write_reg_and(TX_P2, TX_PLL_FILTER, ~V33_SWITCH_ON);
> + sp_write_reg_or(TX_P2, TX_PLL_FILTER5,
> + P5V_PROTECT_PD | SHORT_PROTECT_PD);
> + pr_info("%s %s : 3.3V output disabled\n", LOG_TAG, __func__);
> + } else {
> + for (i = 0; i < 5; i++) {
> + sp_write_reg_and(TX_P2, TX_PLL_FILTER5,
> + (~P5V_PROTECT_PD & ~SHORT_PROTECT_PD));
> + sp_write_reg_or(TX_P2, TX_PLL_FILTER, V33_SWITCH_ON);
> + if (!((u8)sp_i2c_read_byte(TX_P2, TX_PLL_FILTER5)
> + & 0xc0)) {
> + pr_info("%s %s : 3.3V output enabled\n",
> + LOG_TAG, __func__);
> + break;
> + }
> +
> + pr_info("%s %s : VBUS power can not be supplied\n",
> + LOG_TAG, __func__);
> + }
> + }
> +}
> +
> +void sp_tx_clean_state_machine(void)
> +{
> + sp_tx_system_state = STATE_WAITTING_CABLE_PLUG;
> + sp_tx_system_state_bak = STATE_WAITTING_CABLE_PLUG;
> + sp_tx_sc_state = SC_INIT;
> + sp_tx_lt_state = LT_INIT;
> + hcdp_state = HDCP_CAPABLE_CHECK;
> + sp_tx_vo_state = VO_WAIT_VIDEO_STABLE;
> + sp_tx_ao_state = AO_INIT;
> +}
> +
> +u8 sp_tx_cur_states(void)
> +{
> + return sp_tx_system_state;
> +}
> +
> +u8 sp_tx_cur_bw(void)
> +{
> + return g_changed_bandwidth;
> +}
> +
> +void sp_tx_set_bw(u8 bw)
> +{
> + g_changed_bandwidth = bw;
> +}
> +
> +void sp_tx_variable_init(void)
> +{
> + uint i;
> +
> + sp_tx_system_state = STATE_WAITTING_CABLE_PLUG;
> + sp_tx_system_state_bak = STATE_WAITTING_CABLE_PLUG;
> +
> + g_edid_break = 0;
> + g_read_edid_flag = 0;
> + g_edid_checksum = 0;
> + for (i = 0; i < 256; i++)
> + edid_blocks[i] = 0;
> +
> + sp_tx_lt_state = LT_INIT;
> + hcdp_state = HDCP_CAPABLE_CHECK;
> + g_need_clean_status = 0;
> + sp_tx_sc_state = SC_INIT;
> + sp_tx_vo_state = VO_WAIT_VIDEO_STABLE;
> + sp_tx_ao_state = AO_INIT;
> + g_changed_bandwidth = LINK_5P4G;
> + g_hdmi_dvi_status = HDMI_MODE;
> +
> + sp_tx_test_lt = 0;
> + sp_tx_test_bw = 0;
> + sp_tx_test_edid = 0;
> +
> + ds_vid_stb_cntr = 0;
> + hdcp_fail_count = 0;
> +
> +}
> +
> +static void hdmi_rx_tmds_phy_initialization(void)
> +{
> + sp_write_reg(RX_P0, HDMI_RX_TMDS_CTRL_REG2, 0xa9);
> + sp_write_reg(RX_P0, HDMI_RX_TMDS_CTRL_REG7, 0x80);
> +
> + sp_write_reg(RX_P0, HDMI_RX_TMDS_CTRL_REG1, 0x90);
> + sp_write_reg(RX_P0, HDMI_RX_TMDS_CTRL_REG6, 0x92);
> + sp_write_reg(RX_P0, HDMI_RX_TMDS_CTRL_REG20, 0xf2);
> +}
> +
> +void hdmi_rx_initialization(void)
> +{
> + sp_write_reg(RX_P0, RX_MUTE_CTRL, AUD_MUTE | VID_MUTE);
> + sp_write_reg_or(RX_P0, RX_CHIP_CTRL,
> + MAN_HDMI5V_DET | PLLLOCK_CKDT_EN | DIGITAL_CKDT_EN);
> +
> + sp_write_reg_or(RX_P0, RX_SRST, HDCP_MAN_RST | SW_MAN_RST |
> + TMDS_RST | VIDEO_RST);
> + sp_write_reg_and(RX_P0, RX_SRST, (~HDCP_MAN_RST) & (~SW_MAN_RST) &
> + (~TMDS_RST) & (~VIDEO_RST));
> +
> + sp_write_reg_or(RX_P0, RX_AEC_EN0, AEC_EN06 | AEC_EN05);
> + sp_write_reg_or(RX_P0, RX_AEC_EN2, AEC_EN21);
> + sp_write_reg_or(RX_P0, RX_AEC_CTRL, AVC_EN | AAC_OE | AAC_EN);
> +
> + sp_write_reg_and(RX_P0, RX_SYS_PWDN1, ~PWDN_CTRL);
> +
> + sp_write_reg_or(RX_P0, RX_VID_DATA_RNG, R2Y_INPUT_LIMIT);
> + sp_write_reg(RX_P0, 0x65, 0xc4);
> + sp_write_reg(RX_P0, 0x66, 0x18);
> +
> + sp_write_reg(TX_P0, TX_EXTRA_ADDR, 0x50); /* enable DDC stretch */
> +
> + hdmi_rx_tmds_phy_initialization();
> + hdmi_rx_set_hpd(0);
> + hdmi_rx_set_termination(0);
> + pr_info("%s %s : HDMI Rx is initialized...\n", LOG_TAG, __func__);
> +}
> +
> +struct clock_data const pxtal_data[XTAL_CLK_NUM] = {
static const struct clock_data pxtal_data[] = {
Also, if this is an anx78xx specific type, it should have a name like
"struct anx78xx_clock_data".
> + {19, 192},
> + {24, 240},
> + {25, 250},
> + {26, 260},
> + {27, 270},
> + {38, 384},
> + {52, 520},
> + {27, 270},
> +};
> +
> +void xtal_clk_sel(void)
> +{
> + pr_info("%s %s : define XTAL_CLK: %x\n ",
> + LOG_TAG, __func__, (uint)XTAL_27M);
> + sp_write_reg_and_or(TX_P2,
> + TX_ANALOG_DEBUG2, (~0x3c), 0x3c & (XTAL_27M << 2));
> + sp_write_reg(TX_P0, 0xEC, (u8)(((uint)XTAL_CLK_M10)));
> + sp_write_reg(TX_P0, 0xED,
> + (u8)(((uint)XTAL_CLK_M10 & 0xFF00) >> 2) | XTAL_CLK);
> +
> + sp_write_reg(TX_P0,
> + I2C_GEN_10US_TIMER0, (u8)(((uint)XTAL_CLK_M10)));
> + sp_write_reg(TX_P0, I2C_GEN_10US_TIMER1,
> + (u8)(((uint)XTAL_CLK_M10 & 0xFF00) >> 8));
> + sp_write_reg(TX_P0, 0xBF, (u8)(((uint)XTAL_CLK - 1)));
> +
> + sp_write_reg_and_or(RX_P0, 0x49, 0x07,
> + (u8)(((((uint)XTAL_CLK) >> 1) - 2) << 3));
> +
> +}
> +
> +void sp_tx_initialization(void)
> +{
> + sp_write_reg(TX_P0, AUX_CTRL2, 0x30);
> + sp_write_reg_or(TX_P0, AUX_CTRL2, 0x08);
> +
> + sp_write_reg_and(TX_P0, TX_HDCP_CTRL, (~AUTO_EN) & (~AUTO_START));
> + sp_write_reg(TX_P0, OTP_KEY_PROTECT1, OTP_PSW1);
> + sp_write_reg(TX_P0, OTP_KEY_PROTECT2, OTP_PSW2);
> + sp_write_reg(TX_P0, OTP_KEY_PROTECT3, OTP_PSW3);
> + sp_write_reg_or(TX_P0, HDCP_KEY_CMD, DISABLE_SYNC_HDCP);
> + sp_write_reg(TX_P2, SP_TX_VID_CTRL8_REG, VID_VRES_TH);
> +
> + sp_write_reg(TX_P0, HDCP_AUTO_TIMER, HDCP_AUTO_TIMER_VAL);
> + sp_write_reg_or(TX_P0, TX_HDCP_CTRL, LINK_POLLING);
> +
> + sp_write_reg_or(TX_P0, TX_LINK_DEBUG, M_VID_DEBUG);
> + sp_write_reg_or(TX_P2, TX_ANALOG_DEBUG2, POWERON_TIME_1P5MS);
> +
> + xtal_clk_sel();
> + sp_write_reg(TX_P0, AUX_DEFER_CTRL, 0x8C);
> +
> + sp_write_reg_or(TX_P0, TX_DP_POLLING, AUTO_POLLING_DISABLE);
> + /*
> + * Short the link intergrity check timer to speed up bstatus
> + * polling for HDCP CTS item 1A-07
> + */
> + sp_write_reg(TX_P0, SP_TX_LINK_CHK_TIMER, 0x1d);
> + sp_write_reg_or(TX_P0, TX_MISC, EQ_TRAINING_LOOP);
> +
> + sp_write_reg_or(TX_P0, SP_TX_ANALOG_PD_REG, CH0_PD);
> +
> + sp_write_reg(TX_P2, SP_TX_INT_CTRL_REG, 0X01);
> + /* disable HDCP mismatch function for VGA dongle */
> + sp_tx_link_phy_initialization();
> + gen_m_clk_with_downspeading();
> +
> + down_sample_en = 0;
> +}
> +
> +bool slimport_chip_detect(struct anx7814_data *data)
> +{
> + uint16_t id;
> + uint8_t idh = 0, idl = 0;
> + int i;
> +
> + hardware_power_ctl(data, 1);
> +
> + /* check chip id */
> + sp_read_reg(TX_P2, SP_TX_DEV_IDL_REG, &idl);
> + sp_read_reg(TX_P2, SP_TX_DEV_IDH_REG, &idh);
> + id = idl | (idh << 8);
> +
> + pr_info("%s %s : CHIPID: ANX%x\n", LOG_TAG, __func__, id & 0xffff);
> + for (i = 0; i < sizeof(chipid_list) / sizeof(uint16_t); i++) {
> + if (id == chipid_list[i])
> + return 1;
> + }
> + return 0;
bool functions should return "true" and "false".
> +}
> +
> +void slimport_waitting_cable_plug_process(struct anx7814_data *data)
spelling: "_waiting_"
> +{
> + sp_tx_variable_init();
> + hardware_power_ctl(data, 1);
> + goto_next_system_state();
> +}
> +
> +/*
> + * Check if it is ANALOGIX dongle.
> + */
> +unsigned char ANX_OUI[3] = {0x00, 0x22, 0xB9};
static const u8
> +
> +unsigned char is_anx_dongle(void)
> +{
> + unsigned char buf[3];
> +
> + /* 0x0500~0x0502: BRANCH_IEEE_OUI */
> + sp_tx_aux_dpcdread_bytes(0x00, 0x05, 0x00, 3, buf);
> + if (buf[0] == ANX_OUI[0] && buf[1] == ANX_OUI[1]
> + && buf[2] == ANX_OUI[2])
memcmp()
> + return 1;
> +
> + return 0;
> +}
> +
> +void sp_tx_get_rx_bw(unsigned char *bw)
> +{
> + u8 data_buf[4];
> + /*
> + * When ANX dongle connected, if CHIP_ID = 0x7750, bandwidth is 6.75G,
> + * ecause ANX7750 DPCD 0x052x was not available.
"because"
> + */
> + if (is_anx_dongle()) {
> + sp_tx_aux_dpcdread_bytes(0x00, 0x05, 0x03, 0x04, data_buf);
> + if ((data_buf[0] == 0x37) && (data_buf[1] == 0x37)
> + && (data_buf[2] == 0x35) && (data_buf[3] == 0x30))
What are these magic numbers?
> + *bw = LINK_6P75G;
> + else
> + sp_tx_aux_dpcdread_bytes(0x00, 0x05, 0x21, 1, bw);
> + /* just for Debug */
> + *bw = LINK_6P75G;
You just set this twice?
> + } else
Use '{}' on both sides of the else:
} else {
> + sp_tx_aux_dpcdread_bytes(0x00, 0x00, DPCD_MAX_LINK_RATE, 1, bw);
> +}
> +
> +static u8 sp_tx_get_cable_type(enum cable_type_status det_cable_type_state)
> +{
> + u8 ds_port_preset;
> + u8 aux_status;
> + u8 data_buf[16];
> + u8 cur_cable_type;
> +
> + ds_port_preset = 0;
> + cur_cable_type = DWN_STRM_IS_NULL;
> +
> + aux_status =
> + sp_tx_aux_dpcdread_bytes(0x00, 0x00, 0x05, 1, &ds_port_preset);
> + pr_info("%s %s : DPCD 0x005: %x\n",
> + LOG_TAG, __func__, (int)ds_port_preset);
> +
> + switch (det_cable_type_state) {
> + case CHECK_AUXCH:
> + if (AUX_OK == aux_status) {
> + sp_tx_aux_dpcdread_bytes(0x00, 0x00, 0, 0x0c, data_buf);
> + det_cable_type_state = GETTED_CABLE_TYPE;
> + } else {
> + msleep(50);
> + pr_err("%s %s : AUX access error\n",
> + LOG_TAG, __func__);
Why the delay just to print an error message?
dev_err()
> + break;
> + }
> + case GETTED_CABLE_TYPE:
> + switch ((ds_port_preset & (_BIT1 | _BIT2)) >> 1) {
> + case 0x00:
> + cur_cable_type = DWN_STRM_IS_DIGITAL;
> + pr_info("%s %s : Downstream is DP dongle.\n",
> + LOG_TAG, __func__);
> + break;
> + case 0x01:
> + case 0x03:
> + cur_cable_type = DWN_STRM_IS_ANALOG;
> + pr_info("%s %s : Downstream is VGA dongle.\n",
> + LOG_TAG, __func__);
> + break;
> + case 0x02:
> + cur_cable_type = DWN_STRM_IS_HDMI;
> + pr_info("%s %s : Downstream is HDMI dongle.\n",
> + LOG_TAG, __func__);
> + break;
> + default:
> + cur_cable_type = DWN_STRM_IS_NULL;
> + pr_err("%s %s : Downstream can not recognized.\n",
> + LOG_TAG, __func__);
> + break;
> + }
> + default:
> + break;
> + }
> + return cur_cable_type;
> +}
> +
> +u8 sp_tx_get_hdmi_connection(void)
> +{
> + u8 c;
> +
> + if (AUX_OK != sp_tx_aux_dpcdread_bytes(0x00, 0x05, 0x18, 1, &c))
In the kernel, the constant is usually on the other side of the !=, like this:
if (sp_tx_aux_dpcdread_bytes(0x00, 0x05, 0x18, 1, &c) != AUX_OK)
> + return 0;
> +
> + if ((c & 0x41) == 0x41)
magic number?
> + return 1;
> + else
> + return 0;
> +
> +}
> +
> +u8 sp_tx_get_vga_connection(void)
> +{
> + u8 c;
> +
> + if (AUX_OK !=
> + sp_tx_aux_dpcdread_bytes(0x00, 0x02, DPCD_SINK_COUNT, 1, &c)) {
> + pr_err("%s %s : aux error.\n", LOG_TAG, __func__);
> + return 0;
> + }
> +
> + if (c & 0x01)
> + return 1;
> + else
> + return 0;
> +}
> +
> +u8 sp_tx_get_dp_connection(void)
> +{
> + u8 c;
> +
> + if (AUX_OK !=
> + sp_tx_aux_dpcdread_bytes(0x00, 0x02, DPCD_SINK_COUNT, 1, &c))
> + return 0;
> +
> + if (c & 0x1f) {
> + sp_tx_aux_dpcdread_bytes(0x00, 0x00, 0x04, 1, &c);
> + if (c & 0x20) {
> + sp_tx_aux_dpcdread_bytes(0x00, 0x06, 0x00, 1, &c);
> + /*
> + * Bit 5 = SET_DN_DEVICE_DP_PWR_5V
> + * Bit 6 = SET_DN_DEVICE_DP_PWR_12V
> + * Bit 7 = SET_DN_DEVICE_DP_PWR_18V
> + */
> + c = c & 0x1F;
> + sp_tx_aux_dpcdwrite_byte(0x00, 0x06, 0x00, c | 0x20);
> + }
> + return 1;
> + } else
> + return 0;
> +}
> +
> +u8 sp_tx_get_downstream_connection(void)
> +{
> + return sp_tx_get_dp_connection();
> +}
> +
> +void slimport_sink_connection(void)
> +{
The whole "state machine" in this function seems entirely unnecessary.
Just do all required hotplug actions sequentially in a worker,
triggered from a hotplug event.
> + switch (sp_tx_sc_state) {
> + case SC_INIT:
> + sp_tx_sc_state++;
> + case SC_CHECK_CABLE_TYPE:
> + case SC_WAITTING_CABLE_TYPE:
> + default:
> + if (sp_tx_get_cable_type(CHECK_AUXCH) == DWN_STRM_IS_NULL) {
> + sp_tx_sc_state++;
> + if (sp_tx_sc_state >= SC_WAITTING_CABLE_TYPE) {
> + sp_tx_sc_state = SC_NOT_CABLE;
> + pr_info("%s %s : Can not get cable type!\n",
> + LOG_TAG, __func__);
> + }
> + break;
> + }
> +
> + sp_tx_sc_state = SC_SINK_CONNECTED;
> + case SC_SINK_CONNECTED:
> + if (sp_tx_get_downstream_connection())
> + goto_next_system_state();
> + break;
> + case SC_NOT_CABLE:
> + vbus_power_ctrl(0);
> + reg_hardware_reset();
> + break;
> + }
> +}
> +
> +/******************start EDID process********************/
> +void sp_tx_enable_video_input(u8 enable)
> +{
> + u8 c;
> +
> + sp_read_reg(TX_P2, VID_CTRL1, &c);
> + if (enable) {
> + if ((c & VIDEO_EN) != VIDEO_EN) {
The check here doesn't buy you much.
Just always set/clear the enable bit.
> + c = (c & 0xf7) | VIDEO_EN;
> + sp_write_reg(TX_P2, VID_CTRL1, c);
> + pr_info("%s %s : Slimport Video is enabled!\n",
> + LOG_TAG, __func__);
> + }
> + } else {
> + if ((c & VIDEO_EN) == VIDEO_EN) {
> + c &= ~VIDEO_EN;
> + sp_write_reg(TX_P2, VID_CTRL1, c);
> + pr_info("%s %s : Slimport Video is disabled!\n",
> + LOG_TAG, __func__);
> + }
> + }
> +}
> +
> +static u8 read_dvi_hdmi_mode(void)
> +{
> + return 1;
> +}
> +
> +static u8 get_edid_detail(u8 *data_buf)
> +{
> + uint pixclock_edid;
u16 perhaps?
> +
> + pixclock_edid = ((((uint)data_buf[1] << 8))
> + | ((uint)data_buf[0] & 0xFF));
> + if (pixclock_edid <= 5300)
> + return LINK_1P62G;
> + else if ((5300 < pixclock_edid) && (pixclock_edid <= 8900))
> + return LINK_2P7G;
> + else if ((8900 < pixclock_edid) && (pixclock_edid <= 18000))
> + return LINK_5P4G;
> + else
> + return LINK_6P75G;
> +}
> +
[snip...]
> +
> diff --git a/drivers/staging/slimport/slimport_tx_drv.h b/drivers/staging/slimport/slimport_tx_drv.h
> new file mode 100644
> index 0000000..9aaf47d
> --- /dev/null
> +++ b/drivers/staging/slimport/slimport_tx_drv.h
> @@ -0,0 +1,254 @@
> +/*
> + * Copyright(c) 2015, Analogix Semiconductor. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#ifndef _SP_TX_DRV_H
> +#define _SP_TX_DRV_H
> +
> +#include "slimport.h"
> +#include "slimport_tx_reg.h"
> +
> +#define FW_VERSION 0x22
> +
> +#define _BIT0 0x01
> +#define _BIT1 0x02
> +#define _BIT2 0x04
> +#define _BIT3 0x08
> +#define _BIT4 0x10
> +#define _BIT5 0x20
> +#define _BIT6 0x40
> +#define _BIT7 0x80
BIT() already exists.
[snip...]
Ok, enough for now :-).
I'd be happy to review more in detail after you:
(1) clean up the typos, and little nits above
(2) move to the drm/bridge directory
(3) rename the files, variables, types, etc. to anx78xx
(4) plumb through the context struct to all functions that act on the device
(5) use proper messaging (dev_ rather than pr_, _dbg/_err rather than _info)
I think replacing the state machine with an event-driven approach can
wait for a future round of review.
-Dan
On Mon, Sep 7, 2015 at 2:46 PM, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> On Mon, Sep 07, 2015 at 07:41:08AM +0200, Enric Balletbo Serra wrote:
>> 2015-09-07 1:27 GMT+02:00 Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>:
>> > On Sun, Sep 06, 2015 at 11:14:02PM +0200, Enric Balletbo i Serra wrote:
>> >> The ANX7814 is an ultra-low power Full-HD (1080p60) SlimPort transmitter
>> >> designed for portable devices.
>> >>
>> >> This driver adds initial support and supports HDMI to DP pass-through mode.
>> >>
>> >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
>> >> ---
>> >> drivers/staging/Kconfig | 2 +
>> >> drivers/staging/Makefile | 1 +
>> >> drivers/staging/slimport/Kconfig | 7 +
>> >> drivers/staging/slimport/Makefile | 4 +
>> >> drivers/staging/slimport/slimport.c | 301 +++
>> >> drivers/staging/slimport/slimport.h | 49 +
>> >> drivers/staging/slimport/slimport_tx_drv.c | 3293 ++++++++++++++++++++++++++++
>> >> drivers/staging/slimport/slimport_tx_drv.h | 254 +++
>> >> drivers/staging/slimport/slimport_tx_reg.h | 786 +++++++
>> >
>> > Why is this a staging driver?
>> > What prevents it from being merged into the "real" part of the kernel
>> > tree?
>> >
>>
>> I'll be glad to move the driver to their subsystem if you think it's a
>> the better place. Basically there are two reasons why I send the
>> driver to the staging directory. The first one is because my test
>> environment is a bit limited, with my environment I can only test the
>> HDMI to DisplayPort pass-through mode so the driver builds but it's
>> partially tested. The second one is that I expect I'll need to
>> refactor some code, specially in slimport_tx_drv.c file to be
>> accepted, I decided not change too much this file from the original to
>> not break the functionality, so I thought that will be better send
>> first to the staging driver to have first reviews.
>>
>> > All staging drivers need a TODO file, listing what needs to be done and
>> > who is in charge of it. I can't take this without that added.
>> >
>>
>> ok, I'll add in the next series once received some feedback (or move
>> to the video subsystem)
>
> I suggest trying to get it merged "properly" first before having to
> fall-back to the staging subsystem.
>
> thanks,
>
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
