Re: [PATCH v3 4/4] HID: cp2112: Devicetree Support

[Bartosz Golaszewski <bartosz.golaszewski@linaro.org>]

On Mon, 6 Feb 2023 at 17:39, Rob Herring <robh@kernel.org> wrote:
>
> +Bartosz
>
> On Sun, Feb 05, 2023 at 08:54:50AM -0600, Danny Kaehn wrote:
> > Bind i2c and gpio interfaces to subnodes with names
> > "i2c" and "gpio" if they exist, respectively. This
> > allows the gpio and i2c controllers to be described
> > in DT as usual. Additionally, support configuring the
> > i2c bus speed from the clock-frequency property.
> >
> > Signed-off-by: Danny Kaehn <kaehndan@gmail.com>
> > ---
> >  drivers/hid/hid-cp2112.c | 22 ++++++++++++++++++++--
> >  1 file changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
> > index 27cadadda7c9..aa634accdfb0 100644
> > --- a/drivers/hid/hid-cp2112.c
> > +++ b/drivers/hid/hid-cp2112.c
> > @@ -1234,6 +1234,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >       u8 buf[3];
> >       struct cp2112_smbus_config_report config;
> >       struct gpio_irq_chip *girq;
> > +     struct i2c_timings timings;
> >       int ret;
> >
> >       dev = devm_kzalloc(&hdev->dev, sizeof(*dev), GFP_KERNEL);
> > @@ -1292,6 +1293,10 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >               goto err_power_normal;
> >       }
> >
> > +     dev->adap.dev.of_node   = of_get_child_by_name(hdev->dev.of_node, "i2c");
> > +     i2c_parse_fw_timings(&dev->adap.dev, &timings, true);
> > +
> > +     config.clock_speed = cpu_to_be32(timings.bus_freq_hz);
> >       config.retry_time = cpu_to_be16(1);
> >
> >       ret = cp2112_hid_output(hdev, (u8 *)&config, sizeof(config),
> > @@ -1300,7 +1305,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >               hid_err(hdev, "error setting SMBus config\n");
> >               if (ret >= 0)
> >                       ret = -EIO;
> > -             goto err_power_normal;
> > +             goto err_free_i2c_of;
> >       }
> >
> >       hid_set_drvdata(hdev, (void *)dev);
> > @@ -1322,7 +1327,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >
> >       if (ret) {
> >               hid_err(hdev, "error registering i2c adapter\n");
> > -             goto err_power_normal;
> > +             goto err_free_i2c_of;
> >       }
> >
> >       hid_dbg(hdev, "adapter registered\n");
> > @@ -1336,6 +1341,9 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >       dev->gc.ngpio                   = 8;
> >       dev->gc.can_sleep               = 1;
> >       dev->gc.parent                  = &hdev->dev;
> > +#if IS_ENABLED(CONFIG_OF_GPIO)
> > +     dev->gc.of_node                 = of_get_child_by_name(hdev->dev.of_node, "gpio");
> > +#endif
>
> The scarcity of CONFIG_OF_GPIO ifdefs in the tree tells me this is
> wrong. I think you want to use the fwnode pointer instead. GPIO
> maintainers should review this.
>
> Rob

Yep, we're moving away from OF in favor of fwnode - so you'd need to
use device_get_named_child_node() and assign gc.fwnode.

Bart