Re: [PATCH 3/3] staging: slimport: Add anx7814 driver support by analogix.
From: Enric Balletbo Serra
Date: Wed Sep 09 2015 - 03:10:27 EST
Hi Dan,
Many thanks for this first review.
2015-09-09 5:38 GMT+02:00 Daniel Kurtz <djkurtz@xxxxxxxxxxxx>:
> 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
>
I sent yesterday another version moving the driver. I'm not sure if
you were aware. In the second version I moved the driver to
drivers/gpu/drm/i2c instead of drivers/gpu/drm/bridge. Do you think
bridge is the better place ? If so I'll move to bridge directory. I
had doubts about it.
> 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'll include all your comments and send for review again.
> I think replacing the state machine with an event-driven approach can
> wait for a future round of review.
>
Looks a good plan for me, first review more trivial problems.
Best regards,
Enric
> -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/
--
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/