| Message ID | 20220609181106.3695103-6-pmalani@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: Introduce typec-switch binding | expand |
Hi Prashant, On Thu, Jun 09, 2022 at 06:09:44PM +0000, Prashant Malani wrote: > Parse the "switches" node, if available, and count and store the number > of Type-C switches within it. Since we currently don't do anything with > this info, no functional changes are expected from this change. > > This patch sets a foundation for the actual registering of Type-C > switches with the Type-C connector class framework. > > Signed-off-by: Prashant Malani <pmalani@chromium.org> > --- > > Changes since v1: > - No changes. > > drivers/gpu/drm/bridge/analogix/anx7625.c | 20 ++++++++++++++++++++ > drivers/gpu/drm/bridge/analogix/anx7625.h | 1 + > 2 files changed, 21 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c > index 53a5da6c49dd..07ed44c6b839 100644 > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c > @@ -2581,6 +2581,22 @@ static void anx7625_runtime_disable(void *data) > pm_runtime_disable(data); > } > > +static int anx7625_register_typec_switches(struct device *device, struct anx7625_data *ctx) > +{ > + struct device_node *of = NULL; > + int ret = 0; > + > + of = of_get_child_by_name(device->of_node, "switches"); > + if (!of) > + return -ENODEV; > + > + ctx->num_typec_switches = of_get_child_count(of); > + if (ctx->num_typec_switches <= 0) > + return -ENODEV; Since the hardware only allows at most 2 switches (based on the dt-bindings), you should have a define for that limit and check that it isn't exceeded here. Thanks, Nícolas > + > + return ret; > +} > + > static int anx7625_i2c_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > @@ -2686,6 +2702,10 @@ static int anx7625_i2c_probe(struct i2c_client *client, > if (platform->pdata.intp_irq) > queue_work(platform->workqueue, &platform->work); > > + ret = anx7625_register_typec_switches(dev, platform); > + if (ret) > + dev_info(dev, "Didn't register Type C switches, err: %d\n", ret); > + > platform->bridge.funcs = &anx7625_bridge_funcs; > platform->bridge.of_node = client->dev.of_node; > if (!anx7625_of_panel_on_aux_bus(&client->dev)) > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h > index e257a84db962..d5cbca708842 100644 > --- a/drivers/gpu/drm/bridge/analogix/anx7625.h > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.h > @@ -473,6 +473,7 @@ struct anx7625_data { > struct drm_connector *connector; > struct mipi_dsi_device *dsi; > struct drm_dp_aux aux; > + int num_typec_switches; > }; > > #endif /* __ANX7625_H__ */ > -- > 2.36.1.476.g0c4daa206d-goog >
Hi Nícolas, On Mon, Jun 13, 2022 at 1:45 PM Nícolas F. R. A. Prado <nfraprado@collabora.com> wrote: > > Hi Prashant, > > On Thu, Jun 09, 2022 at 06:09:44PM +0000, Prashant Malani wrote: > > Parse the "switches" node, if available, and count and store the number > > of Type-C switches within it. Since we currently don't do anything with > > this info, no functional changes are expected from this change. > > > > This patch sets a foundation for the actual registering of Type-C > > switches with the Type-C connector class framework. > > > > Signed-off-by: Prashant Malani <pmalani@chromium.org> > > --- > > > > Changes since v1: > > - No changes. > > > > drivers/gpu/drm/bridge/analogix/anx7625.c | 20 ++++++++++++++++++++ > > drivers/gpu/drm/bridge/analogix/anx7625.h | 1 + > > 2 files changed, 21 insertions(+) > > > > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c > > index 53a5da6c49dd..07ed44c6b839 100644 > > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c > > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c > > @@ -2581,6 +2581,22 @@ static void anx7625_runtime_disable(void *data) > > pm_runtime_disable(data); > > } > > > > +static int anx7625_register_typec_switches(struct device *device, struct anx7625_data *ctx) > > +{ > > + struct device_node *of = NULL; > > + int ret = 0; > > + > > + of = of_get_child_by_name(device->of_node, "switches"); > > + if (!of) > > + return -ENODEV; > > + > > + ctx->num_typec_switches = of_get_child_count(of); > > + if (ctx->num_typec_switches <= 0) > > + return -ENODEV; > > Since the hardware only allows at most 2 switches (based on the dt-bindings), > you should have a define for that limit and check that it isn't exceeded here. This is already enforced by the DT binding (see the "patternProperties" in patch 4/7, suggested by Krzysztof in the previous version) > > Thanks, > Nícolas > > > + > > + return ret; > > +} > > + > > static int anx7625_i2c_probe(struct i2c_client *client, > > const struct i2c_device_id *id) > > { > > @@ -2686,6 +2702,10 @@ static int anx7625_i2c_probe(struct i2c_client *client, > > if (platform->pdata.intp_irq) > > queue_work(platform->workqueue, &platform->work); > > > > + ret = anx7625_register_typec_switches(dev, platform); > > + if (ret) > > + dev_info(dev, "Didn't register Type C switches, err: %d\n", ret); > > + > > platform->bridge.funcs = &anx7625_bridge_funcs; > > platform->bridge.of_node = client->dev.of_node; > > if (!anx7625_of_panel_on_aux_bus(&client->dev)) > > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h > > index e257a84db962..d5cbca708842 100644 > > --- a/drivers/gpu/drm/bridge/analogix/anx7625.h > > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.h > > @@ -473,6 +473,7 @@ struct anx7625_data { > > struct drm_connector *connector; > > struct mipi_dsi_device *dsi; > > struct drm_dp_aux aux; > > + int num_typec_switches; > > }; > > > > #endif /* __ANX7625_H__ */ > > -- > > 2.36.1.476.g0c4daa206d-goog > >
Il 09/06/22 20:09, Prashant Malani ha scritto: > Parse the "switches" node, if available, and count and store the number > of Type-C switches within it. Since we currently don't do anything with > this info, no functional changes are expected from this change. > > This patch sets a foundation for the actual registering of Type-C > switches with the Type-C connector class framework. > > Signed-off-by: Prashant Malani <pmalani@chromium.org> > --- > > Changes since v1: > - No changes. > > drivers/gpu/drm/bridge/analogix/anx7625.c | 20 ++++++++++++++++++++ > drivers/gpu/drm/bridge/analogix/anx7625.h | 1 + > 2 files changed, 21 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c > index 53a5da6c49dd..07ed44c6b839 100644 > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c > @@ -2581,6 +2581,22 @@ static void anx7625_runtime_disable(void *data) > pm_runtime_disable(data); > } > > +static int anx7625_register_typec_switches(struct device *device, struct anx7625_data *ctx) > +{ > + struct device_node *of = NULL; > + int ret = 0; > + > + of = of_get_child_by_name(device->of_node, "switches"); > + if (!of) > + return -ENODEV; > + > + ctx->num_typec_switches = of_get_child_count(of); > + if (ctx->num_typec_switches <= 0) > + return -ENODEV; > + > + return ret; You aren't using the `ret` variable for anything other than returning zero: remove it and simply return 0 here. > +} > + > static int anx7625_i2c_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > @@ -2686,6 +2702,10 @@ static int anx7625_i2c_probe(struct i2c_client *client, > if (platform->pdata.intp_irq) > queue_work(platform->workqueue, &platform->work); > > + ret = anx7625_register_typec_switches(dev, platform); > + if (ret) > + dev_info(dev, "Didn't register Type C switches, err: %d\n", ret); Type-C switches are optional for this driver and this will print a sort of error on boards that are *not* declaring any switches on purpose (because perhaps they don't have any, or for any other reason). Even though this is a dev_info and not a dev_err, it's still printing an alarming (and useless, in the aforementioned case) message. Please fix this. Regards, Angelo
On Tue, Jun 14, 2022 at 1:22 AM AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > > Il 09/06/22 20:09, Prashant Malani ha scritto: > > Parse the "switches" node, if available, and count and store the number > > of Type-C switches within it. Since we currently don't do anything with > > this info, no functional changes are expected from this change. > > > > This patch sets a foundation for the actual registering of Type-C > > switches with the Type-C connector class framework. > > > > Signed-off-by: Prashant Malani <pmalani@chromium.org> > > --- > > > > Changes since v1: > > - No changes. > > > > drivers/gpu/drm/bridge/analogix/anx7625.c | 20 ++++++++++++++++++++ > > drivers/gpu/drm/bridge/analogix/anx7625.h | 1 + > > 2 files changed, 21 insertions(+) > > > > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c > > index 53a5da6c49dd..07ed44c6b839 100644 > > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c > > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c > > @@ -2581,6 +2581,22 @@ static void anx7625_runtime_disable(void *data) > > pm_runtime_disable(data); > > } > > > > +static int anx7625_register_typec_switches(struct device *device, struct anx7625_data *ctx) > > +{ > > + struct device_node *of = NULL; > > + int ret = 0; > > + > > + of = of_get_child_by_name(device->of_node, "switches"); > > + if (!of) > > + return -ENODEV; > > + > > + ctx->num_typec_switches = of_get_child_count(of); > > + if (ctx->num_typec_switches <= 0) > > + return -ENODEV; > > + > > + return ret; > > You aren't using the `ret` variable for anything other than returning zero: > remove it and simply return 0 here. The very next patch does use it, but sure I'll remove it from here and introduce it in v6. > > > +} > > + > > static int anx7625_i2c_probe(struct i2c_client *client, > > const struct i2c_device_id *id) > > { > > @@ -2686,6 +2702,10 @@ static int anx7625_i2c_probe(struct i2c_client *client, > > if (platform->pdata.intp_irq) > > queue_work(platform->workqueue, &platform->work); > > > > + ret = anx7625_register_typec_switches(dev, platform); > > + if (ret) > > + dev_info(dev, "Didn't register Type C switches, err: %d\n", ret); > > Type-C switches are optional for this driver and this will print a sort of error > on boards that are *not* declaring any switches on purpose (because perhaps they > don't have any, or for any other reason). > > Even though this is a dev_info and not a dev_err, it's still printing an alarming > (and useless, in the aforementioned case) message. I'll go ahead and convert this to dev_warn, but only trigger if there is an error other than ENODEV. > > Please fix this. > > Regards, > Angelo >
diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c index 53a5da6c49dd..07ed44c6b839 100644 --- a/drivers/gpu/drm/bridge/analogix/anx7625.c +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c @@ -2581,6 +2581,22 @@ static void anx7625_runtime_disable(void *data) pm_runtime_disable(data); } +static int anx7625_register_typec_switches(struct device *device, struct anx7625_data *ctx) +{ + struct device_node *of = NULL; + int ret = 0; + + of = of_get_child_by_name(device->of_node, "switches"); + if (!of) + return -ENODEV; + + ctx->num_typec_switches = of_get_child_count(of); + if (ctx->num_typec_switches <= 0) + return -ENODEV; + + return ret; +} + static int anx7625_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -2686,6 +2702,10 @@ static int anx7625_i2c_probe(struct i2c_client *client, if (platform->pdata.intp_irq) queue_work(platform->workqueue, &platform->work); + ret = anx7625_register_typec_switches(dev, platform); + if (ret) + dev_info(dev, "Didn't register Type C switches, err: %d\n", ret); + platform->bridge.funcs = &anx7625_bridge_funcs; platform->bridge.of_node = client->dev.of_node; if (!anx7625_of_panel_on_aux_bus(&client->dev)) diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h index e257a84db962..d5cbca708842 100644 --- a/drivers/gpu/drm/bridge/analogix/anx7625.h +++ b/drivers/gpu/drm/bridge/analogix/anx7625.h @@ -473,6 +473,7 @@ struct anx7625_data { struct drm_connector *connector; struct mipi_dsi_device *dsi; struct drm_dp_aux aux; + int num_typec_switches; }; #endif /* __ANX7625_H__ */
Parse the "switches" node, if available, and count and store the number of Type-C switches within it. Since we currently don't do anything with this info, no functional changes are expected from this change. This patch sets a foundation for the actual registering of Type-C switches with the Type-C connector class framework. Signed-off-by: Prashant Malani <pmalani@chromium.org> --- Changes since v1: - No changes. drivers/gpu/drm/bridge/analogix/anx7625.c | 20 ++++++++++++++++++++ drivers/gpu/drm/bridge/analogix/anx7625.h | 1 + 2 files changed, 21 insertions(+)