Since all the clocks are retrieved by the method
ahci_platform_get_resources() there is no need for the LLD (glue) drivers
to be looking for some particular of them in the kernel clocks table
again. Instead we suggest to add a simple method returning a
device-specific clock with passed connection ID if it is managed to be
found. Otherwise the function will return NULL. Thus the glue-drivers
won't need to either manually touching the hpriv->clks array or calling
clk_get()-friends. The AHCI platform drivers will be able to use the new
function right after the ahci_platform_get_resources() method invocation
and up to the device removal.
Signed-off-by: Serge Semin <[email protected]>
---
Changelog v2:
- Fix some grammar mistakes in the method description.
---
drivers/ata/libahci_platform.c | 27 +++++++++++++++++++++++++++
include/linux/ahci_platform.h | 3 +++
2 files changed, 30 insertions(+)
diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index 3cff86c225fd..7ff6626fd569 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -94,6 +94,33 @@ void ahci_platform_disable_phys(struct ahci_host_priv *hpriv)
}
EXPORT_SYMBOL_GPL(ahci_platform_disable_phys);
+/**
+ * ahci_platform_find_clk - Find platform clock
+ * @hpriv: host private area to store config values
+ * @con_id: clock connection ID
+ *
+ * This function returns a pointer to the clock descriptor of the clock with
+ * the passed ID.
+ *
+ * RETURNS:
+ * Pointer to the clock descriptor on success otherwise NULL
+ */
+struct clk *ahci_platform_find_clk(struct ahci_host_priv *hpriv, const char *con_id)
+{
+ struct clk *clk = NULL;
+ int i;
+
+ for (i = 0; i < hpriv->n_clks; i++) {
+ if (!strcmp(hpriv->clks[i].id, con_id)) {
+ clk = hpriv->clks[i].clk;
+ break;
+ }
+ }
+
+ return clk;
+}
+EXPORT_SYMBOL_GPL(ahci_platform_find_clk);
+
/**
* ahci_platform_enable_clks - Enable platform clocks
* @hpriv: host private area to store config values
diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
index 49e5383d4222..fd964e6a68d6 100644
--- a/include/linux/ahci_platform.h
+++ b/include/linux/ahci_platform.h
@@ -13,6 +13,7 @@
#include <linux/compiler.h>
+struct clk;
struct device;
struct ata_port_info;
struct ahci_host_priv;
@@ -21,6 +22,8 @@ struct scsi_host_template;
int ahci_platform_enable_phys(struct ahci_host_priv *hpriv);
void ahci_platform_disable_phys(struct ahci_host_priv *hpriv);
+struct clk *
+ahci_platform_find_clk(struct ahci_host_priv *hpriv, const char *con_id);
int ahci_platform_enable_clks(struct ahci_host_priv *hpriv);
void ahci_platform_disable_clks(struct ahci_host_priv *hpriv);
int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv);
--
2.35.1
On 2022/05/12 16:26, Serge Semin wrote:
> On Thu, May 12, 2022 at 08:32:37AM +0200, Hannes Reinecke wrote:
>> On 5/12/22 01:17, Serge Semin wrote:
>>> Since all the clocks are retrieved by the method
>>> ahci_platform_get_resources() there is no need for the LLD (glue) drivers
>>> to be looking for some particular of them in the kernel clocks table
>>> again. Instead we suggest to add a simple method returning a
>>> device-specific clock with passed connection ID if it is managed to be
>>> found. Otherwise the function will return NULL. Thus the glue-drivers
>>> won't need to either manually touching the hpriv->clks array or calling
>>> clk_get()-friends. The AHCI platform drivers will be able to use the new
>>> function right after the ahci_platform_get_resources() method invocation
>>> and up to the device removal.
>>>
>>> Signed-off-by: Serge Semin <[email protected]>
>>>
>>> ---
>>>
>>> Changelog v2:
>>> - Fix some grammar mistakes in the method description.
>>> ---
>>> drivers/ata/libahci_platform.c | 27 +++++++++++++++++++++++++++
>>> include/linux/ahci_platform.h | 3 +++
>>> 2 files changed, 30 insertions(+)
>>>
>>> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
>>> index 3cff86c225fd..7ff6626fd569 100644
>>> --- a/drivers/ata/libahci_platform.c
>>> +++ b/drivers/ata/libahci_platform.c
>>> @@ -94,6 +94,33 @@ void ahci_platform_disable_phys(struct ahci_host_priv *hpriv)
>>> }
>>> EXPORT_SYMBOL_GPL(ahci_platform_disable_phys);
>>> +/**
>>> + * ahci_platform_find_clk - Find platform clock
>>> + * @hpriv: host private area to store config values
>>> + * @con_id: clock connection ID
>>> + *
>>> + * This function returns a pointer to the clock descriptor of the clock with
>>> + * the passed ID.
>>> + *
>>> + * RETURNS:
>>> + * Pointer to the clock descriptor on success otherwise NULL
>>> + */
>>> +struct clk *ahci_platform_find_clk(struct ahci_host_priv *hpriv, const char *con_id)
>>> +{
>>> + struct clk *clk = NULL;
>>> + int i;
>>> +
>>> + for (i = 0; i < hpriv->n_clks; i++) {
>>> + if (!strcmp(hpriv->clks[i].id, con_id)) {
>>> + clk = hpriv->clks[i].clk;
>>> + break;
>>> + }
>>> + }
>>> +
>>> + return clk;
>>> +}
>>> +EXPORT_SYMBOL_GPL(ahci_platform_find_clk);
>>> +
>>> /**
>>> * ahci_platform_enable_clks - Enable platform clocks
>>> * @hpriv: host private area to store config values
>>> diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
>>> index 49e5383d4222..fd964e6a68d6 100644
>>> --- a/include/linux/ahci_platform.h
>>> +++ b/include/linux/ahci_platform.h
>>> @@ -13,6 +13,7 @@
>>> #include <linux/compiler.h>
>>> +struct clk;
>>> struct device;
>>> struct ata_port_info;
>>> struct ahci_host_priv;
>>> @@ -21,6 +22,8 @@ struct scsi_host_template;
>>> int ahci_platform_enable_phys(struct ahci_host_priv *hpriv);
>>> void ahci_platform_disable_phys(struct ahci_host_priv *hpriv);
>>> +struct clk *
>>> +ahci_platform_find_clk(struct ahci_host_priv *hpriv, const char *con_id);
>>> int ahci_platform_enable_clks(struct ahci_host_priv *hpriv);
>>> void ahci_platform_disable_clks(struct ahci_host_priv *hpriv);
>>> int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv);
>>
>
>> Where is this function being used?
>
> It will be used in the DWC AHCI SATA driver and can be utilized in the
> rest of the drivers to simplify the available clocks access.
> BTW Damien asked the same question in v1. My response was the same.
Please squash this patch together with the patch introducing the first use of
this function.
>
> -Sergey
>
>>
>> Cheers,
>>
>> Hannes
>> --
>> Dr. Hannes Reinecke Kernel Storage Architect
>> [email protected] +49 911 74053 688
>> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
>> HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
--
Damien Le Moal
Western Digital Research
On Thu, May 12, 2022 at 08:32:37AM +0200, Hannes Reinecke wrote:
> On 5/12/22 01:17, Serge Semin wrote:
> > Since all the clocks are retrieved by the method
> > ahci_platform_get_resources() there is no need for the LLD (glue) drivers
> > to be looking for some particular of them in the kernel clocks table
> > again. Instead we suggest to add a simple method returning a
> > device-specific clock with passed connection ID if it is managed to be
> > found. Otherwise the function will return NULL. Thus the glue-drivers
> > won't need to either manually touching the hpriv->clks array or calling
> > clk_get()-friends. The AHCI platform drivers will be able to use the new
> > function right after the ahci_platform_get_resources() method invocation
> > and up to the device removal.
> >
> > Signed-off-by: Serge Semin <[email protected]>
> >
> > ---
> >
> > Changelog v2:
> > - Fix some grammar mistakes in the method description.
> > ---
> > drivers/ata/libahci_platform.c | 27 +++++++++++++++++++++++++++
> > include/linux/ahci_platform.h | 3 +++
> > 2 files changed, 30 insertions(+)
> >
> > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> > index 3cff86c225fd..7ff6626fd569 100644
> > --- a/drivers/ata/libahci_platform.c
> > +++ b/drivers/ata/libahci_platform.c
> > @@ -94,6 +94,33 @@ void ahci_platform_disable_phys(struct ahci_host_priv *hpriv)
> > }
> > EXPORT_SYMBOL_GPL(ahci_platform_disable_phys);
> > +/**
> > + * ahci_platform_find_clk - Find platform clock
> > + * @hpriv: host private area to store config values
> > + * @con_id: clock connection ID
> > + *
> > + * This function returns a pointer to the clock descriptor of the clock with
> > + * the passed ID.
> > + *
> > + * RETURNS:
> > + * Pointer to the clock descriptor on success otherwise NULL
> > + */
> > +struct clk *ahci_platform_find_clk(struct ahci_host_priv *hpriv, const char *con_id)
> > +{
> > + struct clk *clk = NULL;
> > + int i;
> > +
> > + for (i = 0; i < hpriv->n_clks; i++) {
> > + if (!strcmp(hpriv->clks[i].id, con_id)) {
> > + clk = hpriv->clks[i].clk;
> > + break;
> > + }
> > + }
> > +
> > + return clk;
> > +}
> > +EXPORT_SYMBOL_GPL(ahci_platform_find_clk);
> > +
> > /**
> > * ahci_platform_enable_clks - Enable platform clocks
> > * @hpriv: host private area to store config values
> > diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
> > index 49e5383d4222..fd964e6a68d6 100644
> > --- a/include/linux/ahci_platform.h
> > +++ b/include/linux/ahci_platform.h
> > @@ -13,6 +13,7 @@
> > #include <linux/compiler.h>
> > +struct clk;
> > struct device;
> > struct ata_port_info;
> > struct ahci_host_priv;
> > @@ -21,6 +22,8 @@ struct scsi_host_template;
> > int ahci_platform_enable_phys(struct ahci_host_priv *hpriv);
> > void ahci_platform_disable_phys(struct ahci_host_priv *hpriv);
> > +struct clk *
> > +ahci_platform_find_clk(struct ahci_host_priv *hpriv, const char *con_id);
> > int ahci_platform_enable_clks(struct ahci_host_priv *hpriv);
> > void ahci_platform_disable_clks(struct ahci_host_priv *hpriv);
> > int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv);
>
> Where is this function being used?
It will be used in the DWC AHCI SATA driver and can be utilized in the
rest of the drivers to simplify the available clocks access.
BTW Damien asked the same question in v1. My response was the same.
-Sergey
>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke Kernel Storage Architect
> [email protected] +49 911 74053 688
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 N?rnberg
> HRB 36809 (AG N?rnberg), GF: Felix Imend?rffer
On 5/12/22 01:17, Serge Semin wrote:
> Since all the clocks are retrieved by the method
> ahci_platform_get_resources() there is no need for the LLD (glue) drivers
> to be looking for some particular of them in the kernel clocks table
> again. Instead we suggest to add a simple method returning a
> device-specific clock with passed connection ID if it is managed to be
> found. Otherwise the function will return NULL. Thus the glue-drivers
> won't need to either manually touching the hpriv->clks array or calling
> clk_get()-friends. The AHCI platform drivers will be able to use the new
> function right after the ahci_platform_get_resources() method invocation
> and up to the device removal.
>
> Signed-off-by: Serge Semin <[email protected]>
>
> ---
>
> Changelog v2:
> - Fix some grammar mistakes in the method description.
> ---
> drivers/ata/libahci_platform.c | 27 +++++++++++++++++++++++++++
> include/linux/ahci_platform.h | 3 +++
> 2 files changed, 30 insertions(+)
>
> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> index 3cff86c225fd..7ff6626fd569 100644
> --- a/drivers/ata/libahci_platform.c
> +++ b/drivers/ata/libahci_platform.c
> @@ -94,6 +94,33 @@ void ahci_platform_disable_phys(struct ahci_host_priv *hpriv)
> }
> EXPORT_SYMBOL_GPL(ahci_platform_disable_phys);
>
> +/**
> + * ahci_platform_find_clk - Find platform clock
> + * @hpriv: host private area to store config values
> + * @con_id: clock connection ID
> + *
> + * This function returns a pointer to the clock descriptor of the clock with
> + * the passed ID.
> + *
> + * RETURNS:
> + * Pointer to the clock descriptor on success otherwise NULL
> + */
> +struct clk *ahci_platform_find_clk(struct ahci_host_priv *hpriv, const char *con_id)
> +{
> + struct clk *clk = NULL;
> + int i;
> +
> + for (i = 0; i < hpriv->n_clks; i++) {
> + if (!strcmp(hpriv->clks[i].id, con_id)) {
> + clk = hpriv->clks[i].clk;
> + break;
> + }
> + }
> +
> + return clk;
> +}
> +EXPORT_SYMBOL_GPL(ahci_platform_find_clk);
> +
> /**
> * ahci_platform_enable_clks - Enable platform clocks
> * @hpriv: host private area to store config values
> diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
> index 49e5383d4222..fd964e6a68d6 100644
> --- a/include/linux/ahci_platform.h
> +++ b/include/linux/ahci_platform.h
> @@ -13,6 +13,7 @@
>
> #include <linux/compiler.h>
>
> +struct clk;
> struct device;
> struct ata_port_info;
> struct ahci_host_priv;
> @@ -21,6 +22,8 @@ struct scsi_host_template;
>
> int ahci_platform_enable_phys(struct ahci_host_priv *hpriv);
> void ahci_platform_disable_phys(struct ahci_host_priv *hpriv);
> +struct clk *
> +ahci_platform_find_clk(struct ahci_host_priv *hpriv, const char *con_id);
> int ahci_platform_enable_clks(struct ahci_host_priv *hpriv);
> void ahci_platform_disable_clks(struct ahci_host_priv *hpriv);
> int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv);
Where is this function being used?
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
[email protected] +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
On Fri, May 13, 2022 at 11:32:09AM +0200, Damien Le Moal wrote:
> On 2022/05/12 16:26, Serge Semin wrote:
> > On Thu, May 12, 2022 at 08:32:37AM +0200, Hannes Reinecke wrote:
> >> On 5/12/22 01:17, Serge Semin wrote:
> >>> Since all the clocks are retrieved by the method
> >>> ahci_platform_get_resources() there is no need for the LLD (glue) drivers
> >>> to be looking for some particular of them in the kernel clocks table
> >>> again. Instead we suggest to add a simple method returning a
> >>> device-specific clock with passed connection ID if it is managed to be
> >>> found. Otherwise the function will return NULL. Thus the glue-drivers
> >>> won't need to either manually touching the hpriv->clks array or calling
> >>> clk_get()-friends. The AHCI platform drivers will be able to use the new
> >>> function right after the ahci_platform_get_resources() method invocation
> >>> and up to the device removal.
> >>>
> >>> Signed-off-by: Serge Semin <[email protected]>
> >>>
> >>> ---
> >>>
> >>> Changelog v2:
> >>> - Fix some grammar mistakes in the method description.
> >>> ---
> >>> drivers/ata/libahci_platform.c | 27 +++++++++++++++++++++++++++
> >>> include/linux/ahci_platform.h | 3 +++
> >>> 2 files changed, 30 insertions(+)
> >>>
> >>> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> >>> index 3cff86c225fd..7ff6626fd569 100644
> >>> --- a/drivers/ata/libahci_platform.c
> >>> +++ b/drivers/ata/libahci_platform.c
> >>> @@ -94,6 +94,33 @@ void ahci_platform_disable_phys(struct ahci_host_priv *hpriv)
> >>> }
> >>> EXPORT_SYMBOL_GPL(ahci_platform_disable_phys);
> >>> +/**
> >>> + * ahci_platform_find_clk - Find platform clock
> >>> + * @hpriv: host private area to store config values
> >>> + * @con_id: clock connection ID
> >>> + *
> >>> + * This function returns a pointer to the clock descriptor of the clock with
> >>> + * the passed ID.
> >>> + *
> >>> + * RETURNS:
> >>> + * Pointer to the clock descriptor on success otherwise NULL
> >>> + */
> >>> +struct clk *ahci_platform_find_clk(struct ahci_host_priv *hpriv, const char *con_id)
> >>> +{
> >>> + struct clk *clk = NULL;
> >>> + int i;
> >>> +
> >>> + for (i = 0; i < hpriv->n_clks; i++) {
> >>> + if (!strcmp(hpriv->clks[i].id, con_id)) {
> >>> + clk = hpriv->clks[i].clk;
> >>> + break;
> >>> + }
> >>> + }
> >>> +
> >>> + return clk;
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(ahci_platform_find_clk);
> >>> +
> >>> /**
> >>> * ahci_platform_enable_clks - Enable platform clocks
> >>> * @hpriv: host private area to store config values
> >>> diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
> >>> index 49e5383d4222..fd964e6a68d6 100644
> >>> --- a/include/linux/ahci_platform.h
> >>> +++ b/include/linux/ahci_platform.h
> >>> @@ -13,6 +13,7 @@
> >>> #include <linux/compiler.h>
> >>> +struct clk;
> >>> struct device;
> >>> struct ata_port_info;
> >>> struct ahci_host_priv;
> >>> @@ -21,6 +22,8 @@ struct scsi_host_template;
> >>> int ahci_platform_enable_phys(struct ahci_host_priv *hpriv);
> >>> void ahci_platform_disable_phys(struct ahci_host_priv *hpriv);
> >>> +struct clk *
> >>> +ahci_platform_find_clk(struct ahci_host_priv *hpriv, const char *con_id);
> >>> int ahci_platform_enable_clks(struct ahci_host_priv *hpriv);
> >>> void ahci_platform_disable_clks(struct ahci_host_priv *hpriv);
> >>> int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv);
> >>
> >
> >> Where is this function being used?
> >
> > It will be used in the DWC AHCI SATA driver and can be utilized in the
> > rest of the drivers to simplify the available clocks access.
> > BTW Damien asked the same question in v1. My response was the same.
>
> Please squash this patch together with the patch introducing the first use of
> this function.
I don't find this required seeing the changes introduced here are
coherent and can be considered as preparation to the corresponding
patch. This doesn't break any submitting patch procedure and
doesn't complicate the review process but simplifies it.
-Sergey
>
> >
> > -Sergey
> >
> >>
> >> Cheers,
> >>
> >> Hannes
> >> --
> >> Dr. Hannes Reinecke Kernel Storage Architect
> >> [email protected] +49 911 74053 688
> >> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 N?rnberg
> >> HRB 36809 (AG N?rnberg), GF: Felix Imend?rffer
>
>
> --
> Damien Le Moal
> Western Digital Research