2019-03-11 13:48:13

by Måns Rullgård

Subject: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property

Sometimes it is desirabled to use a separate i2c controller for ddc
access. This adds support for the ddc-i2c-bus property of the
hdmi-connector node, using the specified controller if provided.

Signed-off-by: Mans Rullgard <[email protected]>
---
drivers/gpu/drm/sun4i/sun4i_hdmi.h | 1 +
drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 37 +++++++++++++++++++++++---
2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
index b685ee11623d..b08c4453d47c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
@@ -269,6 +269,7 @@ struct sun4i_hdmi {
struct clk *tmds_clk;

struct i2c_adapter *i2c;
+ struct i2c_adapter *ddc_i2c;

/* Regmap fields for I2C adapter */
struct regmap_field *field_ddc_en;
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
index 061d2e0d9011..5b2fac79f5d6 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
@@ -212,7 +212,7 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
struct edid *edid;
int ret;

- edid = drm_get_edid(connector, hdmi->i2c);
+ edid = drm_get_edid(connector, hdmi->ddc_i2c ?: hdmi->i2c);
if (!edid)
return 0;

@@ -228,6 +228,28 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
return ret;
}

+static struct i2c_adapter *sun4i_hdmi_get_ddc(struct device *dev)
+{
+ struct device_node *phandle, *remote;
+ struct i2c_adapter *ddc;
+
+ remote = of_graph_get_remote_node(dev->of_node, 1, -1);
+ if (!remote)
+ return ERR_PTR(-EINVAL);
+
+ phandle = of_parse_phandle(remote, "ddc-i2c-bus", 0);
+ of_node_put(remote);
+ if (!phandle)
+ return NULL;
+
+ ddc = of_get_i2c_adapter_by_node(phandle);
+ of_node_put(phandle);
+ if (!ddc)
+ return ERR_PTR(-EPROBE_DEFER);
+
+ return ddc;
+}
+
static const struct drm_connector_helper_funcs sun4i_hdmi_connector_helper_funcs = {
.get_modes = sun4i_hdmi_get_modes,
};
@@ -575,6 +597,12 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
goto err_disable_mod_clk;
}

+ hdmi->ddc_i2c = sun4i_hdmi_get_ddc(dev);
+ if (IS_ERR(hdmi->ddc_i2c)) {
+ ret = PTR_ERR(hdmi->ddc_i2c);
+ goto err_del_i2c_adapter;
+ }
+
drm_encoder_helper_add(&hdmi->encoder,
&sun4i_hdmi_helper_funcs);
ret = drm_encoder_init(drm,
@@ -584,14 +612,14 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
NULL);
if (ret) {
dev_err(dev, "Couldn't initialise the HDMI encoder\n");
- goto err_del_i2c_adapter;
+ goto err_put_ddc_i2c;
}

hdmi->encoder.possible_crtcs = drm_of_find_possible_crtcs(drm,
dev->of_node);
if (!hdmi->encoder.possible_crtcs) {
ret = -EPROBE_DEFER;
- goto err_del_i2c_adapter;
+ goto err_put_ddc_i2c;
}

#ifdef CONFIG_DRM_SUN4I_HDMI_CEC
@@ -630,6 +658,8 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
err_cleanup_connector:
cec_delete_adapter(hdmi->cec_adap);
drm_encoder_cleanup(&hdmi->encoder);
+err_put_ddc_i2c:
+ i2c_put_adapter(hdmi->ddc_i2c);
err_del_i2c_adapter:
i2c_del_adapter(hdmi->i2c);
err_disable_mod_clk:
@@ -650,6 +680,7 @@ static void sun4i_hdmi_unbind(struct device *dev, struct device *master,
drm_connector_cleanup(&hdmi->connector);
drm_encoder_cleanup(&hdmi->encoder);
i2c_del_adapter(hdmi->i2c);
+ i2c_put_adapter(hdmi->ddc_i2c);
clk_disable_unprepare(hdmi->mod_clk);
clk_disable_unprepare(hdmi->bus_clk);
}
--
2.21.0

2019-03-11 15:49:11

by Maxime Ripard

Subject: Re: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property

Hi!

On Mon, Mar 11, 2019 at 01:47:13PM +0000, Mans Rullgard wrote:
> Sometimes it is desirabled to use a separate i2c controller for ddc
> access. This adds support for the ddc-i2c-bus property of the
> hdmi-connector node, using the specified controller if provided.
>
> Signed-off-by: Mans Rullgard <[email protected]>
> ---
> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 1 +
> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 37 +++++++++++++++++++++++---
> 2 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> index b685ee11623d..b08c4453d47c 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> @@ -269,6 +269,7 @@ struct sun4i_hdmi {
> struct clk *tmds_clk;
>
> struct i2c_adapter *i2c;
> + struct i2c_adapter *ddc_i2c;
>
> /* Regmap fields for I2C adapter */
> struct regmap_field *field_ddc_en;
> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> index 061d2e0d9011..5b2fac79f5d6 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> @@ -212,7 +212,7 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
> struct edid *edid;
> int ret;
>
> - edid = drm_get_edid(connector, hdmi->i2c);
> + edid = drm_get_edid(connector, hdmi->ddc_i2c ?: hdmi->i2c);

You can't test whether ddc_i2c is NULL or not...

> if (!edid)
> return 0;
>
> @@ -228,6 +228,28 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
> return ret;
> }
>
> +static struct i2c_adapter *sun4i_hdmi_get_ddc(struct device *dev)
> +{
> + struct device_node *phandle, *remote;
> + struct i2c_adapter *ddc;
> +
> + remote = of_graph_get_remote_node(dev->of_node, 1, -1);
> + if (!remote)
> + return ERR_PTR(-EINVAL);
> +
> + phandle = of_parse_phandle(remote, "ddc-i2c-bus", 0);
> + of_node_put(remote);
> + if (!phandle)
> + return NULL;
> +
> + ddc = of_get_i2c_adapter_by_node(phandle);
> + of_node_put(phandle);
> + if (!ddc)
> + return ERR_PTR(-EPROBE_DEFER);
> +
> + return ddc;

... Since even in (most) error cases you're returning a !NULL pointer.

> +}
> +
> static const struct drm_connector_helper_funcs sun4i_hdmi_connector_helper_funcs = {
> .get_modes = sun4i_hdmi_get_modes,
> };
> @@ -575,6 +597,12 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
> goto err_disable_mod_clk;
> }
>
> + hdmi->ddc_i2c = sun4i_hdmi_get_ddc(dev);
> + if (IS_ERR(hdmi->ddc_i2c)) {
> + ret = PTR_ERR(hdmi->ddc_i2c);
> + goto err_del_i2c_adapter;
> + }
> +
> drm_encoder_helper_add(&hdmi->encoder,
> &sun4i_hdmi_helper_funcs);
> ret = drm_encoder_init(drm,
> @@ -584,14 +612,14 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
> NULL);
> if (ret) {
> dev_err(dev, "Couldn't initialise the HDMI encoder\n");
> - goto err_del_i2c_adapter;
> + goto err_put_ddc_i2c;
> }
>
> hdmi->encoder.possible_crtcs = drm_of_find_possible_crtcs(drm,
> dev->of_node);
> if (!hdmi->encoder.possible_crtcs) {
> ret = -EPROBE_DEFER;
> - goto err_del_i2c_adapter;
> + goto err_put_ddc_i2c;
> }
>
> #ifdef CONFIG_DRM_SUN4I_HDMI_CEC
> @@ -630,6 +658,8 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
> err_cleanup_connector:
> cec_delete_adapter(hdmi->cec_adap);
> drm_encoder_cleanup(&hdmi->encoder);
> +err_put_ddc_i2c:
> + i2c_put_adapter(hdmi->ddc_i2c);
> err_del_i2c_adapter:
> i2c_del_adapter(hdmi->i2c);
> err_disable_mod_clk:
> @@ -650,6 +680,7 @@ static void sun4i_hdmi_unbind(struct device *dev, struct device *master,
> drm_connector_cleanup(&hdmi->connector);
> drm_encoder_cleanup(&hdmi->encoder);
> i2c_del_adapter(hdmi->i2c);
> + i2c_put_adapter(hdmi->ddc_i2c);
> clk_disable_unprepare(hdmi->mod_clk);
> clk_disable_unprepare(hdmi->bus_clk);
> }
> --
> 2.21.0
>

It looks good otherwise, thanks!
Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Attachments:

2019-03-11 16:11:52

by Måns Rullgård

Subject: Re: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property

Maxime Ripard <[email protected]> writes:

