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

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(-)

On Mon, Feb 06, 2023 at 07:50:16AM -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");


I believe Andy is actively trying to get rid of of_node from GPIO chips.
And in general, we should be using fwnode and generic device properties
as much as possible.

> +#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);
>  
> -- 
> 2.25.1
> 

Thanks.

On Mon, Feb 06, 2023 at 03:18:26PM -0800, Dmitry Torokhov wrote:
> On Mon, Feb 06, 2023 at 07:50:16AM -0600, Danny Kaehn wrote:

...

> > +#if IS_ENABLED(CONFIG_OF_GPIO)
> > +	dev->gc.of_node			= of_get_child_by_name(hdev->dev.of_node, "gpio");
> 
> 
> I believe Andy is actively trying to get rid of of_node from GPIO chips.
> And in general, we should be using fwnode and generic device properties
> as much as possible.
> 
> > +#endif

Correct. And looking into the code of this patch I don't see any obstacles
to use fwnode APIs. You can Cc a v5 (which is supposed to be fwnode API based)
to me.

On Tue, Feb 7, 2023 at 4:15 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Feb 06, 2023 at 03:18:26PM -0800, Dmitry Torokhov wrote:
> > On Mon, Feb 06, 2023 at 07:50:16AM -0600, Danny Kaehn wrote:
>
> ...
>
> > > +#if IS_ENABLED(CONFIG_OF_GPIO)
> > > +   dev->gc.of_node                 = of_get_child_by_name(hdev->dev.of_node, "gpio");
> >
> >
> > I believe Andy is actively trying to get rid of of_node from GPIO chips.
> > And in general, we should be using fwnode and generic device properties
> > as much as possible.
> >
> > > +#endif
>
> Correct. And looking into the code of this patch I don't see any obstacles
> to use fwnode APIs. You can Cc a v5 (which is supposed to be fwnode API based)
> to me.
>

Sounds great, will do. I looked into doing this with the fwnode
initially, but thought since the capability to describe usb devices in
ACPI doesn't seem to be there, that I should be explicit that this
only works for devicetree--but makes sense that it's better to be
generic at the driver level if possible (especially if of_node is
being removed from gpio chips), so will do!

Thanks,

Danny Kaehn