Re: [PATCH 2/2 v16] drm/bridge: Add I2C based driver for ps8640 bridge

[Russell King - ARM Linux]

On Tue, Jul 12, 2016 at 12:13:52PM +0200, Daniel Vetter wrote:
> Might be better to just do a request_firmware on driver load, and
> simply proceed if it's not there.

That is almost never a good idea - if the driver is built-in, then
the request_firmware call happens before the real rootfs is mounted,
which makes it complicated to get the firmware delivered to the
driver.

Sure, it works nicely if the drivers are built as modules, but not
everyone wants to deal with modules and initramfs images.

If we're wanting to just update the firmware, that should be an
explicit administrative decision requiring an explicit action by the
system administrator, and not done by placing a file in some magic
location so that request_firmware() can find it, which then gets
picked up at boot time/driver load time.  Consider what could happen
if linux-firmware picks up the file and places it in the appropriate
location by default.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 2/2 v16] drm/bridge: Add I2C based driver for ps8640 bridge

[Russell King - ARM Linux]

On Tue, Jul 12, 2016 at 12:13:52PM +0200, Daniel Vetter wrote:
> Might be better to just do a request_firmware on driver load, and
> simply proceed if it's not there.

That is almost never a good idea - if the driver is built-in, then
the request_firmware call happens before the real rootfs is mounted,
which makes it complicated to get the firmware delivered to the
driver.

Sure, it works nicely if the drivers are built as modules, but not
everyone wants to deal with modules and initramfs images.

If we're wanting to just update the firmware, that should be an
explicit administrative decision requiring an explicit action by the
system administrator, and not done by placing a file in some magic
location so that request_firmware() can find it, which then gets
picked up at boot time/driver load time.  Consider what could happen
if linux-firmware picks up the file and places it in the appropriate
location by default.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 2/2 v16] drm/bridge: Add I2C based driver for ps8640 bridge

[Enric Balletbo Serra]

2016-07-12 12:13 GMT+02:00 Daniel Vetter :
> On Wed, Jun 29, 2016 at 6:31 AM, Daniel Kurtz  wrote:
>> On Fri, Jun 17, 2016 at 3:14 AM, Emil Velikov  
>> wrote:
 +static ssize_t ps8640_update_fw_store(struct device *dev,
 + struct device_attribute *attr,
 + const char *buf, size_t count)
 +{
 +   struct i2c_client *client = to_i2c_client(dev);
 +   struct ps8640 *ps_bridge = i2c_get_clientdata(client);
 +   const struct firmware *fw;
 +   int error;
 +
 +   error = request_firmware(, PS_FW_NAME, dev);
>>> Can the device operate without a firmware ? If not, why is the
>>> firmware loaded so later/after user interaction (via sysfs) ? I don't
>>> recall any other driver in DRM to use such an approach.
>>
>> The PS8640 has internal flash, so it should always already have a
>> working firmware.
>> This sysfs interface is useful for user space initiated field firmware 
>> updates.
>
> Might be better to just do a request_firmware on driver load, and
> simply proceed if it's not there. Adding a sysfs interface (which is
> abi) seems way too much overkill for this imo. If you want to upgrade
> the firmware you can then just drop it into the right directory, with
> no further interaction needed.

IMHO I'm not sure if for this use case request_firmware on driver load
is a good idea. Flash the non-volatile internal chip can be a slow
operation and if you forget to remove the firmware after drop it into
the right directory apart from slow down the driver probe you can
damage the chip depending on the write endurance of the chip.

This sysfs interface is used on other subsystems when there is a
non-volatile memory, as example you can see at [1], [2]. Unfortunately
not all are using the same sysfs interface and maybe this should be
standardized (maybe it's an opportunity to define it)

Regards,
 Enric

[1] 
http://lxr.free-electrons.com/source/drivers/input/touchscreen/wdt87xx_i2c.c#L922
[2] http://lxr.free-electrons.com/source/drivers/scsi/pm8001/pm8001_ctl.c#L732


> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2 v16] drm/bridge: Add I2C based driver for ps8640 bridge

[Enric Balletbo Serra]

2016-07-12 12:13 GMT+02:00 Daniel Vetter :
> On Wed, Jun 29, 2016 at 6:31 AM, Daniel Kurtz  wrote:
>> On Fri, Jun 17, 2016 at 3:14 AM, Emil Velikov  
>> wrote:
 +static ssize_t ps8640_update_fw_store(struct device *dev,
 + struct device_attribute *attr,
 + const char *buf, size_t count)
 +{
 +   struct i2c_client *client = to_i2c_client(dev);
 +   struct ps8640 *ps_bridge = i2c_get_clientdata(client);
 +   const struct firmware *fw;
 +   int error;
 +
 +   error = request_firmware(, PS_FW_NAME, dev);
>>> Can the device operate without a firmware ? If not, why is the
>>> firmware loaded so later/after user interaction (via sysfs) ? I don't
>>> recall any other driver in DRM to use such an approach.
>>
>> The PS8640 has internal flash, so it should always already have a
>> working firmware.
>> This sysfs interface is useful for user space initiated field firmware 
>> updates.
>
> Might be better to just do a request_firmware on driver load, and
> simply proceed if it's not there. Adding a sysfs interface (which is
> abi) seems way too much overkill for this imo. If you want to upgrade
> the firmware you can then just drop it into the right directory, with
> no further interaction needed.

IMHO I'm not sure if for this use case request_firmware on driver load
is a good idea. Flash the non-volatile internal chip can be a slow
operation and if you forget to remove the firmware after drop it into
the right directory apart from slow down the driver probe you can
damage the chip depending on the write endurance of the chip.

This sysfs interface is used on other subsystems when there is a
non-volatile memory, as example you can see at [1], [2]. Unfortunately
not all are using the same sysfs interface and maybe this should be
standardized (maybe it's an opportunity to define it)

Regards,
 Enric

[1] 
http://lxr.free-electrons.com/source/drivers/input/touchscreen/wdt87xx_i2c.c#L922
[2] http://lxr.free-electrons.com/source/drivers/scsi/pm8001/pm8001_ctl.c#L732


> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2 v16] drm/bridge: Add I2C based driver for ps8640 bridge

[Daniel Vetter]

On Thu, Aug 04, 2016 at 12:35:59PM +0200, Enric Balletbo Serra wrote:
> 2016-07-12 12:13 GMT+02:00 Daniel Vetter :
> > On Wed, Jun 29, 2016 at 6:31 AM, Daniel Kurtz  wrote:
> >> On Fri, Jun 17, 2016 at 3:14 AM, Emil Velikov  
> >> wrote:
>  +static ssize_t ps8640_update_fw_store(struct device *dev,
>  + struct device_attribute *attr,
>  + const char *buf, size_t count)
>  +{
>  +   struct i2c_client *client = to_i2c_client(dev);
>  +   struct ps8640 *ps_bridge = i2c_get_clientdata(client);
>  +   const struct firmware *fw;
>  +   int error;
>  +
>  +   error = request_firmware(, PS_FW_NAME, dev);
> >>> Can the device operate without a firmware ? If not, why is the
> >>> firmware loaded so later/after user interaction (via sysfs) ? I don't
> >>> recall any other driver in DRM to use such an approach.
> >>
> >> The PS8640 has internal flash, so it should always already have a
> >> working firmware.
> >> This sysfs interface is useful for user space initiated field firmware 
> >> updates.
> >
> > Might be better to just do a request_firmware on driver load, and
> > simply proceed if it's not there. Adding a sysfs interface (which is
> > abi) seems way too much overkill for this imo. If you want to upgrade
> > the firmware you can then just drop it into the right directory, with
> > no further interaction needed.
> 
> IMHO I'm not sure if for this use case request_firmware on driver load
> is a good idea. Flash the non-volatile internal chip can be a slow
> operation and if you forget to remove the firmware after drop it into
> the right directory apart from slow down the driver probe you can
> damage the chip depending on the write endurance of the chip.

Ah ok. Explaining this in the commit message would be really good.

> This sysfs interface is used on other subsystems when there is a
> non-volatile memory, as example you can see at [1], [2]. Unfortunately
> not all are using the same sysfs interface and maybe this should be
> standardized (maybe it's an opportunity to define it)

+1 on standardizing this.
-Daniel
> 
> Regards,
>  Enric
> 
> [1] 
> http://lxr.free-electrons.com/source/drivers/input/touchscreen/wdt87xx_i2c.c#L922
> [2] http://lxr.free-electrons.com/source/drivers/scsi/pm8001/pm8001_ctl.c#L732
> 
> 
> > -Daniel
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> > ___
> > dri-devel mailing list
> > dri-de...@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/2 v16] drm/bridge: Add I2C based driver for ps8640 bridge

[Daniel Vetter]