> Hi!
>
> On Mon, Mar 11, 2019 at 01:47:13PM +0000, Mans Rullgard wrote:
>> Sometimes it is desirabled to use a separate i2c controller for ddc
>> access. This adds support for the ddc-i2c-bus property of the
>> hdmi-connector node, using the specified controller if provided.
>>
>> Signed-off-by: Mans Rullgard <[email protected]>
>> ---
>> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 1 +
>> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 37 +++++++++++++++++++++++---
>> 2 files changed, 35 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> index b685ee11623d..b08c4453d47c 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> @@ -269,6 +269,7 @@ struct sun4i_hdmi {
>> struct clk *tmds_clk;
>>
>> struct i2c_adapter *i2c;
>> + struct i2c_adapter *ddc_i2c;
>>
>> /* Regmap fields for I2C adapter */
>> struct regmap_field *field_ddc_en;
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> index 061d2e0d9011..5b2fac79f5d6 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> @@ -212,7 +212,7 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
>> struct edid *edid;
>> int ret;
>>
>> - edid = drm_get_edid(connector, hdmi->i2c);
>> + edid = drm_get_edid(connector, hdmi->ddc_i2c ?: hdmi->i2c);
>
> You can't test whether ddc_i2c is NULL or not...
>
>> if (!edid)
>> return 0;
>>
>> @@ -228,6 +228,28 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
>> return ret;
>> }
>>
>> +static struct i2c_adapter *sun4i_hdmi_get_ddc(struct device *dev)
>> +{
>> + struct device_node *phandle, *remote;
>> + struct i2c_adapter *ddc;
>> +
>> + remote = of_graph_get_remote_node(dev->of_node, 1, -1);
>> + if (!remote)
>> + return ERR_PTR(-EINVAL);
>> +
>> + phandle = of_parse_phandle(remote, "ddc-i2c-bus", 0);
>> + of_node_put(remote);
>> + if (!phandle)
>> + return NULL;
>> +
>> + ddc = of_get_i2c_adapter_by_node(phandle);
>> + of_node_put(phandle);
>> + if (!ddc)
>> + return ERR_PTR(-EPROBE_DEFER);
>> +
>> + return ddc;
>
> ... Since even in (most) error cases you're returning a !NULL pointer.
>
>> +}
>> +
>> static const struct drm_connector_helper_funcs sun4i_hdmi_connector_helper_funcs = {
>> .get_modes = sun4i_hdmi_get_modes,
>> };
>> @@ -575,6 +597,12 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
>> goto err_disable_mod_clk;
>> }
>>
>> + hdmi->ddc_i2c = sun4i_hdmi_get_ddc(dev);
>> + if (IS_ERR(hdmi->ddc_i2c)) {

... which is checked here.

The property is optional, so the idea was to return null in that case
and use the built-in controller. If the property exists but some error
occurs, we want to abort rather than proceed with the fallback which
almost certainly won't work.

Maybe I got something wrong in that logic.

--
M?ns Rullg?rd

2019-03-14 15:42:41

by Maxime Ripard

Subject: Re: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property

On Mon, Mar 11, 2019 at 04:11:06PM +0000, M?ns Rullg?rd wrote:
> Maxime Ripard <[email protected]> writes:
>
> > Hi!
> >
> > On Mon, Mar 11, 2019 at 01:47:13PM +0000, Mans Rullgard wrote:
> >> Sometimes it is desirabled to use a separate i2c controller for ddc
> >> access. This adds support for the ddc-i2c-bus property of the
> >> hdmi-connector node, using the specified controller if provided.
> >>
> >> Signed-off-by: Mans Rullgard <[email protected]>
> >> ---
> >> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 1 +
> >> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 37 +++++++++++++++++++++++---
> >> 2 files changed, 35 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> >> index b685ee11623d..b08c4453d47c 100644
> >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> >> @@ -269,6 +269,7 @@ struct sun4i_hdmi {
> >> struct clk *tmds_clk;
> >>
> >> struct i2c_adapter *i2c;
> >> + struct i2c_adapter *ddc_i2c;
> >>
> >> /* Regmap fields for I2C adapter */
> >> struct regmap_field *field_ddc_en;
> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> >> index 061d2e0d9011..5b2fac79f5d6 100644
> >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> >> @@ -212,7 +212,7 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
> >> struct edid *edid;
> >> int ret;
> >>
> >> - edid = drm_get_edid(connector, hdmi->i2c);
> >> + edid = drm_get_edid(connector, hdmi->ddc_i2c ?: hdmi->i2c);
> >
> > You can't test whether ddc_i2c is NULL or not...
> >
> >> if (!edid)
> >> return 0;
> >>
> >> @@ -228,6 +228,28 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
> >> return ret;
> >> }
> >>
> >> +static struct i2c_adapter *sun4i_hdmi_get_ddc(struct device *dev)
> >> +{
> >> + struct device_node *phandle, *remote;
> >> + struct i2c_adapter *ddc;
> >> +
> >> + remote = of_graph_get_remote_node(dev->of_node, 1, -1);
> >> + if (!remote)
> >> + return ERR_PTR(-EINVAL);
> >> +
> >> + phandle = of_parse_phandle(remote, "ddc-i2c-bus", 0);
> >> + of_node_put(remote);
> >> + if (!phandle)
> >> + return NULL;
> >> +
> >> + ddc = of_get_i2c_adapter_by_node(phandle);
> >> + of_node_put(phandle);
> >> + if (!ddc)
> >> + return ERR_PTR(-EPROBE_DEFER);
> >> +
> >> + return ddc;
> >
> > ... Since even in (most) error cases you're returning a !NULL pointer.
> >
> >> +}
> >> +
> >> static const struct drm_connector_helper_funcs sun4i_hdmi_connector_helper_funcs = {
> >> .get_modes = sun4i_hdmi_get_modes,
> >> };
> >> @@ -575,6 +597,12 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
> >> goto err_disable_mod_clk;
> >> }
> >>
> >> + hdmi->ddc_i2c = sun4i_hdmi_get_ddc(dev);
> >> + if (IS_ERR(hdmi->ddc_i2c)) {
>
> ... which is checked here.
>
> The property is optional, so the idea was to return null in that case
> and use the built-in controller. If the property exists but some error
> occurs, we want to abort rather than proceed with the fallback which
> almost certainly won't work.
>
> Maybe I got something wrong in that logic.

Indeed, I just got confused. I guess returning ENODEV in such a case,
and testing for that, would make things more obvious.

Thanks!
Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Attachments:

2019-03-14 16:10:13

by Måns Rullgård

Subject: Re: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property

Maxime Ripard <[email protected]> writes:

> On Mon, Mar 11, 2019 at 04:11:06PM +0000, M?ns Rullg?rd wrote:
>> Maxime Ripard <[email protected]> writes:
>>
>> > Hi!
>> >
>> > On Mon, Mar 11, 2019 at 01:47:13PM +0000, Mans Rullgard wrote:
>> >> Sometimes it is desirabled to use a separate i2c controller for ddc
>> >> access. This adds support for the ddc-i2c-bus property of the
>> >> hdmi-connector node, using the specified controller if provided.
>> >>
>> >> Signed-off-by: Mans Rullgard <[email protected]>
>> >> ---
>> >> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 1 +
>> >> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 37 +++++++++++++++++++++++---
>> >> 2 files changed, 35 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> >> index b685ee11623d..b08c4453d47c 100644
>> >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> >> @@ -269,6 +269,7 @@ struct sun4i_hdmi {
>> >> struct clk *tmds_clk;
>> >>
>> >> struct i2c_adapter *i2c;
>> >> + struct i2c_adapter *ddc_i2c;
>> >>
>> >> /* Regmap fields for I2C adapter */
>> >> struct regmap_field *field_ddc_en;
>> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> >> index 061d2e0d9011..5b2fac79f5d6 100644
>> >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> >> @@ -212,7 +212,7 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
>> >> struct edid *edid;
>> >> int ret;
>> >>
>> >> - edid = drm_get_edid(connector, hdmi->i2c);
>> >> + edid = drm_get_edid(connector, hdmi->ddc_i2c ?: hdmi->i2c);
>> >
>> > You can't test whether ddc_i2c is NULL or not...
>> >
>> >> if (!edid)
>> >> return 0;
>> >>
>> >> @@ -228,6 +228,28 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
>> >> return ret;
>> >> }
>> >>
>> >> +static struct i2c_adapter *sun4i_hdmi_get_ddc(struct device *dev)
>> >> +{
>> >> + struct device_node *phandle, *remote;
>> >> + struct i2c_adapter *ddc;
>> >> +
>> >> + remote = of_graph_get_remote_node(dev->of_node, 1, -1);
>> >> + if (!remote)
>> >> + return ERR_PTR(-EINVAL);
>> >> +
>> >> + phandle = of_parse_phandle(remote, "ddc-i2c-bus", 0);
>> >> + of_node_put(remote);
>> >> + if (!phandle)
>> >> + return NULL;
>> >> +
>> >> + ddc = of_get_i2c_adapter_by_node(phandle);
>> >> + of_node_put(phandle);
>> >> + if (!ddc)
>> >> + return ERR_PTR(-EPROBE_DEFER);
>> >> +
>> >> + return ddc;
>> >
>> > ... Since even in (most) error cases you're returning a !NULL pointer.
>> >
>> >> +}
>> >> +
>> >> static const struct drm_connector_helper_funcs sun4i_hdmi_connector_helper_funcs = {
>> >> .get_modes = sun4i_hdmi_get_modes,
>> >> };
>> >> @@ -575,6 +597,12 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
>> >> goto err_disable_mod_clk;
>> >> }
>> >>
>> >> + hdmi->ddc_i2c = sun4i_hdmi_get_ddc(dev);
>> >> + if (IS_ERR(hdmi->ddc_i2c)) {
>>
>> ... which is checked here.
>>
>> The property is optional, so the idea was to return null in that case
>> and use the built-in controller. If the property exists but some error
>> occurs, we want to abort rather than proceed with the fallback which
>> almost certainly won't work.
>>
>> Maybe I got something wrong in that logic.
>
> Indeed, I just got confused. I guess returning ENODEV in such a case,
> and testing for that, would make things more obvious.

There's also a case I hadn't thought of: property exists but isn't a
valid phandle. What do you think is the correct action in that case?

--
M?ns Rullg?rd

2019-03-18 15:52:33

by Maxime Ripard

Subject: Re: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property

On Thu, Mar 14, 2019 at 04:09:13PM +0000, M?ns Rullg?rd wrote:
> Maxime Ripard <[email protected]> writes:
>
> > On Mon, Mar 11, 2019 at 04:11:06PM +0000, M?ns Rullg?rd wrote:
> >> Maxime Ripard <[email protected]> writes:
> >>
> >> > Hi!
> >> >
> >> > On Mon, Mar 11, 2019 at 01:47:13PM +0000, Mans Rullgard wrote:
> >> >> Sometimes it is desirabled to use a separate i2c controller for ddc
> >> >> access. This adds support for the ddc-i2c-bus property of the
> >> >> hdmi-connector node, using the specified controller if provided.
> >> >>
> >> >> Signed-off-by: Mans Rullgard <[email protected]>
> >> >> ---
> >> >> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 1 +
> >> >> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 37 +++++++++++++++++++++++---
> >> >> 2 files changed, 35 insertions(+), 3 deletions(-)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> >> >> index b685ee11623d..b08c4453d47c 100644
> >> >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> >> >> @@ -269,6 +269,7 @@ struct sun4i_hdmi {
> >> >> struct clk *tmds_clk;
> >> >>
> >> >> struct i2c_adapter *i2c;
> >> >> + struct i2c_adapter *ddc_i2c;
> >> >>
> >> >> /* Regmap fields for I2C adapter */
> >> >> struct regmap_field *field_ddc_en;
> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> >> >> index 061d2e0d9011..5b2fac79f5d6 100644
> >> >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> >> >> @@ -212,7 +212,7 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
> >> >> struct edid *edid;
> >> >> int ret;
> >> >>
> >> >> - edid = drm_get_edid(connector, hdmi->i2c);
> >> >> + edid = drm_get_edid(connector, hdmi->ddc_i2c ?: hdmi->i2c);
> >> >
> >> > You can't test whether ddc_i2c is NULL or not...
> >> >
> >> >> if (!edid)
> >> >> return 0;
> >> >>
> >> >> @@ -228,6 +228,28 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
> >> >> return ret;
> >> >> }
> >> >>
> >> >> +static struct i2c_adapter *sun4i_hdmi_get_ddc(struct device *dev)
> >> >> +{
> >> >> + struct device_node *phandle, *remote;
> >> >> + struct i2c_adapter *ddc;
> >> >> +
> >> >> + remote = of_graph_get_remote_node(dev->of_node, 1, -1);
> >> >> + if (!remote)
> >> >> + return ERR_PTR(-EINVAL);
> >> >> +
> >> >> + phandle = of_parse_phandle(remote, "ddc-i2c-bus", 0);
> >> >> + of_node_put(remote);
> >> >> + if (!phandle)
> >> >> + return NULL;
> >> >> +
> >> >> + ddc = of_get_i2c_adapter_by_node(phandle);
> >> >> + of_node_put(phandle);
> >> >> + if (!ddc)
> >> >> + return ERR_PTR(-EPROBE_DEFER);
> >> >> +
> >> >> + return ddc;
> >> >
> >> > ... Since even in (most) error cases you're returning a !NULL pointer.
> >> >
> >> >> +}
> >> >> +
> >> >> static const struct drm_connector_helper_funcs sun4i_hdmi_connector_helper_funcs = {
> >> >> .get_modes = sun4i_hdmi_get_modes,
> >> >> };
> >> >> @@ -575,6 +597,12 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
> >> >> goto err_disable_mod_clk;
> >> >> }
> >> >>
> >> >> + hdmi->ddc_i2c = sun4i_hdmi_get_ddc(dev);
> >> >> + if (IS_ERR(hdmi->ddc_i2c)) {
> >>
> >> ... which is checked here.
> >>
> >> The property is optional, so the idea was to return null in that case
> >> and use the built-in controller. If the property exists but some error
> >> occurs, we want to abort rather than proceed with the fallback which
> >> almost certainly won't work.
> >>
> >> Maybe I got something wrong in that logic.
> >
> > Indeed, I just got confused. I guess returning ENODEV in such a case,
> > and testing for that, would make things more obvious.
>
> There's also a case I hadn't thought of: property exists but isn't a
> valid phandle. What do you think is the correct action in that case?

I think we would have that one covered. of_parse_phandle will return
!NULL, but then of_get_i2c_adapter_by_node will return NULL since we
wouldn't have an associated i2c adapter to the bogus phandle, and you
are checking for that already.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Attachments:

2019-03-18 16:25:01

by Måns Rullgård

Subject: Re: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property

Maxime Ripard <[email protected]> writes:

> On Thu, Mar 14, 2019 at 04:09:13PM +0000, M?ns Rullg?rd wrote:
>> Maxime Ripard <[email protected]> writes:
>>
>> > On Mon, Mar 11, 2019 at 04:11:06PM +0000, M?ns Rullg?rd wrote:
>> >> Maxime Ripard <[email protected]> writes:
>> >>
>> >> > Hi!
>> >> >
>> >> > On Mon, Mar 11, 2019 at 01:47:13PM +0000, Mans Rullgard wrote:
>> >> >> Sometimes it is desirabled to use a separate i2c controller for ddc
>> >> >> access. This adds support for the ddc-i2c-bus property of the
>> >> >> hdmi-connector node, using the specified controller if provided.
>> >> >>
>> >> >> Signed-off-by: Mans Rullgard <[email protected]>
>> >> >> ---
>> >> >> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 1 +
>> >> >> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 37 +++++++++++++++++++++++---
>> >> >> 2 files changed, 35 insertions(+), 3 deletions(-)
>> >> >>
>> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> >> >> index b685ee11623d..b08c4453d47c 100644
>> >> >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> >> >> @@ -269,6 +269,7 @@ struct sun4i_hdmi {
>> >> >> struct clk *tmds_clk;
>> >> >>
>> >> >> struct i2c_adapter *i2c;
>> >> >> + struct i2c_adapter *ddc_i2c;
>> >> >>
>> >> >> /* Regmap fields for I2C adapter */
>> >> >> struct regmap_field *field_ddc_en;
>> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> >> >> index 061d2e0d9011..5b2fac79f5d6 100644
>> >> >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> >> >> @@ -212,7 +212,7 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
>> >> >> struct edid *edid;
>> >> >> int ret;
>> >> >>
>> >> >> - edid = drm_get_edid(connector, hdmi->i2c);
>> >> >> + edid = drm_get_edid(connector, hdmi->ddc_i2c ?: hdmi->i2c);
>> >> >
>> >> > You can't test whether ddc_i2c is NULL or not...
>> >> >
>> >> >> if (!edid)
>> >> >> return 0;
>> >> >>
>> >> >> @@ -228,6 +228,28 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
>> >> >> return ret;
>> >> >> }
>> >> >>
>> >> >> +static struct i2c_adapter *sun4i_hdmi_get_ddc(struct device *dev)
>> >> >> +{
>> >> >> + struct device_node *phandle, *remote;
>> >> >> + struct i2c_adapter *ddc;
>> >> >> +
>> >> >> + remote = of_graph_get_remote_node(dev->of_node, 1, -1);
>> >> >> + if (!remote)
>> >> >> + return ERR_PTR(-EINVAL);
>> >> >> +
>> >> >> + phandle = of_parse_phandle(remote, "ddc-i2c-bus", 0);
>> >> >> + of_node_put(remote);
>> >> >> + if (!phandle)
>> >> >> + return NULL;
>> >> >> +
>> >> >> + ddc = of_get_i2c_adapter_by_node(phandle);
>> >> >> + of_node_put(phandle);
>> >> >> + if (!ddc)
>> >> >> + return ERR_PTR(-EPROBE_DEFER);
>> >> >> +
>> >> >> + return ddc;
>> >> >
>> >> > ... Since even in (most) error cases you're returning a !NULL pointer.
>> >> >
>> >> >> +}
>> >> >> +
>> >> >> static const struct drm_connector_helper_funcs sun4i_hdmi_connector_helper_funcs = {
>> >> >> .get_modes = sun4i_hdmi_get_modes,
>> >> >> };
>> >> >> @@ -575,6 +597,12 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
>> >> >> goto err_disable_mod_clk;
>> >> >> }
>> >> >>
>> >> >> + hdmi->ddc_i2c = sun4i_hdmi_get_ddc(dev);
>> >> >> + if (IS_ERR(hdmi->ddc_i2c)) {
>> >>
>> >> ... which is checked here.
>> >>
>> >> The property is optional, so the idea was to return null in that case
>> >> and use the built-in controller. If the property exists but some error
>> >> occurs, we want to abort rather than proceed with the fallback which
>> >> almost certainly won't work.
>> >>
>> >> Maybe I got something wrong in that logic.
>> >
>> > Indeed, I just got confused. I guess returning ENODEV in such a case,
>> > and testing for that, would make things more obvious.
>>
>> There's also a case I hadn't thought of: property exists but isn't a
>> valid phandle. What do you think is the correct action in that case?
>
> I think we would have that one covered. of_parse_phandle will return
> !NULL, but then of_get_i2c_adapter_by_node will return NULL since we
> wouldn't have an associated i2c adapter to the bogus phandle, and you
> are checking for that already.

of_parse_phandle() doesn't differentiate between a missing property and
an existing non-phandle value. The following cases are possible with
this patch:

- ddc-i2c-bus points to valid i2c controller node: use this for ddc
- no ddc-i2c-bus property: return NULL, use internal i2c
- ddc-i2c-bus exists but isn't a phandle: likewise
- ddc-i2c-bus points to a non-i2c-controller node: EPROBE_DEFER

The last two cases obviously mean the devicetree is invalid, so perhaps
it doesn't matter much what happens then. I don't think it's possible
to distinguish between a well-formed phandle pointing to some bogus node
and a good one where the i2c driver hasn't been probed yet.

--
M?ns Rullg?rd

2019-03-19 12:35:59

by Maxime Ripard

Subject: Re: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property

On Mon, Mar 18, 2019 at 04:23:56PM +0000, M?ns Rullg?rd wrote:
> Maxime Ripard <[email protected]> writes:
>
> > On Thu, Mar 14, 2019 at 04:09:13PM +0000, M?ns Rullg?rd wrote:
> >> Maxime Ripard <[email protected]> writes:
> >>
> >> > On Mon, Mar 11, 2019 at 04:11:06PM +0000, M?ns Rullg?rd wrote:
> >> >> Maxime Ripard <[email protected]> writes:
> >> >>
> >> >> > Hi!
> >> >> >
> >> >> > On Mon, Mar 11, 2019 at 01:47:13PM +0000, Mans Rullgard wrote:
> >> >> >> Sometimes it is desirabled to use a separate i2c controller for ddc
> >> >> >> access. This adds support for the ddc-i2c-bus property of the
> >> >> >> hdmi-connector node, using the specified controller if provided.
> >> >> >>
> >> >> >> Signed-off-by: Mans Rullgard <[email protected]>
> >> >> >> ---
> >> >> >> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 1 +
> >> >> >> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 37 +++++++++++++++++++++++---
> >> >> >> 2 files changed, 35 insertions(+), 3 deletions(-)
> >> >> >>
> >> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> >> >> >> index b685ee11623d..b08c4453d47c 100644
> >> >> >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> >> >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> >> >> >> @@ -269,6 +269,7 @@ struct sun4i_hdmi {
> >> >> >> struct clk *tmds_clk;
> >> >> >>
> >> >> >> struct i2c_adapter *i2c;
> >> >> >> + struct i2c_adapter *ddc_i2c;
> >> >> >>
> >> >> >> /* Regmap fields for I2C adapter */
> >> >> >> struct regmap_field *field_ddc_en;
> >> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> >> >> >> index 061d2e0d9011..5b2fac79f5d6 100644
> >> >> >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> >> >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> >> >> >> @@ -212,7 +212,7 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
> >> >> >> struct edid *edid;
> >> >> >> int ret;
> >> >> >>
> >> >> >> - edid = drm_get_edid(connector, hdmi->i2c);
> >> >> >> + edid = drm_get_edid(connector, hdmi->ddc_i2c ?: hdmi->i2c);
> >> >> >
> >> >> > You can't test whether ddc_i2c is NULL or not...
> >> >> >
> >> >> >> if (!edid)
> >> >> >> return 0;
> >> >> >>
> >> >> >> @@ -228,6 +228,28 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
> >> >> >> return ret;
> >> >> >> }
> >> >> >>
> >> >> >> +static struct i2c_adapter *sun4i_hdmi_get_ddc(struct device *dev)
> >> >> >> +{
> >> >> >> + struct device_node *phandle, *remote;
> >> >> >> + struct i2c_adapter *ddc;
> >> >> >> +
> >> >> >> + remote = of_graph_get_remote_node(dev->of_node, 1, -1);
> >> >> >> + if (!remote)
> >> >> >> + return ERR_PTR(-EINVAL);
> >> >> >> +
> >> >> >> + phandle = of_parse_phandle(remote, "ddc-i2c-bus", 0);
> >> >> >> + of_node_put(remote);
> >> >> >> + if (!phandle)
> >> >> >> + return NULL;
> >> >> >> +
> >> >> >> + ddc = of_get_i2c_adapter_by_node(phandle);
> >> >> >> + of_node_put(phandle);
> >> >> >> + if (!ddc)
> >> >> >> + return ERR_PTR(-EPROBE_DEFER);
> >> >> >> +
> >> >> >> + return ddc;
> >> >> >
> >> >> > ... Since even in (most) error cases you're returning a !NULL pointer.
> >> >> >
> >> >> >> +}
> >> >> >> +
> >> >> >> static const struct drm_connector_helper_funcs sun4i_hdmi_connector_helper_funcs = {
> >> >> >> .get_modes = sun4i_hdmi_get_modes,
> >> >> >> };
> >> >> >> @@ -575,6 +597,12 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
> >> >> >> goto err_disable_mod_clk;
> >> >> >> }
> >> >> >>
> >> >> >> + hdmi->ddc_i2c = sun4i_hdmi_get_ddc(dev);
> >> >> >> + if (IS_ERR(hdmi->ddc_i2c)) {
> >> >>
> >> >> ... which is checked here.
> >> >>
> >> >> The property is optional, so the idea was to return null in that case
> >> >> and use the built-in controller. If the property exists but some error
> >> >> occurs, we want to abort rather than proceed with the fallback which
> >> >> almost certainly won't work.
> >> >>
> >> >> Maybe I got something wrong in that logic.
> >> >
> >> > Indeed, I just got confused. I guess returning ENODEV in such a case,
> >> > and testing for that, would make things more obvious.
> >>
> >> There's also a case I hadn't thought of: property exists but isn't a
> >> valid phandle. What do you think is the correct action in that case?
> >
> > I think we would have that one covered. of_parse_phandle will return
> > !NULL, but then of_get_i2c_adapter_by_node will return NULL since we
> > wouldn't have an associated i2c adapter to the bogus phandle, and you
> > are checking for that already.
>
> of_parse_phandle() doesn't differentiate between a missing property and
> an existing non-phandle value. The following cases are possible with
> this patch:
>
> - ddc-i2c-bus points to valid i2c controller node: use this for ddc
> - no ddc-i2c-bus property: return NULL, use internal i2c
> - ddc-i2c-bus exists but isn't a phandle: likewise
> - ddc-i2c-bus points to a non-i2c-controller node: EPROBE_DEFER
>
> The last two cases obviously mean the devicetree is invalid, so perhaps
> it doesn't matter much what happens then. I don't think it's possible
> to distinguish between a well-formed phandle pointing to some bogus node
> and a good one where the i2c driver hasn't been probed yet.

Ah, I see what you mean now. Yeah, there's not much we can do against
a wrong / corrupted DT. The DT validation would help prevent the third
case, and possibly the fourth, but that's basically the only thing we
can do.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Attachments:

2019-03-19 12:49:39

by Måns Rullgård

Subject: Re: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property

Maxime Ripard <[email protected]> writes:

> On Mon, Mar 18, 2019 at 04:23:56PM +0000, M?ns Rullg?rd wrote:
>> Maxime Ripard <[email protected]> writes:
>>
>> > On Thu, Mar 14, 2019 at 04:09:13PM +0000, M?ns Rullg?rd wrote:
>> >> Maxime Ripard <[email protected]> writes:
>> >>
>> >> > On Mon, Mar 11, 2019 at 04:11:06PM +0000, M?ns Rullg?rd wrote:
>> >> >> Maxime Ripard <[email protected]> writes:
>> >> >>
>> >> >> > Hi!
>> >> >> >
>> >> >> > On Mon, Mar 11, 2019 at 01:47:13PM +0000, Mans Rullgard wrote:
>> >> >> >> Sometimes it is desirabled to use a separate i2c controller for ddc
>> >> >> >> access. This adds support for the ddc-i2c-bus property of the
>> >> >> >> hdmi-connector node, using the specified controller if provided.
>> >> >> >>
>> >> >> >> Signed-off-by: Mans Rullgard <[email protected]>
>> >> >> >> ---
>> >> >> >> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 1 +
>> >> >> >> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 37 +++++++++++++++++++++++---
>> >> >> >> 2 files changed, 35 insertions(+), 3 deletions(-)
>> >> >> >>
>> >> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> >> >> >> index b685ee11623d..b08c4453d47c 100644
>> >> >> >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> >> >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> >> >> >> @@ -269,6 +269,7 @@ struct sun4i_hdmi {
>> >> >> >> struct clk *tmds_clk;
>> >> >> >>
>> >> >> >> struct i2c_adapter *i2c;
>> >> >> >> + struct i2c_adapter *ddc_i2c;
>> >> >> >>
>> >> >> >> /* Regmap fields for I2C adapter */
>> >> >> >> struct regmap_field *field_ddc_en;
>> >> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> >> >> >> index 061d2e0d9011..5b2fac79f5d6 100644
>> >> >> >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> >> >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> >> >> >> @@ -212,7 +212,7 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
>> >> >> >> struct edid *edid;
>> >> >> >> int ret;
>> >> >> >>
>> >> >> >> - edid = drm_get_edid(connector, hdmi->i2c);
>> >> >> >> + edid = drm_get_edid(connector, hdmi->ddc_i2c ?: hdmi->i2c);
>> >> >> >
>> >> >> > You can't test whether ddc_i2c is NULL or not...
>> >> >> >
>> >> >> >> if (!edid)
>> >> >> >> return 0;
>> >> >> >>
>> >> >> >> @@ -228,6 +228,28 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
>> >> >> >> return ret;
>> >> >> >> }
>> >> >> >>
>> >> >> >> +static struct i2c_adapter *sun4i_hdmi_get_ddc(struct device *dev)
>> >> >> >> +{
>> >> >> >> + struct device_node *phandle, *remote;
>> >> >> >> + struct i2c_adapter *ddc;
>> >> >> >> +
>> >> >> >> + remote = of_graph_get_remote_node(dev->of_node, 1, -1);
>> >> >> >> + if (!remote)
>> >> >> >> + return ERR_PTR(-EINVAL);
>> >> >> >> +
>> >> >> >> + phandle = of_parse_phandle(remote, "ddc-i2c-bus", 0);
>> >> >> >> + of_node_put(remote);
>> >> >> >> + if (!phandle)
>> >> >> >> + return NULL;
>> >> >> >> +
>> >> >> >> + ddc = of_get_i2c_adapter_by_node(phandle);
>> >> >> >> + of_node_put(phandle);
>> >> >> >> + if (!ddc)
>> >> >> >> + return ERR_PTR(-EPROBE_DEFER);
>> >> >> >> +
>> >> >> >> + return ddc;
>> >> >> >
>> >> >> > ... Since even in (most) error cases you're returning a !NULL pointer.
>> >> >> >
>> >> >> >> +}
>> >> >> >> +
>> >> >> >> static const struct drm_connector_helper_funcs sun4i_hdmi_connector_helper_funcs = {
>> >> >> >> .get_modes = sun4i_hdmi_get_modes,
>> >> >> >> };
>> >> >> >> @@ -575,6 +597,12 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
>> >> >> >> goto err_disable_mod_clk;
>> >> >> >> }
>> >> >> >>
>> >> >> >> + hdmi->ddc_i2c = sun4i_hdmi_get_ddc(dev);
>> >> >> >> + if (IS_ERR(hdmi->ddc_i2c)) {
>> >> >>
>> >> >> ... which is checked here.
>> >> >>
>> >> >> The property is optional, so the idea was to return null in that case
>> >> >> and use the built-in controller. If the property exists but some error
>> >> >> occurs, we want to abort rather than proceed with the fallback which
>> >> >> almost certainly won't work.
>> >> >>
>> >> >> Maybe I got something wrong in that logic.
>> >> >
>> >> > Indeed, I just got confused. I guess returning ENODEV in such a case,
>> >> > and testing for that, would make things more obvious.
>> >>
>> >> There's also a case I hadn't thought of: property exists but isn't a
>> >> valid phandle. What do you think is the correct action in that case?
>> >
>> > I think we would have that one covered. of_parse_phandle will return
>> > !NULL, but then of_get_i2c_adapter_by_node will return NULL since we
>> > wouldn't have an associated i2c adapter to the bogus phandle, and you
>> > are checking for that already.
>>
>> of_parse_phandle() doesn't differentiate between a missing property and
>> an existing non-phandle value. The following cases are possible with
>> this patch:
>>
>> - ddc-i2c-bus points to valid i2c controller node: use this for ddc
>> - no ddc-i2c-bus property: return NULL, use internal i2c
>> - ddc-i2c-bus exists but isn't a phandle: likewise
>> - ddc-i2c-bus points to a non-i2c-controller node: EPROBE_DEFER
>>
>> The last two cases obviously mean the devicetree is invalid, so perhaps
>> it doesn't matter much what happens then. I don't think it's possible
>> to distinguish between a well-formed phandle pointing to some bogus node
>> and a good one where the i2c driver hasn't been probed yet.
>
> Ah, I see what you mean now. Yeah, there's not much we can do against
> a wrong / corrupted DT. The DT validation would help prevent the third
> case, and possibly the fourth, but that's basically the only thing we
> can do.

We need to return -EPROBE_DEFER in the case that everything is fine but
the i2c driver hasn't been probed yet. From here, that is
indistinguishable from of_parse_phandle() returning a completely bogus
node.

If the ddc-i2c-bus property doesn't contain a phandle at all, we could
either return an error or fall back to the internal i2c. The patch does
the latter because that's less code. I don't think that's any worse
than aborting entirely in terms of user experience.

The choice is up to you. I'm happy to update the patch, but you'll have
to tell me what behaviour you prefer.

--
M?ns Rullg?rd

2019-03-21 15:45:55

by Maxime Ripard

Subject: Re: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property

On Tue, Mar 19, 2019 at 12:48:19PM +0000, M?ns Rullg?rd wrote:
> Maxime Ripard <[email protected]> writes:
>
> > On Mon, Mar 18, 2019 at 04:23:56PM +0000, M?ns Rullg?rd wrote:
> >> Maxime Ripard <[email protected]> writes:
> >>
> >> > On Thu, Mar 14, 2019 at 04:09:13PM +0000, M?ns Rullg?rd wrote:
> >> >> Maxime Ripard <[email protected]> writes:
> >> >>
> >> >> > On Mon, Mar 11, 2019 at 04:11:06PM +0000, M?ns Rullg?rd wrote:
> >> >> >> Maxime Ripard <[email protected]> writes:
> >> >> >>
> >> >> >> > Hi!
> >> >> >> >
> >> >> >> > On Mon, Mar 11, 2019 at 01:47:13PM +0000, Mans Rullgard wrote:
> >> >> >> >> Sometimes it is desirabled to use a separate i2c controller for ddc
> >> >> >> >> access. This adds support for the ddc-i2c-bus property of the
> >> >> >> >> hdmi-connector node, using the specified controller if provided.
> >> >> >> >>
> >> >> >> >> Signed-off-by: Mans Rullgard <[email protected]>
> >> >> >> >> ---
> >> >> >> >> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 1 +
> >> >> >> >> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 37 +++++++++++++++++++++++---
> >> >> >> >> 2 files changed, 35 insertions(+), 3 deletions(-)
> >> >> >> >>
> >> >> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> >> >> >> >> index b685ee11623d..b08c4453d47c 100644
> >> >> >> >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> >> >> >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> >> >> >> >> @@ -269,6 +269,7 @@ struct sun4i_hdmi {
> >> >> >> >> struct clk *tmds_clk;
> >> >> >> >>
> >> >> >> >> struct i2c_adapter *i2c;
> >> >> >> >> + struct i2c_adapter *ddc_i2c;
> >> >> >> >>
> >> >> >> >> /* Regmap fields for I2C adapter */
> >> >> >> >> struct regmap_field *field_ddc_en;
> >> >> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> >> >> >> >> index 061d2e0d9011..5b2fac79f5d6 100644
> >> >> >> >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> >> >> >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> >> >> >> >> @@ -212,7 +212,7 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
> >> >> >> >> struct edid *edid;
> >> >> >> >> int ret;
> >> >> >> >>
> >> >> >> >> - edid = drm_get_edid(connector, hdmi->i2c);
> >> >> >> >> + edid = drm_get_edid(connector, hdmi->ddc_i2c ?: hdmi->i2c);
> >> >> >> >
> >> >> >> > You can't test whether ddc_i2c is NULL or not...
> >> >> >> >
> >> >> >> >> if (!edid)
> >> >> >> >> return 0;
> >> >> >> >>
> >> >> >> >> @@ -228,6 +228,28 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
> >> >> >> >> return ret;
> >> >> >> >> }
> >> >> >> >>
> >> >> >> >> +static struct i2c_adapter *sun4i_hdmi_get_ddc(struct device *dev)
> >> >> >> >> +{
> >> >> >> >> + struct device_node *phandle, *remote;
> >> >> >> >> + struct i2c_adapter *ddc;
> >> >> >> >> +
> >> >> >> >> + remote = of_graph_get_remote_node(dev->of_node, 1, -1);
> >> >> >> >> + if (!remote)
> >> >> >> >> + return ERR_PTR(-EINVAL);
> >> >> >> >> +
> >> >> >> >> + phandle = of_parse_phandle(remote, "ddc-i2c-bus", 0);
> >> >> >> >> + of_node_put(remote);
> >> >> >> >> + if (!phandle)
> >> >> >> >> + return NULL;
> >> >> >> >> +
> >> >> >> >> + ddc = of_get_i2c_adapter_by_node(phandle);
> >> >> >> >> + of_node_put(phandle);
> >> >> >> >> + if (!ddc)
> >> >> >> >> + return ERR_PTR(-EPROBE_DEFER);
> >> >> >> >> +
> >> >> >> >> + return ddc;
> >> >> >> >
> >> >> >> > ... Since even in (most) error cases you're returning a !NULL pointer.
> >> >> >> >
> >> >> >> >> +}
> >> >> >> >> +
> >> >> >> >> static const struct drm_connector_helper_funcs sun4i_hdmi_connector_helper_funcs = {
> >> >> >> >> .get_modes = sun4i_hdmi_get_modes,
> >> >> >> >> };
> >> >> >> >> @@ -575,6 +597,12 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
> >> >> >> >> goto err_disable_mod_clk;
> >> >> >> >> }
> >> >> >> >>
> >> >> >> >> + hdmi->ddc_i2c = sun4i_hdmi_get_ddc(dev);
> >> >> >> >> + if (IS_ERR(hdmi->ddc_i2c)) {
> >> >> >>
> >> >> >> ... which is checked here.
> >> >> >>
> >> >> >> The property is optional, so the idea was to return null in that case
> >> >> >> and use the built-in controller. If the property exists but some error
> >> >> >> occurs, we want to abort rather than proceed with the fallback which
> >> >> >> almost certainly won't work.
> >> >> >>
> >> >> >> Maybe I got something wrong in that logic.
> >> >> >
> >> >> > Indeed, I just got confused. I guess returning ENODEV in such a case,
> >> >> > and testing for that, would make things more obvious.
> >> >>
> >> >> There's also a case I hadn't thought of: property exists but isn't a
> >> >> valid phandle. What do you think is the correct action in that case?
> >> >
> >> > I think we would have that one covered. of_parse_phandle will return
> >> > !NULL, but then of_get_i2c_adapter_by_node will return NULL since we
> >> > wouldn't have an associated i2c adapter to the bogus phandle, and you
> >> > are checking for that already.
> >>
> >> of_parse_phandle() doesn't differentiate between a missing property and
> >> an existing non-phandle value. The following cases are possible with
> >> this patch:
> >>
> >> - ddc-i2c-bus points to valid i2c controller node: use this for ddc
> >> - no ddc-i2c-bus property: return NULL, use internal i2c
> >> - ddc-i2c-bus exists but isn't a phandle: likewise
> >> - ddc-i2c-bus points to a non-i2c-controller node: EPROBE_DEFER
> >>
> >> The last two cases obviously mean the devicetree is invalid, so perhaps
> >> it doesn't matter much what happens then. I don't think it's possible
> >> to distinguish between a well-formed phandle pointing to some bogus node
> >> and a good one where the i2c driver hasn't been probed yet.
> >
> > Ah, I see what you mean now. Yeah, there's not much we can do against
> > a wrong / corrupted DT. The DT validation would help prevent the third
> > case, and possibly the fourth, but that's basically the only thing we
> > can do.
>
> We need to return -EPROBE_DEFER in the case that everything is fine but
> the i2c driver hasn't been probed yet. From here, that is
> indistinguishable from of_parse_phandle() returning a completely bogus
> node.

That's unfortunate, but if we start to not trust the DT content, we
have far worse to deal with.

> If the ddc-i2c-bus property doesn't contain a phandle at all, we could
> either return an error or fall back to the internal i2c. The patch does
> the latter because that's less code. I don't think that's any worse
> than aborting entirely in terms of user experience.

I'm totally fine with the latter behaviour as well. And like I said,
the DT validation can help us prevent that case from happening
entirely at compilation time.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Attachments:

2019-03-21 18:02:20

by Måns Rullgård

Subject: Re: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property

Maxime Ripard <[email protected]> writes:

> On Tue, Mar 19, 2019 at 12:48:19PM +0000, M?ns Rullg?rd wrote:
>> Maxime Ripard <[email protected]> writes:
>>
>> > On Mon, Mar 18, 2019 at 04:23:56PM +0000, M?ns Rullg?rd wrote:
>> >> Maxime Ripard <[email protected]> writes:
>> >>
>> >> > On Thu, Mar 14, 2019 at 04:09:13PM +0000, M?ns Rullg?rd wrote:
>> >> >> Maxime Ripard <[email protected]> writes:
>> >> >>
>> >> >> > On Mon, Mar 11, 2019 at 04:11:06PM +0000, M?ns Rullg?rd wrote:
>> >> >> >> Maxime Ripard <[email protected]> writes:
>> >> >> >>
>> >> >> >> > Hi!
>> >> >> >> >
>> >> >> >> > On Mon, Mar 11, 2019 at 01:47:13PM +0000, Mans Rullgard wrote:
>> >> >> >> >> Sometimes it is desirabled to use a separate i2c controller for ddc
>> >> >> >> >> access. This adds support for the ddc-i2c-bus property of the
>> >> >> >> >> hdmi-connector node, using the specified controller if provided.
>> >> >> >> >>
>> >> >> >> >> Signed-off-by: Mans Rullgard <[email protected]>
>> >> >> >> >> ---
>> >> >> >> >> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 1 +
>> >> >> >> >> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 37 +++++++++++++++++++++++---
>> >> >> >> >> 2 files changed, 35 insertions(+), 3 deletions(-)
>> >> >> >> >>
>> >> >> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> >> >> >> >> index b685ee11623d..b08c4453d47c 100644
>> >> >> >> >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> >> >> >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> >> >> >> >> @@ -269,6 +269,7 @@ struct sun4i_hdmi {
>> >> >> >> >> struct clk *tmds_clk;
>> >> >> >> >>
>> >> >> >> >> struct i2c_adapter *i2c;
>> >> >> >> >> + struct i2c_adapter *ddc_i2c;
>> >> >> >> >>
>> >> >> >> >> /* Regmap fields for I2C adapter */
>> >> >> >> >> struct regmap_field *field_ddc_en;
>> >> >> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> >> >> >> >> index 061d2e0d9011..5b2fac79f5d6 100644
>> >> >> >> >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> >> >> >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> >> >> >> >> @@ -212,7 +212,7 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
>> >> >> >> >> struct edid *edid;
>> >> >> >> >> int ret;
>> >> >> >> >>
>> >> >> >> >> - edid = drm_get_edid(connector, hdmi->i2c);
>> >> >> >> >> + edid = drm_get_edid(connector, hdmi->ddc_i2c ?: hdmi->i2c);
>> >> >> >> >
>> >> >> >> > You can't test whether ddc_i2c is NULL or not...
>> >> >> >> >
>> >> >> >> >> if (!edid)
>> >> >> >> >> return 0;
>> >> >> >> >>
>> >> >> >> >> @@ -228,6 +228,28 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
>> >> >> >> >> return ret;
>> >> >> >> >> }
>> >> >> >> >>
>> >> >> >> >> +static struct i2c_adapter *sun4i_hdmi_get_ddc(struct device *dev)
>> >> >> >> >> +{
>> >> >> >> >> + struct device_node *phandle, *remote;
>> >> >> >> >> + struct i2c_adapter *ddc;
>> >> >> >> >> +
>> >> >> >> >> + remote = of_graph_get_remote_node(dev->of_node, 1, -1);
>> >> >> >> >> + if (!remote)
>> >> >> >> >> + return ERR_PTR(-EINVAL);
>> >> >> >> >> +
>> >> >> >> >> + phandle = of_parse_phandle(remote, "ddc-i2c-bus", 0);
>> >> >> >> >> + of_node_put(remote);
>> >> >> >> >> + if (!phandle)
>> >> >> >> >> + return NULL;
>> >> >> >> >> +
>> >> >> >> >> + ddc = of_get_i2c_adapter_by_node(phandle);
>> >> >> >> >> + of_node_put(phandle);
>> >> >> >> >> + if (!ddc)
>> >> >> >> >> + return ERR_PTR(-EPROBE_DEFER);
>> >> >> >> >> +
>> >> >> >> >> + return ddc;
>> >> >> >> >
>> >> >> >> > ... Since even in (most) error cases you're returning a !NULL pointer.
>> >> >> >> >
>> >> >> >> >> +}
>> >> >> >> >> +
>> >> >> >> >> static const struct drm_connector_helper_funcs sun4i_hdmi_connector_helper_funcs = {
>> >> >> >> >> .get_modes = sun4i_hdmi_get_modes,
>> >> >> >> >> };
>> >> >> >> >> @@ -575,6 +597,12 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
>> >> >> >> >> goto err_disable_mod_clk;
>> >> >> >> >> }
>> >> >> >> >>
>> >> >> >> >> + hdmi->ddc_i2c = sun4i_hdmi_get_ddc(dev);
>> >> >> >> >> + if (IS_ERR(hdmi->ddc_i2c)) {
>> >> >> >>
>> >> >> >> ... which is checked here.
>> >> >> >>
>> >> >> >> The property is optional, so the idea was to return null in that case
>> >> >> >> and use the built-in controller. If the property exists but some error
>> >> >> >> occurs, we want to abort rather than proceed with the fallback which
>> >> >> >> almost certainly won't work.
>> >> >> >>
>> >> >> >> Maybe I got something wrong in that logic.
>> >> >> >
>> >> >> > Indeed, I just got confused. I guess returning ENODEV in such a case,
>> >> >> > and testing for that, would make things more obvious.
>> >> >>
>> >> >> There's also a case I hadn't thought of: property exists but isn't a
>> >> >> valid phandle. What do you think is the correct action in that case?
>> >> >
>> >> > I think we would have that one covered. of_parse_phandle will return
>> >> > !NULL, but then of_get_i2c_adapter_by_node will return NULL since we
>> >> > wouldn't have an associated i2c adapter to the bogus phandle, and you
>> >> > are checking for that already.
>> >>
>> >> of_parse_phandle() doesn't differentiate between a missing property and
>> >> an existing non-phandle value. The following cases are possible with
>> >> this patch:
>> >>
>> >> - ddc-i2c-bus points to valid i2c controller node: use this for ddc
>> >> - no ddc-i2c-bus property: return NULL, use internal i2c
>> >> - ddc-i2c-bus exists but isn't a phandle: likewise
>> >> - ddc-i2c-bus points to a non-i2c-controller node: EPROBE_DEFER
>> >>
>> >> The last two cases obviously mean the devicetree is invalid, so perhaps
>> >> it doesn't matter much what happens then. I don't think it's possible
>> >> to distinguish between a well-formed phandle pointing to some bogus node
>> >> and a good one where the i2c driver hasn't been probed yet.
>> >
>> > Ah, I see what you mean now. Yeah, there's not much we can do against
>> > a wrong / corrupted DT. The DT validation would help prevent the third
>> > case, and possibly the fourth, but that's basically the only thing we
>> > can do.
>>
>> We need to return -EPROBE_DEFER in the case that everything is fine but
>> the i2c driver hasn't been probed yet. From here, that is
>> indistinguishable from of_parse_phandle() returning a completely bogus
>> node.
>
> That's unfortunate, but if we start to not trust the DT content, we
> have far worse to deal with.
>
>> If the ddc-i2c-bus property doesn't contain a phandle at all, we could
>> either return an error or fall back to the internal i2c. The patch does
>> the latter because that's less code. I don't think that's any worse
>> than aborting entirely in terms of user experience.
>
> I'm totally fine with the latter behaviour as well. And like I said,
> the DT validation can help us prevent that case from happening
> entirely at compilation time.

Well, do you want any changes to the patch or not?

--
M?ns Rullg?rd

2019-03-26 19:50:11

by Maxime Ripard

Subject: Re: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property

On Thu, Mar 21, 2019 at 06:00:45PM +0000, M?ns Rullg?rd wrote:
> Maxime Ripard <[email protected]> writes:
>
> > On Tue, Mar 19, 2019 at 12:48:19PM +0000, M?ns Rullg?rd wrote:
> >> Maxime Ripard <[email protected]> writes:
> >>
> >> > On Mon, Mar 18, 2019 at 04:23:56PM +0000, M?ns Rullg?rd wrote:
> >> >> Maxime Ripard <[email protected]> writes:
> >> >>
> >> >> > On Thu, Mar 14, 2019 at 04:09:13PM +0000, M?ns Rullg?rd wrote:
> >> >> >> Maxime Ripard <[email protected]> writes:
> >> >> >>
> >> >> >> > On Mon, Mar 11, 2019 at 04:11:06PM +0000, M?ns Rullg?rd wrote:
> >> >> >> >> Maxime Ripard <[email protected]> writes:
> >> >> >> >>
> >> >> >> >> > Hi!
> >> >> >> >> >
> >> >> >> >> > On Mon, Mar 11, 2019 at 01:47:13PM +0000, Mans Rullgard wrote:
> >> >> >> >> >> Sometimes it is desirabled to use a separate i2c controller for ddc
> >> >> >> >> >> access. This adds support for the ddc-i2c-bus property of the
> >> >> >> >> >> hdmi-connector node, using the specified controller if provided.
> >> >> >> >> >>
> >> >> >> >> >> Signed-off-by: Mans Rullgard <[email protected]>
> >> >> >> >> >> ---
> >> >> >> >> >> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 1 +
> >> >> >> >> >> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 37 +++++++++++++++++++++++---
> >> >> >> >> >> 2 files changed, 35 insertions(+), 3 deletions(-)
> >> >> >> >> >>
> >> >> >> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> >> >> >> >> >> index b685ee11623d..b08c4453d47c 100644
> >> >> >> >> >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> >> >> >> >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> >> >> >> >> >> @@ -269,6 +269,7 @@ struct sun4i_hdmi {
> >> >> >> >> >> struct clk *tmds_clk;
> >> >> >> >> >>
> >> >> >> >> >> struct i2c_adapter *i2c;
> >> >> >> >> >> + struct i2c_adapter *ddc_i2c;
> >> >> >> >> >>
> >> >> >> >> >> /* Regmap fields for I2C adapter */
> >> >> >> >> >> struct regmap_field *field_ddc_en;
> >> >> >> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> >> >> >> >> >> index 061d2e0d9011..5b2fac79f5d6 100644
> >> >> >> >> >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> >> >> >> >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> >> >> >> >> >> @@ -212,7 +212,7 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
> >> >> >> >> >> struct edid *edid;
> >> >> >> >> >> int ret;
> >> >> >> >> >>
> >> >> >> >> >> - edid = drm_get_edid(connector, hdmi->i2c);
> >> >> >> >> >> + edid = drm_get_edid(connector, hdmi->ddc_i2c ?: hdmi->i2c);
> >> >> >> >> >
> >> >> >> >> > You can't test whether ddc_i2c is NULL or not...
> >> >> >> >> >
> >> >> >> >> >> if (!edid)
> >> >> >> >> >> return 0;
> >> >> >> >> >>
> >> >> >> >> >> @@ -228,6 +228,28 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
> >> >> >> >> >> return ret;
> >> >> >> >> >> }
> >> >> >> >> >>
> >> >> >> >> >> +static struct i2c_adapter *sun4i_hdmi_get_ddc(struct device *dev)
> >> >> >> >> >> +{
> >> >> >> >> >> + struct device_node *phandle, *remote;
> >> >> >> >> >> + struct i2c_adapter *ddc;
> >> >> >> >> >> +
> >> >> >> >> >> + remote = of_graph_get_remote_node(dev->of_node, 1, -1);
> >> >> >> >> >> + if (!remote)
> >> >> >> >> >> + return ERR_PTR(-EINVAL);
> >> >> >> >> >> +
> >> >> >> >> >> + phandle = of_parse_phandle(remote, "ddc-i2c-bus", 0);
> >> >> >> >> >> + of_node_put(remote);
> >> >> >> >> >> + if (!phandle)
> >> >> >> >> >> + return NULL;
> >> >> >> >> >> +
> >> >> >> >> >> + ddc = of_get_i2c_adapter_by_node(phandle);
> >> >> >> >> >> + of_node_put(phandle);
> >> >> >> >> >> + if (!ddc)
> >> >> >> >> >> + return ERR_PTR(-EPROBE_DEFER);
> >> >> >> >> >> +
> >> >> >> >> >> + return ddc;
> >> >> >> >> >
> >> >> >> >> > ... Since even in (most) error cases you're returning a !NULL pointer.
> >> >> >> >> >
> >> >> >> >> >> +}
> >> >> >> >> >> +
> >> >> >> >> >> static const struct drm_connector_helper_funcs sun4i_hdmi_connector_helper_funcs = {
> >> >> >> >> >> .get_modes = sun4i_hdmi_get_modes,
> >> >> >> >> >> };
> >> >> >> >> >> @@ -575,6 +597,12 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
> >> >> >> >> >> goto err_disable_mod_clk;
> >> >> >> >> >> }
> >> >> >> >> >>
> >> >> >> >> >> + hdmi->ddc_i2c = sun4i_hdmi_get_ddc(dev);
> >> >> >> >> >> + if (IS_ERR(hdmi->ddc_i2c)) {
> >> >> >> >>
> >> >> >> >> ... which is checked here.
> >> >> >> >>
> >> >> >> >> The property is optional, so the idea was to return null in that case
> >> >> >> >> and use the built-in controller. If the property exists but some error
> >> >> >> >> occurs, we want to abort rather than proceed with the fallback which
> >> >> >> >> almost certainly won't work.
> >> >> >> >>
> >> >> >> >> Maybe I got something wrong in that logic.
> >> >> >> >
> >> >> >> > Indeed, I just got confused. I guess returning ENODEV in such a case,
> >> >> >> > and testing for that, would make things more obvious.
> >> >> >>
> >> >> >> There's also a case I hadn't thought of: property exists but isn't a
> >> >> >> valid phandle. What do you think is the correct action in that case?
> >> >> >
> >> >> > I think we would have that one covered. of_parse_phandle will return
> >> >> > !NULL, but then of_get_i2c_adapter_by_node will return NULL since we
> >> >> > wouldn't have an associated i2c adapter to the bogus phandle, and you
> >> >> > are checking for that already.
> >> >>
> >> >> of_parse_phandle() doesn't differentiate between a missing property and
> >> >> an existing non-phandle value. The following cases are possible with
> >> >> this patch:
> >> >>
> >> >> - ddc-i2c-bus points to valid i2c controller node: use this for ddc
> >> >> - no ddc-i2c-bus property: return NULL, use internal i2c
> >> >> - ddc-i2c-bus exists but isn't a phandle: likewise
> >> >> - ddc-i2c-bus points to a non-i2c-controller node: EPROBE_DEFER
> >> >>
> >> >> The last two cases obviously mean the devicetree is invalid, so perhaps
> >> >> it doesn't matter much what happens then. I don't think it's possible
> >> >> to distinguish between a well-formed phandle pointing to some bogus node
> >> >> and a good one where the i2c driver hasn't been probed yet.
> >> >
> >> > Ah, I see what you mean now. Yeah, there's not much we can do against
> >> > a wrong / corrupted DT. The DT validation would help prevent the third
> >> > case, and possibly the fourth, but that's basically the only thing we
> >> > can do.
> >>
> >> We need to return -EPROBE_DEFER in the case that everything is fine but
> >> the i2c driver hasn't been probed yet. From here, that is
> >> indistinguishable from of_parse_phandle() returning a completely bogus
> >> node.
> >
> > That's unfortunate, but if we start to not trust the DT content, we
> > have far worse to deal with.
> >
> >> If the ddc-i2c-bus property doesn't contain a phandle at all, we could
> >> either return an error or fall back to the internal i2c. The patch does
> >> the latter because that's less code. I don't think that's any worse
> >> than aborting entirely in terms of user experience.
> >
> > I'm totally fine with the latter behaviour as well. And like I said,
> > the DT validation can help us prevent that case from happening
> > entirely at compilation time.
>
> Well, do you want any changes to the patch or not?

Yeah, return an error pointer with ENODEV instead of NULL, and test
for that.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Attachments:

2019-03-28 13:03:59

by Måns Rullgård

Subject: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property

Sometimes it is desirabled to use a separate i2c controller for ddc
access. This adds support for the ddc-i2c-bus property of the
hdmi-connector node, using the specified controller if provided.

Signed-off-by: Mans Rullgard <[email protected]>
---
Changed in v2:
- Return ERR_PTR(-ENODEV) instead of NULL when property is absent
---
drivers/gpu/drm/sun4i/sun4i_hdmi.h | 1 +
drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 40 ++++++++++++++++++++++++--
2 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
index b685ee11623d..b08c4453d47c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
@@ -269,6 +269,7 @@ struct sun4i_hdmi {
struct clk *tmds_clk;

struct i2c_adapter *i2c;
+ struct i2c_adapter *ddc_i2c;

/* Regmap fields for I2C adapter */
struct regmap_field *field_ddc_en;
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
index 416da5376701..a99523e9651f 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
@@ -216,7 +216,7 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
struct edid *edid;
int ret;

- edid = drm_get_edid(connector, hdmi->i2c);
+ edid = drm_get_edid(connector, hdmi->ddc_i2c ?: hdmi->i2c);
if (!edid)
return 0;

@@ -232,6 +232,28 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
return ret;
}

+static struct i2c_adapter *sun4i_hdmi_get_ddc(struct device *dev)
+{
+ struct device_node *phandle, *remote;
+ struct i2c_adapter *ddc;
+
+ remote = of_graph_get_remote_node(dev->of_node, 1, -1);
+ if (!remote)
+ return ERR_PTR(-EINVAL);
+
+ phandle = of_parse_phandle(remote, "ddc-i2c-bus", 0);
+ of_node_put(remote);
+ if (!phandle)
+ return ERR_PTR(-ENODEV);
+
+ ddc = of_get_i2c_adapter_by_node(phandle);
+ of_node_put(phandle);
+ if (!ddc)
+ return ERR_PTR(-EPROBE_DEFER);
+
+ return ddc;
+}
+
static const struct drm_connector_helper_funcs sun4i_hdmi_connector_helper_funcs = {
.get_modes = sun4i_hdmi_get_modes,
};
@@ -579,6 +601,15 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
goto err_disable_mod_clk;
}

