| Message ID | 20210625113435.2539282-2-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] dt-bindings: input/ts/zinitix: Convert to YAML, fix and extend | expand |
Hi Linus, On Fri, Jun 25, 2021 at 01:34:35PM +0200, Linus Walleij wrote: > The supply names of the Zinitix touchscreen were a bit confused, the new > bindings rectifies this. > > To deal with old and new devicetrees, first check if we have "vddo" and in > case that exists assume the old supply names. Else go and look for the new > ones. > > We cannot just get the regulators since we would get an OK and a dummy > regulator: we need to check explicitly for the old supply name. > > Use struct device *dev as a local variable instead of the I2C client since > the device is what we are actually obtaining the resources from. > > Cc: Mark Brown <broonie@kernel.org> > Cc: Michael Srba <Michael.Srba@seznam.cz> > Cc: phone-devel@vger.kernel.org > Cc: devicetree@vger.kernel.org > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > Mark: please check that I'm doing this check the right way, I assume > that since we get regulator dummies this is the way I need to check > for the old regulator name but maybe there are better ways. > --- > drivers/input/touchscreen/zinitix.c | 22 +++++++++++++++++----- > 1 file changed, 17 insertions(+), 5 deletions(-) > > diff --git a/drivers/input/touchscreen/zinitix.c b/drivers/input/touchscreen/zinitix.c > index b8d901099378..7001307382f0 100644 > --- a/drivers/input/touchscreen/zinitix.c > +++ b/drivers/input/touchscreen/zinitix.c > @@ -252,16 +252,28 @@ static int zinitix_init_touch(struct bt541_ts_data *bt541) > > static int zinitix_init_regulators(struct bt541_ts_data *bt541) > { > - struct i2c_client *client = bt541->client; > + struct device *dev = &bt541->client->dev; > int error; > > - bt541->supplies[0].supply = "vdd"; > - bt541->supplies[1].supply = "vddo"; > - error = devm_regulator_bulk_get(&client->dev, > + /* > + * Some older device trees have erroneous names for the regulators, > + * so check if "vddo" is present and in that case use these names > + * and warn. Else use the proper supply names on the component. > + */ > + if (IS_ENABLED(CONFIG_OF) && Why is this check needed? The of_property_*() are stubbed out properly I believe. We might need to check that dev->of_node is not NULL, although I think of_* API handles this properly. > + of_property_read_bool(dev->of_node, "vddo-supply")) { If we go with this I do not like using of_property_read_bool() as this is not a boolean property, but rather of_find_property(). However maybe we should use regulator_get_optional() which will not give a dummy regulator? Still quite awkward, a dedicated API to see if a regulator is defined would be nice. > + bt541->supplies[0].supply = "vdd"; > + bt541->supplies[1].supply = "vddo"; > + } else { > + /* Else use the proper supply names */ > + bt541->supplies[0].supply = "vcca"; > + bt541->supplies[1].supply = "vdd"; > + } > + error = devm_regulator_bulk_get(dev, > ARRAY_SIZE(bt541->supplies), > bt541->supplies); > if (error < 0) { > - dev_err(&client->dev, "Failed to get regulators: %d\n", error); > + dev_err(dev, "Failed to get regulators: %d\n", error); > return error; > } > > -- > 2.31.1 > Thanks.
Hi Dmitry, sorry for late reply! On Sat, Jul 24, 2021 at 3:13 AM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Fri, Jun 25, 2021 at 01:34:35PM +0200, Linus Walleij wrote: > > + /* > > + * Some older device trees have erroneous names for the regulators, > > + * so check if "vddo" is present and in that case use these names > > + * and warn. Else use the proper supply names on the component. > > + */ > > + if (IS_ENABLED(CONFIG_OF) && > > Why is this check needed? The of_property_*() are stubbed out properly I > believe. We might need to check that dev->of_node is not NULL, although > I think of_* API handles this properly. (...) > > + of_property_read_bool(dev->of_node, "vddo-supply")) { > > If we go with this I do not like using of_property_read_bool() as this > is not a boolean property, but rather of_find_property(). These comments are fixed up in Nikita's respin of the series: https://lore.kernel.org/linux-input/20211027181350.91630-4-nikita@trvn.ru/ > However maybe we should use regulator_get_optional() which will not give > a dummy regulator? Still quite awkward, a dedicated API to see if a > regulator is defined would be nice. I guess the option would be to get all four regulators by name and optional, but then we don't detect if more than 2 out of 4 are missing. Not sure, it feels like we have less control over the supplies then. I guess it sadly gets ugly because making mistakes in bindings is ugly in the first place. Yours, Linus Walleij
diff --git a/drivers/input/touchscreen/zinitix.c b/drivers/input/touchscreen/zinitix.c index b8d901099378..7001307382f0 100644 --- a/drivers/input/touchscreen/zinitix.c +++ b/drivers/input/touchscreen/zinitix.c @@ -252,16 +252,28 @@ static int zinitix_init_touch(struct bt541_ts_data *bt541) static int zinitix_init_regulators(struct bt541_ts_data *bt541) { - struct i2c_client *client = bt541->client; + struct device *dev = &bt541->client->dev; int error; - bt541->supplies[0].supply = "vdd"; - bt541->supplies[1].supply = "vddo"; - error = devm_regulator_bulk_get(&client->dev, + /* + * Some older device trees have erroneous names for the regulators, + * so check if "vddo" is present and in that case use these names + * and warn. Else use the proper supply names on the component. + */ + if (IS_ENABLED(CONFIG_OF) && + of_property_read_bool(dev->of_node, "vddo-supply")) { + bt541->supplies[0].supply = "vdd"; + bt541->supplies[1].supply = "vddo"; + } else { + /* Else use the proper supply names */ + bt541->supplies[0].supply = "vcca"; + bt541->supplies[1].supply = "vdd"; + } + error = devm_regulator_bulk_get(dev, ARRAY_SIZE(bt541->supplies), bt541->supplies); if (error < 0) { - dev_err(&client->dev, "Failed to get regulators: %d\n", error); + dev_err(dev, "Failed to get regulators: %d\n", error); return error; }
The supply names of the Zinitix touchscreen were a bit confused, the new bindings rectifies this. To deal with old and new devicetrees, first check if we have "vddo" and in case that exists assume the old supply names. Else go and look for the new ones. We cannot just get the regulators since we would get an OK and a dummy regulator: we need to check explicitly for the old supply name. Use struct device *dev as a local variable instead of the I2C client since the device is what we are actually obtaining the resources from. Cc: Mark Brown <broonie@kernel.org> Cc: Michael Srba <Michael.Srba@seznam.cz> Cc: phone-devel@vger.kernel.org Cc: devicetree@vger.kernel.org Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- Mark: please check that I'm doing this check the right way, I assume that since we get regulator dummies this is the way I need to check for the old regulator name but maybe there are better ways. --- drivers/input/touchscreen/zinitix.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)