On Thu, Aug 04, 2016 at 12:35:59PM +0200, Enric Balletbo Serra wrote:
> 2016-07-12 12:13 GMT+02:00 Daniel Vetter :
> > On Wed, Jun 29, 2016 at 6:31 AM, Daniel Kurtz  wrote:
> >> On Fri, Jun 17, 2016 at 3:14 AM, Emil Velikov  
> >> wrote:
>  +static ssize_t ps8640_update_fw_store(struct device *dev,
>  + struct device_attribute *attr,
>  + const char *buf, size_t count)
>  +{
>  +   struct i2c_client *client = to_i2c_client(dev);
>  +   struct ps8640 *ps_bridge = i2c_get_clientdata(client);
>  +   const struct firmware *fw;
>  +   int error;
>  +
>  +   error = request_firmware(, PS_FW_NAME, dev);
> >>> Can the device operate without a firmware ? If not, why is the
> >>> firmware loaded so later/after user interaction (via sysfs) ? I don't
> >>> recall any other driver in DRM to use such an approach.
> >>
> >> The PS8640 has internal flash, so it should always already have a
> >> working firmware.
> >> This sysfs interface is useful for user space initiated field firmware 
> >> updates.
> >
> > Might be better to just do a request_firmware on driver load, and
> > simply proceed if it's not there. Adding a sysfs interface (which is
> > abi) seems way too much overkill for this imo. If you want to upgrade
> > the firmware you can then just drop it into the right directory, with
> > no further interaction needed.
> 
> IMHO I'm not sure if for this use case request_firmware on driver load
> is a good idea. Flash the non-volatile internal chip can be a slow
> operation and if you forget to remove the firmware after drop it into
> the right directory apart from slow down the driver probe you can
> damage the chip depending on the write endurance of the chip.

Ah ok. Explaining this in the commit message would be really good.

> This sysfs interface is used on other subsystems when there is a
> non-volatile memory, as example you can see at [1], [2]. Unfortunately
> not all are using the same sysfs interface and maybe this should be
> standardized (maybe it's an opportunity to define it)

+1 on standardizing this.
-Daniel
> 
> Regards,
>  Enric
> 
> [1] 
> http://lxr.free-electrons.com/source/drivers/input/touchscreen/wdt87xx_i2c.c#L922
> [2] http://lxr.free-electrons.com/source/drivers/scsi/pm8001/pm8001_ctl.c#L732
> 
> 
> > -Daniel
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> > ___
> > dri-devel mailing list
> > dri-de...@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/2 v16] drm/bridge: Add I2C based driver for ps8640 bridge

[Daniel Vetter]

On Wed, Jun 29, 2016 at 6:31 AM, Daniel Kurtz  wrote:
> On Fri, Jun 17, 2016 at 3:14 AM, Emil Velikov  
> wrote:
>>> +static ssize_t ps8640_update_fw_store(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t count)
>>> +{
>>> +   struct i2c_client *client = to_i2c_client(dev);
>>> +   struct ps8640 *ps_bridge = i2c_get_clientdata(client);
>>> +   const struct firmware *fw;
>>> +   int error;
>>> +
>>> +   error = request_firmware(, PS_FW_NAME, dev);
>> Can the device operate without a firmware ? If not, why is the
>> firmware loaded so later/after user interaction (via sysfs) ? I don't
>> recall any other driver in DRM to use such an approach.
>
> The PS8640 has internal flash, so it should always already have a
> working firmware.
> This sysfs interface is useful for user space initiated field firmware 
> updates.

Might be better to just do a request_firmware on driver load, and
simply proceed if it's not there. Adding a sysfs interface (which is
abi) seems way too much overkill for this imo. If you want to upgrade
the firmware you can then just drop it into the right directory, with
no further interaction needed.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/2 v16] drm/bridge: Add I2C based driver for ps8640 bridge

[Daniel Vetter]

On Wed, Jun 29, 2016 at 6:31 AM, Daniel Kurtz  wrote:
> On Fri, Jun 17, 2016 at 3:14 AM, Emil Velikov  
> wrote:
>>> +static ssize_t ps8640_update_fw_store(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t count)
>>> +{
>>> +   struct i2c_client *client = to_i2c_client(dev);
>>> +   struct ps8640 *ps_bridge = i2c_get_clientdata(client);
>>> +   const struct firmware *fw;
>>> +   int error;
>>> +
>>> +   error = request_firmware(, PS_FW_NAME, dev);
>> Can the device operate without a firmware ? If not, why is the
>> firmware loaded so later/after user interaction (via sysfs) ? I don't
>> recall any other driver in DRM to use such an approach.
>
> The PS8640 has internal flash, so it should always already have a
> working firmware.
> This sysfs interface is useful for user space initiated field firmware 
> updates.

Might be better to just do a request_firmware on driver load, and
simply proceed if it's not there. Adding a sysfs interface (which is
abi) seems way too much overkill for this imo. If you want to upgrade
the firmware you can then just drop it into the right directory, with
no further interaction needed.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/2 v16] drm/bridge: Add I2C based driver for ps8640 bridge

[Daniel Kurtz]

Hi Emil,

One answer inline below.  The rest I leave to Jitao...

[snip...]

On Fri, Jun 17, 2016 at 3:14 AM, Emil Velikov  wrote:

>> +static ssize_t ps8640_update_fw_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> +   struct i2c_client *client = to_i2c_client(dev);
>> +   struct ps8640 *ps_bridge = i2c_get_clientdata(client);
>> +   const struct firmware *fw;
>> +   int error;
>> +
>> +   error = request_firmware(, PS_FW_NAME, dev);
> Can the device operate without a firmware ? If not, why is the
> firmware loaded so later/after user interaction (via sysfs) ? I don't
> recall any other driver in DRM to use such an approach.

The PS8640 has internal flash, so it should always already have a
working firmware.
This sysfs interface is useful for user space initiated field firmware updates.

Regards,
-Daniel

> Regards,
> Emil
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2 v16] drm/bridge: Add I2C based driver for ps8640 bridge

[Daniel Kurtz]

Hi Emil,

One answer inline below.  The rest I leave to Jitao...

[snip...]

On Fri, Jun 17, 2016 at 3:14 AM, Emil Velikov  wrote:

>> +static ssize_t ps8640_update_fw_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> +   struct i2c_client *client = to_i2c_client(dev);
>> +   struct ps8640 *ps_bridge = i2c_get_clientdata(client);
>> +   const struct firmware *fw;
>> +   int error;
>> +
>> +   error = request_firmware(, PS_FW_NAME, dev);
> Can the device operate without a firmware ? If not, why is the
> firmware loaded so later/after user interaction (via sysfs) ? I don't
> recall any other driver in DRM to use such an approach.

The PS8640 has internal flash, so it should always already have a
working firmware.
This sysfs interface is useful for user space initiated field firmware updates.

Regards,
-Daniel

> Regards,
> Emil
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2 v16] drm/bridge: Add I2C based driver for ps8640 bridge

[Emil Velikov]

Hi Jitao Shi,

A few comments/suggestions which I hope you'll find useful. Note that
I'm not an expert in the area so take them with a pinch of salt.

On 2 June 2016 at 10:57, Jitao Shi  wrote:

> +#define WRITE_STATUS_REG_CMD   0x01
> +#define READ_STATUS_REG_CMD0x05
> +#define BUSY   BIT(0)
> +#define CLEAR_ALL_PROTECT  0x00
> +#define BLK_PROTECT_BITS   0x0c
> +#define STATUS_REG_PROTECT BIT(7)
> +#define WRITE_ENABLE_CMD   0x06
> +#define CHIP_ERASE_CMD 0xc7
> +#define MAX_DEVS   0x8
Some of the above are unused - SPI_MAX_RETRY_CNT and PAGE2_SWSPI_CTL
come to might. Please double-check and nuke any unused ones for now ?

Add blank line between the macro definitions and the struct.

> +struct ps8640_info {
> +   u8 family_id;
> +   u8 variant_id;
> +   u16 version;
> +};
> +
> +struct ps8640 {
> +   struct drm_connector connector;
> +   struct drm_bridge bridge;
> +   struct edid *edid;
> +   struct mipi_dsi_device dsi;
> +   struct i2c_client *page[MAX_DEVS];
Throw in an enum that provides symbolic names for the different i2c
clients and rename "page" to clients or anything else that makes it
clearer ? Seems like '1' and '6' are never used.

Using better names than client2/7 in the actual code will be great :-)

> +   struct i2c_client *ddc_i2c;
> +   struct regulator_bulk_data supplies[2];
> +   struct drm_panel *panel;
> +   struct gpio_desc *gpio_rst_n;
> +   struct gpio_desc *gpio_slp_n;
> +   struct gpio_desc *gpio_mode_sel_n;
What does the _n suffix mean in the above three ? Bikeshed:
reset_gpio, sleep_gpio and mode_sel(ect)_gpio could also be used ;-)

