linux-kernel - Re: [PATCH v4 1/4] i2c: thunderx: Clock divisor logic changes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <plzzqgrfs6m5a7mfizzuu2rf4yqt4fgecelvlc4ibthvocokbj@r6iwu46wryku>
Date: Thu, 21 Mar 2024 23:53:50 +0100
From: Andi Shyti <andi.shyti@...nel.org>
To: Piyush Malgujar <pmalgujar@...vell.com>
Cc: linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org, 
	sgarapati@...vell.com, cchavva@...vell.com, jannadurai@...vell.com
Subject: Re: [PATCH v4 1/4] i2c: thunderx: Clock divisor logic changes

Hi Piyush,

looks good, just a few little things.

..

> +#define INITIAL_DELTA_HZ	1000000
> +#define TWSI_MASTER_CLK_REG_DEF_VAL	0x18
> +#define TWSI_MASTER_CLK_REG_OTX2_VAL	0x3

nit: we can have a better alignment here

#define INITIAL_DELTA_HZ		1000000
#define TWSI_MASTER_CLK_REG_DEF_VAL	0x18
#define TWSI_MASTER_CLK_REG_OTX2_VAL	0x03

..

>  void octeon_i2c_set_clock(struct octeon_i2c *i2c)
>  {
>  	int tclk, thp_base, inc, thp_idx, mdiv_idx, ndiv_idx, foscl, diff;
> -	int thp = 0x18, mdiv = 2, ndiv = 0, delta_hz = 1000000;
> +	unsigned int mdiv_min = 2;
> +	/*
> +	 * Find divisors to produce target frequency, start with large delta
> +	 * to cover wider range of divisors, note thp = TCLK half period.
> +	 */
> +	unsigned int thp = TWSI_MASTER_CLK_REG_DEF_VAL, mdiv = 2, ndiv = 0;
> +	unsigned int delta_hz = INITIAL_DELTA_HZ;
> +
> +	bool is_plat_otx2 = octeon_i2c_is_otx2(to_pci_dev(i2c->dev));

nit: please, don't leave space between variable declaration.
nit: please make important assignments not during the
     declaration, but on a different line.

..

> +/**
> + * octeon_i2c_is_otx2 - check for chip ID
> + * @pdev: PCI dev structure
> + *
> + * Returns TRUE if OcteonTX2, FALSE otherwise.

/TRUE/true/, /FALSE/false/

Can you please write it a bit better? At the end this becomes
documentation. Something like:

"Returns true if the device is an OcteonTX2, false otherwise."

> + */
> +static inline bool octeon_i2c_is_otx2(struct pci_dev *pdev)
> +{
> +	u32 chip_id = (pdev->subsystem_device >> 12) & 0xF;

You could use the bitops helpers here. I never remember which one
is the right one, if I remember correctly FIELD_GET() should be
the right one.

Andi