diff mbox series

[v3,4/4] HID: cp2112: Devicetree Support

Message ID 20230205145450.3396-5-kaehndan@gmail.com
State Superseded
Headers show
Series DeviceTree Support for USB-HID Devices and CP2112 | expand

Commit Message

Daniel Kaehn Feb. 5, 2023, 2:54 p.m. UTC
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(-)

Comments

Rob Herring (Arm) Feb. 6, 2023, 4:39 p.m. UTC | #1
+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
Bartosz Golaszewski Feb. 9, 2023, 9:01 p.m. UTC | #2
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
diff mbox series

Patch

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
 
 	dev->irq.name = "cp2112-gpio";
 	dev->irq.irq_startup = cp2112_gpio_irq_startup;
@@ -1376,7 +1384,12 @@  static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 err_gpiochip_remove:
 	gpiochip_remove(&dev->gc);
 err_free_i2c:
+#if IS_ENABLED(CONFIG_OF_GPIO)
+	of_node_put(dev->gc.of_node);
+#endif
 	i2c_del_adapter(&dev->adap);
+err_free_i2c_of:
+	of_node_put(dev->adap.dev.of_node);
 err_power_normal:
 	hid_hw_power(hdev, PM_HINT_NORMAL);
 err_hid_close:
@@ -1391,6 +1404,11 @@  static void cp2112_remove(struct hid_device *hdev)
 	struct cp2112_device *dev = hid_get_drvdata(hdev);
 	int i;
 
+	of_node_put(dev->adap.dev.of_node);
+#if IS_ENABLED(CONFIG_OF_GPIO)
+	of_node_put(dev->gc.of_node);
+#endif
+
 	sysfs_remove_group(&hdev->dev.kobj, &cp2112_attr_group);
 	i2c_del_adapter(&dev->adap);