> +   bool enabled;
> +
> +   /* firmware file info */
> +   struct ps8640_info info;
> +   bool in_fw_update;
> +   /* for firmware update protect */
> +   struct mutex fw_mutex;
> +};
> +
> +static const u8 enc_ctrl_code[6] = { 0xaa, 0x55, 0x50, 0x41, 0x52, 0x44 };
These feel a bit magical. Any chance you can give them symbolic names ?

> +static const u8 hw_chip_id[4] = { 0x00, 0x0a, 0x00, 0x30 };
> +
> +static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
> +{
> +   return container_of(e, struct ps8640, bridge);
> +}
> +
> +static inline struct ps8640 *connector_to_ps8640(struct drm_connector *e)
> +{
> +   return container_of(e, struct ps8640, connector);
> +}
> +
> +static int ps8640_read(struct i2c_client *client, u8 reg, u8 *data,
> +  u16 data_len)
> +{
> +   int ret;
> +   struct i2c_msg msgs[] = {
> +   {
> +.addr = client->addr,
> +.flags = 0,
> +.len = 1,
> +.buf = ,
> +   },
> +   {
> +.addr = client->addr,
> +.flags = I2C_M_RD,
> +.len = data_len,
> +.buf = data,
> +   }
> +   };
> +
> +   ret = i2c_transfer(client->adapter, msgs, 2);
> +
> +   if (ret == 2)
> +   return 0;
> +   if (ret < 0)
> +   return ret;
> +   else
> +   return -EIO;
> +}
> +
> +static int ps8640_write_bytes(struct i2c_client *client, const u8 *data,
> + u16 data_len)
> +{
> +   int ret;
> +   struct i2c_msg msg;
> +
> +   msg.addr = client->addr;
> +   msg.flags = 0;
> +   msg.len = data_len;
> +   msg.buf = (u8 *)data;
> +
> +   ret = i2c_transfer(client->adapter, , 1);
> +   if (ret == 1)
> +   return 0;
> +   if (ret < 0)
> +   return ret;
> +   else
> +   return -EIO;
> +}
> +
> +static int ps8640_write_byte(struct i2c_client *client, u8 reg,  u8 data)
> +{
> +   u8 buf[] = { reg, data };
> +
> +   return ps8640_write_bytes(client, buf, sizeof(buf));
> +}
> +
> +static void ps8640_get_mcu_fw_version(struct ps8640 *ps_bridge)
> +{
> +   struct i2c_client *client = ps_bridge->page[5];
> +   u8 fw_ver[2];
> +
> +   ps8640_read(client, 0x4, fw_ver, sizeof(fw_ver));
> +   ps_bridge->info.version = (fw_ver[0] << 8) | fw_ver[1];
> +
> +   DRM_INFO_ONCE("ps8640 rom fw version %d.%d\n", fw_ver[0], fw_ver[1]);
> +}
> +
> +static int ps8640_bridge_unmute(struct ps8640 *ps_bridge)
> +{
> +   struct i2c_client *client = ps_bridge->page[3];
> +   u8 vdo_ctrl_buf[3] = { PAGE3_SET_ADD, VDO_CTL_ADD, VDO_EN };
> +
> +   return ps8640_write_bytes(client, vdo_ctrl_buf, sizeof(vdo_ctrl_buf));
> +}
> +
> +static int ps8640_bridge_mute(struct ps8640 *ps_bridge)
> +{
> +   struct i2c_client *client = ps_bridge->page[3];
> +   u8 vdo_ctrl_buf[3] = { PAGE3_SET_ADD, VDO_CTL_ADD, VDO_DIS };
> +
> +   return ps8640_write_bytes(client, vdo_ctrl_buf, sizeof(vdo_ctrl_buf));
> +}
> +
> +static void ps8640_pre_enable(struct drm_bridge *bridge)
> +{
> +   struct ps8640 *ps_bridge = 

Re: [PATCH 2/2 v16] drm/bridge: Add I2C based driver for ps8640 bridge

[Emil Velikov]

Hi Jitao Shi,

A few comments/suggestions which I hope you'll find useful. Note that
I'm not an expert in the area so take them with a pinch of salt.

On 2 June 2016 at 10:57, Jitao Shi  wrote:

> +#define WRITE_STATUS_REG_CMD   0x01
> +#define READ_STATUS_REG_CMD0x05
> +#define BUSY   BIT(0)
> +#define CLEAR_ALL_PROTECT  0x00
> +#define BLK_PROTECT_BITS   0x0c
> +#define STATUS_REG_PROTECT BIT(7)
> +#define WRITE_ENABLE_CMD   0x06
> +#define CHIP_ERASE_CMD 0xc7
> +#define MAX_DEVS   0x8
Some of the above are unused - SPI_MAX_RETRY_CNT and PAGE2_SWSPI_CTL
come to might. Please double-check and nuke any unused ones for now ?

Add blank line between the macro definitions and the struct.

> +struct ps8640_info {
> +   u8 family_id;
> +   u8 variant_id;
> +   u16 version;
> +};
> +
> +struct ps8640 {
> +   struct drm_connector connector;
> +   struct drm_bridge bridge;
> +   struct edid *edid;
> +   struct mipi_dsi_device dsi;
> +   struct i2c_client *page[MAX_DEVS];
Throw in an enum that provides symbolic names for the different i2c
clients and rename "page" to clients or anything else that makes it
clearer ? Seems like '1' and '6' are never used.

Using better names than client2/7 in the actual code will be great :-)

> +   struct i2c_client *ddc_i2c;
> +   struct regulator_bulk_data supplies[2];
> +   struct drm_panel *panel;
> +   struct gpio_desc *gpio_rst_n;
> +   struct gpio_desc *gpio_slp_n;
> +   struct gpio_desc *gpio_mode_sel_n;
What does the _n suffix mean in the above three ? Bikeshed:
reset_gpio, sleep_gpio and mode_sel(ect)_gpio could also be used ;-)

