| Message ID | 20190311134713.25876-1-mans@mansr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/sun4i: hdmi: add support for ddc-i2c-bus property | expand |
Maxime Ripard <maxime.ripard@bootlin.com> 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 <mans@mansr.com> >> --- >> 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.
On Mon, Mar 11, 2019 at 04:11:06PM +0000, Måns Rullgård wrote: > Maxime Ripard <maxime.ripard@bootlin.com> 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 <mans@mansr.com> > >> --- > >> 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 <maxime.ripard@bootlin.com> writes: > On Mon, Mar 11, 2019 at 04:11:06PM +0000, Måns Rullgård wrote: >> Maxime Ripard <maxime.ripard@bootlin.com> 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 <mans@mansr.com> >> >> --- >> >> 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?
On Thu, Mar 14, 2019 at 04:09:13PM +0000, Måns Rullgård wrote: > Maxime Ripard <maxime.ripard@bootlin.com> writes: > > > On Mon, Mar 11, 2019 at 04:11:06PM +0000, Måns Rullgård wrote: > >> Maxime Ripard <maxime.ripard@bootlin.com> 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 <mans@mansr.com> > >> >> --- > >> >> 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
Maxime Ripard <maxime.ripard@bootlin.com> writes: > On Thu, Mar 14, 2019 at 04:09:13PM +0000, Måns Rullgård wrote: >> Maxime Ripard <maxime.ripard@bootlin.com> writes: >> >> > On Mon, Mar 11, 2019 at 04:11:06PM +0000, Måns Rullgård wrote: >> >> Maxime Ripard <maxime.ripard@bootlin.com> 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 <mans@mansr.com> >> >> >> --- >> >> >> 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.
On Mon, Mar 18, 2019 at 04:23:56PM +0000, Måns Rullgård wrote: > Maxime Ripard <maxime.ripard@bootlin.com> writes: > > > On Thu, Mar 14, 2019 at 04:09:13PM +0000, Måns Rullgård wrote: > >> Maxime Ripard <maxime.ripard@bootlin.com> writes: > >> > >> > On Mon, Mar 11, 2019 at 04:11:06PM +0000, Måns Rullgård wrote: > >> >> Maxime Ripard <maxime.ripard@bootlin.com> 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 <mans@mansr.com> > >> >> >> --- > >> >> >> 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
Maxime Ripard <maxime.ripard@bootlin.com> writes: > On Mon, Mar 18, 2019 at 04:23:56PM +0000, Måns Rullgård wrote: >> Maxime Ripard <maxime.ripard@bootlin.com> writes: >> >> > On Thu, Mar 14, 2019 at 04:09:13PM +0000, Måns Rullgård wrote: >> >> Maxime Ripard <maxime.ripard@bootlin.com> writes: >> >> >> >> > On Mon, Mar 11, 2019 at 04:11:06PM +0000, Måns Rullgård wrote: >> >> >> Maxime Ripard <maxime.ripard@bootlin.com> 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 <mans@mansr.com> >> >> >> >> --- >> >> >> >> 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.
On Tue, Mar 19, 2019 at 12:48:19PM +0000, Måns Rullgård wrote: > Maxime Ripard <maxime.ripard@bootlin.com> writes: > > > On Mon, Mar 18, 2019 at 04:23:56PM +0000, Måns Rullgård wrote: > >> Maxime Ripard <maxime.ripard@bootlin.com> writes: > >> > >> > On Thu, Mar 14, 2019 at 04:09:13PM +0000, Måns Rullgård wrote: > >> >> Maxime Ripard <maxime.ripard@bootlin.com> writes: > >> >> > >> >> > On Mon, Mar 11, 2019 at 04:11:06PM +0000, Måns Rullgård wrote: > >> >> >> Maxime Ripard <maxime.ripard@bootlin.com> 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 <mans@mansr.com> > >> >> >> >> --- > >> >> >> >> 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
Maxime Ripard <maxime.ripard@bootlin.com> writes: > On Tue, Mar 19, 2019 at 12:48:19PM +0000, Måns Rullgård wrote: >> Maxime Ripard <maxime.ripard@bootlin.com> writes: >> >> > On Mon, Mar 18, 2019 at 04:23:56PM +0000, Måns Rullgård wrote: >> >> Maxime Ripard <maxime.ripard@bootlin.com> writes: >> >> >> >> > On Thu, Mar 14, 2019 at 04:09:13PM +0000, Måns Rullgård wrote: >> >> >> Maxime Ripard <maxime.ripard@bootlin.com> writes: >> >> >> >> >> >> > On Mon, Mar 11, 2019 at 04:11:06PM +0000, Måns Rullgård wrote: >> >> >> >> Maxime Ripard <maxime.ripard@bootlin.com> 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 <mans@mansr.com> >> >> >> >> >> --- >> >> >> >> >> 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?
On Thu, Mar 21, 2019 at 06:00:45PM +0000, Måns Rullgård wrote: > Maxime Ripard <maxime.ripard@bootlin.com> writes: > > > On Tue, Mar 19, 2019 at 12:48:19PM +0000, Måns Rullgård wrote: > >> Maxime Ripard <maxime.ripard@bootlin.com> writes: > >> > >> > On Mon, Mar 18, 2019 at 04:23:56PM +0000, Måns Rullgård wrote: > >> >> Maxime Ripard <maxime.ripard@bootlin.com> writes: > >> >> > >> >> > On Thu, Mar 14, 2019 at 04:09:13PM +0000, Måns Rullgård wrote: > >> >> >> Maxime Ripard <maxime.ripard@bootlin.com> writes: > >> >> >> > >> >> >> > On Mon, Mar 11, 2019 at 04:11:06PM +0000, Måns Rullgård wrote: > >> >> >> >> Maxime Ripard <maxime.ripard@bootlin.com> 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 <mans@mansr.com> > >> >> >> >> >> --- > >> >> >> >> >> 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
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); }
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 <mans@mansr.com> --- drivers/gpu/drm/sun4i/sun4i_hdmi.h | 1 + drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 37 +++++++++++++++++++++++--- 2 files changed, 35 insertions(+), 3 deletions(-)