* [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property @ 2019-03-11 13:47 Mans Rullgard 2019-03-11 15:47 ` Maxime Ripard 0 siblings, 1 reply; 14+ messages in thread From: Mans Rullgard @ 2019-03-11 13:47 UTC (permalink / raw) To: Maxime Ripard, David Airlie, Chen-Yu Tsai Cc: dri-devel, linux-arm-kernel, linux-kernel 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); 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property 2019-03-11 13:47 [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property Mans Rullgard @ 2019-03-11 15:47 ` Maxime Ripard 2019-03-11 16:11 ` Måns Rullgård 0 siblings, 1 reply; 14+ messages in thread From: Maxime Ripard @ 2019-03-11 15:47 UTC (permalink / raw) To: Mans Rullgard Cc: David Airlie, Chen-Yu Tsai, linux-arm-kernel, dri-devel, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 4222 bytes --] 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)) { > + 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 [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property 2019-03-11 15:47 ` Maxime Ripard @ 2019-03-11 16:11 ` Måns Rullgård 2019-03-14 15:41 ` Maxime Ripard 0 siblings, 1 reply; 14+ messages in thread From: Måns Rullgård @ 2019-03-11 16:11 UTC (permalink / raw) To: Maxime Ripard Cc: David Airlie, Chen-Yu Tsai, dri-devel, linux-arm-kernel, linux-kernel 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. -- Måns Rullgård ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property 2019-03-11 16:11 ` Måns Rullgård @ 2019-03-14 15:41 ` Maxime Ripard 2019-03-14 16:09 ` Måns Rullgård 0 siblings, 1 reply; 14+ messages in thread From: Maxime Ripard @ 2019-03-14 15:41 UTC (permalink / raw) To: Måns Rullgård Cc: David Airlie, Chen-Yu Tsai, dri-devel, linux-arm-kernel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 3578 bytes --] 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, Bootlin Embedded Linux and Kernel engineering https://bootlin.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property 2019-03-14 15:41 ` Maxime Ripard @ 2019-03-14 16:09 ` Måns Rullgård 2019-03-18 15:50 ` Maxime Ripard 0 siblings, 1 reply; 14+ messages in thread From: Måns Rullgård @ 2019-03-14 16:09 UTC (permalink / raw) To: Maxime Ripard Cc: David Airlie, Chen-Yu Tsai, dri-devel, linux-arm-kernel, linux-kernel 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? -- Måns Rullgård ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property 2019-03-14 16:09 ` Måns Rullgård @ 2019-03-18 15:50 ` Maxime Ripard 2019-03-18 16:23 ` Måns Rullgård 0 siblings, 1 reply; 14+ messages in thread From: Maxime Ripard @ 2019-03-18 15:50 UTC (permalink / raw) To: Måns Rullgård Cc: David Airlie, Chen-Yu Tsai, dri-devel, linux-arm-kernel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 4348 bytes --] 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property 2019-03-18 15:50 ` Maxime Ripard @ 2019-03-18 16:23 ` Måns Rullgård 2019-03-19 12:34 ` Maxime Ripard 0 siblings, 1 reply; 14+ messages in thread From: Måns Rullgård @ 2019-03-18 16:23 UTC (permalink / raw) To: Maxime Ripard Cc: David Airlie, Chen-Yu Tsai, dri-devel, linux-arm-kernel, linux-kernel 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. -- Måns Rullgård ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property 2019-03-18 16:23 ` Måns Rullgård @ 2019-03-19 12:34 ` Maxime Ripard 2019-03-19 12:48 ` Måns Rullgård 0 siblings, 1 reply; 14+ messages in thread From: Maxime Ripard @ 2019-03-19 12:34 UTC (permalink / raw) To: Måns Rullgård Cc: David Airlie, Chen-Yu Tsai, linux-arm-kernel, dri-devel, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 5725 bytes --] 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 [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property 2019-03-19 12:34 ` Maxime Ripard @ 2019-03-19 12:48 ` Måns Rullgård 2019-03-21 15:44 ` Maxime Ripard 0 siblings, 1 reply; 14+ messages in thread From: Måns Rullgård @ 2019-03-19 12:48 UTC (permalink / raw) To: Maxime Ripard Cc: David Airlie, Chen-Yu Tsai, dri-devel, linux-arm-kernel, linux-kernel 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. -- Måns Rullgård ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property 2019-03-19 12:48 ` Måns Rullgård @ 2019-03-21 15:44 ` Maxime Ripard 2019-03-21 18:00 ` Måns Rullgård 0 siblings, 1 reply; 14+ messages in thread From: Maxime Ripard @ 2019-03-21 15:44 UTC (permalink / raw) To: Måns Rullgård Cc: David Airlie, Chen-Yu Tsai, dri-devel, linux-arm-kernel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 7001 bytes --] 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property 2019-03-21 15:44 ` Maxime Ripard @ 2019-03-21 18:00 ` Måns Rullgård 2019-03-26 19:49 ` Maxime Ripard 0 siblings, 1 reply; 14+ messages in thread From: Måns Rullgård @ 2019-03-21 18:00 UTC (permalink / raw) To: Maxime Ripard Cc: David Airlie, Chen-Yu Tsai, dri-devel, linux-arm-kernel, linux-kernel 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? -- Måns Rullgård ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property 2019-03-21 18:00 ` Måns Rullgård @ 2019-03-26 19:49 ` Maxime Ripard 2019-03-28 13:02 ` Mans Rullgard 0 siblings, 1 reply; 14+ messages in thread From: Maxime Ripard @ 2019-03-26 19:49 UTC (permalink / raw) To: Måns Rullgård Cc: David Airlie, Chen-Yu Tsai, dri-devel, linux-arm-kernel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 7707 bytes --] 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property 2019-03-26 19:49 ` Maxime Ripard @ 2019-03-28 13:02 ` Mans Rullgard 2019-04-01 11:58 ` Maxime Ripard 0 siblings, 1 reply; 14+ messages in thread From: Mans Rullgard @ 2019-03-28 13:02 UTC (permalink / raw) To: Maxime Ripard Cc: David Airlie, Chen-Yu Tsai, dri-devel, linux-arm-kernel, linux-kernel 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> --- 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property 2019-03-28 13:02 ` Mans Rullgard @ 2019-04-01 11:58 ` Maxime Ripard 0 siblings, 0 replies; 14+ messages in thread From: Maxime Ripard @ 2019-04-01 11:58 UTC (permalink / raw) To: Mans Rullgard Cc: David Airlie, Chen-Yu Tsai, linux-arm-kernel, dri-devel, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 442 bytes --] 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 <mans@mansr.com> Applied to drm-misc-next, thanks! Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-04-01 11:58 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-11 13:47 [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property Mans Rullgard 2019-03-11 15:47 ` Maxime Ripard 2019-03-11 16:11 ` Måns Rullgård 2019-03-14 15:41 ` Maxime Ripard 2019-03-14 16:09 ` Måns Rullgård 2019-03-18 15:50 ` Maxime Ripard 2019-03-18 16:23 ` Måns Rullgård 2019-03-19 12:34 ` Maxime Ripard 2019-03-19 12:48 ` Måns Rullgård 2019-03-21 15:44 ` Maxime Ripard 2019-03-21 18:00 ` Måns Rullgård 2019-03-26 19:49 ` Maxime Ripard 2019-03-28 13:02 ` Mans Rullgard 2019-04-01 11:58 ` Maxime Ripard
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).