Re: [PATCH v7 2/3] clk: cs2600: Add Fractional-N clock driver

[Uwe Kleine-König]

On Fri, Aug 07, 2026 at 04:54:05PM +0100, Richard Fitzgerald wrote:
> +#include <dt-bindings/clock/cirrus,cs2600-clock.h>
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/container_of.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/math.h>
> +#include <linux/math64.h>
> +#include <linux/mod_devicetable.h>

Please drop <linux/mod_devicetable.h>. You can rely on <linux/i2c.h> to
provide struct i2c_device_id and of_device_id.

(Background of this request is that <linux/mod_devicetable.h> should go
away soon.)

> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/string.h>
> +
> [...]
> +static const struct of_device_id cs2600_of_match[] = {
> +	{ .compatible = "cirrus,cs2600", },

No comma before a closing } in the same line please.

> +	{}

{ } is the most common way to write the terminator.

> +};
> +MODULE_DEVICE_TABLE(of, cs2600_of_match);
> +
> +static const struct i2c_device_id cs2600_id[] = {
> +	{ .name = "cs2600", },
> +	{}
> +};

Here the same please.

> +MODULE_DEVICE_TABLE(i2c, cs2600_id);
> +
> +static struct i2c_driver cs2600_driver = {
> +	.driver = {
> +		.name = "cs2600",
> +		.of_match_table = cs2600_of_match,
> +	},
> +	.probe		= cs2600_i2c_probe,
> +	.id_table	= cs2600_id,

I'm not a fan of aligning `=`s as it gets inconsistent over time. Here
it's wrong already from the start. Only 2 out of 3 members of the
i2c_driver use alignment and .driver doesn't. If you ask me, just use a
single space before each `=`.

> +};
> +
> +module_i2c_driver(cs2600_driver);

No empty line before module_i2c_driver() please.

> +
> +MODULE_DESCRIPTION("CS2600 clock driver");
> +MODULE_AUTHOR("Paul Handrigan <paulha@xxxxxxxxxxxxxxxxxxxxx>");
> +MODULE_LICENSE("GPL");

Attachment: signature.asc
Description: PGP signature