> +   bool enabled;
> +
> +   /* firmware file info */
> +   struct ps8640_info info;
> +   bool in_fw_update;
> +   /* for firmware update protect */
> +   struct mutex fw_mutex;
> +};
> +
> +static const u8 enc_ctrl_code[6] = { 0xaa, 0x55, 0x50, 0x41, 0x52, 0x44 };
These feel a bit magical. Any chance you can give them symbolic names ?

> +static const u8 hw_chip_id[4] = { 0x00, 0x0a, 0x00, 0x30 };
> +
> +static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
> +{
> +   return container_of(e, struct ps8640, bridge);
> +}
> +
> +static inline struct ps8640 *connector_to_ps8640(struct drm_connector *e)
> +{
> +   return container_of(e, struct ps8640, connector);
> +}
> +
> +static int ps8640_read(struct i2c_client *client, u8 reg, u8 *data,
> +  u16 data_len)
> +{
> +   int ret;
> +   struct i2c_msg msgs[] = {
> +   {
> +.addr = client->addr,
> +.flags = 0,
> +.len = 1,
> +.buf = ,
> +   },
> +   {
> +.addr = client->addr,
> +.flags = I2C_M_RD,
> +.len = data_len,
> +.buf = data,
> +   }
> +   };
> +
> +   ret = i2c_transfer(client->adapter, msgs, 2);
> +
> +   if (ret == 2)
> +   return 0;
> +   if (ret < 0)
> +   return ret;
> +   else
> +   return -EIO;
> +}
> +
> +static int ps8640_write_bytes(struct i2c_client *client, const u8 *data,
> + u16 data_len)
> +{
> +   int ret;
> +   struct i2c_msg msg;
> +
> +   msg.addr = client->addr;
> +   msg.flags = 0;
> +   msg.len = data_len;
> +   msg.buf = (u8 *)data;
> +
> +   ret = i2c_transfer(client->adapter, , 1);
> +   if (ret == 1)
> +   return 0;
> +   if (ret < 0)
> +   return ret;
> +   else
> +   return -EIO;
> +}
> +
> +static int ps8640_write_byte(struct i2c_client *client, u8 reg,  u8 data)
> +{
> +   u8 buf[] = { reg, data };
> +
> +   return ps8640_write_bytes(client, buf, sizeof(buf));
> +}
> +
> +static void ps8640_get_mcu_fw_version(struct ps8640 *ps_bridge)
> +{
> +   struct i2c_client *client = ps_bridge->page[5];
> +   u8 fw_ver[2];
> +
> +   ps8640_read(client, 0x4, fw_ver, sizeof(fw_ver));
> +   ps_bridge->info.version = (fw_ver[0] << 8) | fw_ver[1];
> +
> +   DRM_INFO_ONCE("ps8640 rom fw version %d.%d\n", fw_ver[0], fw_ver[1]);
> +}
> +
> +static int ps8640_bridge_unmute(struct ps8640 *ps_bridge)
> +{
> +   struct i2c_client *client = ps_bridge->page[3];
> +   u8 vdo_ctrl_buf[3] = { PAGE3_SET_ADD, VDO_CTL_ADD, VDO_EN };
> +
> +   return ps8640_write_bytes(client, vdo_ctrl_buf, sizeof(vdo_ctrl_buf));
> +}
> +
> +static int ps8640_bridge_mute(struct ps8640 *ps_bridge)
> +{
> +   struct i2c_client *client = ps_bridge->page[3];
> +   u8 vdo_ctrl_buf[3] = { PAGE3_SET_ADD, VDO_CTL_ADD, VDO_DIS };
> +
> +   return ps8640_write_bytes(client, vdo_ctrl_buf, sizeof(vdo_ctrl_buf));
> +}
> +
> +static void ps8640_pre_enable(struct drm_bridge *bridge)
> +{
> +   struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
> +   

Re: [PATCH 2/2 v16] drm/bridge: Add I2C based driver for ps8640 bridge

[Daniel Kurtz]

Hi Jitao,

On Thu, Jun 2, 2016 at 5:57 PM, Jitao Shi  wrote:
>
> This patch adds drm_bridge driver for parade DSI to eDP bridge chip.
>
> Signed-off-by: Jitao Shi 
> Reviewed-by: Daniel Kurtz 
> ---
> Changes since v15:
>  - Drop drm_connector_(un)register calls from parade ps8640.
>The main DRM driver mtk_drm_drv now calls
>drm_connector_register_all() after drm_dev_register() in the
>mtk_drm_bind() function. That function should iterate over all
>connectors and call drm_connector_register() for each of them.
>So, remove drm_connector_(un)register calls from parade ps8640.

[snip...]

> +static void ps8640_pre_enable(struct drm_bridge *bridge)
> +{
> +   struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
> +   struct i2c_client *client = ps_bridge->page[2];
> +   int err;
> +   u8 set_vdo_done;
> +   ktime_t timeout;
> +
> +   if (ps_bridge->in_fw_update)
> +   return;
> +
> +   if (ps_bridge->enabled)
> +   return;
> +
> +   err = drm_panel_prepare(ps_bridge->panel);
> +   if (err < 0) {
> +   DRM_ERROR("failed to prepare panel: %d\n", err);
> +   return;
> +   }
> +

(1) For patch v10, Philipp Zabel commented that gpio_slp_n &
gpio_rst_n are both active low, and that the device tree should
contain a reset-gpios property with the GPIO_ACTIVE_LOW flag set.
(2) However, you did change the the reset logic from v10 -> v11, but
it isn't clear why (nor mentioned in the patch notes).

v10 (https://patchwork.kernel.org/patch/8357851/) had:

+ gpiod_set_value(ps_bridge->gpio_slp_n, 1);
+ gpiod_set_value(ps_bridge->gpio_rst_n, 0);
+
+ err = regulator_bulk_enable(ARRAY_SIZE(ps_bridge->supplies),
+ps_bridge->supplies);
+ if (err < 0) {
+ DRM_ERROR("cannot enable regulators %d\n", err);
+ goto err_panel_unprepare;
+ }
+
+ usleep_range(500, 700);
+ gpiod_set_value(ps_bridge->gpio_rst_n, 1);

In other words:
  (a) de-assert power down
  (b) assert reset
  (c) enable 1.2 & 3.3 regulators
  (d)  (aka regulator-ramp-delay in device-tree)
  (e) wait an additional 2 ms (as requested by Parade for ps8640 to stabilize?)
  (f) de-assert reset
  (g) wait 200 ms (for ps8640 FW to load?)

This mostly made sense to me, except for step (a)... I'm not sure why
you de-assert power down before enabling the regulators.  It seems
like you'd want to do that later, maybe after reset (can you ask
Paradetech?).

Now (as of v11) it has changed to:

> +   err = regulator_bulk_enable(ARRAY_SIZE(ps_bridge->supplies),
> +   ps_bridge->supplies);
> +   if (err < 0) {
> +   DRM_ERROR("cannot enable regulators %d\n", err);
> +   goto err_panel_unprepare;
> +   }
> +
> +   gpiod_set_value(ps_bridge->gpio_slp_n, 1);
> +   gpiod_set_value(ps_bridge->gpio_rst_n, 0);
> +   usleep_range(2000, 2500);
> +   gpiod_set_value(ps_bridge->gpio_rst_n, 1);

Two additional comments:

(3) if you correctly configure these gpios as GPIO_ACTIVE_LOW, you can
drop the "_n" suffix, which will make the driver code easier to
understand.
(4) "gpio_slp_n" is called "PD#" by the PS8640 datasheet, so a better
name might be: "gpio_power_down".

Thanks,
-Dan

Re: [PATCH 2/2 v16] drm/bridge: Add I2C based driver for ps8640 bridge

[Daniel Kurtz]

Hi Jitao,

On Thu, Jun 2, 2016 at 5:57 PM, Jitao Shi  wrote:
>
> This patch adds drm_bridge driver for parade DSI to eDP bridge chip.
>
> Signed-off-by: Jitao Shi 
> Reviewed-by: Daniel Kurtz 
> ---
> Changes since v15:
>  - Drop drm_connector_(un)register calls from parade ps8640.
>The main DRM driver mtk_drm_drv now calls
>drm_connector_register_all() after drm_dev_register() in the
>mtk_drm_bind() function. That function should iterate over all
>connectors and call drm_connector_register() for each of them.
>So, remove drm_connector_(un)register calls from parade ps8640.

[snip...]

> +static void ps8640_pre_enable(struct drm_bridge *bridge)
> +{
> +   struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
> +   struct i2c_client *client = ps_bridge->page[2];
> +   int err;
> +   u8 set_vdo_done;
> +   ktime_t timeout;
> +
> +   if (ps_bridge->in_fw_update)
> +   return;
> +
> +   if (ps_bridge->enabled)
> +   return;
> +
> +   err = drm_panel_prepare(ps_bridge->panel);
> +   if (err < 0) {
> +   DRM_ERROR("failed to prepare panel: %d\n", err);
> +   return;
> +   }
> +

(1) For patch v10, Philipp Zabel commented that gpio_slp_n &
gpio_rst_n are both active low, and that the device tree should
contain a reset-gpios property with the GPIO_ACTIVE_LOW flag set.
(2) However, you did change the the reset logic from v10 -> v11, but
it isn't clear why (nor mentioned in the patch notes).

v10 (https://patchwork.kernel.org/patch/8357851/) had:

+ gpiod_set_value(ps_bridge->gpio_slp_n, 1);
+ gpiod_set_value(ps_bridge->gpio_rst_n, 0);
+
+ err = regulator_bulk_enable(ARRAY_SIZE(ps_bridge->supplies),
+ps_bridge->supplies);
+ if (err < 0) {
+ DRM_ERROR("cannot enable regulators %d\n", err);
+ goto err_panel_unprepare;
+ }
+
+ usleep_range(500, 700);
+ gpiod_set_value(ps_bridge->gpio_rst_n, 1);

In other words:
  (a) de-assert power down
  (b) assert reset
  (c) enable 1.2 & 3.3 regulators
  (d)  (aka regulator-ramp-delay in device-tree)
  (e) wait an additional 2 ms (as requested by Parade for ps8640 to stabilize?)
  (f) de-assert reset
  (g) wait 200 ms (for ps8640 FW to load?)

This mostly made sense to me, except for step (a)... I'm not sure why
you de-assert power down before enabling the regulators.  It seems
like you'd want to do that later, maybe after reset (can you ask
Paradetech?).

Now (as of v11) it has changed to:

> +   err = regulator_bulk_enable(ARRAY_SIZE(ps_bridge->supplies),
> +   ps_bridge->supplies);
> +   if (err < 0) {
> +   DRM_ERROR("cannot enable regulators %d\n", err);
> +   goto err_panel_unprepare;
> +   }
> +
> +   gpiod_set_value(ps_bridge->gpio_slp_n, 1);
> +   gpiod_set_value(ps_bridge->gpio_rst_n, 0);
> +   usleep_range(2000, 2500);
> +   gpiod_set_value(ps_bridge->gpio_rst_n, 1);

Two additional comments:

(3) if you correctly configure these gpios as GPIO_ACTIVE_LOW, you can
drop the "_n" suffix, which will make the driver code easier to
understand.
(4) "gpio_slp_n" is called "PD#" by the PS8640 datasheet, so a better
name might be: "gpio_power_down".

Thanks,
-Dan