+ hdmi->ddc_i2c = sun4i_hdmi_get_ddc(dev);
+ if (IS_ERR(hdmi->ddc_i2c)) {
+ ret = PTR_ERR(hdmi->ddc_i2c);
+ if (ret == -ENODEV)
+ hdmi->ddc_i2c = NULL;
+ else
+ goto err_del_i2c_adapter;
+ }
+
drm_encoder_helper_add(&hdmi->encoder,
&sun4i_hdmi_helper_funcs);
ret = drm_encoder_init(drm,
@@ -588,14 +619,14 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
NULL);
if (ret) {
dev_err(dev, "Couldn't initialise the HDMI encoder\n");
- goto err_del_i2c_adapter;
+ goto err_put_ddc_i2c;
}

hdmi->encoder.possible_crtcs = drm_of_find_possible_crtcs(drm,
dev->of_node);
if (!hdmi->encoder.possible_crtcs) {
ret = -EPROBE_DEFER;
- goto err_del_i2c_adapter;
+ goto err_put_ddc_i2c;
}

#ifdef CONFIG_DRM_SUN4I_HDMI_CEC
@@ -634,6 +665,8 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master,
err_cleanup_connector:
cec_delete_adapter(hdmi->cec_adap);
drm_encoder_cleanup(&hdmi->encoder);
+err_put_ddc_i2c:
+ i2c_put_adapter(hdmi->ddc_i2c);
err_del_i2c_adapter:
i2c_del_adapter(hdmi->i2c);
err_disable_mod_clk:
@@ -654,6 +687,7 @@ static void sun4i_hdmi_unbind(struct device *dev, struct device *master,
drm_connector_cleanup(&hdmi->connector);
drm_encoder_cleanup(&hdmi->encoder);
i2c_del_adapter(hdmi->i2c);
+ i2c_put_adapter(hdmi->ddc_i2c);
clk_disable_unprepare(hdmi->mod_clk);
clk_disable_unprepare(hdmi->bus_clk);
}
--
2.21.0

2019-04-01 11:59:28

by Maxime Ripard

Subject: Re: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property

On Thu, Mar 28, 2019 at 01:02:49PM +0000, Mans Rullgard wrote:
> Sometimes it is desirabled to use a separate i2c controller for ddc
> access. This adds support for the ddc-i2c-bus property of the
> hdmi-connector node, using the specified controller if provided.
>
> Signed-off-by: Mans Rullgard <[email protected]>

Applied to drm-misc-next, thanks!
Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Attachments: